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

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

On Tue, Jan 31, 2012 at 01:31:59PM -0700, Alex Williamson wrote:
> 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

No problem but motivation should probably go into commit log.

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