Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jul 19, 2011 at 11:39:02PM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@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.kiszka@siemens.com>

Nice, this is possible now that everyone uses pci_host.
Some comments below.

> ---
> 
> 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)

Let's put these in pci_host.h/pci_host.c, and prefix in a way
so we don't want devices to use this mistakenly.
Also please add a comment here saying what this does.
Maybe also note how, if some system device ever does
need misaligned accesses, it will need some special hacks.

> +{
> +    assert(len == 1 || len == 2 || len == 4);
> +
> +    addr &= addr_mask;

A 4 byte access at address 255 would wrap around to 0 with this,
while previously we ignored high bits on write and returned
0 on read.

I'd say let's check config size. With some more comments
this gets:

	if (addr >= pci_config_size(pci_dev)) {
		return;
	}

	if (likely(!(addr & (len - 1)))) {
	       pci_dev->config_write(pci_dev, addr, val, len);
	       return;
	}

	/* Handle unaligned access: split it to two accesses each
	 * half the length of the original one. */
	len = len / 2;
	pci_host_config_write(pci_dev, addr, addr_mask, val, len);
	pci_host_config_write(pci_dev, addr + len, config_size, val >> (len * 8), len);

I am kind of unhappy with adding recursion here but
it does seem to be the least of evils, and
it is rare (and can't happen on the pc).

> +
> +    if (addr & (len - 1)) {

Let's add an unlikely here.

> +        len >>= 1;

I know it's the same but I prefer len /= 2;

> +        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