Re: [PATCH v2 8/8] iommu/arm-smmu-v3: Support IOMMU_DOMAIN_NESTED

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Aug 30, 2024 at 08:16:27AM +0000, Tian, Kevin wrote:
> > 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.

> > @@ -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.

> > +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.

> > +
> > +	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.
> 
> > +
> > +	if (flags || !(master->smmu->features &
> > ARM_SMMU_FEAT_TRANS_S1))
> > +		return ERR_PTR(-EOPNOTSUPP);
> 
> Is it possible when NESTING is supported?
> 
> > +
> > +	if (!(parent->type & __IOMMU_DOMAIN_PAGING))
> > +		return ERR_PTR(-EINVAL);
> 
> Just check parent->nest_parent
> 
> > +
> > +	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

> > +
> > +	/* 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;
	}


> > +
> > +	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

> > +/**
> > + * 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