[PATCH v5 1/4] mm: modify pte format for Svnapot
Conor Dooley
conor at kernel.org
Tue Oct 4 11:33:05 PDT 2022
On Tue, Oct 04, 2022 at 07:00:27PM +0200, Andrew Jones wrote:
> On Mon, Oct 03, 2022 at 09:47:18PM +0800, panqinglin2020 at iscas.ac.cn wrote:
> > From: Qinglin Pan <panqinglin2020 at 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 at 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
More information about the linux-riscv
mailing list