Re: [PATCH v3 02/18] pci: Use pci_sbdf_t in pci_add_device()

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 30/06/2026 à 13:44, Jan Beulich a écrit :
> On 29.06.2026 19:21, Teddy Astie wrote:
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -662,12 +662,11 @@ unsigned int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned 
> > int pos,
> >       return is64bits ? 2 : 1;
> >   }
> >   
> > -int pci_add_device(u16 seg, u8 bus, u8 devfn,
> > -                   const struct pci_dev_info *info, nodeid_t node)
> > +int pci_add_device(pci_sbdf_t sbdf, const struct pci_dev_info *info, nodeid_t 
> > node)
> Nit: Overlong line (like already pointed out for v2).
>
> >   {
> >       struct pci_seg *pseg;
> >       struct pci_dev *pdev;
> > -    unsigned int slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn);
> > +    unsigned int slot = sbdf.devfn, func = sbdf.fn;
> Andrew pointed out an issue here for v2, and you addressed only half of his
> comment.
Ah, I see, I used devfn instead of device.
I guess it would be better to avoid slot vs dev wording as it's easy to 
get wrong.
In this case, I think it would be even better to drop this variable as 
it's barely used (only used at the end for display as a alias for sbdf.dev).
> > @@ -729,14 +727,14 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> >               if ( !pdev->ext_cfg )
> >                   printk(XENLOG_WARNING
> >                          "%pp: VF without extended config space?\n",
> > -                       &pdev->sbdf);
> > +                       &sbdf);
> >           }
> >       }
> >   
> >       if ( !pdev->info.is_virtfn && !pdev->physfn.vf_rlen[0] )
> >       {
> >           unsigned int pos = pci_find_ext_capability(pdev, 
> > PCI_EXT_CAP_ID_SRIOV);
> > -        uint16_t ctrl = pci_conf_read16(pdev->sbdf, pos + PCI_SRIOV_CTRL);
> > +        uint16_t ctrl = pci_conf_read16(sbdf, pos + PCI_SRIOV_CTRL);
> Quoting from my v2 reply:
>
> "Are changes like these two actually worthwhile to make? sbdf, being a function
>   parameter, can be modified in the course of the function. pdev->sbdf, otoh,
>   cannot (for being in a const struct field). If further sbdf, throughout the
>   function, never had its address taken, the compiler may be able to produce
>   better code."
I missed it; though I'm not convinced that would work. While the sbdf 
field of struct pci_dev is const, the pointer to it isn't, so in the 
same regard, the compiler can assume that the pdev pointer changes thus 
can't rely on the const property of the pci_dev::sbdf field.
A better alternative would be to mark the sbdf function param const.

> > @@ -818,14 +816,14 @@ out:
> >       pcidevs_unlock();
> >       if ( !ret )
> >       {
> > -        printk(XENLOG_DEBUG "PCI add %s %pp\n", type, &pdev->sbdf);
> > +        printk(XENLOG_DEBUG "PCI add %s %pp\n", type, &sbdf);
> >           while ( pdev->phantom_stride )
> >           {
> >               func += pdev->phantom_stride;
> >               if ( PCI_SLOT(func) )
> >                   break;
> >               printk(XENLOG_DEBUG "PCI phantom %pp\n",
> > -                   &PCI_SBDF(seg, bus, slot, func));
> > +                   &PCI_SBDF(sbdf.seg, sbdf.bus, slot, func));
> My v2 remark here also wasn't addressed. Please can you make sure to address
> _all_ review comments before sending a new version?
>
> Jan
Teddy

Attachment: OpenPGP_0x660FA9D102CBCFD0.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature