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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.08.2019 16:25, Roger Pau Monné  wrote:
> On Tue, Aug 06, 2019 at 03:10:40PM +0200, Jan Beulich wrote:
> > @@ -1093,22 +1094,17 @@ static void __pci_disable_msi(struct msi
> >    **/
> >   static int __pci_enable_msix(struct msi_info *msi, struct msi_desc **desc)
> >   {
> > -    int pos, nr_entries;
> >       struct pci_dev *pdev;
> > -    u16 control;
> >       u8 slot = PCI_SLOT(msi->devfn);
> >       u8 func = PCI_FUNC(msi->devfn);
> >       struct msi_desc *old_desc;
> Missing newline.
For one this is patch context only anyway. And then I'm afraid this is
an artifact of the ongoing email issue mentioned in the cover letter.
In the list archives this also reflects itself by ...

> >       ASSERT(pcidevs_locked());
... this line not having any indentation at all. Then again, looking
at the copy I have received back from xen-devel, I can't see any issue
there at all.

> > @@ -1187,16 +1183,10 @@ int pci_prepare_msix(u16 seg, u8 bus, u8
> >   {
> >       int rc;
> >       struct pci_dev *pdev;
> > -    u8 slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn);
> > -    unsigned int pos = pci_find_cap_offset(seg, bus, slot, func,
> > -                                           PCI_CAP_ID_MSIX);
> You seem to be missing an empty newline here?
>
> >       if ( !use_msi )
> >           return 0;
> Same here.
Same as above.

> > @@ -1209,13 +1199,7 @@ int pci_prepare_msix(u16 seg, u8 bus, u8
> >           rc = 0;
> >       }
> >       else
> > -    {
> > -        uint16_t control = pci_conf_read16(PCI_SBDF3(seg, bus, devfn),
> > -                                           msix_control_reg(pos));
> > -
> > -        rc = msix_capability_init(pdev, pos, NULL, NULL,
> > -                                  multi_msix_capable(control));
> > -    }
> > +        rc = msix_capability_init(pdev, NULL, NULL);
> >       pcidevs_unlock();
> >       return rc;
> > --- 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.

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

Jan

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