Add Tegra241 (Grace) CMDQV Support (part 1/2)

The CMDQV extension in NVIDIA Tegra241 SoC only supports CS_NONE in the CS field of CMD_SYNC. Add a quirk flag to accommodate that.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 10 ++++++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  4 ++++
 2 files changed, 12 insertions(+), 2 deletions(-)

-----Original Message-----
From: Nicolin Chen <[email protected]>
Sent: Saturday, April 13, 2024 4:44 AM
To: [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: [PATCH v5 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF

When VCMDQs are assigned to a VINTF owned by a guest (HYP_OWN bit unset), only TLB and ATC invalidation commands are supported by the VCMDQ HW. So, add a new helper to scan the input cmds to make sure every single command is supported when selecting a queue.

Note that the guest VM shouldn't have HYP_OWN bit being set regardless of guest kernel driver writing it or not, i.e. the hypervisor running in the host OS should wire this bit to zero when trapping a write access to this VINTF_CONFIG register from a guest kernel.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  7 +--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  5 ++-
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 43 ++++++++++++++++++-
 3 files changed, 49 insertions(+), 6 deletions(-)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3 -2

index ba7a933c1efb..9af6659ea488 100644
@@ -352,10 +352,11 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd,
 struct arm_smmu_cmdq_ent *ent)
 	return 0;
 }
 
-static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct
 arm_smmu_device *smmu)
+static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct
 arm_smmu_device *smmu,
+					       u64 *cmds, int n)
 {
 	if (smmu->tegra241_cmdqv)
-		return tegra241_cmdqv_get_cmdq(smmu);
+		return tegra241_cmdqv_get_cmdq(smmu, cmds, n);

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1 -1

 	return &smmu->cmdq;
 }

@@ -765,7 +766,7 @@ static int arm_smmu_cmdq_issue_cmdlist(struct
 arm_smmu_device *smmu,
 	u32 prod;
 	unsigned long flags;
 	bool owner;
-	struct arm_smmu_cmdq *cmdq = arm_smmu_get_cmdq(smmu);
+	struct arm_smmu_cmdq *cmdq = arm_smmu_get_cmdq(smmu, cmds,
 n);
 	struct arm_smmu_ll_queue llq, head;

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -0

 	int ret = 0;

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +2 -1

index 5b8e463c28eb..fdc3d570cf43 100644
@@ -836,7 +836,8 @@ static inline void
 arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
 struct tegra241_cmdqv *
 tegra241_cmdqv_acpi_probe(struct arm_smmu_device *smmu, int id);
 int tegra241_cmdqv_device_reset(struct arm_smmu_device *smmu);
-struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct
 arm_smmu_device *smmu);
+struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct
 arm_smmu_device *smmu,
+					      u64 *cmds, int n);

#else /* CONFIG_TEGRA241_CMDQV */
static inline struct tegra241_cmdqv * tegra241_cmdqv_acpi_probe(struct arm_smmu_device *smmu, int id)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -1

@@ -850,7 +851,7 @@ static inline int tegra241_cmdqv_device_reset(struct
 arm_smmu_device *smmu)
 }
 
 static inline struct arm_smmu_cmdq *
-tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu)
+tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu, u64 *cmds, int
 n)
 {

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +0 -0

 	return NULL;
 }

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +14 -1

index 15683123a4ce..7aeaf810980c 100644
@@ -262,6 +262,7 @@ struct tegra241_vcmdq {
  * struct tegra241_vintf - Virtual Interface
  * @idx: Global index in the CMDQV HW
  * @enabled: Enabled or not
+ * @hyp_own: Owned by hypervisor (in-kernel)
  * @error: Status error or not
  * @cmdqv: CMDQV HW pointer
  * @vcmdqs: List of VCMDQ pointers

@@ -271,6 +272,7 @@ struct tegra241_vintf {
 	u16 idx;
 
 	bool enabled;
+	bool hyp_own;
 	atomic_t error;	/* Race between interrupts and get_cmdq() */
 
 	struct tegra241_cmdqv *cmdqv;

@@ -369,7 +371,32 @@ static irqreturn_t tegra241_cmdqv_isr(int irq, void
 *devid)
 	return IRQ_HANDLED;
 }
 
-struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct
 arm_smmu_device *smmu)
+static bool tegra241_vintf_support_cmds(struct tegra241_vintf *vintf,
+					u64 *cmds, int n)
+{
+	int i;
+
+	/* VINTF owned by hypervisor can execute any command */
+	if (vintf->hyp_own)
+		return true;
+
+	/* Guest-owned VINTF must Check against the list of supported CMDs
 */
+	for (i = 0; i < n; i++) {
+		switch (FIELD_GET(CMDQ_0_OP, cmds[i *

CMDQ_ENT_DWORDS])) {

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +13 -0

+		case CMDQ_OP_TLBI_NH_ASID:
+		case CMDQ_OP_TLBI_NH_VA:
+		case CMDQ_OP_ATC_INV:
+			continue;
+		default:
+			return false;
+		}
+	}
+
+	return true;
+}
+
+struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct

arm_smmu_device *smmu,

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +5 -0

+					      u64 *cmds, int n)
 {
 	struct tegra241_cmdqv *cmdqv = smmu->tegra241_cmdqv;
 	struct tegra241_vintf *vintf = cmdqv->vintfs[0];

@@ -386,6 +413,10 @@ struct arm_smmu_cmdq
 *tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu)
 	if (atomic_read(&vintf->error))
 		return &smmu->cmdq;
 
+	/* Unsupported CMDs go for smmu->cmdq pathway */
+	if (!tegra241_vintf_support_cmds(vintf, cmds, n))
+		return &smmu->cmdq;
+
 	/*
 	 * Select a vcmdq to use. Here we use a temporal solution to

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +10 -0

 	 * balance out traffic on cmdq issuing: each cmdq has its own

@@ -575,6 +606,11 @@ int tegra241_cmdqv_device_reset(struct
 arm_smmu_device *smmu)
 	if (ret)
 		return ret;
 
+	/*
+	 * Note that HYP_OWN bit is wired to zero when running in guest kernel
+	 * regardless of enabling it here, as !HYP_OWN cmdqs have a restricted
+	 * set of supported commands, by following the HW design.
+	 */
 	regval = FIELD_PREP(VINTF_HYP_OWN, 1);
 	vintf_writel(regval, CONFIG);

@@ -582,6 +618,11 @@ int tegra241_cmdqv_device_reset(struct
 arm_smmu_device *smmu)
 	if (ret)
 		return ret;
 
+	/*
+	 * As being mentioned above, HYP_OWN bit is wired to zero for a guest
+	 * kernel, so read it back from HW to ensure that reflects in hyp_own
+	 */
+	vintf->hyp_own = !!(VINTF_HYP_OWN & vintf_readl(CONFIG));
 	vintf->enabled = !!(VINTF_ENABLED & vintf_readl(STATUS));
 	atomic_set(&vintf->error,

!!FIELD_GET(VINTF_STATUS, vintf_readl(STATUS)));
--
2.43.0

Quoted 16 lines

-----Original Message-----
From: Nicolin Chen <[email protected]>
Sent: Saturday, April 13, 2024 4:44 AM
To: [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]
Subject: [PATCH v5 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF

When VCMDQs are assigned to a VINTF owned by a guest (HYP_OWN bit unset), only TLB and ATC invalidation commands are supported by the VCMDQ HW. So, add a new helper to scan the input cmds to make sure every single command is supported when selecting a queue.

Quoted 4 lines

Note that the guest VM shouldn't have HYP_OWN bit being set regardless of guest kernel driver writing it or not, i.e. the hypervisor running in the host OS should wire this bit to zero when trapping a write access to this VINTF_CONFIG register from a guest kernel.

Just curious, suppose there is a malicious guest with a compromised kernel which bypasses the HYP_OWN bit check and issues other commands, does the hardware has the capability to detect it and not make the host unstable in any way?

The user-space support is to provide uAPIs (via IOMMUFD) for hypervisors in user space to passthrough VCMDQs to VMs, allowing these VMs to access the VCMDQs directly without trappings, i.e. no VM Exits. This gives huge performance improvements: 70% to 90% reductions of TLB invalidation time were measured by various DMA unmap tests running in a guest OS, compared to a nested SMMU CMDQ (with trappings).

-----Original Message-----
From: Jason Gunthorpe <[email protected]>
Sent: Monday, April 15, 2024 6:14 PM
To: Nicolin Chen <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux-arm- [email protected]; [email protected]; Jerry Snitselaar <[email protected]>
Subject: Re: [PATCH v5 0/6] Add Tegra241 (Grace) CMDQV Support (part 1/2)

Quoted 2 lines · Nicolin Chen

The user-space support is to provide uAPIs (via IOMMUFD) for hypervisors in user space to passthrough VCMDQs to VMs, allowing these VMs to

access

Quoted 2 lines

the VCMDQs directly without trappings, i.e. no VM Exits. This gives huge performance improvements: 70% to 90% reductions of TLB invalidation

time

Quoted 1 line

were measured by various DMA unmap tests running in a guest OS,

compared

Quoted 1 line

to a nested SMMU CMDQ (with trappings).

So everyone is on the same page, this is the primary point of this series. The huge speed up of in-VM performance is necessary for the workloads this chip is expected to be running. This series is unique from all the rest because it runs inside a VM, often in the from of a distro release.

It doesn't need the other series or it's own part 2 as it entirely stands alone on bare metal hardware or on top of commercial VM cloud instances runing who-knows-what in their hypervisors.

The other parts are substantially about enabling qemu and the open ecosystem to have fully functional vSMMU3 virtualization.

-----Original Message-----
From: Shameerali Kolothum Thodi
Sent: Wednesday, April 17, 2024 9:01 AM
To: 'Jason Gunthorpe' <[email protected]>; Nicolin Chen <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]; linux-arm- [email protected]; [email protected]; Jerry Snitselaar <[email protected]>
Subject: RE: [PATCH v5 0/6] Add Tegra241 (Grace) CMDQV Support (part 1/2)

Quoted 29 lines

-----Original Message-----
From: Jason Gunthorpe <[email protected]>
Sent: Monday, April 15, 2024 6:14 PM
To: Nicolin Chen <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux-arm- [email protected]; [email protected]; Jerry Snitselaar <[email protected]>
Subject: Re: [PATCH v5 0/6] Add Tegra241 (Grace) CMDQV Support (part 1/2)

Quoted 2 lines · Nicolin Chen

The user-space support is to provide uAPIs (via IOMMUFD) for hypervisors in user space to passthrough VCMDQs to VMs, allowing these VMs to

access

Quoted 2 lines

the VCMDQs directly without trappings, i.e. no VM Exits. This gives huge performance improvements: 70% to 90% reductions of TLB invalidation

time

Quoted 1 line

were measured by various DMA unmap tests running in a guest OS,

compared

Quoted 1 line

to a nested SMMU CMDQ (with trappings).

So everyone is on the same page, this is the primary point of this series. The huge speed up of in-VM performance is necessary for the workloads this chip is expected to be running. This series is unique from all the rest because it runs inside a VM, often in the from of a distro release.

Quoted 6 lines

It doesn't need the other series or it's own part 2 as it entirely stands alone on bare metal hardware or on top of commercial VM cloud instances runing who-knows-what in their hypervisors.

The other parts are substantially about enabling qemu and the open ecosystem to have fully functional vSMMU3 virtualization.

Hi,

We do have plans to revive the SMMUv3 ECMDQ series posted a while back[0] and looking at this series, I am just wondering whether it makes sense to have a similar one with ECMDQ as well? I see that the NVIDIA VCMDQ has a special bit
to restrict the commands that can be issued from user space. If we end up assigning
a ECMDQ to user space, is there any potential risk in doing so?

SMMUV3 spec does say,
"Arm expects that the Non-secure Stream table, Command queue, Event queue and
PRI queue are controlled by the most privileged Non-secure system software. "

Not clear to me what are the major concerns here and maybe we can come up with
something to address that in kernel.

Just to add to that. One idea could be like to have a case where when ECMDQs are detected, use that for issuing limited set of cmds(like stage 1 TLBIs) and use the normal cmdq for rest. Since we use stage 1 for both host and for Guest nested cases and TLBIs are the bottlenecks in most cases I think this should give performance benefits.

We do have plans to revive the SMMUv3 ECMDQ series posted a while back[0] and looking at this series, I am just wondering whether it makes sense to have a similar one with ECMDQ as well? I see that the NVIDIA VCMDQ has a special bit to restrict the commands that can be issued from user space. If we end up assigning a ECMDQ to user space, is there any potential risk in doing so?

Not clear to me what are the major concerns here and maybe we can come up with something to address that in kernel.

-----Original Message-----
From: Jason Gunthorpe <[email protected]>
Sent: Wednesday, April 17, 2024 1:25 PM
To: Shameerali Kolothum Thodi <[email protected]>
Cc: Nicolin Chen <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; Jerry Snitselaar <[email protected]>
Subject: Re: [PATCH v5 0/6] Add Tegra241 (Grace) CMDQV Support (part 1/2)

Quoted 3 lines · Shameerali Kolothum Thodi

We do have plans to revive the SMMUv3 ECMDQ series posted a while back[0] and looking at this series, I am just wondering whether it makes sense to have a similar one with ECMDQ as well? I see that the NVIDIA VCMDQ has a special

bit

Quoted 1 line

to restrict the commands that can be issued from user space. If we end up

assigning

Quoted 1 line

a ECMDQ to user space, is there any potential risk in doing so?

I think there is some risk/trouble, ECMDQ needs some enhancement before it can be really safe to use from less privileged software, and it wasn't designed to have an isolated doorbell page either.

Quoted 1 line

Not clear to me what are the major concerns here and maybe we can come up

with

Quoted 1 line

something to address that in kernel.

I haven't looked deeply but my impression has been the ECMDQ is not workable to support virtualization. At a minimum it has no way to constrain the command flow to a VMID and to do VSID -> PSID translation.

I suggest you talk directly to ARM on this if you are interested in this.