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>
---
v10->v11:
* no changes
v9->v10:
* return if iommu_add_pci_sidband_ids fails
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 | 119 +++++++++++++++++++++++---
1 file changed, 108 insertions(+), 11 deletions(-)
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index df16235057..a3bbfda993 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -1469,14 +1469,37 @@ 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);
+ return ret;
+ }
+ }
+#endif
+ fwspec = dev_iommu_fwspec_get(dev);
if (!fwspec)
return -ENODEV;
@@ -1521,17 +1544,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:
@@ -2624,6 +2668,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);
/*
@@ -2657,7 +2737,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;
@@ -2669,6 +2749,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);
@@ -2687,14 +2782,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.1Hi Mykyta,
I have only skimmed through the patch. But there is something that
caught my eye in both this patch and the one for the SMMUv2.
On 28/05/2025 10:12, Mykyta Poturai wrote:
> + /* 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);
arm_smmu_deassign_dev() can return an error. Can you explain why this is
fine to ignore it?
> +
> + return 0;
> + }
> + }
> +#endif
> +
Cheers,
--
Julien Grall
On 5/28/25 05:12, 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. This is only safe for devices present at boot time. It’s not safe for hotplugged devices, which should be quarantined. -- Sincerely, Demi Marie Obenour (she/her/hers)
Hi Demi, When replying to a thread, please keep the folks in CC unless there is a reason to drop them. Sending to just xen-devel is likely going to be lost. I personally don't always keep an eyes on discussion where I am not CCed, there are too many! So adding re-adding the CC for you and keeping your reply as-is. On 29/05/2025 03:10, Demi Marie Obenour wrote: > On 5/28/25 05:12, 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. > This is only safe for devices present at boot time. It’s not safe for > hotplugged devices, which should be quarantined. -- Julien Grall
On 29/05/2025 03:10, Demi Marie Obenour wrote: >> On 5/28/25 05:12, 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. >> This is only safe for devices present at boot time. It’s not safe for >> hotplugged devices, which should be quarantined. > Hi Demi, Could you please explain a little more in detail what do you think the issues will be here? As far as I understand quarantining is only relevant for transitioning devices between domains. Maybe I am missing something here. -- Mykyta
On 6/4/25 08:48, Julien Grall wrote: > Hi Demi, > > When replying to a thread, please keep the folks in CC unless there is a > reason to drop them. Sending to just xen-devel is likely going to be > lost. I personally don't always keep an eyes on discussion where I am > not CCed, there are too many! > > So adding re-adding the CC for you and keeping your reply as-is. > > On 29/05/2025 03:10, Demi Marie Obenour wrote: >> On 5/28/25 05:12, 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. >> This is only safe for devices present at boot time. It’s not safe for >> hotplugged devices, which should be quarantined. Thank you! This makes me wonder if Thunderbird is a good choice for an email client, or if I should use something like Mutt or Aerc. -- Sincerely, Demi Marie Obenour (she/her/hers)
Hi Demi, On 04/06/2025 19:57, Demi Marie Obenour wrote: > On 6/4/25 08:48, Julien Grall wrote: >> Hi Demi, >> >> When replying to a thread, please keep the folks in CC unless there is a >> reason to drop them. Sending to just xen-devel is likely going to be >> lost. I personally don't always keep an eyes on discussion where I am >> not CCed, there are too many! >> >> So adding re-adding the CC for you and keeping your reply as-is. >> >> On 29/05/2025 03:10, Demi Marie Obenour wrote: >>> On 5/28/25 05:12, 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. >>> This is only safe for devices present at boot time. It’s not safe for >>> hotplugged devices, which should be quarantined. > > Thank you! This makes me wonder if Thunderbird is a good choice for an > email client, or if I should use something like Mutt or Aerc. I have been using Thunderbird for the past 10 years without any issue. The UI tends to offer me two options "reply" and "reply all". I need to click on the latter to CC everyone. I am not sure whether you can hide the "reply" (so drop all the CC) only. Cheers, -- Julien Grall
On Wed, 28 May 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> v10->v11:
> * no changes
>
> v9->v10:
> * return if iommu_add_pci_sidband_ids fails
>
> 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 | 119 +++++++++++++++++++++++---
> 1 file changed, 108 insertions(+), 11 deletions(-)
>
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
> index df16235057..a3bbfda993 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -1469,14 +1469,37 @@ 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);
> + return ret;
> + }
> + }
> +#endif
>
> + fwspec = dev_iommu_fwspec_get(dev);
> if (!fwspec)
> return -ENODEV;
>
> @@ -1521,17 +1544,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:
> @@ -2624,6 +2668,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);
>
> /*
> @@ -2657,7 +2737,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;
> @@ -2669,6 +2749,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);
> @@ -2687,14 +2782,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
>
© 2016 - 2025 Red Hat, Inc.