| Message ID | 20101112174716.3169.64085.stgit@s20.home |
|---|---|
| State | New |
| Headers | show |
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;
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@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. 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;
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;
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> --- hw/msix.c | 15 +++++++-------- hw/pci.c | 20 ++++++++++++++++++-- hw/pci.h | 5 +++-- 3 files changed, 28 insertions(+), 12 deletions(-)