Re: [PATCH v2 2/8] iommu/arm-smmu-v3: Use S2FWB when available

[Nicolin Chen <nicolinc@nvidia.com>]

On Wed, Aug 28, 2024 at 03:30:37PM -0300, Jason Gunthorpe wrote:
> On Tue, Aug 27, 2024 at 12:48:13PM -0700, Nicolin Chen wrote:
> > Hi Jason,
> > 
> > On Tue, Aug 27, 2024 at 12:51:32PM -0300, Jason Gunthorpe wrote:
> > > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> > > index f5d9fd1f45bf49..9b3658aae21005 100644
> > > --- a/drivers/iommu/io-pgtable-arm.c
> > > +++ b/drivers/iommu/io-pgtable-arm.c
> > > @@ -106,6 +106,18 @@
> > >  #define ARM_LPAE_PTE_HAP_FAULT		(((arm_lpae_iopte)0) << 6)
> > >  #define ARM_LPAE_PTE_HAP_READ		(((arm_lpae_iopte)1) << 6)
> > >  #define ARM_LPAE_PTE_HAP_WRITE		(((arm_lpae_iopte)2) << 6)
> > > +/*
> > > + * For !FWB these code to:
> > > + *  1111 = Normal outer write back cachable / Inner Write Back Cachable
> > > + *         Permit S1 to override
> > > + *  0101 = Normal Non-cachable / Inner Non-cachable
> > > + *  0001 = Device / Device-nGnRE
> > > + * For S2FWB these code:
> > > + *  0110 Force Normal Write Back
> > > + *  0101 Normal* is forced Normal-NC, Device unchanged
> > > + *  0001 Force Device-nGnRE
> > > + */
> > > +#define ARM_LPAE_PTE_MEMATTR_FWB_WB	(((arm_lpae_iopte)0x6) << 2)
> > 
> > The other part looks good. Yet, would you mind sharing the location
> > that defines this 0x6 explicitly?
> 
> I'm looking at an older one ARM DDI 0487F.c
> 
> D5.5.5 Stage 2 memory region type and Cacheability attributes when FEAT_S2FWB is implemented
> 
> The text talks about the bits in the PTE, not relative to the MEMATTR
> field, so 6 << 2 encodes to:
> 
>  543210
>  011000
> 
> Then see table D5-40 Effect of bit[4] == 1 on Cacheability and Memory Type)
> 
>  Bit[5] = 0 = is RES0.
>  Bit[4] = 1 = determines the interpretation of bits [3:2].
>  Bits[3:2] == 10 == Normal Write-Back 
> 
> Here Bit means 'bit of the PTE' because the MemAttr does not have 5
> bits.
> 
> > Where it has the followings in D8.6.6:
> >  "For stage 2 translations, if FEAT_MTE_PERM is not implemented, then
> >   FEAT_S2FWB has all of the following effects on the MemAttr[3:2] bits:
> >    - MemAttr[3] is RES0.
> >    - The value of MemAttr[2] determines the interpretation of the
> >      MemAttr[1:0] bits.
> 
> And here the text switches from talking about the PTE bits to the
> Field bits. MemAttr[3] == PTE[5], and the above text matches D5.5
> 
> The use of numbering schemes relative to the start of the field and
> also relative to the PTE is tricky.

I download the version F.c, and the chapter looks cleaner than
the one in K.a. I guess the FEAT_MTE_PERM complicates that...

I double checked the K.a doc, and found a piece of pseudocode
that seems to confirm 0b0110 (0x6) is the correct value:

MemoryAttributes AArch64.S2ApplyFWBMemAttrs(MemoryAttributes s1_memattrs,
					    S2TTWParams walkparams,
					    bits(N) descriptor)
	MemoryAttributes memattrs;
	s2_attr = descriptor<5:2>;
	s2_sh = if walkparams.ds == '1' then walkparams.sh else descriptor<9:8>;
	s2_fnxs = descriptor<11>;
	if s2_attr<2> == '0' then // S2 Device, S1 any
		s2_device = DecodeDevice(s2_attr<1:0>);
		memattrs.memtype = MemType_Device;
		if s1_memattrs.memtype == MemType_Device then
			memattrs.device = S2CombineS1Device(s1_memattrs.device, s2_device);
		else
			memattrs.device = s2_device;
		memattrs.xs = s1_memattrs.xs;
	elsif s2_attr<1:0> == '11' then // S2 attr = S1 attr
		memattrs = s1_memattrs;
	elsif s2_attr<1:0> == '10' then // Force writeback

Thanks
Nicolin