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

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Oct 30, 2024 at 04:29:24PM +0000, Mostafa Saleh wrote:
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index b4b03206afbf48..eb401a4adfedc8 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -295,6 +295,7 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
> >  	case CMDQ_OP_TLBI_NH_ASID:
> >  		cmd[0] |= FIELD_PREP(CMDQ_TLBI_0_ASID, ent->tlbi.asid);
> >  		fallthrough;
> > +	case CMDQ_OP_TLBI_NH_ALL:
> >  	case CMDQ_OP_TLBI_S12_VMALL:
> >  		cmd[0] |= FIELD_PREP(CMDQ_TLBI_0_VMID, ent->tlbi.vmid);
> >  		break;
> > @@ -2230,6 +2231,15 @@ 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->nest_parent) {
> 
> Do we need a sync between the 2 invalidations to order them?

Which two do you mean?

Yes, we must flush the S2 IOTLB before we go on to flush the S1
ASID's.

But __arm_smmu_tlb_inv_range() calls arm_smmu_cmdq_batch_submit() on
all paths which has a sync inside it.

> > +		/*
> > +		 * When the S2 domain changes all the nested S1 ASIDs have to be
> > +		 * flushed too.
> > +		 */
> > +		cmd.opcode = CMDQ_OP_TLBI_NH_ALL;
> > +		arm_smmu_cmdq_issue_cmd_with_sync(smmu_domain->smmu, &cmd);
> > +	}
> > +
> >  	/*
> >  	 * Unfortunately, this can't be leaf-only since we may have
> >  	 * zapped an entire table.

Which was already needed because the lines right below are:
	arm_smmu_atc_inv_domain(smmu_domain, iova, size);

There must be a sync between IOTLB and ATC operations.

We also need a sync between the ATC operation and the all ASID
invalidation, which the _with_sync() should do.

> > +/**
> > + * struct iommu_hwpt_arm_smmuv3 - ARM SMMUv3 Context Descriptor Table info
> > + *                                (IOMMU_HWPT_DATA_ARM_SMMUV3)
> > + *
> 
> That’s supposed to be stream table?

I think when the comment was written the only option was to program a
context descriptor. Now it can do more.. Let's try:

/**
 * struct iommu_hwpt_arm_smmuv3 - ARM SMMUv3 nested STE
 *                                (IOMMU_HWPT_DATA_ARM_SMMUV3)
 *
 * @ste: The first two double words of the user space Stream Table Entry for
 *       the translation. 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: EATS, S1DSS, S1CIR, S1COR, S1CSH, S1STALLD
 *
 * -EIO will be returned if @ste is not legal or contains any non-allowed field.
 * Cfg can be used to select a S1, Bypass or Abort configuration. A Bypass
 * nested domain will translate the same as the nesting parent. The S1 will
 * install a Context Descriptor Table pointing at userspace memory translated
 * by the nesting parent.
 */

Jason