Re: [PATCH v2 10/15] pci: Use pci_sbdf_t in pci_add_device()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.06.2026 16:50, Teddy Astie wrote:
> Also take the opportunity to avoid refetching sbdf from pdev
> since we already have it now.

I question this; see below.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -661,12 +661,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: This line is too long now.

> @@ -700,16 +699,14 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>          pdev->info = *info;
>          if ( pdev->info.is_virtfn )
>          {
> -            struct pci_dev *pf_pdev =
> -                pci_get_pdev(NULL, PCI_SBDF(seg, info->physfn.bus,
> -                                            info->physfn.devfn));
> +            pci_sbdf_t pf_sbdf = PCI_SBDF(sbdf.seg, info->physfn.bus, 
> info->physfn.devfn);

As is this one.

> @@ -728,14 +725,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);

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.

> @@ -817,14 +814,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));

Why sbdf.bus but not sbdf.slot? In fact this is a case where altering sbdf may
help: You then wouldn't need the func local variable anymore either.

> --- a/xen/drivers/pci/physdev.c
> +++ b/xen/drivers/pci/physdev.c
> @@ -50,7 +50,7 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>          }
>  #endif
>  
> -        ret = pci_add_device(add.seg, add.bus, add.devfn, &pdev_info, node);
> +        ret = pci_add_device(PCI_SBDF(add.seg, add.bus, add.devfn), 
> &pdev_info, node);

Yet another overlong line.

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -229,8 +229,7 @@ void setup_hwdom_pci_devices(struct domain *d,
>  int pci_release_devices(struct domain *d);
>  int pci_add_segment(u16 seg);
>  const unsigned long *pci_get_ro_map(u16 seg);
> -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);

And again.

Jan