Message-ID: <ZRI5ZNQMi3GYtS3I@Asurada-Nvidia>
Date:   Mon, 25 Sep 2023 18:52:36 -0700
From:   Nicolin Chen <nicolinc@...dia.com>
To:     Jason Gunthorpe <jgg@...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 09:12:20PM -0300, Jason Gunthorpe wrote:
> 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..

This sounds pretty much like what arm_smmu_write_strtab_ent() is
already doing, but you just want some tidy reorganizations?

So, by separating domain=NULL case to clear_ste(), we could do:

if (cdtab)
	setup_ste_by_cdtab(cdtab, domain); // still needs domain :-/
else if (master->domain->stage == S2)
	setup_ste_by_domain(domain); // literally "by s2_cfg"
else
	setup_ste_bypass();

setup_ste_by_master(); // include this in by_cdtab/by_domain?

> > > > +	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.

Then you meant a translation from valid to another valid will
be corrupted? Though that's how the driver currently switches
between valid STE configurations by staging the STE to bypass
or abort mode via detach_dev()?

>  > > 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 "ordering" in the driver or spec?

I found Robin previous remarks around this topic"
"Strictly I think we are safe to do that - fill in all the S1* fields
 while Config[0] is still 0 and they're ignored, sync, then set
 Config[0]. Adding a CD table under a translation domain should be
 achievable as well, since S1CDMax, S1ContextPtr and S1Fmt can all be
 updated together atomically (although it's still the kind of switcheroo
 where I'd be scared of a massive boulder suddenly rolling out of the
 ceiling...)"

I think this answers most of the questions above?

> > > 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.

Fine.. I can try that then. It looks that we have quite a thing
to do before I can respin the vSMMU series. 

Thanks!
Nic

Powered by blists - more mailing lists