Re: [PATCH v6 1/4] riscv: mm: modify pte format for Svnapot

From: Conor Dooley <conor@kernel.org>
To: Qinglin Pan <panqinglin2020@iscas.ac.cn>
Cc: Andrew Jones <ajones@ventanamicro.com>,
	palmer@dabbelt.com, linux-riscv@lists.infradead.org,
	jeff@riscv.org, xuyinan@ict.ac.cn
Subject: Re: [PATCH v6 1/4] riscv: mm: modify pte format for Svnapot
Date: Thu, 6 Oct 2022 20:47:29 +0100	[thread overview]
Message-ID: <Yz8w0dozxbyz311O@spud> (raw)
In-Reply-To: <071c426e-16be-2720-41c4-cede59e2d82b@iscas.ac.cn>

On Wed, Oct 05, 2022 at 10:46:36PM +0800, Qinglin Pan wrote:
> On 10/5/22 10:17 PM, Andrew Jones wrote:
> > On Wed, Oct 05, 2022 at 07:29:23PM +0800, panqinglin2020@iscas.ac.cn wrote:
> > > From: Qinglin Pan <panqinglin2020@iscas.ac.cn>

> > > diff --git a/arch/riscv/include/asm/pgtable-64.h b/arch/riscv/include/asm/pgtable-64.h
> > > index dc42375c2357..1cd0ffbfbdaa 100644
> > > --- a/arch/riscv/include/asm/pgtable-64.h
> > > +++ b/arch/riscv/include/asm/pgtable-64.h
> > > @@ -74,6 +74,19 @@ typedef struct {
> > >    */
> > >   #define _PAGE_PFN_MASK  GENMASK(53, 10)
> > > +/*
> > > + * [63] Svnapot definitions:
> > > + * 0 Svnapot disabled
> > > + * 1 Svnapot enabled
> > > + */
> > > +#define _PAGE_NAPOT_SHIFT	63
> > > +#define _PAGE_NAPOT		BIT(_PAGE_NAPOT_SHIFT)
> > > +#define NAPOT_CONT64KB_ORDER	4UL
> > > +#define NAPOT_CONT64KB_SHIFT	(NAPOT_CONT64KB_ORDER + PAGE_SHIFT)
> > > +#define NAPOT_CONT64KB_SIZE	BIT(NAPOT_CONT64KB_SHIFT)
> > > +#define NAPOT_CONT64KB_MASK	(NAPOT_CONT64KB_SIZE - 1UL)
> > > +#define NAPOT_64KB_PTE_NUM	BIT(NAPOT_CONT64KB_ORDER)
> > 
> > I still wonder if we shouldn't future-proof a bit with something like
> > 
> > /*
> >   * Only 64KB (order 4) napot ptes supported.
> >   */
> > #define napot_cont_order(pte)	4
> > 
> > #define napot_cont_shift(order)	((order) + PAGE_SHIFT)
> > #define napot_cont_size(order)	BIT(napot_cont_shift(order))
> > #define napot_cont_mask(order)	BIT(napot_cont_size(order) - 1)
> > #define napot_pte_num(order)	BIT(order)
> 
> Maybe we can declare legal napot order in a enum, and use the enum with
> these macros you have mentioned above. I will try to do this in the v8
> patchset.

I'm in a bit of a "keep it simple" camp for these things, I had to run
through this last time to make sure to myself that it made sense. I
think Drew's version is easier to follow despite (or due to?) the
"future-proofing". Whatever you do, a comment wouldn't go amiss I think.

Thanks,
Conor.

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