Re: [Xen-devel] [PATCH v5 08/10] x86/PCI: read MSI-X table entry count early

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Aug 06, 2019 at 04:47:28PM +0200, Jan Beulich wrote:
> On 06.08.2019 16:25, Roger Pau Monné  wrote:
> > On Tue, Aug 06, 2019 at 03:10:40PM +0200, Jan Beulich wrote:
> > > --- a/xen/drivers/passthrough/pci.c
> > > +++ b/xen/drivers/passthrough/pci.c
> > > @@ -324,6 +324,7 @@ static void apply_quirks(struct pci_dev
> > >   static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 
> > > devfn)
> > >   {
> > >       struct pci_dev *pdev;
> > > +    unsigned int pos;
> > 
> > This chunk doesn't seem to match my current code, as there's an empty
> > newline between the declarations and list_for_each_entry.
> 
> Same issue as above.

Sorry for the noise. I jumped straight into this patch.

> > > @@ -350,6 +353,10 @@ static struct pci_dev *ºalloc_pdev(struct
> > >               return NULL;
> > >           }
> > >           spin_lock_init(&msix->table_lock);
> > > +
> > > +        ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
> > > +        msix->nr_entries = msix_table_size(ctrl);
> > 
> > Since you store the number of entries here, why not also store the
> > position of the MSI-X capability since it's also immutable?
> > 
> > That would prevent having to fetch it again in msix_capability_init.
> 
> I do consider this as something worthwhile to do in the future, but
> not here: The field to store this doesn't exist in struct arch_msix
> (yet), and hence would likely want moving from struct msi_attrib.
> This is beyond the scope of this patch.

Oh I see. So the position it's actually stored in msi_attrib, and is
used by both MSI and MSI-X, in which case what I'm proposing would be
worse, since the field would only be used by MSI-X.

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel