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

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Aug 27, 2024 at 02:23:07PM -0700, Nicolin Chen wrote:
> On Tue, Aug 27, 2024 at 12:51:38PM -0300, Jason Gunthorpe wrote:
> > 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.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> With some small nits:
> 
> > @@ -2192,6 +2255,16 @@ static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
> >  	}
> >  	__arm_smmu_tlb_inv_range(&cmd, iova, size, granule, smmu_domain);
> >  
> > +	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2 &&
> > +	    smmu_domain->nest_parent) {
> 
> smmu_domain->nest_parent alone is enough?

Yes, I thought I did that when Robin noted it.. 

> [---]
> > +static int arm_smmu_attach_dev_nested(struct iommu_domain *domain,
> > +				      struct device *dev)
> > +{
> [..]
> > +	if (arm_smmu_ssids_in_use(&master->cd_table) ||
> 
> This feels more like a -EBUSY as it would be unlikely able to
> attach to a different nested domain?

Yeah, we did that in arm_smmu_attach_dev()

> > +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);
> > +
> > +	ret = iommu_copy_struct_from_user(&arg, user_data,
> > +					  IOMMU_HWPT_DATA_ARM_SMMUV3, ste);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	if (flags || !(master->smmu->features & ARM_SMMU_FEAT_TRANS_S1))
> > +		return ERR_PTR(-EOPNOTSUPP);
> 
> A bit redundant to the first sanity against ARM_SMMU_FEAT_NESTING,
> since ARM_SMMU_FEAT_NESTING includes ARM_SMMU_FEAT_TRANS_S1.

Yeah, I think this was ment to be up at the top

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

> > +
> > +	if (!(parent->type & __IOMMU_DOMAIN_PAGING))
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	smmu_parent = to_smmu_domain(parent);
> > +	if (smmu_parent->stage != ARM_SMMU_DOMAIN_S2 ||
> 
> Maybe "!smmu_parent->nest_parent" instead.

Hmm, yes.. Actually we can delete it, and the paging test above.

The core code checks it.

Though I think we missed owner validation there??

@@ -225,7 +225,8 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx,
        if ((flags & ~IOMMU_HWPT_FAULT_ID_VALID) ||
            !user_data->len || !ops->domain_alloc_user)
                return ERR_PTR(-EOPNOTSUPP);
-       if (parent->auto_domain || !parent->nest_parent)
+       if (parent->auto_domain || !parent->nest_parent ||
+           parent->common.domain->owner != ops)
                return ERR_PTR(-EINVAL);

Right??

> [---]
> > +	    smmu_parent->smmu != master->smmu)
> > +		return ERR_PTR(-EINVAL);
> 
> It'd be slightly nicer if we do all the non-arg validations prior
> to calling iommu_copy_struct_from_user(). Then, the following arg
> validations would be closer to the copy().

Sure

> >  struct arm_smmu_entry_writer {
> > @@ -830,6 +849,7 @@ struct arm_smmu_master_domain {
> >  	struct list_head devices_elm;
> >  	struct arm_smmu_master *master;
> >  	ioasid_t ssid;
> > +	u8 nest_parent;
> 
> Would it be nicer to match with the one in struct arm_smmu_domain:
> +	bool				nest_parent : 1;
> ?

Ah, lets just use bool

> > + * 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
> 
> It seems that word-1 is missing EATS.

Yes, this was missed

Jason