On 20.08.2025 14:28, Mykyta Poturai wrote:
> From: Luca Fancellu <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Mykyta Poturai <[email protected]>
> ---
> (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
