Re: [PATCH v2 3/3] xen/pci: add discovered PCI device at boot

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.08.2025 14:28, Mykyta Poturai wrote:
> From: Luca Fancellu <luca.fancellu@xxxxxxx>
> 
> In dom0less mode, there is no dom0 that can call PCI physdev ops to
> register PCI devices to iommu, so it needs to be done by Xen.
> pci_add_device requires some default domain, we don't have hwdom, and
> the guests are not yet created during the PCI init phase, so use dom_io
> as a temporary sentinel before devices are assigned to their target
> domains.
> 
> Rename setup_hwdom_pci_devices to setup_pci_devices and add dom0less
> handling to it.
> 
> In pci_add_device there is a call to xsm that doesn't consider the
> requester of the function to be Xen itself, so add a check to skip
> the call if the owner domain is dom_io, since it means the call is
> coming directly from Xen.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> ---
> (cherry picked from commit eff51e50021b75f5a50533f7de681b2ce044f5bd from
>  the downstream branch poc/pci-passthrough from
>  https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
> 
> v1->v2:
> * integrate add_discovered_pci_devices into existing routines
> * better explain the need for dom_io

What I continue to miss is an explanation of why devices can't go to their
ultimate "destination" domain right away (once those have been created),
i.e. why the dom_io intermediary is necessary in the first place.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -35,6 +35,7 @@
>  #include <xen/msi.h>
>  #include <xsm/xsm.h>
>  #include "ats.h"
> +#include "xen/dom0less-build.h"

No, please don't, at the very least not this way (using quotes rather than
angle brackets). I may guess that it's for is_dom0less_mode(), but even
then I wonder whether that declaration wouldn't better move elsewhere. It
simply feels somewhat wrong to include this header here.

> @@ -1181,19 +1185,21 @@ int __init scan_pci_devices(void)
>      return ret;
>  }
>  
> -struct setup_hwdom {
> +struct setup_ctxt {
>      struct domain *d;
>      int (*handler)(uint8_t devfn, struct pci_dev *pdev);
>  };
>  
> -static void __hwdom_init setup_one_hwdom_device(const struct setup_hwdom 
> *ctxt,
> +static void __hwdom_init setup_one_pci_device(const struct setup_ctxt *ctxt,
>                                                  struct pci_dev *pdev)

Nit: Indentation then also needds to change on this following line.

>  {
>      u8 devfn = pdev->devfn;
> -    int err;
> +    int err = 0;

This doesn't suffice, as ...

>      do {
> -        err = ctxt->handler(devfn, pdev);
> +        if ( ctxt->handler )
> +            err = ctxt->handler(devfn, pdev);
> +
>          if ( err )
>          {
>              printk(XENLOG_ERR "setup %pp for d%d failed (%d)\n",

... below here we may continue the loop even if we got an error. "err"
needs setting unconditionally in the loop body, and hence maybe better
with a conditional expression.

> @@ -1229,18 +1235,26 @@ static int __hwdom_init cf_check 
> _setup_hwdom_pci_devices(
>              if ( !pdev )
>                  continue;
>  
> +            if ( is_dom0less_mode() ) {

We're in a __hwdom_init function. You can't call an __init one from here.

Also nit (style): Brace placement.

> +                int ret = pci_add_device(pdev->seg, pdev->bus, pdev->devfn, 
> NULL,
> +                                         NUMA_NO_NODE, ctxt->d);
> +                if (ret)

Nit (style): Missing blanks.

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -220,9 +220,8 @@ int scan_pci_devices(void);
>  enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn);
>  int find_upstream_bridge(u16 seg, u8 *bus, u8 *devfn, u8 *secbus);
>  
> -void setup_hwdom_pci_devices(struct domain *d,
> -                             int (*handler)(uint8_t devfn,
> -                                            struct pci_dev *pdev));
> +void setup_pci_devices(struct domain *d, int (*handler)(uint8_t devfn,
> +                                                        struct pci_dev 
> *pdev));

I think in this case the 2nd parameter would better remain on the following
line, to limit overall indentation.

Jan