qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Isaku Yamahata
Subject: Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses
Date: Wed, 20 Jul 2011 21:00:27 +0900
User-agent: Mutt/1.5.19 (2009-01-05)

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 <address@hidden>
> 
> 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 <address@hidden>
> ---
> 
> 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]