Re: [PATCH v3 04/19] iommu/arm-smmu-v3: Make STE programming independent of the callers

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Jan 29, 2024 at 07:10:47PM +0000, Mostafa Saleh wrote:

> > Going forward this will use a V=0 transition instead of cycling through
> > ABORT if a hitfull change is required. This seems more appropriate as ABORT
> > will fail DMAs without any logging, but dropping a DMA due to transient
> > V=0 is probably signaling a bug, so the C_BAD_STE is valuable.
> Would the driver do anything in that case, or would just print the log message?

Just log, AFAIK.
 
> > +static bool arm_smmu_write_entry_step(__le64 *cur, const __le64 *cur_used,
> > +				      const __le64 *target,
> > +				      const __le64 *target_used, __le64 *step,
> > +				      __le64 v_bit,
> I think this is confusing here, I believe we have this as an argument as this
> function would be used for CD later, however for this series it is unnecessary,
> IMHO, this should be removed and added in another patch for the CD rework.

It is alot of code churn to do that, even more on the new version.

> > +	used_bits->data[0] |= cpu_to_le64(STRTAB_STE_0_CFG);
> > +	switch (FIELD_GET(STRTAB_STE_0_CFG, le64_to_cpu(ent->data[0]))) {
> > +	case STRTAB_STE_0_CFG_ABORT:
> > +		break;
> > +	case STRTAB_STE_0_CFG_BYPASS:
> > +		used_bits->data[1] |= cpu_to_le64(STRTAB_STE_1_SHCFG);
> > +		break;
> > +	case STRTAB_STE_0_CFG_S1_TRANS:
> > +		used_bits->data[0] |= cpu_to_le64(STRTAB_STE_0_S1FMT |
> > +						  STRTAB_STE_0_S1CTXPTR_MASK |
> > +						  STRTAB_STE_0_S1CDMAX);
> > +		used_bits->data[1] |=
> > +			cpu_to_le64(STRTAB_STE_1_S1DSS | STRTAB_STE_1_S1CIR |
> > +				    STRTAB_STE_1_S1COR | STRTAB_STE_1_S1CSH |
> > +				    STRTAB_STE_1_S1STALLD | STRTAB_STE_1_STRW);
> > +		used_bits->data[1] |= cpu_to_le64(STRTAB_STE_1_EATS);
> > +		break;
> AFAIU, this is missing something like (while passing smmu->features)
> 	used_bits->data[2] |= features & ARM_SMMU_FEAT_NESTING ?
> 			      cpu_to_le64(STRTAB_STE_2_S2VMID) : 0;
> 
> As the SMMUv3 manual says:
> “ For a Non-secure STE when stage 2 is implemented (SMMU_IDR0.S2P == 1)
>   translations resulting from a StreamWorld == NS-EL1 configuration are
>   VMID-tagged with S2VMID when either of stage 1 (Config[0] == 1) or stage 2
>   (Config[1] == 1) provide translation.“
> 
> Which means in case of S1=>S2 switch or vice versa this algorithm will ignore
> VMID while it is used.

Ah, yes, that is a small miss, thanks. I don't think we need the
features test though, s2vmid doesn't mean something different if the
feature is not present..

> > +static void arm_smmu_write_ste(struct arm_smmu_device *smmu, u32 sid,
> > +			       struct arm_smmu_ste *ste,
> > +			       const struct arm_smmu_ste *target)
> > +{
> > +	struct arm_smmu_ste target_used;
> > +	int i;
> > +
> > +	arm_smmu_get_ste_used(target, &target_used);
> > +	/* Masks in arm_smmu_get_ste_used() are up to date */
> > +	for (i = 0; i != ARRAY_SIZE(target->data); i++)
> > +		WARN_ON_ONCE(target->data[i] & ~target_used.data[i]);
> In what situation this would be triggered, is that for future proofing,
> maybe we can move it to arm_smmu_get_ste_used()?

Yes, prevent people from making an error down the road.

It can't be in ste_used due to how this specific algorithm works
iteratively

And in the v4 version it still wouldn't be a good idea at this point
due to how the series slowly migrates STE and CD programming
over. There are cases where the current STE will not have been written
by this code and may not pass this test.

Thanks,
Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel