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

[Conor Dooley <conor@kernel.org>]

On Tue, Oct 04, 2022 at 07:00:27PM +0200, Andrew Jones wrote:
> On Mon, Oct 03, 2022 at 09:47:18PM +0800, panqinglin2020@iscas.ac.cn wrote:
> > From: Qinglin Pan <panqinglin2020@iscas.ac.cn>
> > 
> > This commit adds two erratas to enable/disable svnapot support, patches
> > code dynamicly when "svnapot" is in the "riscv,isa" field of fdt and
> > SVNAPOT compile option is set. It will influence the behavior of
> > has_svnapot function and pte_pfn function. All code dependent on svnapot
> > should make sure that has_svnapot return true firstly.
> > 
> > Also, this commit modifies PTE definition for Svnapot, and creates some
> > functions in pgtable.h to mark a PTE as napot and check if it is a Svnapot
> > PTE. Until now, only 64KB napot size is supported in draft spec, so some
> > macros has only 64KB version.
> > 
> > Signed-off-by: Qinglin Pan <panqinglin2020@iscas.ac.cn>
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index d557cc50295d..4354024aae21 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -383,6 +383,20 @@ config RISCV_ISA_C
> >  
> >  	  If you don't know what to do here, say Y.
> >  
> > +config RISCV_ISA_SVNAPOT
> > +	bool "SVNAPOT extension support"
> > +	depends on 64BIT && MMU
> > +	select RISCV_ALTERNATIVE
> > +	default n
> 
> What's the reason for this to be default no? If there's a trade-off to be
> made which requires opt-in, then it should be described in the text below.

In addition, existing extensions are default y unless they depend on a
compiler option.

> > +	help
> > +	  Say Y if you want to allow kernel to detect SVNAPOT ISA-extension
> > +	  dynamically in boot time and enable its usage.
> > +
> > +	  SVNAPOT extension helps to mark contiguous PTEs as a range
> > +	  of contiguous virtual-to-physical translations, with a naturally
> > +	  aligned power-of-2 (NAPOT) granularity larger than the base 4KB page
> > +	  size.
> > +
> >  config RISCV_ISA_SVPBMT
> >  	bool "SVPBMT extension support"
> >  	depends on 64BIT && MMU
> > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
> > index 19a771085781..f3aff5ef52e4 100644
> > --- a/arch/riscv/include/asm/errata_list.h
> > +++ b/arch/riscv/include/asm/errata_list.h
> > @@ -22,7 +22,8 @@
> >  
> >  #define	CPUFEATURE_SVPBMT 0
> >  #define	CPUFEATURE_ZICBOM 1
> > -#define	CPUFEATURE_NUMBER 2
> > +#define	CPUFEATURE_SVNAPOT 2
> > +#define	CPUFEATURE_NUMBER 3
> 
> nit: Maybe take the opportunity to tab out the numbers to get them all
> lined up. And tab enough for future longer names too.

s/maybe// ;)

> 
> >  
> >  #ifdef __ASSEMBLY__
> >  
> > @@ -142,6 +143,26 @@ asm volatile(ALTERNATIVE_2(						\
> >  	    "r"((unsigned long)(_start) + (_size))			\
> >  	: "a0")
> >  
> > +#define ALT_SVNAPOT(_val)						\
> > +asm(ALTERNATIVE("li %0, 0", "li %0, 1", 0,				\
> > +		CPUFEATURE_SVNAPOT, CONFIG_RISCV_ISA_SVNAPOT)		\
> > +		: "=r"(_val) :)
> > +
> > +#define ALT_SVNAPOT_PTE_PFN(_val, _napot_shift, _pfn_mask, _pfn_shift)	\
> > +asm(ALTERNATIVE("and %0, %0, %1\n\t"					\
> > +		"srli %0, %0, %2\n\t"					\
> > +		__nops(3),						\
> > +		"srli t3, %0, %3\n\t"					\
> > +		"and %0, %0, %1\n\t"					\
> > +		"srli %0, %0, %2\n\t"					\
> > +		"sub  t4, %0, t3\n\t"					\
> > +		"and  %0, %0, t4",					\
> 
> This implements
> 
>   temp = ((pte & _PAGE_PFN_MASK) >> _PAGE_PFN_SHIFT);
>   pfn = temp & (temp - (pte >> _PAGE_NAPOT_SHIFT));
> 
> which for a non-napot pte just returns the same as the non-napot
> case would, but for a napot pte we return the same as the non-napot
> case but with its first set bit cleared, wherever that first set
> bit was. Can you explain to me why that's what we want to do?
> 
> > +		0, CPUFEATURE_SVNAPOT, CONFIG_RISCV_ISA_SVNAPOT)	\
> > +		: "+r"(_val)						\
> > +		: "r"(_pfn_mask),					\
> > +		  "i"(_pfn_shift),					\
> > +		  "i"(_napot_shift))
> 
> Should add t3 and t4 to clobbers list.
> 
> > +
> >  #endif /* __ASSEMBLY__ */
> >  
> >  #endif
> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > index 6f59ec64175e..83b8e21a3573 100644
> > --- a/arch/riscv/include/asm/hwcap.h
> > +++ b/arch/riscv/include/asm/hwcap.h
> > @@ -54,10 +54,11 @@ extern unsigned long elf_hwcap;
> >   */
> >  enum riscv_isa_ext_id {
> >  	RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
> > +	RISCV_ISA_EXT_SSTC,
> > +	RISCV_ISA_EXT_SVNAPOT,
> >  	RISCV_ISA_EXT_SVPBMT,
> >  	RISCV_ISA_EXT_ZICBOM,
> >  	RISCV_ISA_EXT_ZIHINTPAUSE,
> > -	RISCV_ISA_EXT_SSTC,
> 
> Opportunistic alphabetizing an enum is a bit risky. It'd be better if this
> was a separate patch, because, even though nothing should care about the
> order, if something does care about the order, then it'd be easier to
> debug when this when changed alone.

I think for these things it is s/alphabetical/canonical order, given the
patch from Palmer I linked in my previous mail. Although thinking about
it, that means V before S, so SV before SS? Which his patch did not do:
https://lore.kernel.org/linux-riscv/20220920204518.10988-1-palmer@rivosinc.com/

> 
> >  	RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
> >  };
> >  
> > diff --git a/arch/riscv/include/asm/pgtable-64.h b/arch/riscv/include/asm/pgtable-64.h
> > index dc42375c2357..25ec541192e5 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)
> 
> Please make three lined up columns out of these defines.
> 
> #define DEF		VAL
> 
> > +
> >  /*
> >   * [62:61] Svpbmt Memory Type definitions:
> >   *
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > index 7ec936910a96..c3fc3c661699 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -264,10 +264,39 @@ static inline pte_t pud_pte(pud_t pud)
> >  	return __pte(pud_val(pud));
> >  }
> >  
> > +static inline bool has_svnapot(void)
> > +{
> > +	u64 _val;
> > +
> > +	ALT_SVNAPOT(_val);
> 
> Why aren't we using the isa extension static key framework for this?
> 
> > +	return _val;
> > +}
> > +
> > +#ifdef CONFIG_RISCV_ISA_SVNAPOT
> > +
> > +static inline unsigned long pte_napot(pte_t pte)
> > +{
> > +	return pte_val(pte) & _PAGE_NAPOT;
> > +}
> > +
> > +static inline pte_t pte_mknapot(pte_t pte, unsigned int order)
> > +{
> > +	int pos = order - 1 + _PAGE_PFN_SHIFT;
> > +	unsigned long napot_bit = BIT(pos);
> > +	unsigned long napot_mask = ~GENMASK(pos, _PAGE_PFN_SHIFT);
> > +
> > +	return __pte((pte_val(pte) & napot_mask) | napot_bit | _PAGE_NAPOT);
> > +}
> > +#endif /* CONFIG_RISCV_ISA_SVNAPOT */
> > +
> >  /* Yields the page frame number (PFN) of a page table entry */
> >  static inline unsigned long pte_pfn(pte_t pte)
> >  {
> > -	return __page_val_to_pfn(pte_val(pte));
> > +	unsigned long _val  = pte_val(pte);
> > +
> > +	ALT_SVNAPOT_PTE_PFN(_val, _PAGE_NAPOT_SHIFT,
> > +			    _PAGE_PFN_MASK, _PAGE_PFN_SHIFT);
> > +	return _val;
> >  }
> >  
> >  #define pte_page(x)     pfn_to_page(pte_pfn(x))
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index 0be8a2403212..d2a61122c595 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -96,6 +96,7 @@ static struct riscv_isa_ext_data isa_ext_arr[] = {
> >  	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
> >  	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> >  	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> > +	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
> 
> If we want to do a separate isa extension alphabetizing patch, then this
> array would be a good one to do since it's the output order.
> 
> >  	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
> >  };
> >  
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 553d755483ed..3557c5cc6f04 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -204,6 +204,7 @@ void __init riscv_fill_hwcap(void)
> >  				SET_ISA_EXT_MAP("zicbom", RISCV_ISA_EXT_ZICBOM);
> >  				SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
> >  				SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
> > +				SET_ISA_EXT_MAP("svnapot", RISCV_ISA_EXT_SVNAPOT);
> 
> Could alphabetize this too, even though it doesn't matter.
> 
> >  			}
> >  #undef SET_ISA_EXT_MAP
> >  		}
> > @@ -284,6 +285,20 @@ static bool __init_or_module cpufeature_probe_zicbom(unsigned int stage)
> >  	return false;
> >  }
> >  
> > +static bool __init_or_module cpufeature_probe_svnapot(unsigned int stage)
> > +{
> > +#ifdef CONFIG_RISCV_ISA_SVNAPOT

Is there a reason that this cannot be if IS_ENABLED(RISCV_ISA_SVNAPOT)?

> > +	switch (stage) {
> > +	case RISCV_ALTERNATIVES_EARLY_BOOT:
> > +		return false;
> > +	default:
> > +		return riscv_isa_extension_available(NULL, SVNAPOT);
> > +	}
> > +#endif
> > +
> > +	return false;
> > +}
> > +
> >  /*
> >   * Probe presence of individual extensions.
> >   *
> > @@ -301,6 +316,9 @@ static u32 __init_or_module cpufeature_probe(unsigned int stage)
> >  	if (cpufeature_probe_zicbom(stage))
> >  		cpu_req_feature |= (1U << CPUFEATURE_ZICBOM);

While in the area, could this be converted to BIT(CPUFEATURE_ZICBOM)?

> >  
> > +	if (cpufeature_probe_svnapot(stage))
> > +		cpu_req_feature |= BIT(CPUFEATURE_SVNAPOT);
> > +
> >  	return cpu_req_feature;
> >  }
> >  
> > -- 
> > 2.35.1
> >
> 
> Thanks,
> drew
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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