* [PATCH v9 1/8] iommu/arm: Add iommu_dt_xlate()
2025-03-14 13:34 [PATCH v9 0/8] SMMU handling for PCIe Passthrough on ARM Mykyta Poturai
@ 2025-03-14 13:34 ` Mykyta Poturai
2025-04-22 0:39 ` Stefano Stabellini
2025-03-14 13:34 ` [PATCH v9 2/8] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API Mykyta Poturai
` (6 subsequent siblings)
7 siblings, 1 reply; 37+ messages in thread
From: Mykyta Poturai @ 2025-03-14 13:34 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Oleksandr Tyshchenko, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Stewart Hildebrand,
Mykyta Poturai, Julien Grall
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Move code for processing DT IOMMU specifier to a separate helper.
This helper will be re-used for adding PCI devices by the subsequent
patches as we will need exact the same actions for processing
DT PCI-IOMMU specifier.
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
v8->v9:
* remove DT_NO_IOMMU
v7->v8:
* explain NO_IOMMU better and rename to DT_NO_IOMMU
v6->v7:
* explained NO_IOMMU in comments
v5->v6:
* pass ops parameter to iommu_dt_xlate()
* add Julien's R-b
v4->v5:
* rebase on top of "dynamic node programming using overlay dtbo" series
* move #define NO_IOMMU 1 to header
* s/these/this/ inside comment
v3->v4:
* make dt_phandle_args *iommu_spec const
* move !ops->add_device check to helper
v2->v3:
* no change
v1->v2:
* no change
downstream->v1:
* trivial rebase
* s/dt_iommu_xlate/iommu_dt_xlate/
(cherry picked from commit c26bab0415ca303df86aba1d06ef8edc713734d3 from
the downstream branch poc/pci-passthrough from
https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
---
xen/drivers/passthrough/device_tree.c | 42 +++++++++++++++++----------
1 file changed, 27 insertions(+), 15 deletions(-)
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 075fb25a37..4a1971c3fc 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -137,6 +137,30 @@ int iommu_release_dt_devices(struct domain *d)
return 0;
}
+static int iommu_dt_xlate(struct device *dev,
+ const struct dt_phandle_args *iommu_spec,
+ const struct iommu_ops *ops)
+{
+ int rc;
+
+ if ( !ops->dt_xlate )
+ return -EINVAL;
+
+ if ( !dt_device_is_available(iommu_spec->np) )
+ return 1;
+
+ rc = iommu_fwspec_init(dev, &iommu_spec->np->dev);
+ if ( rc )
+ return rc;
+
+ /*
+ * Provide DT IOMMU specifier which describes the IOMMU master
+ * interfaces of that device (device IDs, etc) to the driver.
+ * The driver is responsible to decide how to interpret them.
+ */
+ return ops->dt_xlate(dev, iommu_spec);
+}
+
int iommu_remove_dt_device(struct dt_device_node *np)
{
const struct iommu_ops *ops = iommu_get_ops();
@@ -215,27 +239,15 @@ int iommu_add_dt_device(struct dt_device_node *np)
{
/*
* The driver which supports generic IOMMU DT bindings must have
- * these callback implemented.
+ * this callback implemented.
*/
- if ( !ops->add_device || !ops->dt_xlate )
+ if ( !ops->add_device )
{
rc = -EINVAL;
goto fail;
}
- if ( !dt_device_is_available(iommu_spec.np) )
- break;
-
- rc = iommu_fwspec_init(dev, &iommu_spec.np->dev);
- if ( rc )
- break;
-
- /*
- * Provide DT IOMMU specifier which describes the IOMMU master
- * interfaces of that device (device IDs, etc) to the driver.
- * The driver is responsible to decide how to interpret them.
- */
- rc = ops->dt_xlate(dev, &iommu_spec);
+ rc = iommu_dt_xlate(dev, &iommu_spec, ops);
if ( rc )
break;
--
2.34.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v9 1/8] iommu/arm: Add iommu_dt_xlate()
2025-03-14 13:34 ` [PATCH v9 1/8] iommu/arm: Add iommu_dt_xlate() Mykyta Poturai
@ 2025-04-22 0:39 ` Stefano Stabellini
0 siblings, 0 replies; 37+ messages in thread
From: Stefano Stabellini @ 2025-04-22 0:39 UTC (permalink / raw)
To: Mykyta Poturai
Cc: xen-devel@lists.xenproject.org, Oleksandr Tyshchenko,
Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Stewart Hildebrand, Julien Grall
On Fri, 14 Mar 2025, Mykyta Poturai wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> Move code for processing DT IOMMU specifier to a separate helper.
> This helper will be re-used for adding PCI devices by the subsequent
> patches as we will need exact the same actions for processing
> DT PCI-IOMMU specifier.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> Reviewed-by: Julien Grall <jgrall@amazon.com>
Committed
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v9 2/8] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API
2025-03-14 13:34 [PATCH v9 0/8] SMMU handling for PCIe Passthrough on ARM Mykyta Poturai
2025-03-14 13:34 ` [PATCH v9 1/8] iommu/arm: Add iommu_dt_xlate() Mykyta Poturai
@ 2025-03-14 13:34 ` Mykyta Poturai
2025-03-17 14:56 ` Jan Beulich
2025-03-14 13:34 ` [PATCH v9 3/8] iommu/arm: iommu_add_dt_pci_sideband_ids phantom handling Mykyta Poturai
` (5 subsequent siblings)
7 siblings, 1 reply; 37+ messages in thread
From: Mykyta Poturai @ 2025-03-14 13:34 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Oleksandr Tyshchenko, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Jan Beulich, Roger Pau Monné,
Stewart Hildebrand, Mykyta Poturai
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
The main purpose of this patch is to add a way to register PCI device
(which is behind the IOMMU) using the generic PCI-IOMMU DT bindings [1]
before assigning that device to a domain.
This behaves similarly to the existing iommu_add_dt_device API, except it
handles PCI devices, and it is to be invoked from the add_device hook in the
SMMU driver.
The function dt_map_id to translate an ID through a downstream mapping
(which is also suitable for mapping Requester ID) was borrowed from Linux
(v5.10-rc6) and updated according to the Xen code base.
[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
Regarding pci_for_each_dma_alias question: getting host bridge node
directly seems like a simpler solution with the same result. AFAIU
with pci_for_each_dma_alias in linux we would arrive to the host brige
node anyway, but also try to call dt_map_id for each device along the
way. I am not sure why exactly it is done this way in linux, as
according to the pci-iommu.txt, iommu-map node can only be present in
the PCI root.
v8->v9:
* replace DT_NO_IOMMU with 1
* guard iommu_add_pci_sideband_ids with CONFIG_ARM
v7->v8:
* ENOSYS->EOPNOTSUPP
* move iommu_add_pci_sideband_ids to iommu.c to fix x86 build
* simplify iommu_add_pci_sideband_ids
* add docstrings to iommu_add_{dt_}pci_sideband_ids
v6->v7:
* put iommu_add_pci_sideband_ids under ifdef
* remove ifdef CONFIG_APCI
* style: add newline for symmetry
v5->v6:
* pass ops to iommu_dt_xlate()
v4->v5:
* style: add newlines after variable declarations and before return in iommu.h
* drop device_is_protected() check in iommu_add_dt_pci_sideband_ids()
* rebase on top of ("dynamic node programming using overlay dtbo") series
* fix typo in commit message
* remove #ifdef around dt_map_id() prototype
* move dt_map_id() to xen/common/device_tree.c
* add function name in error prints
* use dprintk for debug prints
* use GENMASK and #include <xen/bitops.h>
* fix typo in comment
* remove unnecessary (int) cast in loop condition
* assign *id_out and return success in case of no translation in dt_map_id()
* don't initialize local variable unnecessarily
* return error in case of ACPI/no DT in iommu_add_{dt_}pci_sideband_ids()
v3->v4:
* wrap #include <asm/acpi.h> and if ( acpi_disabled ) in #ifdef CONFIG_ACPI
* fix Michal's remarks about style, parenthesis, and print formats
* remove !ops->dt_xlate check since it is already in iommu_dt_xlate helper
* rename s/iommu_dt_pci_map_id/dt_map_id/ because it is generic, not specific
to iommu
* update commit description
v2->v3:
* new patch title (was: iommu/arm: Introduce iommu_add_dt_pci_device API)
* renamed function
from: iommu_add_dt_pci_device
to: iommu_add_dt_pci_sideband_ids
* removed stale ops->add_device check
* iommu.h: add empty stub iommu_add_dt_pci_sideband_ids for !HAS_DEVICE_TREE
* iommu.h: add iommu_add_pci_sideband_ids helper
* iommu.h: don't wrap prototype in #ifdef CONFIG_HAS_PCI
* s/iommu_fwspec_free(pci_to_dev(pdev))/iommu_fwspec_free(dev)/
v1->v2:
* remove extra devfn parameter since pdev fully describes the device
* remove ops->add_device() call from iommu_add_dt_pci_device(). Instead, rely on
the existing iommu call in iommu_add_device().
* move the ops->add_device and ops->dt_xlate checks earlier
downstream->v1:
* rebase
* add const qualifier to struct dt_device_node *np arg in dt_map_id()
* add const qualifier to struct dt_device_node *np declaration in iommu_add_pci_device()
* use stdint.h types instead of u8/u32/etc...
* rename functions:
s/dt_iommu_xlate/iommu_dt_xlate/
s/dt_map_id/iommu_dt_pci_map_id/
s/iommu_add_pci_device/iommu_add_dt_pci_device/
* add device_is_protected check in iommu_add_dt_pci_device
* wrap prototypes in CONFIG_HAS_PCI
(cherry picked from commit 734e3bf6ee77e7947667ab8fa96c25b349c2e1da from
the downstream branch poc/pci-passthrough from
https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
---
xen/common/device-tree/device-tree.c | 91 +++++++++++++++++++++++++++
xen/drivers/passthrough/device_tree.c | 42 +++++++++++++
xen/drivers/passthrough/iommu.c | 15 +++++
xen/include/xen/device_tree.h | 23 +++++++
xen/include/xen/iommu.h | 40 +++++++++++-
5 files changed, 210 insertions(+), 1 deletion(-)
diff --git a/xen/common/device-tree/device-tree.c b/xen/common/device-tree/device-tree.c
index d0528c5825..3de7858df6 100644
--- a/xen/common/device-tree/device-tree.c
+++ b/xen/common/device-tree/device-tree.c
@@ -10,6 +10,7 @@
* published by the Free Software Foundation.
*/
+#include <xen/bitops.h>
#include <xen/types.h>
#include <xen/init.h>
#include <xen/guest_access.h>
@@ -2243,6 +2244,96 @@ int dt_get_pci_domain_nr(struct dt_device_node *node)
return (u16)domain;
}
+int dt_map_id(const struct dt_device_node *np, uint32_t id,
+ const char *map_name, const char *map_mask_name,
+ struct dt_device_node **target, uint32_t *id_out)
+{
+ uint32_t map_mask, masked_id, map_len;
+ const __be32 *map = NULL;
+
+ if ( !np || !map_name || (!target && !id_out) )
+ return -EINVAL;
+
+ map = dt_get_property(np, map_name, &map_len);
+ if ( !map )
+ {
+ if ( target )
+ return -ENODEV;
+
+ /* Otherwise, no map implies no translation */
+ *id_out = id;
+ return 0;
+ }
+
+ if ( !map_len || (map_len % (4 * sizeof(*map))) )
+ {
+ printk(XENLOG_ERR "%s(): %s: Error: Bad %s length: %u\n", __func__,
+ np->full_name, map_name, map_len);
+ return -EINVAL;
+ }
+
+ /* The default is to select all bits. */
+ map_mask = GENMASK(31, 0);
+
+ /*
+ * Can be overridden by "{iommu,msi}-map-mask" property.
+ * If dt_property_read_u32() fails, the default is used.
+ */
+ if ( map_mask_name )
+ dt_property_read_u32(np, map_mask_name, &map_mask);
+
+ masked_id = map_mask & id;
+ for ( ; map_len > 0; map_len -= 4 * sizeof(*map), map += 4 )
+ {
+ struct dt_device_node *phandle_node;
+ uint32_t id_base = be32_to_cpup(map + 0);
+ uint32_t phandle = be32_to_cpup(map + 1);
+ uint32_t out_base = be32_to_cpup(map + 2);
+ uint32_t id_len = be32_to_cpup(map + 3);
+
+ if ( id_base & ~map_mask )
+ {
+ printk(XENLOG_ERR "%s(): %s: Invalid %s translation - %s-mask (0x%"PRIx32") ignores id-base (0x%"PRIx32")\n",
+ __func__, np->full_name, map_name, map_name, map_mask,
+ id_base);
+ return -EFAULT;
+ }
+
+ if ( (masked_id < id_base) || (masked_id >= (id_base + id_len)) )
+ continue;
+
+ phandle_node = dt_find_node_by_phandle(phandle);
+ if ( !phandle_node )
+ return -ENODEV;
+
+ if ( target )
+ {
+ if ( !*target )
+ *target = phandle_node;
+
+ if ( *target != phandle_node )
+ continue;
+ }
+
+ if ( id_out )
+ *id_out = masked_id - id_base + out_base;
+
+ dprintk(XENLOG_DEBUG, "%s: %s, using mask %08"PRIx32", id-base: %08"PRIx32", out-base: %08"PRIx32", length: %08"PRIx32", id: %08"PRIx32" -> %08"PRIx32"\n",
+ np->full_name, map_name, map_mask, id_base, out_base, id_len, id,
+ masked_id - id_base + out_base);
+ return 0;
+ }
+
+ dprintk(XENLOG_DEBUG, "%s: no %s translation for id 0x%"PRIx32" on %s\n",
+ np->full_name, map_name, id,
+ (target && *target) ? (*target)->full_name : NULL);
+
+ if ( id_out )
+ *id_out = id;
+
+ return 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 4a1971c3fc..37e1437b65 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -161,6 +161,48 @@ static int iommu_dt_xlate(struct device *dev,
return ops->dt_xlate(dev, iommu_spec);
}
+#ifdef CONFIG_HAS_PCI
+int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
+{
+ const struct iommu_ops *ops = iommu_get_ops();
+ struct dt_phandle_args iommu_spec = { .args_count = 1 };
+ struct device *dev = pci_to_dev(pdev);
+ const struct dt_device_node *np;
+ int rc;
+
+ if ( !iommu_enabled )
+ return 1;
+
+ if ( !ops )
+ return -EINVAL;
+
+ if ( dev_iommu_fwspec_get(dev) )
+ return -EEXIST;
+
+ np = pci_find_host_bridge_node(pdev);
+ if ( !np )
+ return -ENODEV;
+
+ /*
+ * According to the Documentation/devicetree/bindings/pci/pci-iommu.txt
+ * from Linux.
+ */
+ rc = dt_map_id(np, PCI_BDF(pdev->bus, pdev->devfn), "iommu-map",
+ "iommu-map-mask", &iommu_spec.np, iommu_spec.args);
+ if ( rc )
+ return (rc == -ENODEV) ? 1 : rc;
+
+ rc = iommu_dt_xlate(dev, &iommu_spec, ops);
+ if ( rc < 0 )
+ {
+ iommu_fwspec_free(dev);
+ return -EINVAL;
+ }
+
+ return rc;
+}
+#endif /* CONFIG_HAS_PCI */
+
int iommu_remove_dt_device(struct dt_device_node *np)
{
const struct iommu_ops *ops = iommu_get_ops();
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 9e74a1fc72..dfaca67302 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -20,6 +20,7 @@
#include <xen/param.h>
#include <xen/softirq.h>
#include <xen/keyhandler.h>
+#include <xen/acpi.h>
#include <xsm/xsm.h>
#ifdef CONFIG_X86
@@ -744,6 +745,20 @@ int __init iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
return 0;
}
+#ifdef CONFIG_ARM
+int iommu_add_pci_sideband_ids(struct pci_dev *pdev)
+{
+ int ret = -EOPNOTSUPP;
+
+#ifdef CONFIG_HAS_PCI
+ if ( acpi_disabled )
+ ret = iommu_add_dt_pci_sideband_ids(pdev);
+#endif
+
+ return ret;
+}
+#endif
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 5ff763bb80..9254204af6 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -946,6 +946,29 @@ int dt_count_phandle_with_args(const struct dt_device_node *np,
*/
int dt_get_pci_domain_nr(struct dt_device_node *node);
+/**
+ * dt_map_id - Translate an ID through a downstream mapping.
+ * @np: root complex device node.
+ * @id: device ID to map.
+ * @map_name: property name of the map to use.
+ * @map_mask_name: optional property name of the mask to use.
+ * @target: optional pointer to a target device node.
+ * @id_out: optional pointer to receive the translated ID.
+ *
+ * Given a device ID, look up the appropriate implementation-defined
+ * platform ID and/or the target device which receives transactions on that
+ * ID, as per the "iommu-map" and "msi-map" bindings. Either of @target or
+ * @id_out may be NULL if only the other is required. If @target points to
+ * a non-NULL device node pointer, only entries targeting that node will be
+ * matched; if it points to a NULL value, it will receive the device node of
+ * the first matching target phandle, with a reference held.
+ *
+ * Return: 0 on success or a standard error code on failure.
+ */
+int dt_map_id(const struct dt_device_node *np, uint32_t id,
+ const char *map_name, const char *map_mask_name,
+ struct dt_device_node **target, uint32_t *id_out);
+
struct dt_device_node *dt_find_node_by_phandle(dt_phandle handle);
#ifdef CONFIG_DEVICE_TREE_DEBUG
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index b928c67e19..82319016a1 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -215,7 +215,8 @@ int iommu_dt_domain_init(struct domain *d);
int iommu_release_dt_devices(struct domain *d);
/*
- * Helper to add master device to the IOMMU using generic IOMMU DT bindings.
+ * Helpers to add master device to the IOMMU using generic (PCI-)IOMMU
+ * DT bindings.
*
* Return values:
* 0 : device is protected by an IOMMU
@@ -225,6 +226,19 @@ int iommu_release_dt_devices(struct domain *d);
*/
int iommu_add_dt_device(struct dt_device_node *np);
+/*
+ * Fills out the device's IOMMU fwspec with IOMMU ids from the DT.
+ * Ids are specified in the iommu-map property in the host bridge node.
+ * More information on the iommu-map property format can be found in
+ * Documentation/devicetree/bindings/pci/pci-iommu.txt from Linux.
+ *
+ * Return values:
+ * 0 : iommu_fwspec is filled out successfully.
+ * <0 : error while filling out the iommu_fwspec.
+ * >0 : IOMMU is not enabled/present or device is not connected to it.
+ */
+int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev);
+
int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
@@ -238,8 +252,32 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
*/
int iommu_remove_dt_device(struct dt_device_node *np);
+#else /* !HAS_DEVICE_TREE */
+static inline int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
+{
+ return -EOPNOTSUPP;
+}
+
#endif /* HAS_DEVICE_TREE */
+/*
+ * This function is not strictly ARM-specific, but it is only used by ARM
+ * as of now. So wrap it with ifdef to avoid creating dead code on other
+ * architectures. When usage is extended to other architectures, this ifdef
+ * should be removed.
+ */
+#ifdef CONFIG_ARM
+/*
+ * Fills out the device's IOMMU fwspec with IOMMU ids.
+ *
+ * Return values:
+ * 0 : iommu_fwspec is filled out successfully.
+ * <0 : error while filling out the iommu_fwspec.
+ * >0 : IOMMU is not enabled/present or device is not connected to it.
+ */
+int iommu_add_pci_sideband_ids(struct pci_dev *pdev);
+#endif
+
struct page_info;
/*
--
2.34.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v9 2/8] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API
2025-03-14 13:34 ` [PATCH v9 2/8] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API Mykyta Poturai
@ 2025-03-17 14:56 ` Jan Beulich
2025-03-19 15:21 ` Mykyta Poturai
0 siblings, 1 reply; 37+ messages in thread
From: Jan Beulich @ 2025-03-17 14:56 UTC (permalink / raw)
To: Mykyta Poturai
Cc: Oleksandr Tyshchenko, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Roger Pau Monné,
Stewart Hildebrand, xen-devel@lists.xenproject.org
On 14.03.2025 14:34, Mykyta Poturai wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> The main purpose of this patch is to add a way to register PCI device
> (which is behind the IOMMU) using the generic PCI-IOMMU DT bindings [1]
> before assigning that device to a domain.
>
> This behaves similarly to the existing iommu_add_dt_device API, except it
> handles PCI devices, and it is to be invoked from the add_device hook in the
> SMMU driver.
>
> The function dt_map_id to translate an ID through a downstream mapping
> (which is also suitable for mapping Requester ID) was borrowed from Linux
> (v5.10-rc6) and updated according to the Xen code base.
>
> [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> ---
> Regarding pci_for_each_dma_alias question: getting host bridge node
> directly seems like a simpler solution with the same result. AFAIU
> with pci_for_each_dma_alias in linux we would arrive to the host brige
> node anyway, but also try to call dt_map_id for each device along the
> way. I am not sure why exactly it is done this way in linux, as
> according to the pci-iommu.txt, iommu-map node can only be present in
> the PCI root.
>
> v8->v9:
> * replace DT_NO_IOMMU with 1
> * guard iommu_add_pci_sideband_ids with CONFIG_ARM
I fear I'm confused: Isn't this contradicting ...
> v7->v8:
> * ENOSYS->EOPNOTSUPP
> * move iommu_add_pci_sideband_ids to iommu.c to fix x86 build
... this earlier change? Really, with there being no caller, I can't see
why there could be any build issue here affecting only x86. Except for
Misra complaining about unreachable code being introduced, which I'm sure
I said before should be avoided.
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -20,6 +20,7 @@
> #include <xen/param.h>
> #include <xen/softirq.h>
> #include <xen/keyhandler.h>
> +#include <xen/acpi.h>
> #include <xsm/xsm.h>
>
> #ifdef CONFIG_X86
> @@ -744,6 +745,20 @@ int __init iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
> return 0;
> }
>
> +#ifdef CONFIG_ARM
I realize we have CONFIG_X86 here as well (visible even in context of the
earlier hunk. Yet then the goal ought to be to reduce these anomalies, not
add new ones. Since I don't have a clear picture of what's wanted, I'm also
in trouble suggesting any alternative, I'm afraid.
> +int iommu_add_pci_sideband_ids(struct pci_dev *pdev)
> +{
> + int ret = -EOPNOTSUPP;
> +
> +#ifdef CONFIG_HAS_PCI
> + if ( acpi_disabled )
> + ret = iommu_add_dt_pci_sideband_ids(pdev);
> +#endif
> +
> + return ret;
> +}
> +#endif
> +
> /*
> * Local variables:
> * mode: C
Having reached the end of the changes to this file, it's also not quite clear
why xen/acpi.h would need including uniformly. Can't this live inside the
outer #ifdef?
Further, since #ifdef-ary is used here already anyway, why ...
> @@ -238,8 +252,32 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
> */
> int iommu_remove_dt_device(struct dt_device_node *np);
>
> +#else /* !HAS_DEVICE_TREE */
> +static inline int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> #endif /* HAS_DEVICE_TREE */
... resort to a stub when the inner #ifdef could simple be extended?
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v9 2/8] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API
2025-03-17 14:56 ` Jan Beulich
@ 2025-03-19 15:21 ` Mykyta Poturai
2025-03-19 15:28 ` Jan Beulich
0 siblings, 1 reply; 37+ messages in thread
From: Mykyta Poturai @ 2025-03-19 15:21 UTC (permalink / raw)
To: Jan Beulich
Cc: Oleksandr Tyshchenko, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Roger Pau Monné,
Stewart Hildebrand, xen-devel@lists.xenproject.org
On 17.03.25 16:56, Jan Beulich wrote:
> On 14.03.2025 14:34, Mykyta Poturai wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> The main purpose of this patch is to add a way to register PCI device
>> (which is behind the IOMMU) using the generic PCI-IOMMU DT bindings [1]
>> before assigning that device to a domain.
>>
>> This behaves similarly to the existing iommu_add_dt_device API, except it
>> handles PCI devices, and it is to be invoked from the add_device hook in the
>> SMMU driver.
>>
>> The function dt_map_id to translate an ID through a downstream mapping
>> (which is also suitable for mapping Requester ID) was borrowed from Linux
>> (v5.10-rc6) and updated according to the Xen code base.
>>
>> [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
>> ---
>> Regarding pci_for_each_dma_alias question: getting host bridge node
>> directly seems like a simpler solution with the same result. AFAIU
>> with pci_for_each_dma_alias in linux we would arrive to the host brige
>> node anyway, but also try to call dt_map_id for each device along the
>> way. I am not sure why exactly it is done this way in linux, as
>> according to the pci-iommu.txt, iommu-map node can only be present in
>> the PCI root.
>>
>> v8->v9:
>> * replace DT_NO_IOMMU with 1
>> * guard iommu_add_pci_sideband_ids with CONFIG_ARM
>
> I fear I'm confused: Isn't this contradicting ...
>
>> v7->v8:
>> * ENOSYS->EOPNOTSUPP
>> * move iommu_add_pci_sideband_ids to iommu.c to fix x86 build
>
> ... this earlier change? Really, with there being no caller, I can't see
> why there could be any build issue here affecting only x86. Except for
> Misra complaining about unreachable code being introduced, which I'm sure
> I said before should be avoided.
The original reason for moving this function was the conflicting ACPI
and EFI headers, I described it in V8 comments here[1].
>
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -20,6 +20,7 @@
>> #include <xen/param.h>
>> #include <xen/softirq.h>
>> #include <xen/keyhandler.h>
>> +#include <xen/acpi.h>
>> #include <xsm/xsm.h>
>>
>> #ifdef CONFIG_X86
>> @@ -744,6 +745,20 @@ int __init iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
>> return 0;
>> }
>>
>> +#ifdef CONFIG_ARM
>
> I realize we have CONFIG_X86 here as well (visible even in context of the
> earlier hunk. Yet then the goal ought to be to reduce these anomalies, not
> add new ones. Since I don't have a clear picture of what's wanted, I'm also
> in trouble suggesting any alternative, I'm afraid.
>
> Jan
Here is a short summary:
The main problem is that we need this function somewhere, but there is
no good place for it. It is only called on ARM for now but it's not
ARM-specific by nature and can be eventually used on other platforms as
well. It can't be just dropped because of the effort to support the
co-existence of DT and ACPI. It also can't be declared as a static
function because it requires the inclusion of <xen/acpi.h> for
acpi_disabled define, which leads to build errors[1]. And without ifdef
guards it would be a MISRA violation.
[1]
https://patchew.org/Xen/cover.1739182214.git.mykyta._5Fpoturai@epam.com/67b9bd138c12c0df35e5c4b3137c30ad345097d5.1739182214.git.mykyta._5Fpoturai@epam.com/#e6ea52a3-c7ce-411f-8186-cf725aa608f3@epam.com
--
Mykyta
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v9 2/8] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API
2025-03-19 15:21 ` Mykyta Poturai
@ 2025-03-19 15:28 ` Jan Beulich
2025-03-20 10:47 ` Mykyta Poturai
0 siblings, 1 reply; 37+ messages in thread
From: Jan Beulich @ 2025-03-19 15:28 UTC (permalink / raw)
To: Mykyta Poturai
Cc: Oleksandr Tyshchenko, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Roger Pau Monné,
Stewart Hildebrand, xen-devel@lists.xenproject.org
On 19.03.2025 16:21, Mykyta Poturai wrote:
> On 17.03.25 16:56, Jan Beulich wrote:
>> On 14.03.2025 14:34, Mykyta Poturai wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> The main purpose of this patch is to add a way to register PCI device
>>> (which is behind the IOMMU) using the generic PCI-IOMMU DT bindings [1]
>>> before assigning that device to a domain.
>>>
>>> This behaves similarly to the existing iommu_add_dt_device API, except it
>>> handles PCI devices, and it is to be invoked from the add_device hook in the
>>> SMMU driver.
>>>
>>> The function dt_map_id to translate an ID through a downstream mapping
>>> (which is also suitable for mapping Requester ID) was borrowed from Linux
>>> (v5.10-rc6) and updated according to the Xen code base.
>>>
>>> [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
>>> ---
>>> Regarding pci_for_each_dma_alias question: getting host bridge node
>>> directly seems like a simpler solution with the same result. AFAIU
>>> with pci_for_each_dma_alias in linux we would arrive to the host brige
>>> node anyway, but also try to call dt_map_id for each device along the
>>> way. I am not sure why exactly it is done this way in linux, as
>>> according to the pci-iommu.txt, iommu-map node can only be present in
>>> the PCI root.
>>>
>>> v8->v9:
>>> * replace DT_NO_IOMMU with 1
>>> * guard iommu_add_pci_sideband_ids with CONFIG_ARM
>>
>> I fear I'm confused: Isn't this contradicting ...
>>
>>> v7->v8:
>>> * ENOSYS->EOPNOTSUPP
>>> * move iommu_add_pci_sideband_ids to iommu.c to fix x86 build
>>
>> ... this earlier change? Really, with there being no caller, I can't see
>> why there could be any build issue here affecting only x86. Except for
>> Misra complaining about unreachable code being introduced, which I'm sure
>> I said before should be avoided.
>
> The original reason for moving this function was the conflicting ACPI
> and EFI headers, I described it in V8 comments here[1].
>
>>
>>> --- a/xen/drivers/passthrough/iommu.c
>>> +++ b/xen/drivers/passthrough/iommu.c
>>> @@ -20,6 +20,7 @@
>>> #include <xen/param.h>
>>> #include <xen/softirq.h>
>>> #include <xen/keyhandler.h>
>>> +#include <xen/acpi.h>
>>> #include <xsm/xsm.h>
>>>
>>> #ifdef CONFIG_X86
>>> @@ -744,6 +745,20 @@ int __init iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
>>> return 0;
>>> }
>>>
>>> +#ifdef CONFIG_ARM
>>
>> I realize we have CONFIG_X86 here as well (visible even in context of the
>> earlier hunk. Yet then the goal ought to be to reduce these anomalies, not
>> add new ones. Since I don't have a clear picture of what's wanted, I'm also
>> in trouble suggesting any alternative, I'm afraid.
>
> Here is a short summary:
>
> The main problem is that we need this function somewhere, but there is
> no good place for it. It is only called on ARM for now but it's not
> ARM-specific by nature and can be eventually used on other platforms as
> well. It can't be just dropped because of the effort to support the
> co-existence of DT and ACPI. It also can't be declared as a static
> function because it requires the inclusion of <xen/acpi.h> for
> acpi_disabled define, which leads to build errors[1]. And without ifdef
> guards it would be a MISRA violation.
An abridged version of this ought to go in the patch description, I think.
This is special, so it needs calling out.
As to the placement - would making an entirely new .c file possibly help?
(Then, instead of in the patch description, maybe the special aspect could
be put in a code comment at the top of the file.)
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v9 2/8] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API
2025-03-19 15:28 ` Jan Beulich
@ 2025-03-20 10:47 ` Mykyta Poturai
2025-03-20 11:02 ` Jan Beulich
0 siblings, 1 reply; 37+ messages in thread
From: Mykyta Poturai @ 2025-03-20 10:47 UTC (permalink / raw)
To: Jan Beulich
Cc: Oleksandr Tyshchenko, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Roger Pau Monné,
Stewart Hildebrand, xen-devel@lists.xenproject.org
On 19.03.25 17:28, Jan Beulich wrote:
> On 19.03.2025 16:21, Mykyta Poturai wrote:
>> On 17.03.25 16:56, Jan Beulich wrote:
>>> On 14.03.2025 14:34, Mykyta Poturai wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> The main purpose of this patch is to add a way to register PCI device
>>>> (which is behind the IOMMU) using the generic PCI-IOMMU DT bindings [1]
>>>> before assigning that device to a domain.
>>>>
>>>> This behaves similarly to the existing iommu_add_dt_device API, except it
>>>> handles PCI devices, and it is to be invoked from the add_device hook in the
>>>> SMMU driver.
>>>>
>>>> The function dt_map_id to translate an ID through a downstream mapping
>>>> (which is also suitable for mapping Requester ID) was borrowed from Linux
>>>> (v5.10-rc6) and updated according to the Xen code base.
>>>>
>>>> [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt
>>>>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>>> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
>>>> ---
>>>> Regarding pci_for_each_dma_alias question: getting host bridge node
>>>> directly seems like a simpler solution with the same result. AFAIU
>>>> with pci_for_each_dma_alias in linux we would arrive to the host brige
>>>> node anyway, but also try to call dt_map_id for each device along the
>>>> way. I am not sure why exactly it is done this way in linux, as
>>>> according to the pci-iommu.txt, iommu-map node can only be present in
>>>> the PCI root.
>>>>
>>>> v8->v9:
>>>> * replace DT_NO_IOMMU with 1
>>>> * guard iommu_add_pci_sideband_ids with CONFIG_ARM
>>>
>>> I fear I'm confused: Isn't this contradicting ...
>>>
>>>> v7->v8:
>>>> * ENOSYS->EOPNOTSUPP
>>>> * move iommu_add_pci_sideband_ids to iommu.c to fix x86 build
>>>
>>> ... this earlier change? Really, with there being no caller, I can't see
>>> why there could be any build issue here affecting only x86. Except for
>>> Misra complaining about unreachable code being introduced, which I'm sure
>>> I said before should be avoided.
>>
>> The original reason for moving this function was the conflicting ACPI
>> and EFI headers, I described it in V8 comments here[1].
>>
>>>
>>>> --- a/xen/drivers/passthrough/iommu.c
>>>> +++ b/xen/drivers/passthrough/iommu.c
>>>> @@ -20,6 +20,7 @@
>>>> #include <xen/param.h>
>>>> #include <xen/softirq.h>
>>>> #include <xen/keyhandler.h>
>>>> +#include <xen/acpi.h>
>>>> #include <xsm/xsm.h>
>>>>
>>>> #ifdef CONFIG_X86
>>>> @@ -744,6 +745,20 @@ int __init iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
>>>> return 0;
>>>> }
>>>>
>>>> +#ifdef CONFIG_ARM
>>>
>>> I realize we have CONFIG_X86 here as well (visible even in context of the
>>> earlier hunk. Yet then the goal ought to be to reduce these anomalies, not
>>> add new ones. Since I don't have a clear picture of what's wanted, I'm also
>>> in trouble suggesting any alternative, I'm afraid.
>>
>> Here is a short summary:
>>
>> The main problem is that we need this function somewhere, but there is
>> no good place for it. It is only called on ARM for now but it's not
>> ARM-specific by nature and can be eventually used on other platforms as
>> well. It can't be just dropped because of the effort to support the
>> co-existence of DT and ACPI. It also can't be declared as a static
>> function because it requires the inclusion of <xen/acpi.h> for
>> acpi_disabled define, which leads to build errors[1]. And without ifdef
>> guards it would be a MISRA violation.
>
> An abridged version of this ought to go in the patch description, I think.
> This is special, so it needs calling out.
>
> As to the placement - would making an entirely new .c file possibly help?
> (Then, instead of in the patch description, maybe the special aspect could
> be put in a code comment at the top of the file.)
>
> Jan
It seems to me creating a new file would be overkill for one small
function. I considered moving it to xen/drivers/passthrough/arm/iommu.c
to reduce ifdefs but I feared it would suggest that it is arch-specific
a bit too strongly. So maybe move it there after all if you think it
would be better?
--
Mykyta
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v9 2/8] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API
2025-03-20 10:47 ` Mykyta Poturai
@ 2025-03-20 11:02 ` Jan Beulich
2025-04-21 23:37 ` Stefano Stabellini
0 siblings, 1 reply; 37+ messages in thread
From: Jan Beulich @ 2025-03-20 11:02 UTC (permalink / raw)
To: Mykyta Poturai
Cc: Oleksandr Tyshchenko, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Roger Pau Monné,
Stewart Hildebrand, xen-devel@lists.xenproject.org
On 20.03.2025 11:47, Mykyta Poturai wrote:
> On 19.03.25 17:28, Jan Beulich wrote:
>> On 19.03.2025 16:21, Mykyta Poturai wrote:
>>> On 17.03.25 16:56, Jan Beulich wrote:
>>>> On 14.03.2025 14:34, Mykyta Poturai wrote:
>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>
>>>>> The main purpose of this patch is to add a way to register PCI device
>>>>> (which is behind the IOMMU) using the generic PCI-IOMMU DT bindings [1]
>>>>> before assigning that device to a domain.
>>>>>
>>>>> This behaves similarly to the existing iommu_add_dt_device API, except it
>>>>> handles PCI devices, and it is to be invoked from the add_device hook in the
>>>>> SMMU driver.
>>>>>
>>>>> The function dt_map_id to translate an ID through a downstream mapping
>>>>> (which is also suitable for mapping Requester ID) was borrowed from Linux
>>>>> (v5.10-rc6) and updated according to the Xen code base.
>>>>>
>>>>> [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt
>>>>>
>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>>>> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
>>>>> ---
>>>>> Regarding pci_for_each_dma_alias question: getting host bridge node
>>>>> directly seems like a simpler solution with the same result. AFAIU
>>>>> with pci_for_each_dma_alias in linux we would arrive to the host brige
>>>>> node anyway, but also try to call dt_map_id for each device along the
>>>>> way. I am not sure why exactly it is done this way in linux, as
>>>>> according to the pci-iommu.txt, iommu-map node can only be present in
>>>>> the PCI root.
>>>>>
>>>>> v8->v9:
>>>>> * replace DT_NO_IOMMU with 1
>>>>> * guard iommu_add_pci_sideband_ids with CONFIG_ARM
>>>>
>>>> I fear I'm confused: Isn't this contradicting ...
>>>>
>>>>> v7->v8:
>>>>> * ENOSYS->EOPNOTSUPP
>>>>> * move iommu_add_pci_sideband_ids to iommu.c to fix x86 build
>>>>
>>>> ... this earlier change? Really, with there being no caller, I can't see
>>>> why there could be any build issue here affecting only x86. Except for
>>>> Misra complaining about unreachable code being introduced, which I'm sure
>>>> I said before should be avoided.
>>>
>>> The original reason for moving this function was the conflicting ACPI
>>> and EFI headers, I described it in V8 comments here[1].
>>>
>>>>
>>>>> --- a/xen/drivers/passthrough/iommu.c
>>>>> +++ b/xen/drivers/passthrough/iommu.c
>>>>> @@ -20,6 +20,7 @@
>>>>> #include <xen/param.h>
>>>>> #include <xen/softirq.h>
>>>>> #include <xen/keyhandler.h>
>>>>> +#include <xen/acpi.h>
>>>>> #include <xsm/xsm.h>
>>>>>
>>>>> #ifdef CONFIG_X86
>>>>> @@ -744,6 +745,20 @@ int __init iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +#ifdef CONFIG_ARM
>>>>
>>>> I realize we have CONFIG_X86 here as well (visible even in context of the
>>>> earlier hunk. Yet then the goal ought to be to reduce these anomalies, not
>>>> add new ones. Since I don't have a clear picture of what's wanted, I'm also
>>>> in trouble suggesting any alternative, I'm afraid.
>>>
>>> Here is a short summary:
>>>
>>> The main problem is that we need this function somewhere, but there is
>>> no good place for it. It is only called on ARM for now but it's not
>>> ARM-specific by nature and can be eventually used on other platforms as
>>> well. It can't be just dropped because of the effort to support the
>>> co-existence of DT and ACPI. It also can't be declared as a static
>>> function because it requires the inclusion of <xen/acpi.h> for
>>> acpi_disabled define, which leads to build errors[1]. And without ifdef
>>> guards it would be a MISRA violation.
>>
>> An abridged version of this ought to go in the patch description, I think.
>> This is special, so it needs calling out.
>>
>> As to the placement - would making an entirely new .c file possibly help?
>> (Then, instead of in the patch description, maybe the special aspect could
>> be put in a code comment at the top of the file.)
>
> It seems to me creating a new file would be overkill for one small
> function. I considered moving it to xen/drivers/passthrough/arm/iommu.c
> to reduce ifdefs but I feared it would suggest that it is arch-specific
> a bit too strongly. So maybe move it there after all if you think it
> would be better?
Well - with "#ifdef CONFIG_ARM" around it's Arm-specific too, isn't it?
IOW - either have a proper (even if simple) abstraction, or perhaps indeed
put it there (if that's also okay with the Arm maintainers).
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v9 2/8] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API
2025-03-20 11:02 ` Jan Beulich
@ 2025-04-21 23:37 ` Stefano Stabellini
0 siblings, 0 replies; 37+ messages in thread
From: Stefano Stabellini @ 2025-04-21 23:37 UTC (permalink / raw)
To: Jan Beulich
Cc: Mykyta Poturai, Oleksandr Tyshchenko, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel,
Roger Pau Monné, Stewart Hildebrand,
xen-devel@lists.xenproject.org
On Thu, 20 Mar 2025, Jan Beulich wrote:
> On 20.03.2025 11:47, Mykyta Poturai wrote:
> > On 19.03.25 17:28, Jan Beulich wrote:
> >> On 19.03.2025 16:21, Mykyta Poturai wrote:
> >>> On 17.03.25 16:56, Jan Beulich wrote:
> >>>> On 14.03.2025 14:34, Mykyta Poturai wrote:
> >>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> >>>>>
> >>>>> The main purpose of this patch is to add a way to register PCI device
> >>>>> (which is behind the IOMMU) using the generic PCI-IOMMU DT bindings [1]
> >>>>> before assigning that device to a domain.
> >>>>>
> >>>>> This behaves similarly to the existing iommu_add_dt_device API, except it
> >>>>> handles PCI devices, and it is to be invoked from the add_device hook in the
> >>>>> SMMU driver.
> >>>>>
> >>>>> The function dt_map_id to translate an ID through a downstream mapping
> >>>>> (which is also suitable for mapping Requester ID) was borrowed from Linux
> >>>>> (v5.10-rc6) and updated according to the Xen code base.
> >>>>>
> >>>>> [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt
> >>>>>
> >>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> >>>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> >>>>> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> >>>>> ---
> >>>>> Regarding pci_for_each_dma_alias question: getting host bridge node
> >>>>> directly seems like a simpler solution with the same result. AFAIU
> >>>>> with pci_for_each_dma_alias in linux we would arrive to the host brige
> >>>>> node anyway, but also try to call dt_map_id for each device along the
> >>>>> way. I am not sure why exactly it is done this way in linux, as
> >>>>> according to the pci-iommu.txt, iommu-map node can only be present in
> >>>>> the PCI root.
> >>>>>
> >>>>> v8->v9:
> >>>>> * replace DT_NO_IOMMU with 1
> >>>>> * guard iommu_add_pci_sideband_ids with CONFIG_ARM
> >>>>
> >>>> I fear I'm confused: Isn't this contradicting ...
> >>>>
> >>>>> v7->v8:
> >>>>> * ENOSYS->EOPNOTSUPP
> >>>>> * move iommu_add_pci_sideband_ids to iommu.c to fix x86 build
> >>>>
> >>>> ... this earlier change? Really, with there being no caller, I can't see
> >>>> why there could be any build issue here affecting only x86. Except for
> >>>> Misra complaining about unreachable code being introduced, which I'm sure
> >>>> I said before should be avoided.
> >>>
> >>> The original reason for moving this function was the conflicting ACPI
> >>> and EFI headers, I described it in V8 comments here[1].
> >>>
> >>>>
> >>>>> --- a/xen/drivers/passthrough/iommu.c
> >>>>> +++ b/xen/drivers/passthrough/iommu.c
> >>>>> @@ -20,6 +20,7 @@
> >>>>> #include <xen/param.h>
> >>>>> #include <xen/softirq.h>
> >>>>> #include <xen/keyhandler.h>
> >>>>> +#include <xen/acpi.h>
> >>>>> #include <xsm/xsm.h>
> >>>>>
> >>>>> #ifdef CONFIG_X86
> >>>>> @@ -744,6 +745,20 @@ int __init iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
> >>>>> return 0;
> >>>>> }
> >>>>>
> >>>>> +#ifdef CONFIG_ARM
> >>>>
> >>>> I realize we have CONFIG_X86 here as well (visible even in context of the
> >>>> earlier hunk. Yet then the goal ought to be to reduce these anomalies, not
> >>>> add new ones. Since I don't have a clear picture of what's wanted, I'm also
> >>>> in trouble suggesting any alternative, I'm afraid.
> >>>
> >>> Here is a short summary:
> >>>
> >>> The main problem is that we need this function somewhere, but there is
> >>> no good place for it. It is only called on ARM for now but it's not
> >>> ARM-specific by nature and can be eventually used on other platforms as
> >>> well. It can't be just dropped because of the effort to support the
> >>> co-existence of DT and ACPI. It also can't be declared as a static
> >>> function because it requires the inclusion of <xen/acpi.h> for
> >>> acpi_disabled define, which leads to build errors[1]. And without ifdef
> >>> guards it would be a MISRA violation.
> >>
> >> An abridged version of this ought to go in the patch description, I think.
> >> This is special, so it needs calling out.
> >>
> >> As to the placement - would making an entirely new .c file possibly help?
> >> (Then, instead of in the patch description, maybe the special aspect could
> >> be put in a code comment at the top of the file.)
> >
> > It seems to me creating a new file would be overkill for one small
> > function. I considered moving it to xen/drivers/passthrough/arm/iommu.c
> > to reduce ifdefs but I feared it would suggest that it is arch-specific
> > a bit too strongly. So maybe move it there after all if you think it
> > would be better?
>
> Well - with "#ifdef CONFIG_ARM" around it's Arm-specific too, isn't it?
> IOW - either have a proper (even if simple) abstraction, or perhaps indeed
> put it there (if that's also okay with the Arm maintainers).
I am OK with that
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v9 3/8] iommu/arm: iommu_add_dt_pci_sideband_ids phantom handling
2025-03-14 13:34 [PATCH v9 0/8] SMMU handling for PCIe Passthrough on ARM Mykyta Poturai
2025-03-14 13:34 ` [PATCH v9 1/8] iommu/arm: Add iommu_dt_xlate() Mykyta Poturai
2025-03-14 13:34 ` [PATCH v9 2/8] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API Mykyta Poturai
@ 2025-03-14 13:34 ` Mykyta Poturai
2025-04-21 23:50 ` Stefano Stabellini
2025-03-14 13:34 ` [PATCH v9 4/8] xen/arm: smmuv2: Add PCI devices support for SMMUv2 Mykyta Poturai
` (4 subsequent siblings)
7 siblings, 1 reply; 37+ messages in thread
From: Mykyta Poturai @ 2025-03-14 13:34 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Stewart Hildebrand, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel
From: Stewart Hildebrand <stewart.hildebrand@amd.com>
Handle phantom functions in iommu_add_dt_pci_sideband_ids(). Each phantom
function will have a unique requestor ID (RID)/BDF. On ARM, we need to
map/translate the RID/BDF to an AXI stream ID for each phantom function
according to the pci-iommu device tree mapping [1]. The RID/BDF -> AXI stream ID
mapping in DT could allow phantom devices (i.e. devices with phantom functions)
to use different AXI stream IDs based on the (phantom) function.
[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v8->v9:
* replace DT_NO_IOMMU with 1
v7->v8:
* no change
v6->v7:
* no change
v5->v6:
* no change
v4->v5:
* no change
v3->v4:
* s/iommu_dt_pci_map_id/dt_map_id/
v2->v3:
* new patch title (was: iommu/arm: iommu_add_dt_pci_device phantom handling)
* rework loop to reduce duplication
* s/iommu_fwspec_free(pci_to_dev(pdev))/iommu_fwspec_free(dev)/
v1->v2:
* new patch
---
TODO: investigate Jan's comment [2]
[2] https://lore.kernel.org/xen-devel/806a2978-19fb-4d31-ab6a-35ea7317c8de@suse.com/
---
xen/drivers/passthrough/device_tree.c | 33 ++++++++++++++++-----------
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 37e1437b65..f5850a2607 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -169,6 +169,7 @@ int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
struct device *dev = pci_to_dev(pdev);
const struct dt_device_node *np;
int rc;
+ unsigned int devfn = pdev->devfn;
if ( !iommu_enabled )
return 1;
@@ -183,21 +184,27 @@ int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
if ( !np )
return -ENODEV;
- /*
- * According to the Documentation/devicetree/bindings/pci/pci-iommu.txt
- * from Linux.
- */
- rc = dt_map_id(np, PCI_BDF(pdev->bus, pdev->devfn), "iommu-map",
- "iommu-map-mask", &iommu_spec.np, iommu_spec.args);
- if ( rc )
- return (rc == -ENODEV) ? 1 : rc;
+ do {
+ /*
+ * According to the Documentation/devicetree/bindings/pci/pci-iommu.txt
+ * from Linux.
+ */
+ rc = dt_map_id(np, PCI_BDF(pdev->bus, devfn), "iommu-map",
+ "iommu-map-mask", &iommu_spec.np, iommu_spec.args);
+ if ( rc )
+ return (rc == -ENODEV) ? 1 : rc;
- rc = iommu_dt_xlate(dev, &iommu_spec, ops);
- if ( rc < 0 )
- {
- iommu_fwspec_free(dev);
- return -EINVAL;
+ rc = iommu_dt_xlate(dev, &iommu_spec, ops);
+ if ( rc < 0 )
+ {
+ iommu_fwspec_free(dev);
+ return -EINVAL;
+ }
+
+ devfn += pdev->phantom_stride;
}
+ while ( (devfn != pdev->devfn) &&
+ (PCI_SLOT(devfn) == PCI_SLOT(pdev->devfn)) );
return rc;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v9 3/8] iommu/arm: iommu_add_dt_pci_sideband_ids phantom handling
2025-03-14 13:34 ` [PATCH v9 3/8] iommu/arm: iommu_add_dt_pci_sideband_ids phantom handling Mykyta Poturai
@ 2025-04-21 23:50 ` Stefano Stabellini
0 siblings, 0 replies; 37+ messages in thread
From: Stefano Stabellini @ 2025-04-21 23:50 UTC (permalink / raw)
To: Mykyta Poturai
Cc: xen-devel@lists.xenproject.org, Stewart Hildebrand,
Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel
On Fri, 14 Mar 2025, Mykyta Poturai wrote:
> From: Stewart Hildebrand <stewart.hildebrand@amd.com>
>
> Handle phantom functions in iommu_add_dt_pci_sideband_ids(). Each phantom
> function will have a unique requestor ID (RID)/BDF. On ARM, we need to
> map/translate the RID/BDF to an AXI stream ID for each phantom function
> according to the pci-iommu device tree mapping [1]. The RID/BDF -> AXI stream ID
> mapping in DT could allow phantom devices (i.e. devices with phantom functions)
> to use different AXI stream IDs based on the (phantom) function.
>
> [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt
>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> v8->v9:
> * replace DT_NO_IOMMU with 1
>
> v7->v8:
> * no change
>
> v6->v7:
> * no change
>
> v5->v6:
> * no change
>
> v4->v5:
> * no change
>
> v3->v4:
> * s/iommu_dt_pci_map_id/dt_map_id/
>
> v2->v3:
> * new patch title (was: iommu/arm: iommu_add_dt_pci_device phantom handling)
> * rework loop to reduce duplication
> * s/iommu_fwspec_free(pci_to_dev(pdev))/iommu_fwspec_free(dev)/
>
> v1->v2:
> * new patch
>
> ---
> TODO: investigate Jan's comment [2]
> [2] https://lore.kernel.org/xen-devel/806a2978-19fb-4d31-ab6a-35ea7317c8de@suse.com/
> ---
> xen/drivers/passthrough/device_tree.c | 33 ++++++++++++++++-----------
> 1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 37e1437b65..f5850a2607 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -169,6 +169,7 @@ int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
> struct device *dev = pci_to_dev(pdev);
> const struct dt_device_node *np;
> int rc;
> + unsigned int devfn = pdev->devfn;
>
> if ( !iommu_enabled )
> return 1;
> @@ -183,21 +184,27 @@ int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
> if ( !np )
> return -ENODEV;
>
> - /*
> - * According to the Documentation/devicetree/bindings/pci/pci-iommu.txt
> - * from Linux.
> - */
> - rc = dt_map_id(np, PCI_BDF(pdev->bus, pdev->devfn), "iommu-map",
> - "iommu-map-mask", &iommu_spec.np, iommu_spec.args);
> - if ( rc )
> - return (rc == -ENODEV) ? 1 : rc;
> + do {
> + /*
> + * According to the Documentation/devicetree/bindings/pci/pci-iommu.txt
> + * from Linux.
> + */
> + rc = dt_map_id(np, PCI_BDF(pdev->bus, devfn), "iommu-map",
> + "iommu-map-mask", &iommu_spec.np, iommu_spec.args);
> + if ( rc )
> + return (rc == -ENODEV) ? 1 : rc;
>
> - rc = iommu_dt_xlate(dev, &iommu_spec, ops);
> - if ( rc < 0 )
> - {
> - iommu_fwspec_free(dev);
> - return -EINVAL;
> + rc = iommu_dt_xlate(dev, &iommu_spec, ops);
> + if ( rc < 0 )
> + {
> + iommu_fwspec_free(dev);
> + return -EINVAL;
> + }
> +
> + devfn += pdev->phantom_stride;
> }
> + while ( (devfn != pdev->devfn) &&
> + (PCI_SLOT(devfn) == PCI_SLOT(pdev->devfn)) );
>
> return rc;
> }
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v9 4/8] xen/arm: smmuv2: Add PCI devices support for SMMUv2
2025-03-14 13:34 [PATCH v9 0/8] SMMU handling for PCIe Passthrough on ARM Mykyta Poturai
` (2 preceding siblings ...)
2025-03-14 13:34 ` [PATCH v9 3/8] iommu/arm: iommu_add_dt_pci_sideband_ids phantom handling Mykyta Poturai
@ 2025-03-14 13:34 ` Mykyta Poturai
2025-04-22 0:18 ` Stefano Stabellini
2025-03-14 13:34 ` [PATCH v9 5/8] xen/arm: smmuv3: Add PCI devices support for SMMUv3 Mykyta Poturai
` (3 subsequent siblings)
7 siblings, 1 reply; 37+ messages in thread
From: Mykyta Poturai @ 2025-03-14 13:34 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Oleksandr Andrushchenko, Julien Grall, Rahul Singh,
Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Oleksandr Tyshchenko, Stewart Hildebrand,
Mykyta Poturai
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Implement support for PCI devices in the SMMU driver. Make arm_smmu_master
structure to hold a pointer to the device to allow it to hold PCI devices.
Trigger iommu-map parsing when new PCI device is added. Add checks to
assign/deassign functions to ensure PCI devices are handled correctly.
Implement basic quarantining.
All pci devices are automatically assigned to hardware domain if it exists
to ensure it can probe them.
TODO:
Implement scratch page quarantining support.
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
v8->v9:
* no change
v7->v8:
* no change
v6->v7:
* use d->pci_lock in arm_smmu_assign_dev()
* remove !is_hardware_domain and pdev->domain == d checks in assign to
support future dom0less use case when dom0 is using vPCI
* remove stale todo in dev_get_dev_node
* don't print ""
* remove redundant dt_device_is_protected check
* remove assign/deassing prints
* change assign logic to remove reassign reimplementation
* check if pdev->domain exists before assigning to it
* explain pdev->devfn check
* make reassign check stricter and update comment
v5->v6:
* check for hardware_domain == NULL (dom0less test case)
* locking: assign pdev->domain before list_add()
v4->v5:
* assign device to pdev->domain (usually dom0) by default in add_device() hook
* deassign from hwdom
* rebase on top of ("dynamic node programming using overlay dtbo") series
* remove TODO in comment about device prints
* add TODO regarding locking
* fixup after dropping ("xen/arm: Move is_protected flag to struct device")
v3->v4:
* add new device_is_protected check in add_device hook to match SMMUv3 and
IPMMU-VMSA drivers
v2->v3:
* invoke iommu_add_pci_sideband_ids() from add_device hook
v1->v2:
* ignore add_device/assign_device/reassign_device calls for phantom functions
(i.e. devfn != pdev->devfn)
downstream->v1:
* wrap unused function in #ifdef 0
* remove the remove_device() stub since it was submitted separately to the list
[XEN][PATCH v6 12/19] xen/smmu: Add remove_device callback for smmu_iommu ops
https://lists.xenproject.org/archives/html/xen-devel/2023-05/msg00204.html
* arm_smmu_(de)assign_dev: return error instead of crashing system
* update condition in arm_smmu_reassign_dev
* style fixup
* add && !is_hardware_domain(d) into condition in arm_smmu_assign_dev()
(cherry picked from commit 0c11a7f65f044c26d87d1e27ac6283ef1f9cfb7a from
the downstream branch spider-master from
https://github.com/xen-troops/xen.git)
---
xen/drivers/passthrough/arm/smmu.c | 190 ++++++++++++++++++++++-------
1 file changed, 147 insertions(+), 43 deletions(-)
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 03d22bce1e..cfddcbb1ad 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -132,11 +132,21 @@ enum irqreturn {
typedef enum irqreturn irqreturn_t;
-/* Device logger functions
- * TODO: Handle PCI
- */
-#define dev_print(dev, lvl, fmt, ...) \
- printk(lvl "smmu: %s: " fmt, dt_node_full_name(dev_to_dt(dev)), ## __VA_ARGS__)
+/* Device logger functions */
+#ifndef CONFIG_HAS_PCI
+#define dev_print(dev, lvl, fmt, ...) \
+ printk(lvl "smmu: %s: " fmt, dev_name(dev), ## __VA_ARGS__)
+#else
+#define dev_print(dev, lvl, fmt, ...) ({ \
+ if ( !dev_is_pci((dev)) ) \
+ printk(lvl "smmu: %s: " fmt, dev_name((dev)), ## __VA_ARGS__); \
+ else \
+ { \
+ struct pci_dev *pdev = dev_to_pci((dev)); \
+ printk(lvl "smmu: %pp: " fmt, &pdev->sbdf, ## __VA_ARGS__); \
+ } \
+})
+#endif
#define dev_dbg(dev, fmt, ...) dev_print(dev, XENLOG_DEBUG, fmt, ## __VA_ARGS__)
#define dev_notice(dev, fmt, ...) dev_print(dev, XENLOG_INFO, fmt, ## __VA_ARGS__)
@@ -188,6 +198,7 @@ static void __iomem *devm_ioremap_resource(struct device *dev,
* Xen: PCI functions
* TODO: It should be implemented when PCI will be supported
*/
+#if 0 /* unused */
#define to_pci_dev(dev) (NULL)
static inline int pci_for_each_dma_alias(struct pci_dev *pdev,
int (*fn) (struct pci_dev *pdev,
@@ -197,6 +208,7 @@ static inline int pci_for_each_dma_alias(struct pci_dev *pdev,
BUG();
return 0;
}
+#endif
/* Xen: misc */
#define PHYS_MASK_SHIFT PADDR_BITS
@@ -631,7 +643,7 @@ struct arm_smmu_master_cfg {
for (i = 0; idx = (cfg)->smendx[i], (i) < (num); ++(i))
struct arm_smmu_master {
- struct device_node *of_node;
+ struct device *dev;
struct rb_node node;
struct arm_smmu_master_cfg cfg;
};
@@ -723,7 +735,7 @@ arm_smmu_get_fwspec(struct arm_smmu_master_cfg *cfg)
{
struct arm_smmu_master *master = container_of(cfg,
struct arm_smmu_master, cfg);
- return dev_iommu_fwspec_get(&master->of_node->dev);
+ return dev_iommu_fwspec_get(master->dev);
}
static void parse_driver_options(struct arm_smmu_device *smmu)
@@ -742,21 +754,11 @@ static void parse_driver_options(struct arm_smmu_device *smmu)
static struct device_node *dev_get_dev_node(struct device *dev)
{
-#if 0 /* Xen: TODO: Add support for PCI */
- if (dev_is_pci(dev)) {
- struct pci_bus *bus = to_pci_dev(dev)->bus;
-
- while (!pci_is_root_bus(bus))
- bus = bus->parent;
- return bus->bridge->parent->of_node;
- }
-#endif
-
return dev->of_node;
}
static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
- struct device_node *dev_node)
+ struct device *dev)
{
struct rb_node *node = smmu->masters.rb_node;
@@ -765,9 +767,9 @@ static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
master = container_of(node, struct arm_smmu_master, node);
- if (dev_node < master->of_node)
+ if (dev < master->dev)
node = node->rb_left;
- else if (dev_node > master->of_node)
+ else if (dev > master->dev)
node = node->rb_right;
else
return master;
@@ -802,9 +804,9 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
= container_of(*new, struct arm_smmu_master, node);
parent = *new;
- if (master->of_node < this->of_node)
+ if (master->dev < this->dev)
new = &((*new)->rb_left);
- else if (master->of_node > this->of_node)
+ else if (master->dev > this->dev)
new = &((*new)->rb_right);
else
return -EEXIST;
@@ -836,28 +838,30 @@ static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
struct arm_smmu_master *master;
struct device_node *dev_node = dev_get_dev_node(dev);
- master = find_smmu_master(smmu, dev_node);
+ master = find_smmu_master(smmu, dev);
if (master) {
dev_err(dev,
- "rejecting multiple registrations for master device %s\n",
- dev_node->name);
+ "rejecting multiple registrations for master device\n");
return -EBUSY;
}
master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
if (!master)
return -ENOMEM;
- master->of_node = dev_node;
+ master->dev = dev;
- /* Xen: Let Xen know that the device is protected by an SMMU */
- dt_device_set_protected(dev_node);
+ if ( !dev_is_pci(dev) )
+ {
+ /* Xen: Let Xen know that the device is protected by an SMMU */
+ dt_device_set_protected(dev_node);
+ }
for (i = 0; i < fwspec->num_ids; ++i) {
if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
(fwspec->ids[i] >= smmu->num_mapping_groups)) {
dev_err(dev,
- "stream ID for master device %s greater than maximum allowed (%d)\n",
- dev_node->name, smmu->num_mapping_groups);
+ "SMMU stream ID %d is greater than maximum allowed (%d)\n",
+ fwspec->ids[i], smmu->num_mapping_groups);
return -ERANGE;
}
master->cfg.smendx[i] = INVALID_SMENDX;
@@ -872,7 +876,7 @@ static int arm_smmu_dt_remove_device_legacy(struct arm_smmu_device *smmu,
struct device_node *dev_node = dev_get_dev_node(dev);
int ret;
- master = find_smmu_master(smmu, dev_node);
+ master = find_smmu_master(smmu, dev);
if (master == NULL) {
dev_err(dev,
"No registrations found for master device %s\n",
@@ -884,8 +888,9 @@ static int arm_smmu_dt_remove_device_legacy(struct arm_smmu_device *smmu,
if (ret)
return ret;
- /* Protected by dt_host_lock and dtdevs_lock as caller holds these locks. */
- dev_node->is_protected = false;
+ if ( !dev_is_pci(dev) )
+ /* Protected by dt_host_lock and dtdevs_lock as caller holds these locks. */
+ dev_node->is_protected = false;
kfree(master);
return 0;
@@ -914,6 +919,12 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
fwspec);
}
+/* Forward declaration */
+static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
+ struct device *dev, u32 flag);
+static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn,
+ struct device *dev);
+
/*
* The driver which supports generic IOMMU DT bindings must have this
* callback implemented.
@@ -938,6 +949,23 @@ static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
{
struct arm_smmu_device *smmu;
struct iommu_fwspec *fwspec;
+ int ret;
+
+#ifdef CONFIG_HAS_PCI
+ if ( dev_is_pci(dev) )
+ {
+ struct pci_dev *pdev = dev_to_pci(dev);
+ int ret;
+
+ /* Ignore calls for phantom functions */
+ if ( devfn != pdev->devfn )
+ return 0;
+
+ ret = iommu_add_pci_sideband_ids(pdev);
+ if ( ret < 0 )
+ iommu_fwspec_free(dev);
+ }
+#endif
fwspec = dev_iommu_fwspec_get(dev);
if (fwspec == NULL)
@@ -947,7 +975,25 @@ static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
if (smmu == NULL)
return -ENXIO;
- return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec);
+ ret = arm_smmu_dt_add_device_legacy(smmu, dev, fwspec);
+ if ( ret )
+ return ret;
+
+#ifdef CONFIG_HAS_PCI
+ if ( dev_is_pci(dev) )
+ {
+ struct pci_dev *pdev = dev_to_pci(dev);
+
+ /*
+ * During PHYSDEVOP_pci_device_add, Xen does not assign the
+ * device, so we must do it here.
+ */
+ if ( pdev->domain )
+ ret = arm_smmu_assign_dev(pdev->domain, devfn, dev, 0);
+ }
+#endif
+
+ return ret;
}
static int arm_smmu_dt_xlate_generic(struct device *dev,
@@ -970,11 +1016,10 @@ static struct arm_smmu_device *find_smmu_for_device(struct device *dev)
{
struct arm_smmu_device *smmu;
struct arm_smmu_master *master = NULL;
- struct device_node *dev_node = dev_get_dev_node(dev);
spin_lock(&arm_smmu_devices_lock);
list_for_each_entry(smmu, &arm_smmu_devices, list) {
- master = find_smmu_master(smmu, dev_node);
+ master = find_smmu_master(smmu, dev);
if (master)
break;
}
@@ -2066,6 +2111,7 @@ static bool arm_smmu_capable(enum iommu_cap cap)
}
#endif
+#if 0 /* Not used */
static int __arm_smmu_get_pci_sid(struct pci_dev *pdev, u16 alias, void *data)
{
*((u16 *)data) = alias;
@@ -2076,6 +2122,7 @@ static void __arm_smmu_release_pci_iommudata(void *data)
{
kfree(data);
}
+#endif
static int arm_smmu_add_device(struct device *dev)
{
@@ -2083,12 +2130,13 @@ static int arm_smmu_add_device(struct device *dev)
struct arm_smmu_master_cfg *cfg;
struct iommu_group *group;
void (*releasefn)(void *data) = NULL;
- int ret;
smmu = find_smmu_for_device(dev);
if (!smmu)
return -ENODEV;
+ /* There is no need to distinguish here, thanks to PCI-IOMMU DT bindings */
+#if 0
if (dev_is_pci(dev)) {
struct pci_dev *pdev = to_pci_dev(dev);
struct iommu_fwspec *fwspec;
@@ -2113,10 +2161,12 @@ static int arm_smmu_add_device(struct device *dev)
&fwspec->ids[0]);
releasefn = __arm_smmu_release_pci_iommudata;
cfg->smmu = smmu;
- } else {
+ } else
+#endif
+ {
struct arm_smmu_master *master;
- master = find_smmu_master(smmu, dev->of_node);
+ master = find_smmu_master(smmu, dev);
if (!master) {
return -ENODEV;
}
@@ -2784,6 +2834,42 @@ static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
return -ENOMEM;
}
+#ifdef CONFIG_HAS_PCI
+ if ( dev_is_pci(dev) )
+ {
+ struct pci_dev *pdev = dev_to_pci(dev);
+
+ /* Ignore calls for phantom functions */
+ if ( devfn != pdev->devfn )
+ return 0;
+
+ ASSERT(pcidevs_locked());
+
+ write_lock(&pdev->domain->pci_lock);
+ list_del(&pdev->domain_list);
+ write_unlock(&pdev->domain->pci_lock);
+
+ pdev->domain = d;
+
+ write_lock(&d->pci_lock);
+ list_add(&pdev->domain_list, &d->pdev_list);
+ write_unlock(&d->pci_lock);
+
+ /* dom_io is used as a sentinel for quarantined devices */
+ if ( d == dom_io )
+ {
+ struct iommu_domain *domain = dev_iommu_domain(dev);
+ if ( !iommu_quarantine )
+ return 0;
+
+ if ( domain && domain->priv )
+ arm_smmu_deassign_dev(domain->priv->cfg.domain, devfn, dev);
+
+ return 0;
+ }
+ }
+#endif
+
if (!dev_iommu_group(dev)) {
ret = arm_smmu_add_device(dev);
if (ret)
@@ -2833,11 +2919,27 @@ out:
return ret;
}
-static int arm_smmu_deassign_dev(struct domain *d, struct device *dev)
+static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn,
+ struct device *dev)
{
struct iommu_domain *domain = dev_iommu_domain(dev);
struct arm_smmu_xen_domain *xen_domain;
+#ifdef CONFIG_HAS_PCI
+ if ( dev_is_pci(dev) )
+ {
+ struct pci_dev *pdev = dev_to_pci(dev);
+
+ /* Ignore calls for phantom functions */
+ if ( devfn != pdev->devfn )
+ return 0;
+
+ /* dom_io is used as a sentinel for quarantined devices */
+ if ( d == dom_io )
+ return 0;
+ }
+#endif
+
xen_domain = dom_iommu(d)->arch.priv;
if (!domain || domain->priv->cfg.domain != d) {
@@ -2864,14 +2966,16 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t,
{
int ret = 0;
- /* Don't allow remapping on other domain than hwdom */
- if ( t && !is_hardware_domain(t) )
+ /* Don't allow remapping on other domain than hwdom
+ * or dom_io for PCI devices
+ */
+ if ( t && !is_hardware_domain(t) && (t != dom_io || !dev_is_pci(dev)) )
return -EPERM;
if (t == s)
return 0;
- ret = arm_smmu_deassign_dev(s, dev);
+ ret = arm_smmu_deassign_dev(s, devfn, dev);
if (ret)
return ret;
--
2.34.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v9 4/8] xen/arm: smmuv2: Add PCI devices support for SMMUv2
2025-03-14 13:34 ` [PATCH v9 4/8] xen/arm: smmuv2: Add PCI devices support for SMMUv2 Mykyta Poturai
@ 2025-04-22 0:18 ` Stefano Stabellini
0 siblings, 0 replies; 37+ messages in thread
From: Stefano Stabellini @ 2025-04-22 0:18 UTC (permalink / raw)
To: Mykyta Poturai
Cc: xen-devel@lists.xenproject.org, Oleksandr Andrushchenko,
Julien Grall, Rahul Singh, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Oleksandr Tyshchenko,
Stewart Hildebrand
On Fri, 14 Mar 2025, Mykyta Poturai wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> Implement support for PCI devices in the SMMU driver. Make arm_smmu_master
> structure to hold a pointer to the device to allow it to hold PCI devices.
> Trigger iommu-map parsing when new PCI device is added. Add checks to
> assign/deassign functions to ensure PCI devices are handled correctly.
> Implement basic quarantining.
>
> All pci devices are automatically assigned to hardware domain if it exists
> to ensure it can probe them.
>
> TODO:
> Implement scratch page quarantining support.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> ---
> v8->v9:
> * no change
>
> v7->v8:
> * no change
>
> v6->v7:
> * use d->pci_lock in arm_smmu_assign_dev()
> * remove !is_hardware_domain and pdev->domain == d checks in assign to
> support future dom0less use case when dom0 is using vPCI
> * remove stale todo in dev_get_dev_node
> * don't print ""
> * remove redundant dt_device_is_protected check
> * remove assign/deassing prints
> * change assign logic to remove reassign reimplementation
> * check if pdev->domain exists before assigning to it
> * explain pdev->devfn check
> * make reassign check stricter and update comment
>
> v5->v6:
> * check for hardware_domain == NULL (dom0less test case)
> * locking: assign pdev->domain before list_add()
>
> v4->v5:
> * assign device to pdev->domain (usually dom0) by default in add_device() hook
> * deassign from hwdom
> * rebase on top of ("dynamic node programming using overlay dtbo") series
> * remove TODO in comment about device prints
> * add TODO regarding locking
> * fixup after dropping ("xen/arm: Move is_protected flag to struct device")
>
> v3->v4:
> * add new device_is_protected check in add_device hook to match SMMUv3 and
> IPMMU-VMSA drivers
>
> v2->v3:
> * invoke iommu_add_pci_sideband_ids() from add_device hook
>
> v1->v2:
> * ignore add_device/assign_device/reassign_device calls for phantom functions
> (i.e. devfn != pdev->devfn)
>
> downstream->v1:
> * wrap unused function in #ifdef 0
> * remove the remove_device() stub since it was submitted separately to the list
> [XEN][PATCH v6 12/19] xen/smmu: Add remove_device callback for smmu_iommu ops
> https://lists.xenproject.org/archives/html/xen-devel/2023-05/msg00204.html
> * arm_smmu_(de)assign_dev: return error instead of crashing system
> * update condition in arm_smmu_reassign_dev
> * style fixup
> * add && !is_hardware_domain(d) into condition in arm_smmu_assign_dev()
>
> (cherry picked from commit 0c11a7f65f044c26d87d1e27ac6283ef1f9cfb7a from
> the downstream branch spider-master from
> https://github.com/xen-troops/xen.git)
> ---
> xen/drivers/passthrough/arm/smmu.c | 190 ++++++++++++++++++++++-------
> 1 file changed, 147 insertions(+), 43 deletions(-)
>
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 03d22bce1e..cfddcbb1ad 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -132,11 +132,21 @@ enum irqreturn {
>
> typedef enum irqreturn irqreturn_t;
>
> -/* Device logger functions
> - * TODO: Handle PCI
> - */
> -#define dev_print(dev, lvl, fmt, ...) \
> - printk(lvl "smmu: %s: " fmt, dt_node_full_name(dev_to_dt(dev)), ## __VA_ARGS__)
> +/* Device logger functions */
> +#ifndef CONFIG_HAS_PCI
> +#define dev_print(dev, lvl, fmt, ...) \
> + printk(lvl "smmu: %s: " fmt, dev_name(dev), ## __VA_ARGS__)
> +#else
> +#define dev_print(dev, lvl, fmt, ...) ({ \
> + if ( !dev_is_pci((dev)) ) \
> + printk(lvl "smmu: %s: " fmt, dev_name((dev)), ## __VA_ARGS__); \
> + else \
> + { \
> + struct pci_dev *pdev = dev_to_pci((dev)); \
> + printk(lvl "smmu: %pp: " fmt, &pdev->sbdf, ## __VA_ARGS__); \
> + } \
> +})
> +#endif
>
> #define dev_dbg(dev, fmt, ...) dev_print(dev, XENLOG_DEBUG, fmt, ## __VA_ARGS__)
> #define dev_notice(dev, fmt, ...) dev_print(dev, XENLOG_INFO, fmt, ## __VA_ARGS__)
> @@ -188,6 +198,7 @@ static void __iomem *devm_ioremap_resource(struct device *dev,
> * Xen: PCI functions
> * TODO: It should be implemented when PCI will be supported
> */
> +#if 0 /* unused */
> #define to_pci_dev(dev) (NULL)
> static inline int pci_for_each_dma_alias(struct pci_dev *pdev,
> int (*fn) (struct pci_dev *pdev,
> @@ -197,6 +208,7 @@ static inline int pci_for_each_dma_alias(struct pci_dev *pdev,
> BUG();
> return 0;
> }
> +#endif
I think we should remove them if they are not used.
> /* Xen: misc */
> #define PHYS_MASK_SHIFT PADDR_BITS
> @@ -631,7 +643,7 @@ struct arm_smmu_master_cfg {
> for (i = 0; idx = (cfg)->smendx[i], (i) < (num); ++(i))
>
> struct arm_smmu_master {
> - struct device_node *of_node;
> + struct device *dev;
> struct rb_node node;
> struct arm_smmu_master_cfg cfg;
> };
> @@ -723,7 +735,7 @@ arm_smmu_get_fwspec(struct arm_smmu_master_cfg *cfg)
> {
> struct arm_smmu_master *master = container_of(cfg,
> struct arm_smmu_master, cfg);
> - return dev_iommu_fwspec_get(&master->of_node->dev);
> + return dev_iommu_fwspec_get(master->dev);
> }
>
> static void parse_driver_options(struct arm_smmu_device *smmu)
> @@ -742,21 +754,11 @@ static void parse_driver_options(struct arm_smmu_device *smmu)
>
> static struct device_node *dev_get_dev_node(struct device *dev)
> {
> -#if 0 /* Xen: TODO: Add support for PCI */
> - if (dev_is_pci(dev)) {
> - struct pci_bus *bus = to_pci_dev(dev)->bus;
> -
> - while (!pci_is_root_bus(bus))
> - bus = bus->parent;
> - return bus->bridge->parent->of_node;
> - }
> -#endif
> -
> return dev->of_node;
> }
>
> static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
> - struct device_node *dev_node)
> + struct device *dev)
> {
> struct rb_node *node = smmu->masters.rb_node;
>
> @@ -765,9 +767,9 @@ static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
>
> master = container_of(node, struct arm_smmu_master, node);
>
> - if (dev_node < master->of_node)
> + if (dev < master->dev)
> node = node->rb_left;
> - else if (dev_node > master->of_node)
> + else if (dev > master->dev)
> node = node->rb_right;
> else
> return master;
> @@ -802,9 +804,9 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
> = container_of(*new, struct arm_smmu_master, node);
>
> parent = *new;
> - if (master->of_node < this->of_node)
> + if (master->dev < this->dev)
> new = &((*new)->rb_left);
> - else if (master->of_node > this->of_node)
> + else if (master->dev > this->dev)
> new = &((*new)->rb_right);
> else
> return -EEXIST;
> @@ -836,28 +838,30 @@ static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
> struct arm_smmu_master *master;
> struct device_node *dev_node = dev_get_dev_node(dev);
>
> - master = find_smmu_master(smmu, dev_node);
> + master = find_smmu_master(smmu, dev);
> if (master) {
> dev_err(dev,
> - "rejecting multiple registrations for master device %s\n",
> - dev_node->name);
> + "rejecting multiple registrations for master device\n");
> return -EBUSY;
> }
>
> master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
> if (!master)
> return -ENOMEM;
> - master->of_node = dev_node;
> + master->dev = dev;
>
> - /* Xen: Let Xen know that the device is protected by an SMMU */
> - dt_device_set_protected(dev_node);
> + if ( !dev_is_pci(dev) )
> + {
> + /* Xen: Let Xen know that the device is protected by an SMMU */
> + dt_device_set_protected(dev_node);
> + }
>
> for (i = 0; i < fwspec->num_ids; ++i) {
> if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
> (fwspec->ids[i] >= smmu->num_mapping_groups)) {
> dev_err(dev,
> - "stream ID for master device %s greater than maximum allowed (%d)\n",
> - dev_node->name, smmu->num_mapping_groups);
> + "SMMU stream ID %d is greater than maximum allowed (%d)\n",
> + fwspec->ids[i], smmu->num_mapping_groups);
> return -ERANGE;
> }
> master->cfg.smendx[i] = INVALID_SMENDX;
> @@ -872,7 +876,7 @@ static int arm_smmu_dt_remove_device_legacy(struct arm_smmu_device *smmu,
> struct device_node *dev_node = dev_get_dev_node(dev);
> int ret;
>
> - master = find_smmu_master(smmu, dev_node);
> + master = find_smmu_master(smmu, dev);
> if (master == NULL) {
> dev_err(dev,
> "No registrations found for master device %s\n",
> @@ -884,8 +888,9 @@ static int arm_smmu_dt_remove_device_legacy(struct arm_smmu_device *smmu,
> if (ret)
> return ret;
>
> - /* Protected by dt_host_lock and dtdevs_lock as caller holds these locks. */
> - dev_node->is_protected = false;
> + if ( !dev_is_pci(dev) )
> + /* Protected by dt_host_lock and dtdevs_lock as caller holds these locks. */
> + dev_node->is_protected = false;
>
> kfree(master);
> return 0;
> @@ -914,6 +919,12 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
> fwspec);
> }
>
> +/* Forward declaration */
> +static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
> + struct device *dev, u32 flag);
> +static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn,
> + struct device *dev);
> +
> /*
> * The driver which supports generic IOMMU DT bindings must have this
> * callback implemented.
> @@ -938,6 +949,23 @@ static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
> {
> struct arm_smmu_device *smmu;
> struct iommu_fwspec *fwspec;
> + int ret;
> +
> +#ifdef CONFIG_HAS_PCI
> + if ( dev_is_pci(dev) )
> + {
> + struct pci_dev *pdev = dev_to_pci(dev);
> + int ret;
> +
> + /* Ignore calls for phantom functions */
> + if ( devfn != pdev->devfn )
> + return 0;
> +
> + ret = iommu_add_pci_sideband_ids(pdev);
> + if ( ret < 0 )
> + iommu_fwspec_free(dev);
Should we return here in case of errors?
> + }
> +#endif
>
> fwspec = dev_iommu_fwspec_get(dev);
> if (fwspec == NULL)
> @@ -947,7 +975,25 @@ static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
> if (smmu == NULL)
> return -ENXIO;
>
> - return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec);
> + ret = arm_smmu_dt_add_device_legacy(smmu, dev, fwspec);
> + if ( ret )
> + return ret;
> +
> +#ifdef CONFIG_HAS_PCI
> + if ( dev_is_pci(dev) )
> + {
> + struct pci_dev *pdev = dev_to_pci(dev);
> +
> + /*
> + * During PHYSDEVOP_pci_device_add, Xen does not assign the
> + * device, so we must do it here.
> + */
> + if ( pdev->domain )
> + ret = arm_smmu_assign_dev(pdev->domain, devfn, dev, 0);
> + }
> +#endif
> +
> + return ret;
> }
>
> static int arm_smmu_dt_xlate_generic(struct device *dev,
> @@ -970,11 +1016,10 @@ static struct arm_smmu_device *find_smmu_for_device(struct device *dev)
> {
> struct arm_smmu_device *smmu;
> struct arm_smmu_master *master = NULL;
> - struct device_node *dev_node = dev_get_dev_node(dev);
>
> spin_lock(&arm_smmu_devices_lock);
> list_for_each_entry(smmu, &arm_smmu_devices, list) {
> - master = find_smmu_master(smmu, dev_node);
> + master = find_smmu_master(smmu, dev);
> if (master)
> break;
> }
> @@ -2066,6 +2111,7 @@ static bool arm_smmu_capable(enum iommu_cap cap)
> }
> #endif
>
> +#if 0 /* Not used */
> static int __arm_smmu_get_pci_sid(struct pci_dev *pdev, u16 alias, void *data)
> {
> *((u16 *)data) = alias;
> @@ -2076,6 +2122,7 @@ static void __arm_smmu_release_pci_iommudata(void *data)
> {
> kfree(data);
> }
> +#endif
>
> static int arm_smmu_add_device(struct device *dev)
> {
> @@ -2083,12 +2130,13 @@ static int arm_smmu_add_device(struct device *dev)
> struct arm_smmu_master_cfg *cfg;
> struct iommu_group *group;
> void (*releasefn)(void *data) = NULL;
> - int ret;
>
> smmu = find_smmu_for_device(dev);
> if (!smmu)
> return -ENODEV;
>
> + /* There is no need to distinguish here, thanks to PCI-IOMMU DT bindings */
Let's remove this code then. This driver deviated too much from the
Linux original anyway to worry about the ability to update the code
> +#if 0
> if (dev_is_pci(dev)) {
> struct pci_dev *pdev = to_pci_dev(dev);
> struct iommu_fwspec *fwspec;
> @@ -2113,10 +2161,12 @@ static int arm_smmu_add_device(struct device *dev)
> &fwspec->ids[0]);
> releasefn = __arm_smmu_release_pci_iommudata;
> cfg->smmu = smmu;
> - } else {
> + } else
> +#endif
> + {
> struct arm_smmu_master *master;
>
> - master = find_smmu_master(smmu, dev->of_node);
> + master = find_smmu_master(smmu, dev);
> if (!master) {
> return -ENODEV;
> }
> @@ -2784,6 +2834,42 @@ static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
> return -ENOMEM;
> }
>
> +#ifdef CONFIG_HAS_PCI
> + if ( dev_is_pci(dev) )
> + {
> + struct pci_dev *pdev = dev_to_pci(dev);
> +
> + /* Ignore calls for phantom functions */
> + if ( devfn != pdev->devfn )
> + return 0;
> +
> + ASSERT(pcidevs_locked());
> +
> + write_lock(&pdev->domain->pci_lock);
> + list_del(&pdev->domain_list);
> + write_unlock(&pdev->domain->pci_lock);
> +
> + pdev->domain = d;
> +
> + write_lock(&d->pci_lock);
> + list_add(&pdev->domain_list, &d->pdev_list);
> + write_unlock(&d->pci_lock);
> +
> + /* dom_io is used as a sentinel for quarantined devices */
> + if ( d == dom_io )
> + {
> + struct iommu_domain *domain = dev_iommu_domain(dev);
> + if ( !iommu_quarantine )
> + return 0;
> +
> + if ( domain && domain->priv )
> + arm_smmu_deassign_dev(domain->priv->cfg.domain, devfn, dev);
> +
> + return 0;
> + }
> + }
> +#endif
> +
> if (!dev_iommu_group(dev)) {
> ret = arm_smmu_add_device(dev);
> if (ret)
> @@ -2833,11 +2919,27 @@ out:
> return ret;
> }
>
> -static int arm_smmu_deassign_dev(struct domain *d, struct device *dev)
> +static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn,
> + struct device *dev)
> {
> struct iommu_domain *domain = dev_iommu_domain(dev);
> struct arm_smmu_xen_domain *xen_domain;
>
> +#ifdef CONFIG_HAS_PCI
> + if ( dev_is_pci(dev) )
> + {
> + struct pci_dev *pdev = dev_to_pci(dev);
> +
> + /* Ignore calls for phantom functions */
> + if ( devfn != pdev->devfn )
> + return 0;
> +
> + /* dom_io is used as a sentinel for quarantined devices */
> + if ( d == dom_io )
> + return 0;
> + }
> +#endif
> +
> xen_domain = dom_iommu(d)->arch.priv;
>
> if (!domain || domain->priv->cfg.domain != d) {
> @@ -2864,14 +2966,16 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t,
> {
> int ret = 0;
>
> - /* Don't allow remapping on other domain than hwdom */
> - if ( t && !is_hardware_domain(t) )
> + /* Don't allow remapping on other domain than hwdom
> + * or dom_io for PCI devices
> + */
> + if ( t && !is_hardware_domain(t) && (t != dom_io || !dev_is_pci(dev)) )
> return -EPERM;
>
> if (t == s)
> return 0;
>
> - ret = arm_smmu_deassign_dev(s, dev);
> + ret = arm_smmu_deassign_dev(s, devfn, dev);
> if (ret)
> return ret;
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v9 5/8] xen/arm: smmuv3: Add PCI devices support for SMMUv3
2025-03-14 13:34 [PATCH v9 0/8] SMMU handling for PCIe Passthrough on ARM Mykyta Poturai
` (3 preceding siblings ...)
2025-03-14 13:34 ` [PATCH v9 4/8] xen/arm: smmuv2: Add PCI devices support for SMMUv2 Mykyta Poturai
@ 2025-03-14 13:34 ` Mykyta Poturai
2025-04-22 0:20 ` Stefano Stabellini
2025-03-14 13:34 ` [PATCH v9 6/8] xen/arm: Fix mapping for PCI bridge mmio region Mykyta Poturai
` (2 subsequent siblings)
7 siblings, 1 reply; 37+ messages in thread
From: Mykyta Poturai @ 2025-03-14 13:34 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Rahul Singh, Bertrand Marquis, Stefano Stabellini, Julien Grall,
Michal Orzel, Volodymyr Babchuk, Stewart Hildebrand,
Mykyta Poturai
From: Rahul Singh <rahul.singh@arm.com>
Implement support for PCI devices in the SMMU driver. Trigger iommu-map
parsing when new PCI device is added. Add checks to assign/deassign
functions to ensure PCI devices are handled correctly. Implement basic
quarantining.
All pci devices are automatically assigned to hardware domain if it exists
to ensure it can probe them.
TODO:
Implement scratch page quarantining support.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
v8->v9:
* no change
v7->v8:
* no change
v6->v7:
* address TODO: use d->pci_lock in arm_smmu_assign_dev()
* remove !is_hardware_domain and pdev->domain == d checks in assign to
support future dom0less use case when dom0 is using vPCI
* check if pdev->domain exists before assigning to it
* don't print ""
* change assign logic to remove reassign reimplementation
* explain pdev->devfn check
* make reassign check stricter and update comment
v5->v6:
* check for hardware_domain == NULL (dom0less test case)
* locking: assign pdev->domain before list_add()
v4->v5:
* deassign from hwdom
* add TODO regarding locking
* fixup after dropping ("xen/arm: Move is_protected flag to struct device")
v3->v4:
* no change
v2->v3:
* rebase
* invoke iommu_add_pci_sideband_ids() from add_device hook
v1->v2:
* ignore add_device/assign_device/reassign_device calls for phantom functions
(i.e. devfn != pdev->devfn)
downstream->v1:
* rebase
* move 2 replacements of s/dt_device_set_protected(dev_to_dt(dev))/device_set_protected(dev)/
from this commit to ("xen/arm: Move is_protected flag to struct device")
so as to not break ability to bisect
* adjust patch title (remove stray space)
* arm_smmu_(de)assign_dev: return error instead of crashing system
* remove arm_smmu_remove_device() stub
* update condition in arm_smmu_reassign_dev
* style fixup
(cherry picked from commit 7ed6c3ab250d899fe6e893a514278e406a2893e8 from
the downstream branch poc/pci-passthrough from
https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
---
xen/drivers/passthrough/arm/smmu-v3.c | 117 +++++++++++++++++++++++---
1 file changed, 106 insertions(+), 11 deletions(-)
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index cee5724022..9c7c13f800 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -1467,14 +1467,35 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
}
/* Forward declaration */
static struct arm_smmu_device *arm_smmu_get_by_dev(const struct device *dev);
+static int arm_smmu_assign_dev(struct domain *d, u8 devfn, struct device *dev,
+ u32 flag);
+static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn,
+ struct device *dev);
static int arm_smmu_add_device(u8 devfn, struct device *dev)
{
int i, ret;
struct arm_smmu_device *smmu;
struct arm_smmu_master *master;
- struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct iommu_fwspec *fwspec;
+
+#ifdef CONFIG_HAS_PCI
+ if ( dev_is_pci(dev) )
+ {
+ struct pci_dev *pdev = dev_to_pci(dev);
+ int ret;
+
+ /* Ignore calls for phantom functions */
+ if ( devfn != pdev->devfn )
+ return 0;
+
+ ret = iommu_add_pci_sideband_ids(pdev);
+ if ( ret < 0 )
+ iommu_fwspec_free(dev);
+ }
+#endif
+ fwspec = dev_iommu_fwspec_get(dev);
if (!fwspec)
return -ENODEV;
@@ -1519,17 +1540,38 @@ static int arm_smmu_add_device(u8 devfn, struct device *dev)
*/
arm_smmu_enable_pasid(master);
- if (dt_device_is_protected(dev_to_dt(dev))) {
- dev_err(dev, "Already added to SMMUv3\n");
- return -EEXIST;
- }
+ if ( !dev_is_pci(dev) )
+ {
+ if (dt_device_is_protected(dev_to_dt(dev))) {
+ dev_err(dev, "Already added to SMMUv3\n");
+ return -EEXIST;
+ }
- /* Let Xen know that the master device is protected by an IOMMU. */
- dt_device_set_protected(dev_to_dt(dev));
+ /* Let Xen know that the master device is protected by an IOMMU. */
+ dt_device_set_protected(dev_to_dt(dev));
+ }
dev_info(dev, "Added master device (SMMUv3 %s StreamIds %u)\n",
dev_name(fwspec->iommu_dev), fwspec->num_ids);
+#ifdef CONFIG_HAS_PCI
+ if ( dev_is_pci(dev) )
+ {
+ struct pci_dev *pdev = dev_to_pci(dev);
+
+ /*
+ * During PHYSDEVOP_pci_device_add, Xen does not assign the
+ * device, so we must do it here.
+ */
+ if ( pdev->domain )
+ {
+ ret = arm_smmu_assign_dev(pdev->domain, devfn, dev, 0);
+ if (ret)
+ goto err_free_master;
+ }
+ }
+#endif
+
return 0;
err_free_master:
@@ -2622,6 +2664,42 @@ static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
struct arm_smmu_domain *smmu_domain;
struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
+#ifdef CONFIG_HAS_PCI
+ if ( dev_is_pci(dev) )
+ {
+ struct pci_dev *pdev = dev_to_pci(dev);
+
+ /* Ignore calls for phantom functions */
+ if ( devfn != pdev->devfn )
+ return 0;
+
+ ASSERT(pcidevs_locked());
+
+ write_lock(&pdev->domain->pci_lock);
+ list_del(&pdev->domain_list);
+ write_unlock(&pdev->domain->pci_lock);
+
+ pdev->domain = d;
+
+ write_lock(&d->pci_lock);
+ list_add(&pdev->domain_list, &d->pdev_list);
+ write_unlock(&d->pci_lock);
+
+ /* dom_io is used as a sentinel for quarantined devices */
+ if ( d == dom_io )
+ {
+ struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+ if ( !iommu_quarantine )
+ return 0;
+
+ if ( master && master->domain )
+ arm_smmu_deassign_dev(master->domain->d, devfn, dev);
+
+ return 0;
+ }
+ }
+#endif
+
spin_lock(&xen_domain->lock);
/*
@@ -2655,7 +2733,7 @@ out:
return ret;
}
-static int arm_smmu_deassign_dev(struct domain *d, struct device *dev)
+static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn, struct device *dev)
{
struct iommu_domain *io_domain = arm_smmu_get_domain(d, dev);
struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
@@ -2667,6 +2745,21 @@ static int arm_smmu_deassign_dev(struct domain *d, struct device *dev)
return -ESRCH;
}
+#ifdef CONFIG_HAS_PCI
+ if ( dev_is_pci(dev) )
+ {
+ struct pci_dev *pdev = dev_to_pci(dev);
+
+ /* Ignore calls for phantom functions */
+ if ( devfn != pdev->devfn )
+ return 0;
+
+ /* dom_io is used as a sentinel for quarantined devices */
+ if ( d == dom_io )
+ return 0;
+ }
+#endif
+
spin_lock(&xen_domain->lock);
arm_smmu_detach_dev(master);
@@ -2685,14 +2778,16 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t,
{
int ret = 0;
- /* Don't allow remapping on other domain than hwdom */
- if ( t && !is_hardware_domain(t) )
+ /* Don't allow remapping on other domain than hwdom
+ * or dom_io for PCI devices
+ */
+ if ( t && !is_hardware_domain(t) && (t != dom_io || !dev_is_pci(dev)) )
return -EPERM;
if (t == s)
return 0;
- ret = arm_smmu_deassign_dev(s, dev);
+ ret = arm_smmu_deassign_dev(s, devfn, dev);
if (ret)
return ret;
--
2.34.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v9 5/8] xen/arm: smmuv3: Add PCI devices support for SMMUv3
2025-03-14 13:34 ` [PATCH v9 5/8] xen/arm: smmuv3: Add PCI devices support for SMMUv3 Mykyta Poturai
@ 2025-04-22 0:20 ` Stefano Stabellini
0 siblings, 0 replies; 37+ messages in thread
From: Stefano Stabellini @ 2025-04-22 0:20 UTC (permalink / raw)
To: Mykyta Poturai
Cc: xen-devel@lists.xenproject.org, Rahul Singh, Bertrand Marquis,
Stefano Stabellini, Julien Grall, Michal Orzel, Volodymyr Babchuk,
Stewart Hildebrand
On Fri, 14 Mar 2025, Mykyta Poturai wrote:
> From: Rahul Singh <rahul.singh@arm.com>
>
> Implement support for PCI devices in the SMMU driver. Trigger iommu-map
> parsing when new PCI device is added. Add checks to assign/deassign
> functions to ensure PCI devices are handled correctly. Implement basic
> quarantining.
>
> All pci devices are automatically assigned to hardware domain if it exists
> to ensure it can probe them.
>
> TODO:
> Implement scratch page quarantining support.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> ---
> v8->v9:
> * no change
>
> v7->v8:
> * no change
>
> v6->v7:
> * address TODO: use d->pci_lock in arm_smmu_assign_dev()
> * remove !is_hardware_domain and pdev->domain == d checks in assign to
> support future dom0less use case when dom0 is using vPCI
> * check if pdev->domain exists before assigning to it
> * don't print ""
> * change assign logic to remove reassign reimplementation
> * explain pdev->devfn check
> * make reassign check stricter and update comment
>
> v5->v6:
> * check for hardware_domain == NULL (dom0less test case)
> * locking: assign pdev->domain before list_add()
>
> v4->v5:
> * deassign from hwdom
> * add TODO regarding locking
> * fixup after dropping ("xen/arm: Move is_protected flag to struct device")
>
> v3->v4:
> * no change
>
> v2->v3:
> * rebase
> * invoke iommu_add_pci_sideband_ids() from add_device hook
>
> v1->v2:
> * ignore add_device/assign_device/reassign_device calls for phantom functions
> (i.e. devfn != pdev->devfn)
>
> downstream->v1:
> * rebase
> * move 2 replacements of s/dt_device_set_protected(dev_to_dt(dev))/device_set_protected(dev)/
> from this commit to ("xen/arm: Move is_protected flag to struct device")
> so as to not break ability to bisect
> * adjust patch title (remove stray space)
> * arm_smmu_(de)assign_dev: return error instead of crashing system
> * remove arm_smmu_remove_device() stub
> * update condition in arm_smmu_reassign_dev
> * style fixup
>
> (cherry picked from commit 7ed6c3ab250d899fe6e893a514278e406a2893e8 from
> the downstream branch poc/pci-passthrough from
> https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
> ---
> xen/drivers/passthrough/arm/smmu-v3.c | 117 +++++++++++++++++++++++---
> 1 file changed, 106 insertions(+), 11 deletions(-)
>
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
> index cee5724022..9c7c13f800 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -1467,14 +1467,35 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
> }
> /* Forward declaration */
> static struct arm_smmu_device *arm_smmu_get_by_dev(const struct device *dev);
> +static int arm_smmu_assign_dev(struct domain *d, u8 devfn, struct device *dev,
> + u32 flag);
> +static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn,
> + struct device *dev);
>
> static int arm_smmu_add_device(u8 devfn, struct device *dev)
> {
> int i, ret;
> struct arm_smmu_device *smmu;
> struct arm_smmu_master *master;
> - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> + struct iommu_fwspec *fwspec;
> +
> +#ifdef CONFIG_HAS_PCI
> + if ( dev_is_pci(dev) )
> + {
> + struct pci_dev *pdev = dev_to_pci(dev);
> + int ret;
> +
> + /* Ignore calls for phantom functions */
> + if ( devfn != pdev->devfn )
> + return 0;
> +
> + ret = iommu_add_pci_sideband_ids(pdev);
> + if ( ret < 0 )
> + iommu_fwspec_free(dev);
Do we need to return on error?
> + }
> +#endif
>
> + fwspec = dev_iommu_fwspec_get(dev);
> if (!fwspec)
> return -ENODEV;
>
> @@ -1519,17 +1540,38 @@ static int arm_smmu_add_device(u8 devfn, struct device *dev)
> */
> arm_smmu_enable_pasid(master);
>
> - if (dt_device_is_protected(dev_to_dt(dev))) {
> - dev_err(dev, "Already added to SMMUv3\n");
> - return -EEXIST;
> - }
> + if ( !dev_is_pci(dev) )
> + {
> + if (dt_device_is_protected(dev_to_dt(dev))) {
> + dev_err(dev, "Already added to SMMUv3\n");
> + return -EEXIST;
> + }
>
> - /* Let Xen know that the master device is protected by an IOMMU. */
> - dt_device_set_protected(dev_to_dt(dev));
> + /* Let Xen know that the master device is protected by an IOMMU. */
> + dt_device_set_protected(dev_to_dt(dev));
> + }
>
> dev_info(dev, "Added master device (SMMUv3 %s StreamIds %u)\n",
> dev_name(fwspec->iommu_dev), fwspec->num_ids);
>
> +#ifdef CONFIG_HAS_PCI
> + if ( dev_is_pci(dev) )
> + {
> + struct pci_dev *pdev = dev_to_pci(dev);
> +
> + /*
> + * During PHYSDEVOP_pci_device_add, Xen does not assign the
> + * device, so we must do it here.
> + */
> + if ( pdev->domain )
> + {
> + ret = arm_smmu_assign_dev(pdev->domain, devfn, dev, 0);
> + if (ret)
> + goto err_free_master;
> + }
> + }
> +#endif
> +
> return 0;
>
> err_free_master:
> @@ -2622,6 +2664,42 @@ static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
> struct arm_smmu_domain *smmu_domain;
> struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
>
> +#ifdef CONFIG_HAS_PCI
> + if ( dev_is_pci(dev) )
> + {
> + struct pci_dev *pdev = dev_to_pci(dev);
> +
> + /* Ignore calls for phantom functions */
> + if ( devfn != pdev->devfn )
> + return 0;
> +
> + ASSERT(pcidevs_locked());
> +
> + write_lock(&pdev->domain->pci_lock);
> + list_del(&pdev->domain_list);
> + write_unlock(&pdev->domain->pci_lock);
> +
> + pdev->domain = d;
> +
> + write_lock(&d->pci_lock);
> + list_add(&pdev->domain_list, &d->pdev_list);
> + write_unlock(&d->pci_lock);
> +
> + /* dom_io is used as a sentinel for quarantined devices */
> + if ( d == dom_io )
> + {
> + struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> + if ( !iommu_quarantine )
> + return 0;
> +
> + if ( master && master->domain )
> + arm_smmu_deassign_dev(master->domain->d, devfn, dev);
> +
> + return 0;
> + }
> + }
> +#endif
> +
> spin_lock(&xen_domain->lock);
>
> /*
> @@ -2655,7 +2733,7 @@ out:
> return ret;
> }
>
> -static int arm_smmu_deassign_dev(struct domain *d, struct device *dev)
> +static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn, struct device *dev)
> {
> struct iommu_domain *io_domain = arm_smmu_get_domain(d, dev);
> struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
> @@ -2667,6 +2745,21 @@ static int arm_smmu_deassign_dev(struct domain *d, struct device *dev)
> return -ESRCH;
> }
>
> +#ifdef CONFIG_HAS_PCI
> + if ( dev_is_pci(dev) )
> + {
> + struct pci_dev *pdev = dev_to_pci(dev);
> +
> + /* Ignore calls for phantom functions */
> + if ( devfn != pdev->devfn )
> + return 0;
> +
> + /* dom_io is used as a sentinel for quarantined devices */
> + if ( d == dom_io )
> + return 0;
> + }
> +#endif
> +
> spin_lock(&xen_domain->lock);
>
> arm_smmu_detach_dev(master);
> @@ -2685,14 +2778,16 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t,
> {
> int ret = 0;
>
> - /* Don't allow remapping on other domain than hwdom */
> - if ( t && !is_hardware_domain(t) )
> + /* Don't allow remapping on other domain than hwdom
> + * or dom_io for PCI devices
> + */
> + if ( t && !is_hardware_domain(t) && (t != dom_io || !dev_is_pci(dev)) )
> return -EPERM;
>
> if (t == s)
> return 0;
>
> - ret = arm_smmu_deassign_dev(s, dev);
> + ret = arm_smmu_deassign_dev(s, devfn, dev);
> if (ret)
> return ret;
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v9 6/8] xen/arm: Fix mapping for PCI bridge mmio region
2025-03-14 13:34 [PATCH v9 0/8] SMMU handling for PCIe Passthrough on ARM Mykyta Poturai
` (4 preceding siblings ...)
2025-03-14 13:34 ` [PATCH v9 5/8] xen/arm: smmuv3: Add PCI devices support for SMMUv3 Mykyta Poturai
@ 2025-03-14 13:34 ` Mykyta Poturai
2025-03-14 13:34 ` [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no Mykyta Poturai
2025-03-14 13:34 ` [PATCH v9 8/8] xen/arm: Map ITS doorbell register to IOMMU page tables Mykyta Poturai
7 siblings, 0 replies; 37+ messages in thread
From: Mykyta Poturai @ 2025-03-14 13:34 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Rahul Singh, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Stewart Hildebrand, Julien Grall
From: Rahul Singh <rahul.singh@arm.com>
Current code skip the mapping for PCI bridge MMIO region to dom0 when
pci_passthrough_enabled flag is set. Mapping should be skip when
has_vpci(d) is enabled for the domain, as we need to skip the mapping
only when VPCI handler are registered for ECAM.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
This patch was originally picked up from [1]
v8->v9:
* no change
v7->v8:
* no change
v6->v7:
* add Julien's A-b
v5->v6:
* drop unrelated change in xen/arch/arm/domain_build.c:handle_linux_pci_domain()
v4->v5:
* new patch
changes since picking up from [1]:
* rebase on top of "dynamic node programming using overlay dtbo" series
* replace !is_pci_passthrough_enabled() check with !IS_ENABLED(CONFIG_HAS_PCI)
instead of removing
[1] https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg00483.html
---
xen/arch/arm/device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 5610cddcba..25847d60ee 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -268,7 +268,7 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
.d = d,
.p2mt = p2mt,
.skip_mapping = !own_device ||
- (is_pci_passthrough_enabled() &&
+ (has_vpci(d) &&
(device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
.iomem_ranges = iomem_ranges,
.irq_ranges = irq_ranges
--
2.34.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-03-14 13:34 [PATCH v9 0/8] SMMU handling for PCIe Passthrough on ARM Mykyta Poturai
` (5 preceding siblings ...)
2025-03-14 13:34 ` [PATCH v9 6/8] xen/arm: Fix mapping for PCI bridge mmio region Mykyta Poturai
@ 2025-03-14 13:34 ` Mykyta Poturai
2025-03-17 15:07 ` Jan Beulich
` (2 more replies)
2025-03-14 13:34 ` [PATCH v9 8/8] xen/arm: Map ITS doorbell register to IOMMU page tables Mykyta Poturai
7 siblings, 3 replies; 37+ messages in thread
From: Mykyta Poturai @ 2025-03-14 13:34 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Stewart Hildebrand, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Jan Beulich, Roger Pau Monné, Mykyta Poturai
From: Stewart Hildebrand <stewart.hildebrand@amd.com>
Enable the use of IOMMU + PCI in dom0 without having to specify
"pci-passthrough=yes". We rely on dom0 to initialize the PCI controller
and perform a PHYSDEVOP_pci_device_add call to add each device to SMMU.
Enable pci_init() for initializing Xen's internal PCI subsystem, and
allow PHYSDEVOP_pci_device_add when pci-passthrough is disabled.
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
hmm. Since
dec9e02f3190 ("xen: avoid generation of stub <asm/pci.h> header")
Should we also move is_pci_passthrough_enabled() back to xen/arch/arm/include/asm/pci.h ?
Not sure if PPC/RISC-V will plan on using this check.
v8-v9:
* move iommu_enabled check inside is_pci_passthrough_enabled()
v7->v8:
* bring back x86 definition of is_pci_passthrough_enabled()
v6->v7:
* remove x86 definition of is_pci_passthrough_enabled()
* update comments
* make pci_physdev_op checks stricter
v5->v6:
* new patch - this effectively replaces
("Revert "xen/arm: Add cmdline boot option "pci-passthrough = <boolean>""")
---
xen/arch/arm/domain_build.c | 2 +-
xen/arch/arm/include/asm/pci.h | 5 +----
xen/arch/arm/pci/pci.c | 11 ++++++++++-
xen/arch/x86/include/asm/pci.h | 2 +-
xen/drivers/pci/physdev.c | 4 ++--
xen/include/xen/pci.h | 2 +-
6 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 7b47abade1..fbd6db9438 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -526,7 +526,7 @@ static int __init handle_linux_pci_domain(struct kernel_info *kinfo,
uint16_t segment;
int res;
- if ( !is_pci_passthrough_enabled() )
+ if ( !is_pci_passthrough_enabled(false) )
return 0;
if ( !dt_device_type_is_equal(node, "pci") )
diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 7f77226c9b..3ae85b4666 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -111,10 +111,7 @@ pci_find_host_bridge_node(const struct pci_dev *pdev);
int pci_get_host_bridge_segment(const struct dt_device_node *node,
uint16_t *segment);
-static always_inline bool is_pci_passthrough_enabled(void)
-{
- return pci_passthrough_enabled;
-}
+bool is_pci_passthrough_enabled(bool dom0);
void arch_pci_init_pdev(struct pci_dev *pdev);
diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
index 78b97beaef..79bb8728a4 100644
--- a/xen/arch/arm/pci/pci.c
+++ b/xen/arch/arm/pci/pci.c
@@ -16,9 +16,18 @@
#include <xen/device_tree.h>
#include <xen/errno.h>
#include <xen/init.h>
+#include <xen/iommu.h>
#include <xen/param.h>
#include <xen/pci.h>
+bool is_pci_passthrough_enabled(bool dom0)
+{
+ if ( dom0 )
+ return pci_passthrough_enabled || iommu_enabled;
+
+ return pci_passthrough_enabled;
+}
+
/*
* PIRQ event channels are not supported on Arm, so nothing to do.
*/
@@ -85,7 +94,7 @@ static int __init pci_init(void)
* Enable PCI passthrough when has been enabled explicitly
* (pci-passthrough=on).
*/
- if ( !pci_passthrough_enabled )
+ if ( !is_pci_passthrough_enabled(true) )
return 0;
pci_segments_init();
diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
index fd5480d67d..bffeaa507d 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -50,7 +50,7 @@ extern int pci_mmcfg_config_num;
extern struct acpi_mcfg_allocation *pci_mmcfg_config;
/* Unlike ARM, PCI passthrough is always enabled for x86. */
-static always_inline bool is_pci_passthrough_enabled(void)
+static always_inline bool is_pci_passthrough_enabled(__maybe_unused bool dom0)
{
return true;
}
diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
index 0161a85e1e..18448b94b3 100644
--- a/xen/drivers/pci/physdev.c
+++ b/xen/drivers/pci/physdev.c
@@ -19,7 +19,7 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
struct pci_dev_info pdev_info;
nodeid_t node = NUMA_NO_NODE;
- if ( !is_pci_passthrough_enabled() )
+ if ( !is_pci_passthrough_enabled(true) )
return -EOPNOTSUPP;
ret = -EFAULT;
@@ -57,7 +57,7 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
case PHYSDEVOP_pci_device_remove: {
struct physdev_pci_device dev;
- if ( !is_pci_passthrough_enabled() )
+ if ( !is_pci_passthrough_enabled(true) )
return -EOPNOTSUPP;
ret = -EFAULT;
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index f784e91160..c4a49cf584 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -74,7 +74,7 @@ typedef union {
struct arch_pci_dev { };
-static inline bool is_pci_passthrough_enabled(void)
+static inline bool is_pci_passthrough_enabled(__maybe_unused bool dom0)
{
return false;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-03-14 13:34 ` [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no Mykyta Poturai
@ 2025-03-17 15:07 ` Jan Beulich
2025-03-21 10:56 ` Mykyta Poturai
2025-04-28 8:21 ` Mykyta Poturai
2025-04-22 0:25 ` Stefano Stabellini
2025-04-28 8:54 ` Julien Grall
2 siblings, 2 replies; 37+ messages in thread
From: Jan Beulich @ 2025-03-17 15:07 UTC (permalink / raw)
To: Mykyta Poturai
Cc: Stewart Hildebrand, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Roger Pau Monné,
xen-devel@lists.xenproject.org
On 14.03.2025 14:34, Mykyta Poturai wrote:
> --- a/xen/arch/arm/pci/pci.c
> +++ b/xen/arch/arm/pci/pci.c
> @@ -16,9 +16,18 @@
> #include <xen/device_tree.h>
> #include <xen/errno.h>
> #include <xen/init.h>
> +#include <xen/iommu.h>
> #include <xen/param.h>
> #include <xen/pci.h>
>
> +bool is_pci_passthrough_enabled(bool dom0)
> +{
> + if ( dom0 )
> + return pci_passthrough_enabled || iommu_enabled;
As I think I said before - the function's name now no longer expresses
what it really checks. That (imo heavily) misleading at the use sites
of this function.
> + return pci_passthrough_enabled;
> +}
Personally I also view it as undesirable that the global
pci_passthrough_enabled is evaluated twice in this function. Better
might be e.g.
bool is_pci_passthrough_enabled(bool dom0)
{
if ( pci_passthrough_enabled )
return true;
return dom0 && iommu_enabled;
}
Yet then I'm not a maintainer of this code.
> @@ -85,7 +94,7 @@ static int __init pci_init(void)
> * Enable PCI passthrough when has been enabled explicitly
> * (pci-passthrough=on).
> */
> - if ( !pci_passthrough_enabled )
> + if ( !is_pci_passthrough_enabled(true) )
There's no Dom0 in sight anywhere here, is there? How can you pass true
as argument for the "dom0" parameter?
> --- a/xen/arch/x86/include/asm/pci.h
> +++ b/xen/arch/x86/include/asm/pci.h
> @@ -50,7 +50,7 @@ extern int pci_mmcfg_config_num;
> extern struct acpi_mcfg_allocation *pci_mmcfg_config;
>
> /* Unlike ARM, PCI passthrough is always enabled for x86. */
> -static always_inline bool is_pci_passthrough_enabled(void)
> +static always_inline bool is_pci_passthrough_enabled(__maybe_unused bool dom0)
Function parmeters don't need such annotation.
> --- a/xen/drivers/pci/physdev.c
> +++ b/xen/drivers/pci/physdev.c
> @@ -19,7 +19,7 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> struct pci_dev_info pdev_info;
> nodeid_t node = NUMA_NO_NODE;
>
> - if ( !is_pci_passthrough_enabled() )
> + if ( !is_pci_passthrough_enabled(true) )
> return -EOPNOTSUPP;
Seeing the function's parameter name, how do you know it's Dom0 calling
here?
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-03-17 15:07 ` Jan Beulich
@ 2025-03-21 10:56 ` Mykyta Poturai
2025-03-21 13:41 ` Jan Beulich
2025-04-28 8:21 ` Mykyta Poturai
1 sibling, 1 reply; 37+ messages in thread
From: Mykyta Poturai @ 2025-03-21 10:56 UTC (permalink / raw)
To: Jan Beulich
Cc: Stewart Hildebrand, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Roger Pau Monné,
xen-devel@lists.xenproject.org
On 17.03.25 17:07, Jan Beulich wrote:
> On 14.03.2025 14:34, Mykyta Poturai wrote:
>> --- a/xen/arch/arm/pci/pci.c
>> +++ b/xen/arch/arm/pci/pci.c
>> @@ -16,9 +16,18 @@
>> #include <xen/device_tree.h>
>> #include <xen/errno.h>
>> #include <xen/init.h>
>> +#include <xen/iommu.h>
>> #include <xen/param.h>
>> #include <xen/pci.h>
>>
>> +bool is_pci_passthrough_enabled(bool dom0)
>> +{
>> + if ( dom0 )
>> + return pci_passthrough_enabled || iommu_enabled;
>
> As I think I said before - the function's name now no longer expresses
> what it really checks. That (imo heavily) misleading at the use sites
> of this function.
I am afraid I don't understand your concern. It still checks if PCI
passthrough is enabled. With just the change that ARM needs some extra
logic for Dom0 PCI to work properly. Maybe change the parameter name to
something like "for_pci_hwdom"?
>> + return pci_passthrough_enabled;
>> +}
>
> Personally I also view it as undesirable that the global
> pci_passthrough_enabled is evaluated twice in this function. Better
> might be e.g.
>
> bool is_pci_passthrough_enabled(bool dom0)
> {
> if ( pci_passthrough_enabled )
> return true;
>
> return dom0 && iommu_enabled;
> }
>
> Yet then I'm not a maintainer of this code.
>
Agree, will change in the next version.
>> @@ -85,7 +94,7 @@ static int __init pci_init(void)
>> * Enable PCI passthrough when has been enabled explicitly
>> * (pci-passthrough=on).
>> */
>> - if ( !pci_passthrough_enabled )
>> + if ( !is_pci_passthrough_enabled(true) )
>
> There's no Dom0 in sight anywhere here, is there? How can you pass true
> as argument for the "dom0" parameter?
>
This should be read as "is pci passthrough enabled for Dom0?" and if it
is we definitely need to do a PCI init.
I've also done some investigations on possible ways to remove the
Dom0/other domains distinction, but I'm afraid this is the most
reasonable way to make PCI functional on Dom0 without explicitly
enabling PCI passthrough.
SMMU is configured to trigger a fault on all transactions by default and
we can't statically map PCI devices to Dom0, so the only other way is to
put PCI in full passthrough mode, which I think is not safe enough.
And we also can't drop this patch as it was directly requested by Julien
here [1].
>> --- a/xen/arch/x86/include/asm/pci.h
>> +++ b/xen/arch/x86/include/asm/pci.h
>> @@ -50,7 +50,7 @@ extern int pci_mmcfg_config_num;
>> extern struct acpi_mcfg_allocation *pci_mmcfg_config;
>>
>> /* Unlike ARM, PCI passthrough is always enabled for x86. */
>> -static always_inline bool is_pci_passthrough_enabled(void)
>> +static always_inline bool is_pci_passthrough_enabled(__maybe_unused bool dom0)
>
> Function parmeters don't need such annotation.
>
Got it.
>> --- a/xen/drivers/pci/physdev.c
>> +++ b/xen/drivers/pci/physdev.c
>> @@ -19,7 +19,7 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> struct pci_dev_info pdev_info;
>> nodeid_t node = NUMA_NO_NODE;
>>
>> - if ( !is_pci_passthrough_enabled() )
>> + if ( !is_pci_passthrough_enabled(true) )
>> return -EOPNOTSUPP;
>
> Seeing the function's parameter name, how do you know it's Dom0 calling
> here?
>
> Jan
Is this a functional or naming concern? If it is about naming then can
it also be solved by renaming the parameter?
Regarding functional issues, I have assumed that only hwdom can make
physdev operations, but after checking it, this assumption seems to be
correct on x86 but wrong on Arm.
I expected there would be a check in do_arm_physdev_op() or somewhere
near it, similar to how it is done in x86, but there are none. I'm not
sure if it is intentional or by mistake, I think it needs some
clarification by Arm folks.
[1]
https://patchew.org/Xen/20230607030220.22698-1-stewart.hildebrand@amd.com/#2731b06d-4a54-f51c-2285-ea2cf1fac3ba@xen.org
--
Mykyta
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-03-21 10:56 ` Mykyta Poturai
@ 2025-03-21 13:41 ` Jan Beulich
2025-03-21 14:50 ` Mykyta Poturai
0 siblings, 1 reply; 37+ messages in thread
From: Jan Beulich @ 2025-03-21 13:41 UTC (permalink / raw)
To: Mykyta Poturai
Cc: Stewart Hildebrand, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Roger Pau Monné,
xen-devel@lists.xenproject.org
On 21.03.2025 11:56, Mykyta Poturai wrote:
> On 17.03.25 17:07, Jan Beulich wrote:
>> On 14.03.2025 14:34, Mykyta Poturai wrote:
>>> --- a/xen/arch/arm/pci/pci.c
>>> +++ b/xen/arch/arm/pci/pci.c
>>> @@ -16,9 +16,18 @@
>>> #include <xen/device_tree.h>
>>> #include <xen/errno.h>
>>> #include <xen/init.h>
>>> +#include <xen/iommu.h>
>>> #include <xen/param.h>
>>> #include <xen/pci.h>
>>>
>>> +bool is_pci_passthrough_enabled(bool dom0)
>>> +{
>>> + if ( dom0 )
>>> + return pci_passthrough_enabled || iommu_enabled;
>>
>> As I think I said before - the function's name now no longer expresses
>> what it really checks. That (imo heavily) misleading at the use sites
>> of this function.
>
> I am afraid I don't understand your concern. It still checks if PCI
> passthrough is enabled. With just the change that ARM needs some extra
> logic for Dom0 PCI to work properly.
Conceptually there's no such thing as "pass through" for Dom0. Hence the
name of the function itself isn't correctly reflecting what it's checking
for. iommu_enabled is a prereq for pass-through to be enabled, but it
doesn't imply that's necessarily the case.
> Maybe change the parameter name to
> something like "for_pci_hwdom"?
That may help below, yes. But not here.
> >>> @@ -85,7 +94,7 @@ static int __init pci_init(void)
>>> * Enable PCI passthrough when has been enabled explicitly
>>> * (pci-passthrough=on).
>>> */
>>> - if ( !pci_passthrough_enabled )
>>> + if ( !is_pci_passthrough_enabled(true) )
>>
>> There's no Dom0 in sight anywhere here, is there? How can you pass true
>> as argument for the "dom0" parameter?
>
> This should be read as "is pci passthrough enabled for Dom0?" and if it
> is we definitely need to do a PCI init.
>
> I've also done some investigations on possible ways to remove the
> Dom0/other domains distinction, but I'm afraid this is the most
> reasonable way to make PCI functional on Dom0 without explicitly
> enabling PCI passthrough.
>
> SMMU is configured to trigger a fault on all transactions by default and
> we can't statically map PCI devices to Dom0, so the only other way is to
> put PCI in full passthrough mode, which I think is not safe enough.
> And we also can't drop this patch as it was directly requested by Julien
> here [1].
>
>>> --- a/xen/drivers/pci/physdev.c
>>> +++ b/xen/drivers/pci/physdev.c
>>> @@ -19,7 +19,7 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>> struct pci_dev_info pdev_info;
>>> nodeid_t node = NUMA_NO_NODE;
>>>
>>> - if ( !is_pci_passthrough_enabled() )
>>> + if ( !is_pci_passthrough_enabled(true) )
>>> return -EOPNOTSUPP;
>>
>> Seeing the function's parameter name, how do you know it's Dom0 calling
>> here?
>
> Is this a functional or naming concern? If it is about naming then can
> it also be solved by renaming the parameter?
The renaming suggested above would resolve this, yes. Whether "for_pci_hwdom"
or anything alike is a good parameter name is a different question.
> Regarding functional issues, I have assumed that only hwdom can make
> physdev operations, but after checking it, this assumption seems to be
> correct on x86 but wrong on Arm.
> I expected there would be a check in do_arm_physdev_op() or somewhere
> near it, similar to how it is done in x86, but there are none. I'm not
> sure if it is intentional or by mistake, I think it needs some
> clarification by Arm folks.
Hmm, looking at x86'es do_physdev_op() I fear I can't see such a check there
either. And indeed there are certain PHYSDEVOP_* which DomU-s may also make
use of.
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-03-21 13:41 ` Jan Beulich
@ 2025-03-21 14:50 ` Mykyta Poturai
2025-03-21 15:03 ` Jan Beulich
0 siblings, 1 reply; 37+ messages in thread
From: Mykyta Poturai @ 2025-03-21 14:50 UTC (permalink / raw)
To: Jan Beulich
Cc: Stewart Hildebrand, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Roger Pau Monné,
xen-devel@lists.xenproject.org
On 21.03.25 15:41, Jan Beulich wrote:
> On 21.03.2025 11:56, Mykyta Poturai wrote:
>> On 17.03.25 17:07, Jan Beulich wrote:
>>> On 14.03.2025 14:34, Mykyta Poturai wrote:
>>>> --- a/xen/arch/arm/pci/pci.c
>>>> +++ b/xen/arch/arm/pci/pci.c
>>>> @@ -16,9 +16,18 @@
>>>> #include <xen/device_tree.h>
>>>> #include <xen/errno.h>
>>>> #include <xen/init.h>
>>>> +#include <xen/iommu.h>
>>>> #include <xen/param.h>
>>>> #include <xen/pci.h>
>>>>
>>>> +bool is_pci_passthrough_enabled(bool dom0)
>>>> +{
>>>> + if ( dom0 )
>>>> + return pci_passthrough_enabled || iommu_enabled;
>>>
>>> As I think I said before - the function's name now no longer expresses
>>> what it really checks. That (imo heavily) misleading at the use sites
>>> of this function.
>>
>> I am afraid I don't understand your concern. It still checks if PCI
>> passthrough is enabled. With just the change that ARM needs some extra
>> logic for Dom0 PCI to work properly.
>
> Conceptually there's no such thing as "pass through" for Dom0. Hence the
> name of the function itself isn't correctly reflecting what it's checking
> for. iommu_enabled is a prereq for pass-through to be enabled, but it
> doesn't imply that's necessarily the case.
Okay, now I think I get it. Yes from that point of view it seems kind of
wrong. Maybe use a separate function then, something like "hwdom_has_vpci"?
>> Maybe change the parameter name to
>> something like "for_pci_hwdom"?
>
> That may help below, yes. But not here.
>
>>>>> @@ -85,7 +94,7 @@ static int __init pci_init(void)
>>>> * Enable PCI passthrough when has been enabled explicitly
>>>> * (pci-passthrough=on).
>>>> */
>>>> - if ( !pci_passthrough_enabled )
>>>> + if ( !is_pci_passthrough_enabled(true) )
>>>
>>> There's no Dom0 in sight anywhere here, is there? How can you pass true
>>> as argument for the "dom0" parameter?
>>
>> This should be read as "is pci passthrough enabled for Dom0?" and if it
>> is we definitely need to do a PCI init.
>>
>> I've also done some investigations on possible ways to remove the
>> Dom0/other domains distinction, but I'm afraid this is the most
>> reasonable way to make PCI functional on Dom0 without explicitly
>> enabling PCI passthrough.
>>
>> SMMU is configured to trigger a fault on all transactions by default and
>> we can't statically map PCI devices to Dom0, so the only other way is to
>> put PCI in full passthrough mode, which I think is not safe enough.
>> And we also can't drop this patch as it was directly requested by Julien
>> here [1].
>>
>>>> --- a/xen/drivers/pci/physdev.c
>>>> +++ b/xen/drivers/pci/physdev.c
>>>> @@ -19,7 +19,7 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>> struct pci_dev_info pdev_info;
>>>> nodeid_t node = NUMA_NO_NODE;
>>>>
>>>> - if ( !is_pci_passthrough_enabled() )
>>>> + if ( !is_pci_passthrough_enabled(true) )
>>>> return -EOPNOTSUPP;
>>>
>>> Seeing the function's parameter name, how do you know it's Dom0 calling
>>> here?
>>
>> Is this a functional or naming concern? If it is about naming then can
>> it also be solved by renaming the parameter?
>
> The renaming suggested above would resolve this, yes. Whether "for_pci_hwdom"
> or anything alike is a good parameter name is a different question.
>
>> Regarding functional issues, I have assumed that only hwdom can make
>> physdev operations, but after checking it, this assumption seems to be
>> correct on x86 but wrong on Arm.
>> I expected there would be a check in do_arm_physdev_op() or somewhere
>> near it, similar to how it is done in x86, but there are none. I'm not
>> sure if it is intentional or by mistake, I think it needs some
>> clarification by Arm folks.
>
> Hmm, looking at x86'es do_physdev_op() I fear I can't see such a check there
> either. And indeed there are certain PHYSDEVOP_* which DomU-s may also make
> use of.
>
> Jan
It is one level above in hvm_physdev_op()
case PHYSDEVOP_setup_gsi:
case PHYSDEVOP_pci_mmcfg_reserved:
case PHYSDEVOP_pci_device_add:
case PHYSDEVOP_pci_device_remove:
case PHYSDEVOP_pci_device_reset:
case PHYSDEVOP_dbgp_op:
if ( !is_hardware_domain(currd) )
return -ENOSYS;
break;
--
Mykyta
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-03-21 14:50 ` Mykyta Poturai
@ 2025-03-21 15:03 ` Jan Beulich
0 siblings, 0 replies; 37+ messages in thread
From: Jan Beulich @ 2025-03-21 15:03 UTC (permalink / raw)
To: Mykyta Poturai
Cc: Stewart Hildebrand, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Roger Pau Monné,
xen-devel@lists.xenproject.org
On 21.03.2025 15:50, Mykyta Poturai wrote:
> On 21.03.25 15:41, Jan Beulich wrote:
>> On 21.03.2025 11:56, Mykyta Poturai wrote:
>>> On 17.03.25 17:07, Jan Beulich wrote:
>>>> On 14.03.2025 14:34, Mykyta Poturai wrote:
>>>>> --- a/xen/drivers/pci/physdev.c
>>>>> +++ b/xen/drivers/pci/physdev.c
>>>>> @@ -19,7 +19,7 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>> struct pci_dev_info pdev_info;
>>>>> nodeid_t node = NUMA_NO_NODE;
>>>>>
>>>>> - if ( !is_pci_passthrough_enabled() )
>>>>> + if ( !is_pci_passthrough_enabled(true) )
>>>>> return -EOPNOTSUPP;
>>>>
>>>> Seeing the function's parameter name, how do you know it's Dom0 calling
>>>> here?
>>>
>>> Is this a functional or naming concern? If it is about naming then can
>>> it also be solved by renaming the parameter?
>>
>> The renaming suggested above would resolve this, yes. Whether "for_pci_hwdom"
>> or anything alike is a good parameter name is a different question.
>>
>>> Regarding functional issues, I have assumed that only hwdom can make
>>> physdev operations, but after checking it, this assumption seems to be
>>> correct on x86 but wrong on Arm.
>>> I expected there would be a check in do_arm_physdev_op() or somewhere
>>> near it, similar to how it is done in x86, but there are none. I'm not
>>> sure if it is intentional or by mistake, I think it needs some
>>> clarification by Arm folks.
>>
>> Hmm, looking at x86'es do_physdev_op() I fear I can't see such a check there
>> either. And indeed there are certain PHYSDEVOP_* which DomU-s may also make
>> use of.
>
> It is one level above in hvm_physdev_op()
>
> case PHYSDEVOP_setup_gsi:
> case PHYSDEVOP_pci_mmcfg_reserved:
> case PHYSDEVOP_pci_device_add:
> case PHYSDEVOP_pci_device_remove:
> case PHYSDEVOP_pci_device_reset:
> case PHYSDEVOP_dbgp_op:
> if ( !is_hardware_domain(currd) )
> return -ENOSYS;
> break;
But that's for just HVM guests, and only a functional restriction, not a
security one.
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-03-17 15:07 ` Jan Beulich
2025-03-21 10:56 ` Mykyta Poturai
@ 2025-04-28 8:21 ` Mykyta Poturai
2025-04-28 9:01 ` Jan Beulich
1 sibling, 1 reply; 37+ messages in thread
From: Mykyta Poturai @ 2025-04-28 8:21 UTC (permalink / raw)
To: Jan Beulich
Cc: Stewart Hildebrand, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Roger Pau Monné,
xen-devel@lists.xenproject.org
On 17.03.25 17:07, Jan Beulich wrote:
> On 14.03.2025 14:34, Mykyta Poturai wrote:
>> --- a/xen/arch/arm/pci/pci.c
>> +++ b/xen/arch/arm/pci/pci.c
>> @@ -16,9 +16,18 @@
>> #include <xen/device_tree.h>
>> #include <xen/errno.h>
>> #include <xen/init.h>
>> +#include <xen/iommu.h>
>> #include <xen/param.h>
>> #include <xen/pci.h>
>>
>> +bool is_pci_passthrough_enabled(bool dom0)
>> +{
>> + if ( dom0 )
>> + return pci_passthrough_enabled || iommu_enabled;
>
> As I think I said before - the function's name now no longer expresses
> what it really checks. That (imo heavily) misleading at the use sites
> of this function.
Hi Jan,
I've spent some more time thinking about how to better deal with this.
In the end, I think your earlier suggestion about introducing a new arch
specific function is the best approach, but I want to agree on the
naming before sending new patches. Would "arch_requires_pci_physdev" be
an appropriate name in your opinion?
At the call sites it will look like this:
case PHYSDEVOP_pci_device_remove: {
struct physdev_pci_device dev;
if ( !is_pci_passthrough_enabled() && !arch_requires_pci_physdev())
return -EOPNOTSUPP;
--
Mykyta
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-04-28 8:21 ` Mykyta Poturai
@ 2025-04-28 9:01 ` Jan Beulich
2025-04-28 11:26 ` Mykyta Poturai
0 siblings, 1 reply; 37+ messages in thread
From: Jan Beulich @ 2025-04-28 9:01 UTC (permalink / raw)
To: Mykyta Poturai
Cc: Stewart Hildebrand, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Roger Pau Monné,
xen-devel@lists.xenproject.org
On 28.04.2025 10:21, Mykyta Poturai wrote:
> On 17.03.25 17:07, Jan Beulich wrote:
>> On 14.03.2025 14:34, Mykyta Poturai wrote:
>>> --- a/xen/arch/arm/pci/pci.c
>>> +++ b/xen/arch/arm/pci/pci.c
>>> @@ -16,9 +16,18 @@
>>> #include <xen/device_tree.h>
>>> #include <xen/errno.h>
>>> #include <xen/init.h>
>>> +#include <xen/iommu.h>
>>> #include <xen/param.h>
>>> #include <xen/pci.h>
>>>
>>> +bool is_pci_passthrough_enabled(bool dom0)
>>> +{
>>> + if ( dom0 )
>>> + return pci_passthrough_enabled || iommu_enabled;
>>
>> As I think I said before - the function's name now no longer expresses
>> what it really checks. That (imo heavily) misleading at the use sites
>> of this function.
>
> I've spent some more time thinking about how to better deal with this.
> In the end, I think your earlier suggestion about introducing a new arch
> specific function is the best approach, but I want to agree on the
> naming before sending new patches. Would "arch_requires_pci_physdev" be
> an appropriate name in your opinion?
>
> At the call sites it will look like this:
> case PHYSDEVOP_pci_device_remove: {
> struct physdev_pci_device dev;
>
> if ( !is_pci_passthrough_enabled() && !arch_requires_pci_physdev())
> return -EOPNOTSUPP;
There are several questions that affect naming: Is it really "requires"? Is
it really all PCI-related physdevops? Is the ordering of naming elements in
line with what we use elsewhere (arch_ first is, but perhaps either pci or
physdevop wants to move earlier)?
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-04-28 9:01 ` Jan Beulich
@ 2025-04-28 11:26 ` Mykyta Poturai
2025-04-28 11:36 ` Jan Beulich
0 siblings, 1 reply; 37+ messages in thread
From: Mykyta Poturai @ 2025-04-28 11:26 UTC (permalink / raw)
To: Jan Beulich
Cc: Stewart Hildebrand, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Roger Pau Monné,
xen-devel@lists.xenproject.org
On 28.04.25 12:01, Jan Beulich wrote:
> On 28.04.2025 10:21, Mykyta Poturai wrote:
>> On 17.03.25 17:07, Jan Beulich wrote:
>>> On 14.03.2025 14:34, Mykyta Poturai wrote:
>>>> --- a/xen/arch/arm/pci/pci.c
>>>> +++ b/xen/arch/arm/pci/pci.c
>>>> @@ -16,9 +16,18 @@
>>>> #include <xen/device_tree.h>
>>>> #include <xen/errno.h>
>>>> #include <xen/init.h>
>>>> +#include <xen/iommu.h>
>>>> #include <xen/param.h>
>>>> #include <xen/pci.h>
>>>>
>>>> +bool is_pci_passthrough_enabled(bool dom0)
>>>> +{
>>>> + if ( dom0 )
>>>> + return pci_passthrough_enabled || iommu_enabled;
>>>
>>> As I think I said before - the function's name now no longer expresses
>>> what it really checks. That (imo heavily) misleading at the use sites
>>> of this function.
>>
>> I've spent some more time thinking about how to better deal with this.
>> In the end, I think your earlier suggestion about introducing a new arch
>> specific function is the best approach, but I want to agree on the
>> naming before sending new patches. Would "arch_requires_pci_physdev" be
>> an appropriate name in your opinion?
>>
>> At the call sites it will look like this:
>> case PHYSDEVOP_pci_device_remove: {
>> struct physdev_pci_device dev;
>>
>> if ( !is_pci_passthrough_enabled() && !arch_requires_pci_physdev())
>> return -EOPNOTSUPP;
>
> There are several questions that affect naming: Is it really "requires"? Is
> it really all PCI-related physdevops? Is the ordering of naming elements in
> line with what we use elsewhere (arch_ first is, but perhaps either pci or
> physdevop wants to move earlier)?
>
> Jan
I understand the issue with the ordering, will
"arch_pci_requires_physdev_ops" or "arch_physdev_pci_update_required" be
better? Regarding the specific ops, only add/remove are needed, but I am
not sure how to elegantly encode this in the name. Maybe you can suggest
something better if you have something specific in mind?
--
Mykyta
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-04-28 11:26 ` Mykyta Poturai
@ 2025-04-28 11:36 ` Jan Beulich
0 siblings, 0 replies; 37+ messages in thread
From: Jan Beulich @ 2025-04-28 11:36 UTC (permalink / raw)
To: Mykyta Poturai
Cc: Stewart Hildebrand, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Roger Pau Monné,
xen-devel@lists.xenproject.org
On 28.04.2025 13:26, Mykyta Poturai wrote:
> On 28.04.25 12:01, Jan Beulich wrote:
>> On 28.04.2025 10:21, Mykyta Poturai wrote:
>>> On 17.03.25 17:07, Jan Beulich wrote:
>>>> On 14.03.2025 14:34, Mykyta Poturai wrote:
>>>>> --- a/xen/arch/arm/pci/pci.c
>>>>> +++ b/xen/arch/arm/pci/pci.c
>>>>> @@ -16,9 +16,18 @@
>>>>> #include <xen/device_tree.h>
>>>>> #include <xen/errno.h>
>>>>> #include <xen/init.h>
>>>>> +#include <xen/iommu.h>
>>>>> #include <xen/param.h>
>>>>> #include <xen/pci.h>
>>>>>
>>>>> +bool is_pci_passthrough_enabled(bool dom0)
>>>>> +{
>>>>> + if ( dom0 )
>>>>> + return pci_passthrough_enabled || iommu_enabled;
>>>>
>>>> As I think I said before - the function's name now no longer expresses
>>>> what it really checks. That (imo heavily) misleading at the use sites
>>>> of this function.
>>>
>>> I've spent some more time thinking about how to better deal with this.
>>> In the end, I think your earlier suggestion about introducing a new arch
>>> specific function is the best approach, but I want to agree on the
>>> naming before sending new patches. Would "arch_requires_pci_physdev" be
>>> an appropriate name in your opinion?
>>>
>>> At the call sites it will look like this:
>>> case PHYSDEVOP_pci_device_remove: {
>>> struct physdev_pci_device dev;
>>>
>>> if ( !is_pci_passthrough_enabled() && !arch_requires_pci_physdev())
>>> return -EOPNOTSUPP;
>>
>> There are several questions that affect naming: Is it really "requires"? Is
>> it really all PCI-related physdevops? Is the ordering of naming elements in
>> line with what we use elsewhere (arch_ first is, but perhaps either pci or
>> physdevop wants to move earlier)?
>
> I understand the issue with the ordering, will
> "arch_pci_requires_physdev_ops" or "arch_physdev_pci_update_required" be
> better? Regarding the specific ops, only add/remove are needed, but I am
> not sure how to elegantly encode this in the name. Maybe you can suggest
> something better if you have something specific in mind?
Simply arch_pci_device_physdevop()? This would also avoid the question if
it's "requires", "wants", or yet something else. (I'm not going to insist
that the verb be omitted, though. If it's included, I'd ask though that
it match in tense the "enabled" in the other predicate.) And it would cover
PHYSDEVOP_pci_device_reset as well, which sooner or later I expect you'll
discover you want/need Dom0 to issue, too.
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-03-14 13:34 ` [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no Mykyta Poturai
2025-03-17 15:07 ` Jan Beulich
@ 2025-04-22 0:25 ` Stefano Stabellini
2025-04-28 8:54 ` Julien Grall
2 siblings, 0 replies; 37+ messages in thread
From: Stefano Stabellini @ 2025-04-22 0:25 UTC (permalink / raw)
To: Mykyta Poturai
Cc: xen-devel@lists.xenproject.org, Stewart Hildebrand,
Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Andrew Cooper, Anthony PERARD, Jan Beulich,
Roger Pau Monné
On Fri, 14 Mar 2025, Mykyta Poturai wrote:
> From: Stewart Hildebrand <stewart.hildebrand@amd.com>
>
> Enable the use of IOMMU + PCI in dom0 without having to specify
> "pci-passthrough=yes". We rely on dom0 to initialize the PCI controller
> and perform a PHYSDEVOP_pci_device_add call to add each device to SMMU.
>
> Enable pci_init() for initializing Xen's internal PCI subsystem, and
> allow PHYSDEVOP_pci_device_add when pci-passthrough is disabled.
>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> ---
> hmm. Since
> dec9e02f3190 ("xen: avoid generation of stub <asm/pci.h> header")
> Should we also move is_pci_passthrough_enabled() back to xen/arch/arm/include/asm/pci.h ?
> Not sure if PPC/RISC-V will plan on using this check.
>
> v8-v9:
> * move iommu_enabled check inside is_pci_passthrough_enabled()
>
> v7->v8:
> * bring back x86 definition of is_pci_passthrough_enabled()
>
> v6->v7:
> * remove x86 definition of is_pci_passthrough_enabled()
> * update comments
> * make pci_physdev_op checks stricter
>
> v5->v6:
> * new patch - this effectively replaces
> ("Revert "xen/arm: Add cmdline boot option "pci-passthrough = <boolean>""")
> ---
> xen/arch/arm/domain_build.c | 2 +-
> xen/arch/arm/include/asm/pci.h | 5 +----
> xen/arch/arm/pci/pci.c | 11 ++++++++++-
> xen/arch/x86/include/asm/pci.h | 2 +-
> xen/drivers/pci/physdev.c | 4 ++--
> xen/include/xen/pci.h | 2 +-
> 6 files changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 7b47abade1..fbd6db9438 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -526,7 +526,7 @@ static int __init handle_linux_pci_domain(struct kernel_info *kinfo,
> uint16_t segment;
> int res;
>
> - if ( !is_pci_passthrough_enabled() )
> + if ( !is_pci_passthrough_enabled(false) )
> return 0;
>
> if ( !dt_device_type_is_equal(node, "pci") )
> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
> index 7f77226c9b..3ae85b4666 100644
> --- a/xen/arch/arm/include/asm/pci.h
> +++ b/xen/arch/arm/include/asm/pci.h
> @@ -111,10 +111,7 @@ pci_find_host_bridge_node(const struct pci_dev *pdev);
> int pci_get_host_bridge_segment(const struct dt_device_node *node,
> uint16_t *segment);
>
> -static always_inline bool is_pci_passthrough_enabled(void)
> -{
> - return pci_passthrough_enabled;
> -}
> +bool is_pci_passthrough_enabled(bool dom0);
>
> void arch_pci_init_pdev(struct pci_dev *pdev);
>
> diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
> index 78b97beaef..79bb8728a4 100644
> --- a/xen/arch/arm/pci/pci.c
> +++ b/xen/arch/arm/pci/pci.c
> @@ -16,9 +16,18 @@
> #include <xen/device_tree.h>
> #include <xen/errno.h>
> #include <xen/init.h>
> +#include <xen/iommu.h>
> #include <xen/param.h>
> #include <xen/pci.h>
>
> +bool is_pci_passthrough_enabled(bool dom0)
> +{
> + if ( dom0 )
> + return pci_passthrough_enabled || iommu_enabled;
Could this be written as:
if ( is_hardware_domain(current->domain) )
return pci_passthrough_enabled || iommu_enabled;
If so, then I think it would be better, if not:
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> + return pci_passthrough_enabled;
> +}
> +
> /*
> * PIRQ event channels are not supported on Arm, so nothing to do.
> */
> @@ -85,7 +94,7 @@ static int __init pci_init(void)
> * Enable PCI passthrough when has been enabled explicitly
> * (pci-passthrough=on).
> */
> - if ( !pci_passthrough_enabled )
> + if ( !is_pci_passthrough_enabled(true) )
> return 0;
>
> pci_segments_init();
> diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
> index fd5480d67d..bffeaa507d 100644
> --- a/xen/arch/x86/include/asm/pci.h
> +++ b/xen/arch/x86/include/asm/pci.h
> @@ -50,7 +50,7 @@ extern int pci_mmcfg_config_num;
> extern struct acpi_mcfg_allocation *pci_mmcfg_config;
>
> /* Unlike ARM, PCI passthrough is always enabled for x86. */
> -static always_inline bool is_pci_passthrough_enabled(void)
> +static always_inline bool is_pci_passthrough_enabled(__maybe_unused bool dom0)
> {
> return true;
> }
> diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
> index 0161a85e1e..18448b94b3 100644
> --- a/xen/drivers/pci/physdev.c
> +++ b/xen/drivers/pci/physdev.c
> @@ -19,7 +19,7 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> struct pci_dev_info pdev_info;
> nodeid_t node = NUMA_NO_NODE;
>
> - if ( !is_pci_passthrough_enabled() )
> + if ( !is_pci_passthrough_enabled(true) )
> return -EOPNOTSUPP;
>
> ret = -EFAULT;
> @@ -57,7 +57,7 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> case PHYSDEVOP_pci_device_remove: {
> struct physdev_pci_device dev;
>
> - if ( !is_pci_passthrough_enabled() )
> + if ( !is_pci_passthrough_enabled(true) )
> return -EOPNOTSUPP;
>
> ret = -EFAULT;
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index f784e91160..c4a49cf584 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -74,7 +74,7 @@ typedef union {
>
> struct arch_pci_dev { };
>
> -static inline bool is_pci_passthrough_enabled(void)
> +static inline bool is_pci_passthrough_enabled(__maybe_unused bool dom0)
> {
> return false;
> }
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-03-14 13:34 ` [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no Mykyta Poturai
2025-03-17 15:07 ` Jan Beulich
2025-04-22 0:25 ` Stefano Stabellini
@ 2025-04-28 8:54 ` Julien Grall
2025-04-28 12:31 ` Mykyta Poturai
2 siblings, 1 reply; 37+ messages in thread
From: Julien Grall @ 2025-04-28 8:54 UTC (permalink / raw)
To: Mykyta Poturai, xen-devel@lists.xenproject.org
Cc: Stewart Hildebrand, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, Anthony PERARD,
Jan Beulich, Roger Pau Monné
Hi Mykyta,
On 14/03/2025 13:34, Mykyta Poturai wrote:
> From: Stewart Hildebrand <stewart.hildebrand@amd.com>
>
> Enable the use of IOMMU + PCI in dom0 without having to specify
> "pci-passthrough=yes". We rely on dom0 to initialize the PCI controller
> and perform a PHYSDEVOP_pci_device_add call to add each device to SMMU.
It would be good to explain why Xen cannot initialize the PCI
controller. Asking, because the reason is the PCI controller is too
complex, then you will likely need the same approach for PCI passthrough...
>
> Enable pci_init() for initializing Xen's internal PCI subsystem, and
> allow PHYSDEVOP_pci_device_add when pci-passthrough is disabled.
Effectively, wouldn't this mean dom0 always *have* to call
PHYSDEVOP_pci_device_add? Otherwise, how would dom0 know whether it
needs to call PHSYDEVOP_pci_device_add?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-04-28 8:54 ` Julien Grall
@ 2025-04-28 12:31 ` Mykyta Poturai
2025-04-28 12:55 ` Julien Grall
0 siblings, 1 reply; 37+ messages in thread
From: Mykyta Poturai @ 2025-04-28 12:31 UTC (permalink / raw)
To: Julien Grall, xen-devel@lists.xenproject.org
Cc: Stewart Hildebrand, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, Anthony PERARD,
Jan Beulich, Roger Pau Monné
On 28.04.25 11:54, Julien Grall wrote:
> Hi Mykyta,
>
> On 14/03/2025 13:34, Mykyta Poturai wrote:
>> From: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>
>> Enable the use of IOMMU + PCI in dom0 without having to specify
>> "pci-passthrough=yes". We rely on dom0 to initialize the PCI controller
>> and perform a PHYSDEVOP_pci_device_add call to add each device to SMMU.
>
> It would be good to explain why Xen cannot initialize the PCI
> controller. Asking, because the reason is the PCI controller is too
> complex, then you will likely need the same approach for PCI passthrough...
I think the main reason for this is complexity and the possibility of
additional dependencies: there could be external clocks or reset pins
that the PCI host depends on for working correctly. I will add this to
the commit message. Regarding PCI passthrough, it is already using the
same approach (at least on Arm). There are patches for enabling Xen on
Arm to perform bus enumeration by itself by Luca Fancellu, but I haven't
yet got to test them in a meaningful way.
>>
>> Enable pci_init() for initializing Xen's internal PCI subsystem, and
>> allow PHYSDEVOP_pci_device_add when pci-passthrough is disabled.
>
> Effectively, wouldn't this mean dom0 always *have* to call
> PHYSDEVOP_pci_device_add? Otherwise, how would dom0 know whether it
> needs to call PHSYDEVOP_pci_device_add?
>
> Cheers,
>
Yes, I can't say for every system but with PCI host behind SMMU the
PHYSDEVOP_pci_device_add call is required to use DMA.
--
Mykyta
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-04-28 12:31 ` Mykyta Poturai
@ 2025-04-28 12:55 ` Julien Grall
2025-04-28 14:28 ` Mykyta Poturai
0 siblings, 1 reply; 37+ messages in thread
From: Julien Grall @ 2025-04-28 12:55 UTC (permalink / raw)
To: Mykyta Poturai, xen-devel@lists.xenproject.org
Cc: Stewart Hildebrand, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, Anthony PERARD,
Jan Beulich, Roger Pau Monné
Hi,
On 28/04/2025 13:31, Mykyta Poturai wrote:
> On 28.04.25 11:54, Julien Grall wrote:
>> Hi Mykyta,
>>
>> On 14/03/2025 13:34, Mykyta Poturai wrote:
>>> From: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>>
>>> Enable the use of IOMMU + PCI in dom0 without having to specify
>>> "pci-passthrough=yes". We rely on dom0 to initialize the PCI controller
>>> and perform a PHYSDEVOP_pci_device_add call to add each device to SMMU.
>>
>> It would be good to explain why Xen cannot initialize the PCI
>> controller. Asking, because the reason is the PCI controller is too
>> complex, then you will likely need the same approach for PCI passthrough...
>
> I think the main reason for this is complexity and the possibility of
> additional dependencies: there could be external clocks or reset pins
> that the PCI host depends on for working correctly. I will add this to
> the commit message. Regarding PCI passthrough, it is already using the
> same approach (at least on Arm). There are patches for enabling Xen on
> Arm to perform bus enumeration by itself by Luca Fancellu, but I haven't
> yet got to test them in a meaningful way.
Can you provide a link to the series? I would like to make sure we have
a coherent approach. In particular, it is not clear to me how Dom0 and
Xen will be able to coordinate the access to the PCI controller. Are we
going to have a mediator?
>
>>>
>>> Enable pci_init() for initializing Xen's internal PCI subsystem, and
>>> allow PHYSDEVOP_pci_device_add when pci-passthrough is disabled.
>>
>> Effectively, wouldn't this mean dom0 always *have* to call
>> PHYSDEVOP_pci_device_add? Otherwise, how would dom0 know whether it
>> needs to call PHSYDEVOP_pci_device_add?
>>
>> Cheers,
>>
>
> Yes, I can't say for every system but with PCI host behind SMMU the
> PHYSDEVOP_pci_device_add call is required to use DMA.
Dom0 will not be able to know whether a device is protected by an IOMMU.
So I guess it means the OS will need to be able to cope with an error
(like on x86).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-04-28 12:55 ` Julien Grall
@ 2025-04-28 14:28 ` Mykyta Poturai
2025-04-28 18:15 ` Julien Grall
0 siblings, 1 reply; 37+ messages in thread
From: Mykyta Poturai @ 2025-04-28 14:28 UTC (permalink / raw)
To: Julien Grall, xen-devel@lists.xenproject.org
Cc: Stewart Hildebrand, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, Anthony PERARD,
Jan Beulich, Roger Pau Monné
On 28.04.25 15:55, Julien Grall wrote:
> Hi,
>
> On 28/04/2025 13:31, Mykyta Poturai wrote:
>> On 28.04.25 11:54, Julien Grall wrote:
>>> Hi Mykyta,
>>>
>>> On 14/03/2025 13:34, Mykyta Poturai wrote:
>>>> From: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>>>
>>>> Enable the use of IOMMU + PCI in dom0 without having to specify
>>>> "pci-passthrough=yes". We rely on dom0 to initialize the PCI controller
>>>> and perform a PHYSDEVOP_pci_device_add call to add each device to SMMU.
>>>
>>> It would be good to explain why Xen cannot initialize the PCI
>>> controller. Asking, because the reason is the PCI controller is too
>>> complex, then you will likely need the same approach for PCI
>>> passthrough...
>>
>> I think the main reason for this is complexity and the possibility of
>> additional dependencies: there could be external clocks or reset pins
>> that the PCI host depends on for working correctly. I will add this to
>> the commit message. Regarding PCI passthrough, it is already using the
>> same approach (at least on Arm). There are patches for enabling Xen on
>> Arm to perform bus enumeration by itself by Luca Fancellu, but I haven't
>> yet got to test them in a meaningful way.
>
> Can you provide a link to the series? I would like to make sure we have
> a coherent approach. In particular, it is not clear to me how Dom0 and
> Xen will be able to coordinate the access to the PCI controller. Are we
> going to have a mediator?
>
Here is a link to my WIP branch
https://github.com/Deedone/xen/tree/pci_passthrough_wip
Although it is slightly outdated due to recent review activity, I will
updated it soon with all of the recent changes.
Luca's commits begin from c68a5cbb1a7d17907551159c99ab5c87ce0dedf9
I wasn't able to find them sent to any mailing lists, but I plan to also
send them after the base case with Dom0 enumeration stabilizes.
There is no mediator, Dom0 configures the host controller, enumerates
child devices, and then gives complete access to some of them to Xen.
Xen only does "logical" operations with child devices and does not
change any of the host controller base settings. To ensure that Dom0
will not mess with the child devices, tools bind them to the stub
driver. Theoretically, Dom0 can bind to those devices again and break
something, but it can also do a lot of other breaking stuff so I don't
think there is a good reason to invent some forms of protection from it.
After some time with pci-scan changes merged it should become possible
to make Xen do the enumeration, and only give Dom0 the virtual PCI bus,
which would prevent it from accessing non-owned devices.
--
Mykyta
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-04-28 14:28 ` Mykyta Poturai
@ 2025-04-28 18:15 ` Julien Grall
2025-04-29 11:48 ` Mykyta Poturai
0 siblings, 1 reply; 37+ messages in thread
From: Julien Grall @ 2025-04-28 18:15 UTC (permalink / raw)
To: Mykyta Poturai, xen-devel@lists.xenproject.org
Cc: Stewart Hildebrand, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, Anthony PERARD,
Jan Beulich, Roger Pau Monné
Hi Mykyta,
On 28/04/2025 15:28, Mykyta Poturai wrote:
> On 28.04.25 15:55, Julien Grall wrote:
>> Hi,
>>
>> On 28/04/2025 13:31, Mykyta Poturai wrote:
>>> On 28.04.25 11:54, Julien Grall wrote:
>>>> Hi Mykyta,
>>>>
>>>> On 14/03/2025 13:34, Mykyta Poturai wrote:
>>>>> From: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>>>>
>>>>> Enable the use of IOMMU + PCI in dom0 without having to specify
>>>>> "pci-passthrough=yes". We rely on dom0 to initialize the PCI controller
>>>>> and perform a PHYSDEVOP_pci_device_add call to add each device to SMMU.
>>>>
>>>> It would be good to explain why Xen cannot initialize the PCI
>>>> controller. Asking, because the reason is the PCI controller is too
>>>> complex, then you will likely need the same approach for PCI
>>>> passthrough...
>>>
>>> I think the main reason for this is complexity and the possibility of
>>> additional dependencies: there could be external clocks or reset pins
>>> that the PCI host depends on for working correctly. I will add this to
>>> the commit message. Regarding PCI passthrough, it is already using the
>>> same approach (at least on Arm). There are patches for enabling Xen on
>>> Arm to perform bus enumeration by itself by Luca Fancellu, but I haven't
>>> yet got to test them in a meaningful way.
>>
>> Can you provide a link to the series? I would like to make sure we have
>> a coherent approach. In particular, it is not clear to me how Dom0 and
>> Xen will be able to coordinate the access to the PCI controller. Are we
>> going to have a mediator?
>>
>
> Here is a link to my WIP branch
> https://github.com/Deedone/xen/tree/pci_passthrough_wip
> Although it is slightly outdated due to recent review activity, I will
> updated it soon with all of the recent changes.
>
> Luca's commits begin from c68a5cbb1a7d17907551159c99ab5c87ce0dedf9
>
> I wasn't able to find them sent to any mailing lists, but I plan to also
> send them after the base case with Dom0 enumeration stabilizes.
I don't think you can stabilize one without the other. I am worry the
interface may not work properly for PCI passthrough. So until then, I
would say this should be marked as unsupported (maybe protected by
CONFIG_UNSUPPORTED).
>
> There is no mediator, Dom0 configures the host controller, enumerates
> child devices, and then gives complete access to some of them to Xen.
> Xen only does "logical" operations with child devices and does not
> change any of the host controller base settings.
I am not sure I fully understanding this. Both dom0 will need to access
the configuration space. So you would need to ensure there is only one
accessing the configuration space at a give time.
> To ensure that Dom0
> will not mess with the child devices, tools bind them to the stub
> driver. Theoretically, Dom0 can bind to those devices again and break
> something, but it can also do a lot of other breaking stuff so I don't
> think there is a good reason to invent some forms of protection from it.
We should not trust dom0 to do the right thing. But reading ...
>
> After some time with pci-scan changes merged it should become possible
> to make Xen do the enumeration, and only give Dom0 the virtual PCI bus,
> which would prevent it from accessing non-owned devices.
... this it sounds like this would be temporary. Do you have patches
already on the mailing list?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
2025-04-28 18:15 ` Julien Grall
@ 2025-04-29 11:48 ` Mykyta Poturai
0 siblings, 0 replies; 37+ messages in thread
From: Mykyta Poturai @ 2025-04-29 11:48 UTC (permalink / raw)
To: Julien Grall, xen-devel@lists.xenproject.org
Cc: Stewart Hildebrand, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, Anthony PERARD,
Jan Beulich, Roger Pau Monné
On 28.04.25 21:15, Julien Grall wrote:
> Hi Mykyta,
>
> On 28/04/2025 15:28, Mykyta Poturai wrote:
>> On 28.04.25 15:55, Julien Grall wrote:
>>> Hi,
>>>
>>> On 28/04/2025 13:31, Mykyta Poturai wrote:
>>>> On 28.04.25 11:54, Julien Grall wrote:
>>>>> Hi Mykyta,
>>>>>
>>>>> On 14/03/2025 13:34, Mykyta Poturai wrote:
>>>>>> From: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>>>>>
>>>>>> Enable the use of IOMMU + PCI in dom0 without having to specify
>>>>>> "pci-passthrough=yes". We rely on dom0 to initialize the PCI
>>>>>> controller
>>>>>> and perform a PHYSDEVOP_pci_device_add call to add each device to
>>>>>> SMMU.
>>>>>
>>>>> It would be good to explain why Xen cannot initialize the PCI
>>>>> controller. Asking, because the reason is the PCI controller is too
>>>>> complex, then you will likely need the same approach for PCI
>>>>> passthrough...
>>>>
>>>> I think the main reason for this is complexity and the possibility of
>>>> additional dependencies: there could be external clocks or reset pins
>>>> that the PCI host depends on for working correctly. I will add this to
>>>> the commit message. Regarding PCI passthrough, it is already using the
>>>> same approach (at least on Arm). There are patches for enabling Xen on
>>>> Arm to perform bus enumeration by itself by Luca Fancellu, but I
>>>> haven't
>>>> yet got to test them in a meaningful way.
>>>
>>> Can you provide a link to the series? I would like to make sure we have
>>> a coherent approach. In particular, it is not clear to me how Dom0 and
>>> Xen will be able to coordinate the access to the PCI controller. Are we
>>> going to have a mediator?
>>>
>>
>> Here is a link to my WIP branch
>> https://github.com/Deedone/xen/tree/pci_passthrough_wip
>> Although it is slightly outdated due to recent review activity, I will
>> updated it soon with all of the recent changes.
>>
>> Luca's commits begin from c68a5cbb1a7d17907551159c99ab5c87ce0dedf9
>>
>> I wasn't able to find them sent to any mailing lists, but I plan to also
>> send them after the base case with Dom0 enumeration stabilizes.
>
> I don't think you can stabilize one without the other. I am worry the
> interface may not work properly for PCI passthrough. So until then, I
> would say this should be marked as unsupported (maybe protected by
> CONFIG_UNSUPPORTED).
It is currently not possible to enable HAS_PCI on Arm at all. I will add
the UNSUPPORTED guard along with the future changes to Kconfig that will
make enabling HAS_PCI on Arm possible.
>>
>> There is no mediator, Dom0 configures the host controller, enumerates
>> child devices, and then gives complete access to some of them to Xen.
>> Xen only does "logical" operations with child devices and does not
>> change any of the host controller base settings.
>
> I am not sure I fully understanding this. Both dom0 will need to access
> the configuration space. So you would need to ensure there is only one
> accessing the configuration space at a give time.
>
If a controller doesn't require specific configuration for mapping child
bus then Xen should access only child's config space and if Dom0 ignores
this device there will be no conflicts. For more complex controllers
like the ones on R-Car boards it is possible to implement safer ways of
mapping config space via Xen, example patch here[1] as temporary solution.
>> To ensure that Dom0
>> will not mess with the child devices, tools bind them to the stub
>> driver. Theoretically, Dom0 can bind to those devices again and break
>> something, but it can also do a lot of other breaking stuff so I don't
>> think there is a good reason to invent some forms of protection from it.
>
> We should not trust dom0 to do the right thing. But reading ...
>
>>
>> After some time with pci-scan changes merged it should become possible
>> to make Xen do the enumeration, and only give Dom0 the virtual PCI bus,
>> which would prevent it from accessing non-owned devices.
>
> ... this it sounds like this would be temporary. Do you have patches
> already on the mailing list?
> > Cheers,
>
Not yet, I am planning to start working on them after dealing with MSI
support on Arm.
[1]:
https://github.com/xen-troops/meta-xt-prod-devel-rcar-gen4/blob/master/layers/meta-xt-domd-gen4/recipes-kernel/linux/linux-renesas/0005-HACK-pcie-renesas-emulate-reading-from-ECAM-under-Xe.patch
--
Mykyta
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v9 8/8] xen/arm: Map ITS doorbell register to IOMMU page tables
2025-03-14 13:34 [PATCH v9 0/8] SMMU handling for PCIe Passthrough on ARM Mykyta Poturai
` (6 preceding siblings ...)
2025-03-14 13:34 ` [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no Mykyta Poturai
@ 2025-03-14 13:34 ` Mykyta Poturai
2025-04-16 10:52 ` Julien Grall
2025-04-22 0:31 ` Stefano Stabellini
7 siblings, 2 replies; 37+ messages in thread
From: Mykyta Poturai @ 2025-03-14 13:34 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Rahul Singh, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Stewart Hildebrand,
Mykyta Poturai
From: Rahul Singh <rahul.singh@arm.com>
When ITS is enabled and PCI devices that are behind an SMMU generate an
MSI interrupt, SMMU fault will be observed as there is currently no
mapping in p2m table for the ITS translation register (GITS_TRANSLATER).
A mapping is required in the iommu page tables so that the device can
generate the MSI interrupt writing to the GITS_TRANSLATER register.
The GITS_TRANSLATER register is a 32-bit register, and there is nothing
else in a page containing it, so map that page.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
This patch was originally picked up from [1], and commit description
loosely borrowed from [2].
Example SMMUv3 fault (qemu-system-aarch64 virt model), ITS base 0x8080000:
(XEN) SMMUv3: /smmuv3@9050000: event 0x10 received:
(XEN) SMMUv3: /smmuv3@9050000: 0x0000000800000010
(XEN) SMMUv3: /smmuv3@9050000: 0x0000008000000000
(XEN) SMMUv3: /smmuv3@9050000: 0x0000000008090040
(XEN) SMMUv3: /smmuv3@9050000: 0x0000000000000000
Example SMMUv2 fault (AMD/Xilinx Versal), ITS base 0xf9020000:
(XEN) smmu: /axi/smmu@fd800000: Unhandled context fault: fsr=0x402, iova=0xf9030040, fsynr=0x12, cb=0
v8->v9:
* no changes
v7->v8:
* no changes
v6->v7:
* add tlb flush after mapping
* style: update formatting
* revert back to printk with XENLOG_G_ERR
v5->v6:
* switch to iommu_map() interface
* fix page_count argument
* style fixup
* use gprintk instead of printk
* add my Signed-off-by
* move to vgic_v3_its_init_virtual()
v4->v5:
* new patch
[1] https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg00483.html
[2] https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/6232a0d53377009bb7fbc3c3ab81d0153734be6b
---
xen/arch/arm/vgic-v3-its.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index c65c1dbf52..376254f206 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -1478,6 +1478,26 @@ static int vgic_v3_its_init_virtual(struct domain *d, paddr_t guest_addr,
register_mmio_handler(d, &vgic_its_mmio_handler, guest_addr, SZ_64K, its);
+ if ( is_iommu_enabled(its->d) )
+ {
+ mfn_t mfn = maddr_to_mfn(its->doorbell_address);
+ unsigned int flush_flags = 0;
+ int ret = iommu_map(its->d, _dfn(mfn_x(mfn)), mfn, 1, IOMMUF_writable,
+ &flush_flags);
+
+ if ( ret < 0 )
+ {
+ printk(XENLOG_G_ERR
+ "GICv3: Map ITS translation register for %pd failed.\n",
+ its->d);
+ return ret;
+ }
+
+ ret = iommu_iotlb_flush(its->d, _dfn(mfn_x(mfn)), 1, flush_flags);
+ if ( ret < 0 )
+ return ret;
+ }
+
/* Register the virtual ITS to be able to clean it up later. */
list_add_tail(&its->vits_list, &d->arch.vgic.vits_list);
--
2.34.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v9 8/8] xen/arm: Map ITS doorbell register to IOMMU page tables
2025-03-14 13:34 ` [PATCH v9 8/8] xen/arm: Map ITS doorbell register to IOMMU page tables Mykyta Poturai
@ 2025-04-16 10:52 ` Julien Grall
2025-04-22 0:31 ` Stefano Stabellini
1 sibling, 0 replies; 37+ messages in thread
From: Julien Grall @ 2025-04-16 10:52 UTC (permalink / raw)
To: Mykyta Poturai, xen-devel@lists.xenproject.org
Cc: Rahul Singh, Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Stewart Hildebrand
Hi Mykyta,
On 14/03/2025 22:34, Mykyta Poturai wrote:
> From: Rahul Singh <rahul.singh@arm.com>
>
> When ITS is enabled and PCI devices that are behind an SMMU generate an
> MSI interrupt, SMMU fault will be observed as there is currently no
> mapping in p2m table for the ITS translation register (GITS_TRANSLATER).
>
> A mapping is required in the iommu page tables so that the device can
> generate the MSI interrupt writing to the GITS_TRANSLATER register.
>
> The GITS_TRANSLATER register is a 32-bit register, and there is nothing
> else in a page containing it, so map that page.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> ---
> This patch was originally picked up from [1], and commit description
> loosely borrowed from [2].
>
> Example SMMUv3 fault (qemu-system-aarch64 virt model), ITS base 0x8080000:
>
> (XEN) SMMUv3: /smmuv3@9050000: event 0x10 received:
> (XEN) SMMUv3: /smmuv3@9050000: 0x0000000800000010
> (XEN) SMMUv3: /smmuv3@9050000: 0x0000008000000000
> (XEN) SMMUv3: /smmuv3@9050000: 0x0000000008090040
> (XEN) SMMUv3: /smmuv3@9050000: 0x0000000000000000
>
> Example SMMUv2 fault (AMD/Xilinx Versal), ITS base 0xf9020000:
>
> (XEN) smmu: /axi/smmu@fd800000: Unhandled context fault: fsr=0x402, iova=0xf9030040, fsynr=0x12, cb=0
>
> v8->v9:
> * no changes
>
> v7->v8:
> * no changes
>
> v6->v7:
> * add tlb flush after mapping
> * style: update formatting
> * revert back to printk with XENLOG_G_ERR
>
> v5->v6:
> * switch to iommu_map() interface
> * fix page_count argument
> * style fixup
> * use gprintk instead of printk
> * add my Signed-off-by
> * move to vgic_v3_its_init_virtual()
>
> v4->v5:
> * new patch
>
> [1] https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg00483.html
> [2] https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/6232a0d53377009bb7fbc3c3ab81d0153734be6b
> ---
> xen/arch/arm/vgic-v3-its.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index c65c1dbf52..376254f206 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -1478,6 +1478,26 @@ static int vgic_v3_its_init_virtual(struct domain *d, paddr_t guest_addr,
>
> register_mmio_handler(d, &vgic_its_mmio_handler, guest_addr, SZ_64K, its);
>
> + if ( is_iommu_enabled(its->d) )
> + {
> + mfn_t mfn = maddr_to_mfn(its->doorbell_address);
> + unsigned int flush_flags = 0;
> + int ret = iommu_map(its->d, _dfn(mfn_x(mfn)), mfn, 1, IOMMUF_writable,
> + &flush_flags);
While looking at your patches for the guest vITS support. I noticed this
line is not updated for domU. Regardless of the one vs many vITS
discussion, I don't think this wants to be mapped 1:1. Instead, you want
to map the guest doorbell page to the host doorbell page.
The flush would need to be updated accordingly.
Cheers,
> +
> + if ( ret < 0 )
> + {
> + printk(XENLOG_G_ERR
> + "GICv3: Map ITS translation register for %pd failed.\n",
> + its->d);
> + return ret;
> + }
> +
> + ret = iommu_iotlb_flush(its->d, _dfn(mfn_x(mfn)), 1, flush_flags);
> + if ( ret < 0 )
> + return ret;
> + }
> +
> /* Register the virtual ITS to be able to clean it up later. */
> list_add_tail(&its->vits_list, &d->arch.vgic.vits_list);
>
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v9 8/8] xen/arm: Map ITS doorbell register to IOMMU page tables
2025-03-14 13:34 ` [PATCH v9 8/8] xen/arm: Map ITS doorbell register to IOMMU page tables Mykyta Poturai
2025-04-16 10:52 ` Julien Grall
@ 2025-04-22 0:31 ` Stefano Stabellini
1 sibling, 0 replies; 37+ messages in thread
From: Stefano Stabellini @ 2025-04-22 0:31 UTC (permalink / raw)
To: Mykyta Poturai
Cc: xen-devel@lists.xenproject.org, Rahul Singh, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel, Volodymyr Babchuk,
Stewart Hildebrand
On Fri, 14 Mar 2025, Mykyta Poturai wrote:
> From: Rahul Singh <rahul.singh@arm.com>
>
> When ITS is enabled and PCI devices that are behind an SMMU generate an
> MSI interrupt, SMMU fault will be observed as there is currently no
> mapping in p2m table for the ITS translation register (GITS_TRANSLATER).
>
> A mapping is required in the iommu page tables so that the device can
> generate the MSI interrupt writing to the GITS_TRANSLATER register.
>
> The GITS_TRANSLATER register is a 32-bit register, and there is nothing
> else in a page containing it, so map that page.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> ---
> This patch was originally picked up from [1], and commit description
> loosely borrowed from [2].
>
> Example SMMUv3 fault (qemu-system-aarch64 virt model), ITS base 0x8080000:
>
> (XEN) SMMUv3: /smmuv3@9050000: event 0x10 received:
> (XEN) SMMUv3: /smmuv3@9050000: 0x0000000800000010
> (XEN) SMMUv3: /smmuv3@9050000: 0x0000008000000000
> (XEN) SMMUv3: /smmuv3@9050000: 0x0000000008090040
> (XEN) SMMUv3: /smmuv3@9050000: 0x0000000000000000
>
> Example SMMUv2 fault (AMD/Xilinx Versal), ITS base 0xf9020000:
>
> (XEN) smmu: /axi/smmu@fd800000: Unhandled context fault: fsr=0x402, iova=0xf9030040, fsynr=0x12, cb=0
>
> v8->v9:
> * no changes
>
> v7->v8:
> * no changes
>
> v6->v7:
> * add tlb flush after mapping
> * style: update formatting
> * revert back to printk with XENLOG_G_ERR
>
> v5->v6:
> * switch to iommu_map() interface
> * fix page_count argument
> * style fixup
> * use gprintk instead of printk
> * add my Signed-off-by
> * move to vgic_v3_its_init_virtual()
>
> v4->v5:
> * new patch
>
> [1] https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg00483.html
> [2] https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/6232a0d53377009bb7fbc3c3ab81d0153734be6b
> ---
> xen/arch/arm/vgic-v3-its.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index c65c1dbf52..376254f206 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -1478,6 +1478,26 @@ static int vgic_v3_its_init_virtual(struct domain *d, paddr_t guest_addr,
>
> register_mmio_handler(d, &vgic_its_mmio_handler, guest_addr, SZ_64K, its);
>
> + if ( is_iommu_enabled(its->d) )
> + {
> + mfn_t mfn = maddr_to_mfn(its->doorbell_address);
> + unsigned int flush_flags = 0;
> + int ret = iommu_map(its->d, _dfn(mfn_x(mfn)), mfn, 1, IOMMUF_writable,
> + &flush_flags);
Should this be:
int ret = iommu_map(its->d, _dfn(PFN_DOWN(its->doorbell_address)), mfn, 1, IOMMUF_writable,
&flush_flags);
?
> + if ( ret < 0 )
> + {
> + printk(XENLOG_G_ERR
> + "GICv3: Map ITS translation register for %pd failed.\n",
> + its->d);
> + return ret;
> + }
> +
> + ret = iommu_iotlb_flush(its->d, _dfn(mfn_x(mfn)), 1, flush_flags);
And this:
ret = iommu_iotlb_flush(its->d, _dfn(PFN_DOWN(its->doorbell_address)), 1, flush_flags);
?
The original code in this patch assumes that the mapping is 1:1 but
actually its->doorbell_address is set to guest_addr +
ITS_DOORBELL_OFFSET and could potentially be not 1:1 ?
> + if ( ret < 0 )
> + return ret;
> + }
> +
> /* Register the virtual ITS to be able to clean it up later. */
> list_add_tail(&its->vits_list, &d->arch.vgic.vits_list);
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 37+ messages in thread