Message-ID: <20230926001220.GL13733@nvidia.com>
Date:   Mon, 25 Sep 2023 21:12:20 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Nicolin Chen <nicolinc@...dia.com>
Cc:     will@...nel.org, robin.murphy@....com, joro@...tes.org,
        jean-philippe@...aro.org, mshavit@...gle.com,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        iommu@...ts.linux.dev
Subject: Re: [PATCH v4 2/2] iommu/arm-smmu-v3: Refactor
 arm_smmu_write_strtab_ent()

On Mon, Sep 25, 2023 at 01:03:10PM -0700, Nicolin Chen wrote:
> On Mon, Sep 25, 2023 at 03:35:23PM -0300, Jason Gunthorpe wrote:
> > On Wed, Sep 20, 2023 at 01:52:04PM -0700, Nicolin Chen wrote:
> > >  
> > > +static void arm_smmu_ste_stage2_translate(struct arm_smmu_master *master,
> > > +					  u64 *ste)
> > > +{
> > > +	struct arm_smmu_domain *smmu_domain = master->domain;
> > > +	struct arm_smmu_device *smmu = master->smmu;
> > > +	struct arm_smmu_s2_cfg *s2_cfg;
> > > +
> > > +	switch (smmu_domain->stage) {
> > > +	case ARM_SMMU_DOMAIN_NESTED:
> > > +	case ARM_SMMU_DOMAIN_S2:
> > > +		s2_cfg = &smmu_domain->s2_cfg;
> > > +		break;
> > > +	default:
> > > +		WARN_ON(1);
> > > +		return;
> > > +	}
> > > +
> > > +	ste[0] |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_S2_TRANS);
> > > +
> > > +	if (smmu->features & ARM_SMMU_FEAT_STALLS && !master->stall_enabled)
> > > +		ste[1] |= STRTAB_STE_1_S1STALLD;
> > > +
> > > +	if (master->ats_enabled)
> > > +		ste[1] |= FIELD_PREP(STRTAB_STE_1_EATS, STRTAB_STE_1_EATS_TRANS);
> > 
> > These master bits probably belong in their own function 'setup ste for master'
> > 
> > The s1 and s2 cases are duplicating these things.
> 
> OK. I thought that writing these helpers in form of STE.Config
> field configurations could be more straightforward despite some
> duplications.

Ah, well, if you take that approach then maybe (and the names too) but
I'm not sure that is the best way..

The approach I had in mind was to go down a path depending on the
configuration of the master. eg if you have a type of domain or a cd
or whatever. That would imply a config, but not necessarily be
organized by config..

> > static void arm_smmu_clear_strtab_ent(struct arm_smmu_master *master,
> > 				      __le64 *dst)
> > {
> > 	u64 ste[4] = {};
> > 
> > 	ste[0] = STRTAB_STE_0_V;
> > 	if (disable_bypass)
> > 		arm_smmu_ste_abort(ste);
> > 	else
> > 		arm_smmu_ste_bypass(ste);
> > 	arm_smmu_store_ste(master, dst, &ste);
> > }
> > 
> > And use clear_strtab_ent from detach ??
> 
> We still need bypass() in arm_smmu_write_strtab_ent(). But this
> removes the abort() call and its confusing if-condition, I see.

Well, it sort of starts to set things up to not be domain sensitive in
this path.. Maybe it is a diversion on the path to just removing that
part of attach.
 
> > (but then I wonder why not set V=0 instead of STE.Config = abort?)
> 
> It seems that the difference is whether there would be or not a
> C_BAD_STE event, i.e. "STE.Config = abort" is a silent way for
> a disabled/disconnected device.

Makes sense

> > > +	for (i = 1; i < 4; i++) {
> > > +		if (dst[i] == cpu_to_le64(ste[i]))
> > > +			continue;
> > > +		dst[i] = cpu_to_le64(ste[i]);
> > > +		ste_sync_all = true;
> > > +	}
> > 
> > This isn't going to work if the transition is from a fully valid STE
> > to an invalid one, it will corrupt the still in-use bytes.
> 
> The driver currently doesn't have a case of unsetting STE_0_V?

Sorry, I didn't mean invalid, I ment different but valid.

 > > Though current code does this:
> > 
> > 		dst[0] = cpu_to_le64(val);
> > 		dst[1] = cpu_to_le64(FIELD_PREP(STRTAB_STE_1_SHCFG,
> > 						STRTAB_STE_1_SHCFG_INCOMING));
> > 		dst[2] = 0; /* Nuke the VMID */
> > 
> > Which I don't really understand either? Why is it OK to wipe the VMID
> > out of order with the STE.Config change?
> > 
> > Be sure to read the part of the SMMU spec talking about how to update
> > these things, 3.21.3.1 Configuration structure update procedure and
> > nearby.
> > 
> > Regardless there are clearly two orders in the existing code
> > 
> > Write 0,1,2,flush (translation -> bypass/fault)
> > 
> > Write 3,2,1,flush,0,flush (bypass/fault -> translation)
> > 
> > You still have to preserve both behaviors.
> > 
> > (interestingly neither seem to follow the guidance of the ARM manual,
> > so huh)
> 
> Again, it is probably because the driver never reverts the V
> bit back to 0? While chapter 3.21.3.1 is about V=0 <=> V=1.

No, the driver is using the config in a similar way to V. From what I
can tell it is making a bunch of assumptions that allow this to be OK,
but you have to follow the ordering it already has..


> > The bigger question is does this have to be more generic to handle
> > S1DSS which is it's original design goal?
> 
> It feels an overkill. Maybe "Refactor arm_smmu_write_strtab_ent()"
> is just a too big topic for my goal...

Maybe, it depends if it is necessary
 
> Overall, this version doesn't really dramatically change any STE
> configuration flow compared to what the current driver does, but
> only adds the S1DSS bypass setting. I'd prefer keeping this in a
> smaller scope..

Well, no, this stuff does seem to change the allowed state transitions
that this routine will encounter, or at the very least it sets the
stage for new state transitions that it cannot handle (eg under
iommufd control w/PASID we have problems)

However.. if you imagine staying within the existing kernel driver
behavior maybe just setting S1DSS does work out. It feels very
fragile, it depends on alot of other stuff also being just so.

So, at least for this series you might get buy without, but do check
all the different combinations of attachment's that are possible and
see that the STE doesn't become incorrect.

If it is OK then this patch can be its own series, it needs doing
anyhow.

Jason

Powered by blists - more mailing lists