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

[Alex Williamson]

On Sat, 2010-11-13 at 23:05 +0200, Michael S. Tsirkin wrote:
> 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 <address@hidden>
> 
> 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.

That's what I figured.  I'm not going to fight for this one, but I do
like the consistency of accessing all capabilities directly instead of
having a few that are more equal than the rest.  Can the benefit of
having msix_cap or msi_cap even be measured?  In my casual observations
with device assignment, those registers don't get touched enough to
warrant a special case either.  We've got a lot bigger problems if we
can't keep a couple tables in sync between an add and delete call.
Thanks,

Alex

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