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

Jason Gunthorpe
2024-10-30T23:59:38Z

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