On Fri, Aug 30, 2024 at 08:16:27AM +0000, Tian, Kevin wrote:

quoted

From: Jason Gunthorpe <jgg@nvidia.com>
Sent: Tuesday, August 27, 2024 11:52 PM

For SMMUv3 a IOMMU_DOMAIN_NESTED is composed of a S2
iommu_domain acting
as the parent and a user provided STE fragment that defines the CD table
and related data with addresses translated by the S2 iommu_domain.

The kernel only permits userspace to control certain allowed bits of the
STE that are safe for user/guest control.

IOTLB maintenance is a bit subtle here, the S1 implicitly includes the S2
translation, but there is no way of knowing which S1 entries refer to a
range of S2.

For the IOTLB we follow ARM's guidance and issue a
CMDQ_OP_TLBI_NH_ALL to
flush all ASIDs from the VMID after flushing the S2 on any change to the
S2.

Similarly we have to flush the entire ATC if the S2 is changed.

it's clearer to mention that ATS is not supported at this point. 

As things have ended up we need the viommu series to come along with
this to enable the full feature, and viommu supports ATS invalidation.

Ideally I'd like to merge them both together.

quoted

@@ -2614,7 +2687,8 @@ arm_smmu_find_master_domain(struct

arm_smmu_domain *smmu_domain,
 	list_for_each_entry(master_domain, &smmu_domain->devices,
 			    devices_elm) {
 		if (master_domain->master == master &&
-		    master_domain->ssid == ssid)
+		    master_domain->ssid == ssid &&
+		    master_domain->nest_parent == nest_parent)
 			return master_domain;
 	}

there are two nest_parent flags in master_domain and smmu_domain.
Probably duplicating?

Sort of, sort of not..

This is a bit awkward right now because the arm_smmu_domain is still
per-instance, so the domain->nest_parent exists to control flushing of
the VMID

But we also have the per-attachment 'master_domain' struct, and there
the nest_parent controls flushing of the ATC.

In the end arm_smmu_domain will stop being per-instance and per-attach
'master_domain' would have the vmid and the nest_parent only. I'm
aiming for something like how VTD and RISCV are doing their flushing,
with a list of flush instructions attached to the domain.

So for now we have the in-between state where a S2 marked as parent
will avoid the ATC flush when directly attached to a RID but not the
ASID flush. Eventually we will be able to avoid both.

quoted

+static struct iommu_domain *
+arm_smmu_domain_alloc_nesting(struct device *dev, u32 flags,
+			      struct iommu_domain *parent,
+			      const struct iommu_user_data *user_data)
+{
+	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct arm_smmu_nested_domain *nested_domain;
+	struct arm_smmu_domain *smmu_parent;
+	struct iommu_hwpt_arm_smmuv3 arg;
+	unsigned int eats;
+	unsigned int cfg;
+	int ret;
+
+	if (!(master->smmu->features & ARM_SMMU_FEAT_NESTING))
+		return ERR_PTR(-EOPNOTSUPP);
+
+	/*
+	 * Must support some way to prevent the VM from bypassing the
cache
+	 * because VFIO currently does not do any cache maintenance.
+	 */
+	if (!(fwspec->flags & IOMMU_FWSPEC_PCI_RC_CANWBS) &&
+	    !(master->smmu->features & ARM_SMMU_FEAT_S2FWB))
+		return ERR_PTR(-EOPNOTSUPP);

this can be saved if we guard the setting of NESTING upon them.

IOMMU_FWSPEC_PCI_RC_CANWBS is per-device, FEAT_NESTING is SMMU global,
they can't really be combined.

quoted

+
+	ret = iommu_copy_struct_from_user(&arg, user_data,
+
IOMMU_HWPT_DATA_ARM_SMMUV3, ste);
+	if (ret)
+		return ERR_PTR(ret);

prefer to allocating resource after static condition checks below.

quoted

+
+	if (flags || !(master->smmu->features &
ARM_SMMU_FEAT_TRANS_S1))
+		return ERR_PTR(-EOPNOTSUPP);

Is it possible when NESTING is supported?

quoted

+
+	if (!(parent->type & __IOMMU_DOMAIN_PAGING))
+		return ERR_PTR(-EINVAL);

Just check parent->nest_parent

quoted

+
+	smmu_parent = to_smmu_domain(parent);
+	if (smmu_parent->stage != ARM_SMMU_DOMAIN_S2 ||
+	    smmu_parent->smmu != master->smmu)
+		return ERR_PTR(-EINVAL);

again S2 should be implied when parent->nest_parent is true.

I think I did all of these for Nicolin

quoted

+
+	/* EIO is reserved for invalid STE data. */
+	if ((arg.ste[0] & ~STRTAB_STE_0_NESTING_ALLOWED) ||
+	    (arg.ste[1] & ~STRTAB_STE_1_NESTING_ALLOWED))
+		return ERR_PTR(-EIO);
+
+	cfg = FIELD_GET(STRTAB_STE_0_CFG, le64_to_cpu(arg.ste[0]));
+	if (cfg != STRTAB_STE_0_CFG_ABORT && cfg !=
STRTAB_STE_0_CFG_BYPASS &&
+	    cfg != STRTAB_STE_0_CFG_S1_TRANS)
+		return ERR_PTR(-EIO);

If vSTE is invalid those bits can be ignored?

Yes, but also I was expecting the VMM to sanitize that.. Let's have
the kernel do it. Move the validation to a function and then:

static int arm_smmu_validate_vste(struct iommu_hwpt_arm_smmuv3 *arg,
				  unsigned int *eats)
{
	unsigned int cfg;

	if (!(arg->ste[0] & STRTAB_STE_0_V)) {
		memset(arg->ste, 0, sizeof(arg->ste));
		return 0;
	}

quoted

+
+	eats = FIELD_GET(STRTAB_STE_1_EATS, le64_to_cpu(arg.ste[1]));
+	if (eats != STRTAB_STE_1_EATS_ABT)
+		return ERR_PTR(-EIO);
+
+	if (cfg != STRTAB_STE_0_CFG_S1_TRANS)
+		eats = STRTAB_STE_1_EATS_ABT;

this check sounds redundant. If the last check passes then eats is
already set to _ABT. 

Yes.. This hunk needs to go into this patch:

https://lore.kernel.org/linux-iommu/3962bef2ca6ab9bd06a52910f114345ecfe48ba6.1724776335.git.nicolinc@nvidia.com/T/#u (local)

quoted

+/**
+ * struct iommu_hwpt_arm_smmuv3 - ARM SMMUv3 Context Descriptor
Table info
+ *                                (IOMMU_HWPT_DATA_ARM_SMMUV3)
+ *
+ * @ste: The first two double words of the user space Stream Table Entry
for
+ *       a user stage-1 Context Descriptor Table. Must be little-endian.
+ *       Allowed fields: (Refer to "5.2 Stream Table Entry" in SMMUv3 HW
Spec)
+ *       - word-0: V, Cfg, S1Fmt, S1ContextPtr, S1CDMax
+ *       - word-1: S1DSS, S1CIR, S1COR, S1CSH, S1STALLD

Not sure whether EATS should be documented here or not. It's handled 
but must be ZERO at this point.

Let's put it in the above patch

Thanks,
Jason