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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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