Re: [PATCH v7 7/9] iommu/arm-smmu-v3: Move the CD generation for SVA into a function

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Apr 17, 2024 at 12:37:19AM -0700, Nicolin Chen wrote:
> On Tue, Apr 16, 2024 at 04:28:18PM -0300, Jason Gunthorpe wrote:
> > Pull all the calculations for building the CD table entry for a mmu_struct
> > into arm_smmu_make_sva_cd().
> > 
> > Call it in the two places installing the SVA CD table entry.
> > 
> > Open code the last caller of arm_smmu_update_ctx_desc_devices() and remove
> > the function.
> > 
> > Remove arm_smmu_write_ctx_desc() since all callers are gone. Add the
> > locking assertions to arm_smmu_alloc_cd_ptr() since
> > arm_smmu_update_ctx_desc_devices() was the last problematic caller.
> > 
> > Remove quiet_cd since all users are gone, arm_smmu_make_sva_cd() creates
> > the same value.
> > 
> > The behavior of quiet_cd changes slightly, the old implementation edited
> > the CD in place to set CTXDESC_CD_0_TCR_EPD0 assuming it was a SVA CD
> > entry. This version generates a full CD entry with a 0 TTB0 and relies on
> > arm_smmu_write_cd_entry() to install it hitlessly.
> > 
> > Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> > Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> > +static void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
> > +				 struct arm_smmu_master *master,
> > +				 struct mm_struct *mm, u16 asid)
> > +{
> > +	u64 par;
> > +
> > +	memset(target, 0, sizeof(*target));
> > +
> > +	par = cpuid_feature_extract_unsigned_field(
> > +		read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1),
> > +		ID_AA64MMFR0_EL1_PARANGE_SHIFT);
> > +
> > +	target->data[0] = cpu_to_le64(
> > +		CTXDESC_CD_0_TCR_EPD1 |
> > +#ifdef __BIG_ENDIAN
> > +		CTXDESC_CD_0_ENDI |
> > +#endif
> > +		CTXDESC_CD_0_V |
> > +		FIELD_PREP(CTXDESC_CD_0_TCR_IPS, par) |
> > +		CTXDESC_CD_0_AA64 |
> > +		(master->stall_enabled ? CTXDESC_CD_0_S : 0) |
> > +		CTXDESC_CD_0_R |
> > +		CTXDESC_CD_0_A |
> > +		CTXDESC_CD_0_ASET |
> > +		FIELD_PREP(CTXDESC_CD_0_ASID, asid));
> 
> This is set for the new "quiet_cd" case too. IIUIC, it is used to
> ease the switching back to a normal CD, i.e. mm != NULL case?

If ASID is used by HW (eg for negative caching) then this is correct.

If ASID is not used by HW then this could be 0'd and we could adjust
the used calculation. It is still functionally correct as-is, just
slightly confusing.

I didn't notice anything in the spec about ASID interaction with
EPD0. The spec was otherwise pretty clear about which fields become
IGNORED by EPD0/1.

So I'm assuming ASID can be used by HW and must be set.

AFAICT this is what the current code does, when it programs "quiet_cd"
it doesn't actually write the whole CD it just flips EPD0 to 1
in-place. Since this is only done from a CD already programmed to a
SVA the ASID remains set.

Jason