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 <alex.williamson@xxxxxxxxxx>
>
> 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;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
