[PATCH v1 1/3] arm/pci: Add pci-scan boot argument

[PATCH v1 1/3] arm/pci: Add pci-scan boot argument

[Mykyta Poturai]

From: Edward Pickup <Edward.Pickup@arm.com>

This patch adds a Xen boot arguments that, if enabled, causes a call to
existing code to scan pci devices enumerated by the firmware.

This patch also makes an existing debug function viewable outside its
translation unit, and uses this to dump the PCI devices found.
The debug message is controlled by config DEBUG.

Additionally, this patch modifies segment loading to ensure that PCI
devices on other segments are properly found.

This will be needed ahead of dom0less support for pci passthrough on
arm.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Edward Pickup <Edward.Pickup@arm.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
(cherry picked from commit bce463e1588a45e1bfdf59fc0d5f88b16604e439 from
 the downstream branch poc/pci-passthrough from
 https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)

changes since cherry-pick:
* s/always_inline/inline/
* replace additional kconfig option with config DEBUG

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
 docs/misc/xen-command-line.pandoc  |  7 +++++++
 xen/arch/arm/include/asm/pci.h     | 11 +++++++++++
 xen/arch/arm/pci/pci-host-common.c |  1 +
 xen/arch/arm/pci/pci.c             | 26 ++++++++++++++++++++++++--
 xen/drivers/passthrough/pci.c      |  2 +-
 xen/include/xen/pci.h              |  1 +
 6 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 6865a61220..72c122ea32 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2058,6 +2058,13 @@ This option can be specified more than once (up to 8 times at present).
 
 Flag to enable or disable support for PCI passthrough
 
+### pci-scan (arm)
+> `= <boolean>`
+
+> Default: `false`
+
+Flag to enable or disable Xen PCI scan at boot.
+
 ### pcid (x86)
 > `= <boolean> | xpti=<bool>`
 
diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 08ffcd4438..0afdc90de0 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -23,6 +23,7 @@
 #define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
 
 extern bool pci_passthrough_enabled;
+extern bool pci_scan_enabled;
 
 struct rangeset;
 
@@ -128,6 +129,11 @@ static always_inline bool is_pci_passthrough_enabled(void)
     return pci_passthrough_enabled;
 }
 
+static inline bool is_pci_scan_enabled(void)
+{
+    return pci_scan_enabled;
+}
+
 void arch_pci_init_pdev(struct pci_dev *pdev);
 
 int pci_get_new_domain_nr(void);
@@ -155,6 +161,11 @@ bool arch_pci_device_physdevop(void);
 
 #else   /*!CONFIG_HAS_PCI*/
 
+static inline bool is_pci_scan_enabled(void)
+{
+    return false;
+}
+
 struct pci_dev;
 
 static inline void arch_pci_init_pdev(struct pci_dev *pdev) {}
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index 487c545f3a..d3481b05eb 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -284,6 +284,7 @@ pci_host_common_probe(struct dt_device_node *dev,
     }
 
     pci_add_host_bridge(bridge);
+    pci_add_segment(bridge->segment);
 
     return bridge;
 
diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
index beb1f971fa..eea264db0e 100644
--- a/xen/arch/arm/pci/pci.c
+++ b/xen/arch/arm/pci/pci.c
@@ -91,8 +91,13 @@ bool arch_pci_device_physdevop(void)
 bool __read_mostly pci_passthrough_enabled;
 boolean_param("pci-passthrough", pci_passthrough_enabled);
 
+/* By default pci scan is disabled. */
+bool __read_mostly pci_scan_enabled;
+boolean_param("pci-scan", pci_scan_enabled);
+
 static int __init pci_init(void)
 {
+    int ret;
     /*
      * Enable PCI passthrough when has been enabled explicitly
      * (pci-passthrough=on).
@@ -104,9 +109,26 @@ static int __init pci_init(void)
         panic("Could not initialize PCI segment 0\n");
 
     if ( acpi_disabled )
-        return dt_pci_init();
+        ret = dt_pci_init();
     else
-        return acpi_pci_init();
+        ret = acpi_pci_init();
+
+    if ( ret < 0 )
+        return ret;
+
+    if ( is_pci_scan_enabled() )
+    {
+        ret = scan_pci_devices();
+
+        if ( ret < 0 )
+            return ret;
+
+#ifdef DEBUG
+        dump_pci_devices('c');
+#endif
+    }
+
+    return 0;
 }
 __initcall(pci_init);
 
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 3edcfa8a04..fa03dafac7 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1384,7 +1384,7 @@ static int cf_check _dump_pci_devices(struct pci_seg *pseg, void *arg)
     return 0;
 }
 
-static void cf_check dump_pci_devices(unsigned char ch)
+void cf_check dump_pci_devices(unsigned char ch)
 {
     printk("==== PCI devices ====\n");
     pcidevs_lock();
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 130c2a8c1a..5c242278b9 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -217,6 +217,7 @@ static always_inline bool pcidevs_trylock(void)
 bool pci_known_segment(u16 seg);
 bool pci_device_detect(u16 seg, u8 bus, u8 dev, u8 func);
 int scan_pci_devices(void);
+void dump_pci_devices(unsigned char ch);
 enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn);
 int find_upstream_bridge(u16 seg, u8 *bus, u8 *devfn, u8 *secbus);
 
-- 
2.34.1

[PATCH v1 0/3] dom0less pci passthrough support on Arm part 1

[Mykyta Poturai]

This series adds basic PCI device enumeration in Xen. This will allow us to not
rely on Dom0 enumeration for supported controllers, which will enable PCI
passthrough for dom0less setups.

Enumeration is disabled by default and can be enabled with "pci-scan" cmdline
option. Discovered devices are added to Xen and assigned to dom_io by default.

Edward Pickup (1):
  arm/pci: Add pci-scan boot argument

Luca Fancellu (2):
  xen/pci: modify pci_add_device to handle device add by Xen
  xen/pci: add discovered PCI device at boot

 docs/misc/xen-command-line.pandoc  |  7 ++++
 xen/arch/arm/include/asm/pci.h     | 11 ++++++
 xen/arch/arm/pci/pci-host-common.c |  1 +
 xen/arch/arm/pci/pci.c             | 27 ++++++++++++-
 xen/arch/x86/physdev.c             |  9 ++---
 xen/drivers/passthrough/pci.c      | 62 +++++++++++++++++++++++-------
 xen/drivers/pci/physdev.c          |  3 +-
 xen/include/xen/pci.h              |  4 +-
 8 files changed, 102 insertions(+), 22 deletions(-)

-- 
2.34.1

[PATCH v1 2/3] xen/pci: modify pci_add_device to handle device add by Xen

[Mykyta Poturai]

From: Luca Fancellu <luca.fancellu@arm.com>

Currently pci_add_device is called from hypercalls requested by Dom0
to add pci devices and when the device has no domain associated with
it, it is assumed that hardware_domain is the owner.

On the dom0less scenario, the enumeration is performed by the
firmware and Xen at boot time might want to assign some pci devices
to guests, so it has to firstly add the device and then assign it to
the final guest.

Modify pci_add_device to have the owner domain passed as a parameter
to the function, so that when it is called from the hypercall the
owner would be the hardware domain, otherwise when Xen is calling it,
it would be another domain since hw domain could not be there
(dom0less guests without Dom0 use case).

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@arm.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
(cherry picked from commit f0c85d9043f7c9402e85b73361c8a13c683428ca from
 the downstream branch poc/pci-passthrough from
 https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)

changes since cherry-pick:
* s/hardware_domain/d/ in some write_unlock calls in xen/drivers/passthrough/pci.c
---
 xen/arch/x86/physdev.c        |  9 ++++-----
 xen/drivers/passthrough/pci.c | 32 ++++++++++++++++++++------------
 xen/drivers/pci/physdev.c     |  3 ++-
 xen/include/xen/pci.h         |  2 +-
 4 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 4dfa1c0191..9e894ce665 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -446,8 +446,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( copy_from_guest(&manage_pci, arg, 1) != 0 )
             break;
 
-        ret = pci_add_device(0, manage_pci.bus, manage_pci.devfn,
-                             NULL, NUMA_NO_NODE);
+        ret = pci_add_device(hardware_domain, 0, manage_pci.bus,
+                             manage_pci.devfn, NULL, NUMA_NO_NODE);
         break;
     }
 
@@ -477,9 +477,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         pdev_info.is_virtfn = manage_pci_ext.is_virtfn;
         pdev_info.physfn.bus = manage_pci_ext.physfn.bus;
         pdev_info.physfn.devfn = manage_pci_ext.physfn.devfn;
-        ret = pci_add_device(0, manage_pci_ext.bus,
-                             manage_pci_ext.devfn,
-                             &pdev_info, NUMA_NO_NODE);
+        ret = pci_add_device(hardware_domain, 0, manage_pci_ext.bus,
+                             manage_pci_ext.devfn, &pdev_info, NUMA_NO_NODE);
         break;
     }
 
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index fa03dafac7..49a39d69db 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -654,7 +654,7 @@ 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,
+int pci_add_device(struct domain *d, u16 seg, u8 bus, u8 devfn,
                    const struct pci_dev_info *info, nodeid_t node)
 {
     struct pci_seg *pseg;
@@ -672,9 +672,12 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
     else
         type = "device";
 
-    ret = xsm_resource_plug_pci(XSM_PRIV, (seg << 16) | (bus << 8) | devfn);
-    if ( ret )
-        return ret;
+    if ( d != dom_io )
+    {
+        ret = xsm_resource_plug_pci(XSM_PRIV, (seg << 16) | (bus << 8) | devfn);
+        if ( ret )
+            return ret;
+    }
 
     ret = -ENOMEM;
 
@@ -767,9 +770,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
     ret = 0;
     if ( !pdev->domain )
     {
-        pdev->domain = hardware_domain;
-        write_lock(&hardware_domain->pci_lock);
-        list_add(&pdev->domain_list, &hardware_domain->pdev_list);
+        pdev->domain = d;
+        write_lock(&d->pci_lock);
+        list_add(&pdev->domain_list, &pdev->domain->pdev_list);
 
         /*
          * For devices not discovered by Xen during boot, add vPCI handlers
@@ -779,25 +782,30 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
         if ( ret )
         {
             list_del(&pdev->domain_list);
-            write_unlock(&hardware_domain->pci_lock);
+            write_unlock(&d->pci_lock);
             pdev->domain = NULL;
             printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
             goto out;
         }
-        write_unlock(&hardware_domain->pci_lock);
+        write_unlock(&d->pci_lock);
         ret = iommu_add_device(pdev);
         if ( ret )
         {
-            write_lock(&hardware_domain->pci_lock);
+            write_lock(&d->pci_lock);
             vpci_deassign_device(pdev);
             list_del(&pdev->domain_list);
-            write_unlock(&hardware_domain->pci_lock);
+            write_unlock(&d->pci_lock);
             pdev->domain = NULL;
             goto out;
         }
     }
-    else
+    else if ( pdev->domain == d )
         iommu_enable_device(pdev);
+    else
+    {
+        ret = -EINVAL;
+        goto out;
+    }
 
     pci_enable_acs(pdev);
 
diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
index d46501b884..1b59bf1246 100644
--- a/xen/drivers/pci/physdev.c
+++ b/xen/drivers/pci/physdev.c
@@ -50,7 +50,8 @@ 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(hardware_domain, add.seg, add.bus, add.devfn,
+                             &pdev_info, node);
         break;
     }
 
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 5c242278b9..77a44aea70 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -227,7 +227,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,
+int pci_add_device(struct domain *d, u16 seg, u8 bus, u8 devfn,
                    const struct pci_dev_info *info, nodeid_t node);
 int pci_remove_device(u16 seg, u8 bus, u8 devfn);
 int pci_ro_device(int seg, int bus, int devfn);
-- 
2.34.1

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

[Mykyta Poturai]

From: Luca Fancellu <luca.fancellu@arm.com>

Create add_discovered_pci_devices function that calls pci_device_add
on every PCI device discovered.
The devices will be added to dom_io so that they can be assigned
later to other domains.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
(cherry picked from commit eff51e50021b75f5a50533f7de681b2ce044f5bd from
 the downstream branch poc/pci-passthrough from
 https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
---
 xen/arch/arm/pci/pci.c        |  1 +
 xen/drivers/passthrough/pci.c | 28 ++++++++++++++++++++++++++++
 xen/include/xen/pci.h         |  1 +
 3 files changed, 30 insertions(+)

diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
index eea264db0e..b2426878ee 100644
--- a/xen/arch/arm/pci/pci.c
+++ b/xen/arch/arm/pci/pci.c
@@ -123,6 +123,7 @@ static int __init pci_init(void)
         if ( ret < 0 )
             return ret;
 
+        add_discovered_pci_devices();
 #ifdef DEBUG
         dump_pci_devices('c');
 #endif
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 49a39d69db..d6f1c78701 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1180,6 +1180,34 @@ int __init scan_pci_devices(void)
     return ret;
 }
 
+static int __init _add_discovered_pci_devices(struct pci_seg *pseg, void *arg)
+{
+    struct pci_dev *pdev;
+    int ret = 0;
+
+    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
+    {
+        ret = pci_add_device(dom_io, pdev->seg, pdev->bus, pdev->devfn, NULL,
+                             NUMA_NO_NODE);
+        if ( ret < 0 )
+        {
+            printk(XENLOG_ERR
+                   "%pp: Failure adding the discovered pci device (Error %d)\n",
+                   &pdev->sbdf, ret);
+            break;
+        }
+    }
+
+    return ret;
+}
+
+void __init add_discovered_pci_devices(void)
+{
+    pcidevs_lock();
+    pci_segments_iterate(_add_discovered_pci_devices, NULL);
+    pcidevs_unlock();
+}
+
 struct setup_hwdom {
     struct domain *d;
     int (*handler)(uint8_t devfn, struct pci_dev *pdev);
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 77a44aea70..81c0c23604 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -217,6 +217,7 @@ static always_inline bool pcidevs_trylock(void)
 bool pci_known_segment(u16 seg);
 bool pci_device_detect(u16 seg, u8 bus, u8 dev, u8 func);
 int scan_pci_devices(void);
+void add_discovered_pci_devices(void);
 void dump_pci_devices(unsigned char ch);
 enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn);
 int find_upstream_bridge(u16 seg, u8 *bus, u8 *devfn, u8 *secbus);
-- 
2.34.1

Re: [PATCH v1 1/3] arm/pci: Add pci-scan boot argument

[Jan Beulich]

On 01.08.2025 11:22, Mykyta Poturai wrote:
> From: Edward Pickup <Edward.Pickup@arm.com>
> 
> This patch adds a Xen boot arguments that, if enabled, causes a call to
> existing code to scan pci devices enumerated by the firmware.
> 
> This patch also makes an existing debug function viewable outside its
> translation unit, and uses this to dump the PCI devices found.
> The debug message is controlled by config DEBUG.
> 
> Additionally, this patch modifies segment loading to ensure that PCI
> devices on other segments are properly found.
> 
> This will be needed ahead of dom0less support for pci passthrough on
> arm.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Edward Pickup <Edward.Pickup@arm.com>

Considering the From: above and this order of S-o-b: Who is it really that
was the original author here?

> --- a/xen/arch/arm/include/asm/pci.h
> +++ b/xen/arch/arm/include/asm/pci.h
> @@ -23,6 +23,7 @@
>  #define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
>  
>  extern bool pci_passthrough_enabled;
> +extern bool pci_scan_enabled;

With the variable non-static, ...

> @@ -128,6 +129,11 @@ static always_inline bool is_pci_passthrough_enabled(void)
>      return pci_passthrough_enabled;
>  }
>  
> +static inline bool is_pci_scan_enabled(void)
> +{
> +    return pci_scan_enabled;
> +}
> +
>  void arch_pci_init_pdev(struct pci_dev *pdev);
>  
>  int pci_get_new_domain_nr(void);
> @@ -155,6 +161,11 @@ bool arch_pci_device_physdevop(void);
>  
>  #else   /*!CONFIG_HAS_PCI*/
>  
> +static inline bool is_pci_scan_enabled(void)
> +{
> +    return false;
> +}

... what's the point of the wrappers? Constrain the variable as such to
HAS_PCI=y, and use "#define pci_scan_enabled false" in the opposite case.
Just like we do elsewhere in a number of cases.

> --- a/xen/arch/arm/pci/pci.c
> +++ b/xen/arch/arm/pci/pci.c
> @@ -91,8 +91,13 @@ bool arch_pci_device_physdevop(void)
>  bool __read_mostly pci_passthrough_enabled;
>  boolean_param("pci-passthrough", pci_passthrough_enabled);
>  
> +/* By default pci scan is disabled. */
> +bool __read_mostly pci_scan_enabled;

__ro_after_init?

> +boolean_param("pci-scan", pci_scan_enabled);
> +
>  static int __init pci_init(void)
>  {
> +    int ret;
>      /*

Nit: Blank line please between declaration(s) and statement(s).

> @@ -104,9 +109,26 @@ static int __init pci_init(void)
>          panic("Could not initialize PCI segment 0\n");
>  
>      if ( acpi_disabled )
> -        return dt_pci_init();
> +        ret = dt_pci_init();
>      else
> -        return acpi_pci_init();
> +        ret = acpi_pci_init();
> +
> +    if ( ret < 0 )
> +        return ret;
> +
> +    if ( is_pci_scan_enabled() )
> +    {
> +        ret = scan_pci_devices();
> +
> +        if ( ret < 0 )
> +            return ret;
> +
> +#ifdef DEBUG
> +        dump_pci_devices('c');
> +#endif

If I was a maintainer of this code, I would request such to be dropped.
Or if there was a good reason to have such, I think it would want to be
arch-independent.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1384,7 +1384,7 @@ static int cf_check _dump_pci_devices(struct pci_seg *pseg, void *arg)
>      return 0;
>  }
>  
> -static void cf_check dump_pci_devices(unsigned char ch)
> +void cf_check dump_pci_devices(unsigned char ch)

Note the cf_check here. It, for some reason, ...

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -217,6 +217,7 @@ static always_inline bool pcidevs_trylock(void)
>  bool pci_known_segment(u16 seg);
>  bool pci_device_detect(u16 seg, u8 bus, u8 dev, u8 func);
>  int scan_pci_devices(void);
> +void dump_pci_devices(unsigned char ch);

... needs reproducing on the declaration. What about x86 though? It'll end up
as a non-static function with no caller outside of the defining CU, hence
violating some Misra rule.

Jan

Re: [PATCH v1 2/3] xen/pci: modify pci_add_device to handle device add by Xen

[Jan Beulich]

On 01.08.2025 11:22, Mykyta Poturai wrote:
> From: Luca Fancellu <luca.fancellu@arm.com>
> 
> Currently pci_add_device is called from hypercalls requested by Dom0
> to add pci devices and when the device has no domain associated with
> it, it is assumed that hardware_domain is the owner.
> 
> On the dom0less scenario, the enumeration is performed by the
> firmware and Xen at boot time might want to assign some pci devices
> to guests, so it has to firstly add the device and then assign it to
> the final guest.
> 
> Modify pci_add_device to have the owner domain passed as a parameter
> to the function, so that when it is called from the hypercall the
> owner would be the hardware domain, otherwise when Xen is calling it,
> it would be another domain since hw domain could not be there
> (dom0less guests without Dom0 use case).
> 
> 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.

I don't understand this particular aspect: All call sites pass
hardware_domain. Checking against dom_io therefore looks unnecessary, or
at least premature. In Misra's terms it is "dead code" that you add.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -654,7 +654,7 @@ 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,
> +int pci_add_device(struct domain *d, u16 seg, u8 bus, u8 devfn,
>                     const struct pci_dev_info *info, nodeid_t node)

For a pci_*() function I don't think it's appropriate to have a domain (not a
PCI domain, aka segment, which would be fine) as first parameter.

While you touch this it might also be nice if you switched u<N> to uint<N>_t.

> --- a/xen/drivers/pci/physdev.c
> +++ b/xen/drivers/pci/physdev.c
> @@ -50,7 +50,8 @@ 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(hardware_domain, add.seg, add.bus, add.devfn,
> +                             &pdev_info, node);

Here as well as in the x86-specific counterparts: Why hardware_domain, i.e.
why not current->domain? Yes, what you do is in line with how the code has
behaved so far, but now that you want to change it, it needs to be sorted
what it is that's really wanted. This btw also plays into the XSM aspect
that you're fiddling with.

Also, what if you end up passing NULL here in the dom0less case?

Jan

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

[Jan Beulich]

On 01.08.2025 11:22, Mykyta Poturai wrote:
> Create add_discovered_pci_devices function that calls pci_device_add
> on every PCI device discovered.
> The devices will be added to dom_io so that they can be assigned
> later to other domains.

And why's the intermediate step necessary? IOW can't they be assigned to their
target domains right away, and only whatever's left would go to DOM_IO?

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1180,6 +1180,34 @@ int __init scan_pci_devices(void)
>      return ret;
>  }
>  
> +static int __init _add_discovered_pci_devices(struct pci_seg *pseg, void *arg)
> +{
> +    struct pci_dev *pdev;
> +    int ret = 0;
> +
> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> +    {
> +        ret = pci_add_device(dom_io, pdev->seg, pdev->bus, pdev->devfn, NULL,
> +                             NUMA_NO_NODE);
> +        if ( ret < 0 )
> +        {
> +            printk(XENLOG_ERR
> +                   "%pp: Failure adding the discovered pci device (Error %d)\n",
> +                   &pdev->sbdf, ret);
> +            break;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +void __init add_discovered_pci_devices(void)
> +{
> +    pcidevs_lock();
> +    pci_segments_iterate(_add_discovered_pci_devices, NULL);
> +    pcidevs_unlock();
> +}

This looks to merely be a specialized form of what ...

>  struct setup_hwdom {
>      struct domain *d;
>      int (*handler)(uint8_t devfn, struct pci_dev *pdev);

... follows below here. By generalizing what we have (perhaps from the top, i.e.
iommu_hwdom_init()), you'd also avoid violating Misra rule 2.1 on x86, as you add
unreachable code there.

Jan

Re: [PATCH v1 1/3] arm/pci: Add pci-scan boot argument

[Luca Fancellu]

Hi Jan,

> On 4 Aug 2025, at 09:10, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 01.08.2025 11:22, Mykyta Poturai wrote:
>> From: Edward Pickup <Edward.Pickup@arm.com>
>> 
>> This patch adds a Xen boot arguments that, if enabled, causes a call to
>> existing code to scan pci devices enumerated by the firmware.
>> 
>> This patch also makes an existing debug function viewable outside its
>> translation unit, and uses this to dump the PCI devices found.
>> The debug message is controlled by config DEBUG.
>> 
>> Additionally, this patch modifies segment loading to ensure that PCI
>> devices on other segments are properly found.
>> 
>> This will be needed ahead of dom0less support for pci passthrough on
>> arm.
>> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> Signed-off-by: Edward Pickup <Edward.Pickup@arm.com>
> 
> Considering the From: above and this order of S-o-b: Who is it really that
> was the original author here?

I think this patch was mine, probably some issues from Edward, anyway he doesn’t work at arm anymore.

Cheers,
Luca

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

[Mykyta Poturai]

On 04.08.25 11:35, Jan Beulich wrote:
> On 01.08.2025 11:22, Mykyta Poturai wrote:
>> Create add_discovered_pci_devices function that calls pci_device_add
>> on every PCI device discovered.
>> The devices will be added to dom_io so that they can be assigned
>> later to other domains.
> 
> And why's the intermediate step necessary? IOW can't they be assigned to their
> target domains right away, and only whatever's left would go to DOM_IO?

For Dom0less case, guest domains are not yet created at this point.

>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -1180,6 +1180,34 @@ int __init scan_pci_devices(void)
>>       return ret;
>>   }
>>   
>> +static int __init _add_discovered_pci_devices(struct pci_seg *pseg, void *arg)
>> +{
>> +    struct pci_dev *pdev;
>> +    int ret = 0;
>> +
>> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>> +    {
>> +        ret = pci_add_device(dom_io, pdev->seg, pdev->bus, pdev->devfn, NULL,
>> +                             NUMA_NO_NODE);
>> +        if ( ret < 0 )
>> +        {
>> +            printk(XENLOG_ERR
>> +                   "%pp: Failure adding the discovered pci device (Error %d)\n",
>> +                   &pdev->sbdf, ret);
>> +            break;
>> +        }
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +void __init add_discovered_pci_devices(void)
>> +{
>> +    pcidevs_lock();
>> +    pci_segments_iterate(_add_discovered_pci_devices, NULL);
>> +    pcidevs_unlock();
>> +}
> 
> This looks to merely be a specialized form of what ...
> 
>>   struct setup_hwdom {
>>       struct domain *d;
>>       int (*handler)(uint8_t devfn, struct pci_dev *pdev);
> 
> ... follows below here. By generalizing what we have (perhaps from the top, i.e.
> iommu_hwdom_init()), you'd also avoid violating Misra rule 2.1 on x86, as you add
> unreachable code there.
> 
> Jan

Can you please elaborate a little further on how you see this 
generalization? With routines you mentioned being specifically for 
hwdom, which may not exist, and the different approaches to PCI 
initialization on Arm/x86 (as far as I understand on x86 all of the PCI 
related stuff is initialized from iommu helpers, while Arm has a 
dedicated init step) I am afraid I can't find a nice point of contact to 
generalize this, but I can be missing somthing due to unfamiliarity with 
x86 code.

Another way of addressing possible MISRA violation I can see is moving 
the add_discovered_pci_devices to xen/arch/arm/pci/pci.c so it will only 
be compiled when used.

-- 
Mykyta

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

[Jan Beulich]

On 12.08.2025 13:09, Mykyta Poturai wrote:
> On 04.08.25 11:35, Jan Beulich wrote:
>> On 01.08.2025 11:22, Mykyta Poturai wrote:
>>> Create add_discovered_pci_devices function that calls pci_device_add
>>> on every PCI device discovered.
>>> The devices will be added to dom_io so that they can be assigned
>>> later to other domains.
>>
>> And why's the intermediate step necessary? IOW can't they be assigned to their
>> target domains right away, and only whatever's left would go to DOM_IO?
> 
> For Dom0less case, guest domains are not yet created at this point.

I understand that, but this doesn't answer my question. Once the guest domains
are there, can't you directly assign their devices to them?

>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -1180,6 +1180,34 @@ int __init scan_pci_devices(void)
>>>       return ret;
>>>   }
>>>   
>>> +static int __init _add_discovered_pci_devices(struct pci_seg *pseg, void *arg)
>>> +{
>>> +    struct pci_dev *pdev;
>>> +    int ret = 0;
>>> +
>>> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>>> +    {
>>> +        ret = pci_add_device(dom_io, pdev->seg, pdev->bus, pdev->devfn, NULL,
>>> +                             NUMA_NO_NODE);
>>> +        if ( ret < 0 )
>>> +        {
>>> +            printk(XENLOG_ERR
>>> +                   "%pp: Failure adding the discovered pci device (Error %d)\n",
>>> +                   &pdev->sbdf, ret);
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +void __init add_discovered_pci_devices(void)
>>> +{
>>> +    pcidevs_lock();
>>> +    pci_segments_iterate(_add_discovered_pci_devices, NULL);
>>> +    pcidevs_unlock();
>>> +}
>>
>> This looks to merely be a specialized form of what ...
>>
>>>   struct setup_hwdom {
>>>       struct domain *d;
>>>       int (*handler)(uint8_t devfn, struct pci_dev *pdev);
>>
>> ... follows below here. By generalizing what we have (perhaps from the top, i.e.
>> iommu_hwdom_init()), you'd also avoid violating Misra rule 2.1 on x86, as you add
>> unreachable code there.
> 
> Can you please elaborate a little further on how you see this 
> generalization? With routines you mentioned being specifically for 
> hwdom, which may not exist,

That's precisely the generalization I'm having in mind. What's hwdom-only
now ought to be possible to be made usable in wider manner.

> and the different approaches to PCI 
> initialization on Arm/x86 (as far as I understand on x86 all of the PCI 
> related stuff is initialized from iommu helpers, while Arm has a 
> dedicated init step)

This, too, may want harmonizing.

> I am afraid I can't find a nice point of contact to 
> generalize this, but I can be missing somthing due to unfamiliarity with 
> x86 code.
> 
> Another way of addressing possible MISRA violation I can see is moving 
> the add_discovered_pci_devices to xen/arch/arm/pci/pci.c so it will only 
> be compiled when used.

And pose a problem when e.g. RISC-V also wants the same.

Jan