Hi. This clean up looks good basically. But when conventional pci device is accessed via MMCONFIG area, addr &= addr_mask doesn't work as expected. The config area of [256, 4K) of conventional pci should have no effect.
thanks, On Tue, Jul 19, 2011 at 11:39:02PM +0200, Jan Kiszka wrote: > From: Jan Kiszka <jan.kis...@siemens.com> > > Introduce pci_config_read/write helpers to split up config space > accesses that are not length-aligned. This particularly avoids that each > and every device needs to check for config space overruns. Also move the > access length assertion to the new helpers. > > Signed-off-by: Jan Kiszka <jan.kis...@siemens.com> > --- > > This will also simplify my cap refactorings for the device-assignment > code in qemu-kvm. > > hw/pci.c | 43 +++++++++++++++++++++++++++++++++++++++---- > hw/pci.h | 5 +++++ > hw/pci_host.c | 14 ++++++-------- > hw/pcie_host.c | 12 ++++++------ > 4 files changed, 56 insertions(+), 18 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index b904a4e..6957ece 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -1104,12 +1104,48 @@ static void pci_update_irq_disabled(PCIDevice *d, int > was_irq_disabled) > } > } > > +void pci_config_write(PCIDevice *pci_dev, uint32_t addr, uint32_t addr_mask, > + uint32_t val, uint32_t len) > +{ > + assert(len == 1 || len == 2 || len == 4); > + > + addr &= addr_mask; > + > + if (addr & (len - 1)) { > + len >>= 1; > + pci_config_write(pci_dev, addr, addr_mask, val, len); > + pci_config_write(pci_dev, addr + len, addr_mask, > + val >> (len * 8), len); > + } else { > + pci_dev->config_write(pci_dev, addr, val, len); > + } > +} > + > +uint32_t pci_config_read(PCIDevice *pci_dev, uint32_t addr, uint32_t > addr_mask, > + uint32_t len) > +{ > + uint32_t val; > + > + assert(len == 1 || len == 2 || len == 4); > + > + addr &= addr_mask; > + > + if (addr & (len - 1)) { > + len >>= 1; > + val = pci_config_read(pci_dev, addr, addr_mask, len); > + val |= pci_config_read(pci_dev, addr + len, > + addr_mask, len) << (len * 8); > + } else { > + val = pci_dev->config_read(pci_dev, addr, len); > + } > + return val; > +} > + > uint32_t pci_default_read_config(PCIDevice *d, > uint32_t address, int len) > { > uint32_t val = 0; > - assert(len == 1 || len == 2 || len == 4); > - len = MIN(len, pci_config_size(d) - address); > + > memcpy(&val, d->config + address, len); > return le32_to_cpu(val); > } > @@ -1117,9 +1153,8 @@ uint32_t pci_default_read_config(PCIDevice *d, > void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int > l) > { > int i, was_irq_disabled = pci_irq_disabled(d); > - uint32_t config_size = pci_config_size(d); > > - for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) { > + for (i = 0; i < l; val >>= 8, ++i) { > uint8_t wmask = d->wmask[addr + i]; > uint8_t w1cmask = d->w1cmask[addr + i]; > assert(!(wmask & w1cmask)); > diff --git a/hw/pci.h b/hw/pci.h > index c220745..3d5b39c 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -482,4 +482,9 @@ static inline uint32_t pci_config_size(const PCIDevice *d) > return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE : > PCI_CONFIG_SPACE_SIZE; > } > > +void pci_config_write(PCIDevice *pci_dev, uint32_t addr, uint32_t addr_mask, > + uint32_t val, uint32_t len); > +uint32_t pci_config_read(PCIDevice *pci_dev, uint32_t addr, uint32_t > addr_mask, > + uint32_t len); > + > #endif > diff --git a/hw/pci_host.c b/hw/pci_host.c > index 728e2d4..db888fb 100644 > --- a/hw/pci_host.c > +++ b/hw/pci_host.c > @@ -50,30 +50,28 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus > *bus, uint32_t addr) > void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len) > { > PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); > - uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); > > - if (!pci_dev) > + if (!pci_dev) { > return; > + } > > PCI_DPRINTF("%s: %s: addr=%02" PRIx32 " val=%08" PRIx32 " len=%d\n", > - __func__, pci_dev->name, config_addr, val, len); > - pci_dev->config_write(pci_dev, config_addr, val, len); > + __func__, pci_dev->name, addr, val, len); > + pci_config_write(pci_dev, addr, PCI_CONFIG_SPACE_SIZE - 1, val, len); > } > > uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) > { > PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); > - uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); > uint32_t val; > > - assert(len == 1 || len == 2 || len == 4); > if (!pci_dev) { > return ~0x0; > } > > - val = pci_dev->config_read(pci_dev, config_addr, len); > + val = pci_config_read(pci_dev, addr, PCI_CONFIG_SPACE_SIZE - 1, len); > PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n", > - __func__, pci_dev->name, config_addr, val, len); > + __func__, pci_dev->name, addr, val, len); > > return val; > } > diff --git a/hw/pcie_host.c b/hw/pcie_host.c > index b749865..e10d7a3 100644 > --- a/hw/pcie_host.c > +++ b/hw/pcie_host.c > @@ -57,22 +57,22 @@ static void pcie_mmcfg_data_write(PCIBus *s, > { > PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr); > > - if (!pci_dev) > + if (!pci_dev) { > return; > - > - pci_dev->config_write(pci_dev, > - PCIE_MMCFG_CONFOFFSET(mmcfg_addr), val, len); > + } > + pci_config_write(pci_dev, PCIE_MMCFG_CONFOFFSET(mmcfg_addr), > + PCIE_MMCFG_CONFOFFSET_MASK, val, len); > } > > static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t addr, int len) > { > PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, addr); > > - assert(len == 1 || len == 2 || len == 4); > if (!pci_dev) { > return ~0x0; > } > - return pci_dev->config_read(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), len); > + return pci_config_read(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), > + PCIE_MMCFG_CONFOFFSET_MASK, len); > } > > static void pcie_mmcfg_data_writeb(void *opaque, > -- > 1.7.3.4 > -- yamahata