Re: [PATCH 5/9] pci-assign: Only calculate maximum MSI-X vector entries once

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 2012-01-31 at 22:18 +0200, Michael S. Tsirkin wrote:
> On Sat, Jan 28, 2012 at 07:22:04AM -0700, Alex Williamson wrote:
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> 
> Why? Optimization?

Because in 6/9 we'd have to calculate it again for resetting the msix
table and in 9/9 we use it to drop writes outside of the valid vector
range.  I suspect we might actually want to call msix_reset on device
reset, so that means we'd be recalculating it every time we reset the
device, write to the msix table, and again if we're actually updating
msix entries.  Redundancy exceeded my cost of two bytes metric.  Thanks,

Alex

> >  hw/device-assignment.c |   17 +++++++----------
> >  hw/device-assignment.h |    1 +
> >  2 files changed, 8 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> > index 422ee00..af614d3 100644
> > --- a/hw/device-assignment.c
> > +++ b/hw/device-assignment.c
> > @@ -971,20 +971,14 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev)
> >  static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
> >  {
> >      AssignedDevice *adev = DO_UPCAST(AssignedDevice, dev, pci_dev);
> > -    uint16_t entries_nr = 0, entries_max_nr;
> > -    int pos = 0, i, r = 0;
> > +    uint16_t entries_nr = 0;
> > +    int i, r = 0;
> >      struct kvm_assigned_msix_nr msix_nr;
> >      struct kvm_assigned_msix_entry msix_entry;
> >      MSIXTableEntry *entry = adev->msix_table;
> >  
> > -    pos = pci_find_capability(pci_dev, PCI_CAP_ID_MSIX);
> > -
> > -    entries_max_nr = *(uint16_t *)(pci_dev->config + pos + 2);
> > -    entries_max_nr &= PCI_MSIX_FLAGS_QSIZE;
> > -    entries_max_nr += 1;
> > -
> >      /* Get the usable entry number for allocating */
> > -    for (i = 0; i < entries_max_nr; i++, entry++) {
> > +    for (i = 0; i < adev->msix_max; i++, entry++) {
> >          /* Ignore unused entry even it's unmasked */
> >          if (entry->data == 0) {
> >              continue;
> > @@ -1012,7 +1006,7 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
> >      msix_entry.assigned_dev_id = msix_nr.assigned_dev_id;
> >      entries_nr = 0;
> >      entry = adev->msix_table;
> > -    for (i = 0; i < entries_max_nr; i++, entry++) {
> > +    for (i = 0; i < adev->msix_max; i++, entry++) {
> >          if (entries_nr >= msix_nr.entry_nr)
> >              break;
> >          if (entry->data == 0) {
> > @@ -1225,6 +1219,9 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
> >          bar_nr = msix_table_entry & PCI_MSIX_FLAGS_BIRMASK;
> >          msix_table_entry &= ~PCI_MSIX_FLAGS_BIRMASK;
> >          dev->msix_table_addr = pci_region[bar_nr].base_addr + msix_table_entry;
> > +        dev->msix_max = pci_get_word(pci_dev->config + pos + PCI_MSIX_FLAGS);
> > +        dev->msix_max &= PCI_MSIX_FLAGS_QSIZE;
> > +        dev->msix_max += 1;
> >      }
> >  
> >      /* Minimal PM support, nothing writable, device appears to NAK changes */
> > diff --git a/hw/device-assignment.h b/hw/device-assignment.h
> > index e229303..a909fb2 100644
> > --- a/hw/device-assignment.h
> > +++ b/hw/device-assignment.h
> > @@ -119,6 +119,7 @@ typedef struct AssignedDevice {
> >      struct kvm_irq_routing_entry *entry;
> >      MSIXTableEntry *msix_table;
> >      target_phys_addr_t msix_table_addr;
> > +    uint16_t msix_max;
> >      MemoryRegion mmio;
> >      char *configfd_name;
> >      int32_t bootindex;
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html