[Qemu-devel] Re: [PATCH v2 9/9] pci: Store capability offsets in PCIDevice

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

On Fri, Nov 12, 2010 at 10:47:21AM -0700, Alex Williamson wrote:
> This not only makes pci_find_capability a directly lookup, but also
> allows us to better track added capabilities and avoids the proliferation
> of random additional capability offset markers.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

There shouldn't be any need to store offsets
separately as find_capability gives you the value,
and duplicating same data in two places is bad
as we need to keep them in sync now.

We track offset to msi and msix capabilities as an optimization:
because they are used on data path. I can't see why would
we need to optimize any other capability like this.

> ---
> 
>  hw/msix.c |   15 +++++++--------
>  hw/pci.c  |   20 ++++++++++++++++++--
>  hw/pci.h  |    5 +++--
>  3 files changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index b98b34a..060f27b 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -204,7 +204,6 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
>          pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
>                       bar_nr);
>      }
> -    pdev->msix_cap = config_offset;
>      /* Make flags bit writeable. */
>      pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
>  	    MSIX_MASKALL_MASK;
> @@ -253,7 +252,8 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
>  
>  static int msix_function_masked(PCIDevice *dev)
>  {
> -    return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> +    return dev->config[dev->caps[PCI_CAP_ID_MSIX] +
> +                       MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
>  }
>  
>  static int msix_is_masked(PCIDevice *dev, int vector)
> @@ -275,7 +275,7 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector)
>  void msix_write_config(PCIDevice *dev, uint32_t addr,
>                         uint32_t val, int len)
>  {
> -    unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
> +    unsigned enable_pos = dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET;
>      int vector;
>  
>      if (!range_covers_byte(addr, len, enable_pos)) {
> @@ -334,7 +334,7 @@ static CPUReadMemoryFunc * const msix_mmio_read[] = {
>  void msix_mmio_map(PCIDevice *d, int region_num,
>                     pcibus_t addr, pcibus_t size, int type)
>  {
> -    uint8_t *config = d->config + d->msix_cap;
> +    uint8_t *config = d->config + d->caps[PCI_CAP_ID_MSIX];
>      uint32_t table = pci_get_long(config + MSIX_TABLE_OFFSET);
>      uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1);
>      /* TODO: for assigned devices, we'll want to make it possible to map
> @@ -437,7 +437,6 @@ int msix_uninit(PCIDevice *dev)
>      if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
>          return 0;
>      pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> -    dev->msix_cap = 0;
>      msix_free_irq_entries(dev);
>      dev->msix_entries_nr = 0;
>      cpu_unregister_io_memory(dev->msix_mmio_index);
> @@ -493,7 +492,7 @@ int msix_present(PCIDevice *dev)
>  int msix_enabled(PCIDevice *dev)
>  {
>      return (dev->cap_present & QEMU_PCI_CAP_MSIX) &&
> -        (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> +        (dev->config[dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET] &
>           MSIX_ENABLE_MASK);
>  }
>  
> @@ -534,8 +533,8 @@ void msix_reset(PCIDevice *dev)
>      if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
>          return;
>      msix_free_irq_entries(dev);
> -    dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
> -	    ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
> +    dev->config[dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET] &=
> +	    ~dev->wmask[dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET];
>      memset(dev->msix_table_page, 0, MSIX_PAGE_SIZE);
>      msix_mask_all(dev, dev->msix_entries_nr);
>  }
> diff --git a/hw/pci.c b/hw/pci.c
> index bc25be7..773afa5 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1990,15 +1990,24 @@ int pci_add_capability_at_offset(PCIDevice *pdev, uint8_t cap_id,
>  {
>      uint8_t i, *config = pdev->config + offset;
>  
> +    /* Check overlap with existing capabilities, valid cap, already added */
>      for (i = 0; i < size; i++) {
>          if (pdev->config_map[offset + i]) {
>              return -EFAULT;
>          }
>      }
>  
> +    if (!cap_id || cap_id > PCI_CAP_ID_MAX) {
> +        return -EINVAL;
> +    }
> +
> +    if (pdev->caps[cap_id]) {
> +        return -EFAULT;
> +    }
> +
>      config[PCI_CAP_LIST_ID] = cap_id;
>      config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
> -    pdev->config[PCI_CAPABILITY_LIST] = offset;
> +    pdev->caps[cap_id] = pdev->config[PCI_CAPABILITY_LIST] = offset;
>      memset(pdev->config_map + offset, cap_id, size);
>      /* Make capability read-only by default */
>      memset(pdev->wmask + offset, 0, size);
> @@ -2033,6 +2042,7 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
>      /* Clear cmask as device-specific registers can't be checked */
>      memset(pdev->cmask + offset, 0, size);
>      memset(pdev->config_map + offset, 0, size);
> +    pdev->caps[cap_id] = 0;
>  
>      if (!pdev->config[PCI_CAPABILITY_LIST]) {
>          pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
> @@ -2041,7 +2051,13 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
>  
>  uint8_t pci_find_capability(PCIDevice *pdev, uint8_t cap_id)
>  {
> -    return pci_find_capability_list(pdev, cap_id, NULL);
> +    if (cap_id == PCI_CAP_ID_BASIC) {
> +        return 0;
> +    }
> +    if (cap_id && cap_id <= PCI_CAP_ID_MAX) {
> +        return pdev->caps[cap_id];
> +    }
> +    return 0xff;
>  }
>  
>  static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
> diff --git a/hw/pci.h b/hw/pci.h
> index cea1c3a..b4c19ba 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -110,6 +110,7 @@ typedef struct PCIIORegion {
>  #define PCI_NUM_PINS 4 /* A-D */
>  
>  #define PCI_CAP_ID_BASIC 0xff
> +#define PCI_CAP_ID_MAX PCI_CAP_ID_AF
>  
>  /* Bits in cap_present field. */
>  enum {
> @@ -170,8 +171,8 @@ struct PCIDevice {
>      /* Capability bits */
>      uint32_t cap_present;
>  
> -    /* Offset of MSI-X capability in config space */
> -    uint8_t msix_cap;
> +    /* Offset capabilities in config space */
> +    uint8_t caps[PCI_CAP_ID_MAX + 1];
>  
>      /* MSI-X entries */
>      int msix_entries_nr;