Re: [PATCH V11 03/17] riscv: Use Zicbop in arch_xchg when available

[Andrew Jones <ajones@ventanamicro.com>]

On Fri, Sep 15, 2023 at 12:37:50PM +0100, Conor Dooley wrote:
> Yo,
> 
> On Thu, Sep 14, 2023 at 04:47:18PM +0200, Andrew Jones wrote:
> > On Thu, Sep 14, 2023 at 04:25:53PM +0200, Andrew Jones wrote:
> > > On Sun, Sep 10, 2023 at 04:28:57AM -0400, guoren@kernel.org wrote:
> > > > From: Guo Ren <guoren@linux.alibaba.com>
> > > > 
> > > > Cache-block prefetch instructions are HINTs to the hardware to
> > > > indicate that software intends to perform a particular type of
> > > > memory access in the near future. Enable ARCH_HAS_PREFETCHW and
> > > > improve the arch_xchg for qspinlock xchg_tail.
> > > > 
> > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > > ---
> > > >  arch/riscv/Kconfig                 | 15 +++++++++++++++
> > > >  arch/riscv/include/asm/cmpxchg.h   |  4 +++-
> > > >  arch/riscv/include/asm/hwcap.h     |  1 +
> > > >  arch/riscv/include/asm/insn-def.h  |  5 +++++
> > > >  arch/riscv/include/asm/processor.h | 13 +++++++++++++
> > > >  arch/riscv/kernel/cpufeature.c     |  1 +
> > > >  6 files changed, 38 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > index e9ae6fa232c3..2c346fe169c1 100644
> > > > --- a/arch/riscv/Kconfig
> > > > +++ b/arch/riscv/Kconfig
> > > > @@ -617,6 +617,21 @@ config RISCV_ISA_ZICBOZ
> > > >  
> > > >  	   If you don't know what to do here, say Y.
> > > >  
> > > > +config RISCV_ISA_ZICBOP
> > > 
> > > Even if we're not concerned with looping over blocks yet, I think we
> > > should introduce zicbop block size DT parsing at the same time we bring
> > > zicbop support to the kernel (it's just more copy+paste from zicbom and
> > > zicboz). It's a bit annoying that the CMO spec doesn't state that block
> > > sizes should be the same for m/z/p. And, the fact that m/z/p are all
> > > separate extensions leads us to needing to parse block sizes for all
> > > three, despite the fact that in practice they'll probably be the same.
> > 
> > Although, I saw on a different mailing list that Andrei Warkentin
> > interpreted section 2.7 "Software Discovery" of the spec, which states
> > 
> > """
> > The initial set of CMO extensions requires the following information to be
> > discovered by software:
> > 
> > * The size of the cache block for management and prefetch instructions
> > * The size of the cache block for zero instructions
> > * CBIE support at each privilege level
> > 
> > Other general cache characteristics may also be specified in the discovery
> > mechanism.
> > """
> > 
> > as management and prefetch having the same block size and only zero
> > potentially having a different size. That looks like a reasonable
> > interpretation to me, too.
> 
> TBH, I don't really care what ambiguous wording the spec has used, we
> have the opportunity to make better decisions if we please. I hate the
> fact that the specs are often not abundantly clear about things like this.
> 
> > So, we could maybe proceed with assuming we
> > can use zicbom_block_size for prefetch, for now. If a platform comes along
> > that interpreted the spec differently, requiring prefetch block size to
> > be specified separately, then we'll cross that bridge when we get there.
> 
> That said, I think I suggested originally having the zicboz stuff default
> to the zicbom size too, so I'd be happy with prefetch stuff working
> exclusively that way until someone comes along looking for different sizes.
> The binding should be updated though since
> 
>   riscv,cbom-block-size:
>     $ref: /schemas/types.yaml#/definitions/uint32
>     description:
>       The blocksize in bytes for the Zicbom cache operations.
> 
> would no longer be a complete description.
> 
> While thinking about new wording though, it feels really clunky to describe
> it like:
> 	The block size in bytes for the Zicbom cache operations, Zicbop
> 	cache operations will default to this block size where not
> 	explicitly defined.
> 
> since there's then no way to actually define the block size if it is
> different. Unless you've got some magic wording, I'd rather document
> riscv,cbop-block-size, even if we are going to use riscv,cbom-block-size
> as the default.
>

Sounds good to me, but if it's documented, then we should probably
implement its parsing. Then, at that point, I wonder if it makes sense to
have the fallback at all, or if it's not better just to require all the
DTs to be explicit (even if redundant).

Thanks,
drew

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