[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 11/22] xen/arm: vsmmuv3: Attach Stage-1 configuration to SMMUv3 hardware
- To: Milan Djokic <milan_djokic@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
- From: Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>
- Date: Sat, 10 Oct 2026 17:47:51 +0300
- Authentication-results: eu.smtp.expurgate.cloud; dkim=pass header.s=20251104 header.d=gmail.com header.i="@gmail.com" header.h="Content-Transfer-Encoding:Content-Type:In-Reply-To:From:Content-Language:References:Cc:To:Subject:User-Agent:MIME-Version:Date:Message-ID"
- Cc: Rahul Singh <rahul.singh@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Michal Orzel <michal.orzel@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Roger Pau Monné <roger@xxxxxxxxxxxxxx>
- Delivery-date: Sat, 10 Oct 2026 14:48:22 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On 10/7/26 03:07, Milan Djokic wrote:
Hello Milan
From: Rahul Singh <rahul.singh@xxxxxxx>
Attach the Stage-1 configuration to device STE to support nested
translation for the guests.
My review of this is based on the full series being applied and includes
the surrounding code context.
Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
Signed-off-by: Milan Djokic <milan_djokic@xxxxxxxx>
---
xen/arch/arm/include/asm/iommu.h | 7 ++
xen/drivers/passthrough/arm/smmu-v3.c | 105 +++++++++++++++++++++++++
xen/drivers/passthrough/arm/smmu-v3.h | 1 +
xen/drivers/passthrough/arm/vsmmu-v3.c | 18 +++++
xen/include/xen/iommu.h | 6 ++
5 files changed, 137 insertions(+)
diff --git a/xen/arch/arm/include/asm/iommu.h b/xen/arch/arm/include/asm/iommu.h
index ad15477e24..56bc9314a7 100644
--- a/xen/arch/arm/include/asm/iommu.h
+++ b/xen/arch/arm/include/asm/iommu.h
@@ -20,6 +20,13 @@ struct arch_iommu
void *priv;
};
+struct iommu_guest_config {
+ paddr_t s1ctxptr;
+ uint8_t config;
+ uint8_t s1fmt;
+ uint8_t s1cdmax;
+};
+
const struct iommu_ops *iommu_get_ops(void);
void iommu_set_ops(const struct iommu_ops *ops);
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index 9d2b8a708b..196ac660ef 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -3003,6 +3003,37 @@ static struct arm_smmu_device *arm_smmu_get_by_dev(const
struct device *dev)
return NULL;
}
+static struct iommu_domain *arm_smmu_get_domain_by_sid(struct domain *d,
+ u32 sid)
+{
+ int i;
+ unsigned long flags;
+ struct iommu_domain *io_domain;
+ struct arm_smmu_domain *smmu_domain;
+ struct arm_smmu_master *master;
+ struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
+
+ /*
+ * Loop through the &xen_domain->contexts to locate a context
+ * associated with the target SMMU and device SID
+ */
+ list_for_each_entry(io_domain, &xen_domain->contexts, list) {
+ smmu_domain = to_smmu_domain(io_domain);
+
+ spin_lock_irqsave(&smmu_domain->devices_lock, flags);
+ list_for_each_entry(master, &smmu_domain->devices, domain_head)
{
+ for (i = 0; i < master->num_streams; i++) {
+ if (sid != master->streams[i].id)
+ continue;
+
spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+ return io_domain;
+ }
+ }
+ spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+ }
+ return NULL;
+}
+
static struct iommu_domain *arm_smmu_get_domain(struct domain *d,
struct device *dev)
{
@@ -3216,6 +3247,79 @@ static void arm_smmu_iommu_xen_domain_teardown(struct
domain *d)
xfree(xen_domain);
}
+static int arm_smmu_attach_guest_config(struct domain *d, uint32_t sid,
+ const struct iommu_guest_config *cfg)
+{
+ int ret = -EINVAL;
+ unsigned long flags;
+ struct arm_smmu_master *master;
+ struct arm_smmu_domain *smmu_domain;
+ struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
+ struct iommu_domain *io_domain = arm_smmu_get_domain_by_sid(d, sid);
+
+ if (!io_domain)
+ return -ENODEV;
+
+ smmu_domain = to_smmu_domain(io_domain);
I might be wrong, but it feels to me that io_domain should be fetched
after acquiring the lock below since concurrent deassign can free the
io_domain. arm_smmu_deassign_dev() takes xen_domain->lock, decrements
io_domain->ref, and if it hits 0, calls arm_smmu_destroy_iommu_domain()
which frees the memory.
+
+ spin_lock(&xen_domain->lock);
+
+ switch (cfg->config) {
+ case ARM_SMMU_DOMAIN_ABORT:
+ smmu_domain->abort = true;
+ smmu_domain->stage = cfg->config;
+ break;
+ case ARM_SMMU_DOMAIN_BYPASS:
+ smmu_domain->abort = false;
+ smmu_domain->stage = cfg->config;
I am not quite sure where the best place is to leave this comment, but
unless I am missing something, it looks like a guest can trivially
disable stage-2 translation.
If the guest requests CFG_BYPASS, arm_vsmmu_decode_ste() sets bypassed
and guest_cfg.config becomes ARM_SMMU_DOMAIN_BYPASS.
arm_smmu_attach_guest_config() then sets that stage. Consequently,
arm_smmu_write_strtab_ent() finds neither s1_cfg nor s2_cfg and writes
the physical STE as CFG_BYPASS, which bypasses both stages.
So any device the guest puts in bypass (which Linux might legitimately
do for identity domains) can read and write any physical memory,
including Xen’s memory. Even for well-behaved guests, this is
problematic: the device would use IOVA (which equals IPA) directly as a
host physical address instead of going through the domain’s P2M.
I think that guest bypass request should map to S2-only physical STE.
+ break;
+ case ARM_SMMU_DOMAIN_S1:
+ /* Check stage-1 support */
+ if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_TRANS_S1)) {
+ dev_info(smmu_domain->smmu->dev,
+ "stage-1 not implemented\n");
+ goto out;
+ }
+
+ /* If stage-2 is not supported, configure stage-1 only */
+ if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_TRANS_S2)) {
+ dev_warn(smmu_domain->smmu->dev,
+ "SMMU does not implement stage-2
translation support. "
+ "Fallback to stage-1-only
translation configuration\n");
+ /* Enable Stage-1 translation. */
+ smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
As I already mentioned for commit #02/22, Without stage-2 there is no
layer to turn the stage-1 output's IPA into PA. Should we allow this
fallback only for privileged, direct-mapped domains?
And apologies if this has already been discussed.
+ }
+ else {
+ /* Check nested support */
+ if (!(smmu_domain->smmu->features &
ARM_SMMU_FEAT_NESTING)) {
+ dev_info(smmu_domain->smmu->dev,
+ "Nested translation not
supported\n");
+ goto out;
According to commit #07/22 "MMU-600 variant < 2 and all MMU-700" are
affected. But it seems that Xen still offers SMMU, a guest still sees
SMMU and might believe it is working. The attach fails with only a log
line (gdprintk in arm_vsmmu_handle_cmds()) and the guest is never told.
Device DMA then might land in the wrong guest memory.
Please consider gating this at viommu_get_type() or at domain creation.
+ }
+ smmu_domain->stage = ARM_SMMU_DOMAIN_NESTED;
+ }
+
+ /* Enable Stage-1 translation. */
+ smmu_domain->s1_cfg.s1ctxptr = cfg->s1ctxptr;
+ smmu_domain->s1_cfg.s1fmt = cfg->s1fmt;
+ smmu_domain->s1_cfg.s1cdmax = cfg->s1cdmax;
+ smmu_domain->abort = false;
Since all this stored per smmu_domain, one domain-wide S1 config
(bypass, abort, context pointer) is applied to all of device’s SIDs
(from the design doc I got this as intentional for current driver
implementation) ...
+ break;
+ default:
+ goto out;
+ }
+
+ spin_lock_irqsave(&smmu_domain->devices_lock, flags);
+ list_for_each_entry(master, &smmu_domain->devices, domain_head)
+ arm_smmu_install_ste_for_dev(master);
... ^^^ loops over all masters and applies the identical config to all
of them. If, lets say, a device (PCI?) exposes several SIDs, and the
guest tries to program SID 0 with one mappings and SID 1 with a
different mappings, the second command will silently overwrite SID 0's
mappings.
I do not want to ask for extra work, but should Xen at least report an
error when a guest programs different configs, rather than silently
overwriting the previous one? Or at least leave a TODO?
+ spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+
+ ret = 0;
+out:
+ spin_unlock(&xen_domain->lock);
+ return ret;
+}
+
static const struct iommu_ops arm_smmu_iommu_ops = {
.page_sizes = PAGE_SIZE_4K,
.init = arm_smmu_iommu_xen_domain_init,
@@ -3228,6 +3332,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
.unmap_page = arm_iommu_unmap_page,
.dt_xlate = arm_smmu_dt_xlate,
.add_device = arm_smmu_add_device,
+ .attach_guest_config = arm_smmu_attach_guest_config,
};
static __init int arm_smmu_dt_init(struct dt_device_node *dev,
diff --git a/xen/drivers/passthrough/arm/smmu-v3.h
b/xen/drivers/passthrough/arm/smmu-v3.h
index ef492098ae..4a912be740 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.h
+++ b/xen/drivers/passthrough/arm/smmu-v3.h
@@ -402,6 +402,7 @@ enum arm_smmu_domain_stage {
ARM_SMMU_DOMAIN_S2,
ARM_SMMU_DOMAIN_NESTED,
ARM_SMMU_DOMAIN_BYPASS,
+ ARM_SMMU_DOMAIN_ABORT,
};
/* Xen specific code. */
diff --git a/xen/drivers/passthrough/arm/vsmmu-v3.c
b/xen/drivers/passthrough/arm/vsmmu-v3.c
index 25ba734fa7..3f557d1c4d 100644
--- a/xen/drivers/passthrough/arm/vsmmu-v3.c
+++ b/xen/drivers/passthrough/arm/vsmmu-v3.c
@@ -319,8 +319,11 @@ static int arm_vsmmu_handle_cfgi_ste(struct virt_smmu
*smmu, uint64_t *cmdptr)
{
int ret;
uint64_t ste[STRTAB_STE_DWORDS];
+ struct domain *d = smmu->d;
+ struct domain_iommu *hd = dom_iommu(d);
struct arm_vsmmu_s1_trans_cfg s1_cfg = {0};
uint32_t sid = smmu_cmd_get_sid(cmdptr[0]);
+ struct iommu_guest_config guest_cfg = {0};
ret = arm_vsmmu_find_ste(smmu, sid, ste);
if ( ret )
@@ -330,6 +333,21 @@ static int arm_vsmmu_handle_cfgi_ste(struct virt_smmu
*smmu, uint64_t *cmdptr)
if ( ret )
return (ret == -EAGAIN ) ? 0 : ret;
+ guest_cfg.s1ctxptr = s1_cfg.s1ctxptr;
+ guest_cfg.s1fmt = s1_cfg.s1fmt;
+ guest_cfg.s1cdmax = s1_cfg.s1cdmax;
+
+ if ( s1_cfg.bypassed )
+ guest_cfg.config = ARM_SMMU_DOMAIN_BYPASS;
+ else if ( s1_cfg.aborted )
+ guest_cfg.config = ARM_SMMU_DOMAIN_ABORT;
+ else
+ guest_cfg.config = ARM_SMMU_DOMAIN_S1;
+
+ ret = hd->platform_ops->attach_guest_config(d, sid, &guest_cfg);
+ if ( ret )
+ return ret;
+
return 0;
}
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 37c4a1dc82..253f8bb6cb 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -312,6 +312,7 @@ static inline int iommu_add_dt_pci_sideband_ids(struct
pci_dev *pdev)
#endif /* HAS_DEVICE_TREE_DISCOVERY */
struct page_info;
+struct iommu_guest_config;
/*
* Any non-zero value returned from callbacks of this type will cause the
@@ -387,6 +388,11 @@ struct iommu_ops {
#endif
/* Inhibit all interrupt generation, to be used at shutdown. */
void (*quiesce)(void);
+
+#ifdef CONFIG_ARM
+ int (*attach_guest_config)(struct domain *d, uint32_t sid,
+ const struct iommu_guest_config *cfg);
+#endif
};
/*
|
