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

[Nicolin Chen <nicolinc@nvidia.com>]

On Wed, Apr 17, 2024 at 10:17:27AM -0300, Jason Gunthorpe wrote:
> 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.

Oh right. I misunderstood the last part of the commit message
about the quiet_cd. It's clear now. Thanks!

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