Re: [PATCH v9 1/3] riscv: mm: modify pte format for Svnapot

Qinglin Pan
2022-12-08T17:35:10Z

Hi all,

On 2022/12/8 12:51, Qinglin Pan wrote:
> Hey!
>
> On 2022/12/8 02:46, Conor Dooley wrote:
>> Hey!
>>
>> Couple small remarks and questions for you.
>>
>> On Sun, Dec 04, 2022 at 10:11:35PM +0800, panqinglin2020@iscas.ac.cn
>> wrote:
>>> From: Qinglin Pan <panqinglin2020@iscas.ac.cn>
>>>
>>> Add one alternative to enable/disable svnapot support, enable this
>>> static
>>> key when "svnapot" is in the "riscv,isa" field of fdt and SVNAPOT
>>> compile
>>> option is set. It will influence the behavior of has_svnapot. All code
>>> dependent on svnapot should make sure that has_svnapot return true
>>> firstly.
>>>
>>> Modify 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 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 ef8d66de5f38..1d8477c0af7c 100644
>>> --- a/arch/riscv/Kconfig
>>> +++ b/arch/riscv/Kconfig
>>> @@ -395,6 +395,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 y
>>> +    help
>>> +      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 4180312d2a70..beadb1126ed9 100644
>>> --- a/arch/riscv/include/asm/errata_list.h
>>> +++ b/arch/riscv/include/asm/errata_list.h
>>> @@ -22,9 +22,10 @@
>>>   #define    ERRATA_THEAD_NUMBER 3
>>>   #endif
>>> -#define    CPUFEATURE_SVPBMT 0
>>> -#define    CPUFEATURE_ZICBOM 1
>>> -#define    CPUFEATURE_NUMBER 2
>>> +#define    CPUFEATURE_SVPBMT    0
>>> +#define    CPUFEATURE_ZICBOM    1
>>> +#define    CPUFEATURE_SVNAPOT    2
>>> +#define    CPUFEATURE_NUMBER    3
>>>   #ifdef __ASSEMBLY__
>>> @@ -156,6 +157,14 @@ asm volatile(ALTERNATIVE(                        \
>>>       : "=r" (__ovl) :                        \
>>>       : "memory")
>>> +#define ALT_SVNAPOT(_val)                        \
>>> +asm(ALTERNATIVE(                            \
>>> +    "li %0, 0",                            \
>>> +    "li %0, 1",                            \
>>> +        0, CPUFEATURE_SVNAPOT, CONFIG_RISCV_ISA_SVNAPOT)    \
>>> +    : "=r" (_val) :                            \
>>> +    : "memory")
>>> +
>>>   #endif /* __ASSEMBLY__ */
>>>   #endif
>>> diff --git a/arch/riscv/include/asm/hwcap.h
>>> b/arch/riscv/include/asm/hwcap.h
>>> index b22525290073..4cbc1f45ab26 100644
>>> --- a/arch/riscv/include/asm/hwcap.h
>>> +++ b/arch/riscv/include/asm/hwcap.h
>>> @@ -54,6 +54,7 @@ extern unsigned long elf_hwcap;
>>>    */
>>>   enum riscv_isa_ext_id {
>>>       RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
>>> +    RISCV_ISA_EXT_SVNAPOT,
>>>       RISCV_ISA_EXT_SVPBMT,
>>>       RISCV_ISA_EXT_ZICBOM,
>>>       RISCV_ISA_EXT_ZIHINTPAUSE,
>>> @@ -87,7 +88,6 @@ static __always_inline int riscv_isa_ext2key(int num)
>>>   {
>>>       switch (num) {
>>>       case RISCV_ISA_EXT_f:
>>> -        return RISCV_ISA_EXT_KEY_FPU;
>>>       case RISCV_ISA_EXT_d:
>>>           return RISCV_ISA_EXT_KEY_FPU;
>>>       case RISCV_ISA_EXT_ZIHINTPAUSE:
>>> diff --git a/arch/riscv/include/asm/page.h
>>> b/arch/riscv/include/asm/page.h
>>> index ac70b0fd9a9a..349fad5e35de 100644
>>> --- a/arch/riscv/include/asm/page.h
>>> +++ b/arch/riscv/include/asm/page.h
>>> @@ -16,11 +16,6 @@
>>>   #define PAGE_SIZE    (_AC(1, UL) << PAGE_SHIFT)
>>>   #define PAGE_MASK    (~(PAGE_SIZE - 1))
>>> -#ifdef CONFIG_64BIT
>>> -#define HUGE_MAX_HSTATE        2
>>> -#else
>>> -#define HUGE_MAX_HSTATE        1
>>> -#endif
>>>   #define HPAGE_SHIFT        PMD_SHIFT
>>>   #define HPAGE_SIZE        (_AC(1, UL) << HPAGE_SHIFT)
>>>   #define HPAGE_MASK              (~(HPAGE_SIZE - 1))
>>> diff --git a/arch/riscv/include/asm/pgtable-64.h
>>> b/arch/riscv/include/asm/pgtable-64.h
>>> index dc42375c2357..9611833907ec 100644
>>> --- a/arch/riscv/include/asm/pgtable-64.h
>>> +++ b/arch/riscv/include/asm/pgtable-64.h
>>> @@ -74,6 +74,40 @@ 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)
>>> +/*
>>> + * Only 64KB (order 4) napot ptes supported.
>>> + */
>>> +#define NAPOT_CONT_ORDER_BASE 4
>>> +enum napot_cont_order {
>>> +    NAPOT_CONT64KB_ORDER = NAPOT_CONT_ORDER_BASE,
>>> +    NAPOT_ORDER_MAX,
>>> +};
>>> +
>>> +#define for_each_napot_order(order)                        \
>>> +    for (order = NAPOT_CONT_ORDER_BASE; order < NAPOT_ORDER_MAX;
>>> order++)
>>> +#define for_each_napot_order_rev(order)                        \
>>
>> Would it be terrible to do s/rev/reverse to make things more obvious?
>
> Maybe we should just keep it in the same style as existing macros like
> for_each_mem_range/for_each_mem_range_rev :)
>
>>
>>> +    for (order = NAPOT_ORDER_MAX - 1;                    \
>>> +         order >= NAPOT_CONT_ORDER_BASE; order--)
>>> +#define napot_cont_order(val)    (__builtin_ctzl((val.pte >>
>>> _PAGE_PFN_SHIFT) << 1))
>>> +
>>> +#define napot_cont_shift(order)    ((order) + PAGE_SHIFT)
>>> +#define napot_cont_size(order)    BIT(napot_cont_shift(order))
>>> +#define napot_cont_mask(order)    (~(napot_cont_size(order) - 1UL))
>>> +#define napot_pte_num(order)    BIT(order)
>>> +
>>> +#ifdef CONFIG_RISCV_ISA_SVNAPOT
>>> +#define HUGE_MAX_HSTATE        (2 + (NAPOT_ORDER_MAX -
>>> NAPOT_CONT_ORDER_BASE))
>>> +#else
>>> +#define HUGE_MAX_HSTATE        2
>>> +#endif
>>
>> This is coming from a place of *complete* ignorance:
>> If CONFIG_RISCV_ISA_SVNAPOT is enabled in the kernel but there is no
>> support for it on a platform is it okay to change the value of
>> HUGE_MAX_HSTATE? Apologies if I've missed something obvious.
>
> :(
> You are right. If CONFIG_RISCV_ISA_SVNAPOT is enabled, HUGE_MAX_HSTATE
> will always be 3 whether has_svnapot() is true or false. I will try to
> fix this.

I am really expecting that HUGE_MAX_HSTATE can change according to the
platform situation when CONFIG_RISCV_ISA_SVNAPOT is enabled. But it
seems impossible to achive this with a minor patch. Because this value
is used as some static allocated arrays' length (for example, hstates in
mm/hugetlb.c:52) in arch-independent code :(

This immuture HUGE_MAX_HSTATE value will cause no functional error but
it really makes some objects used only partially when
CONFIG_RISCV_ISA_SVNAPOT is enabled but the platform has no svnapot
support :( I am not sure whether this patch can be accepted with this
feature? Or any other helpful idea to fix this problem?

Please let me know if I ignore any information about it.

Regards,
Qinglin.

>
>>
>>> +
>>>   /*
>>>    * [62:61] Svpbmt Memory Type definitions:
>>>    *
>>> diff --git a/arch/riscv/include/asm/pgtable.h
>>> b/arch/riscv/include/asm/pgtable.h
>>> index c61ae83aadee..99957b1270f2 100644
>>> --- a/arch/riscv/include/asm/pgtable.h
>>> +++ b/arch/riscv/include/asm/pgtable.h
>>> @@ -6,10 +6,12 @@
>>>   #ifndef _ASM_RISCV_PGTABLE_H
>>>   #define _ASM_RISCV_PGTABLE_H
>>> +#include <linux/jump_label.h>
>>>   #include <linux/mmzone.h>
>>>   #include <linux/sizes.h>
>>>   #include <asm/pgtable-bits.h>
>>> +#include <asm/hwcap.h>
>>>   #ifndef CONFIG_MMU
>>>   #define KERNEL_LINK_ADDR    PAGE_OFFSET
>>> @@ -264,10 +266,49 @@ static inline pte_t pud_pte(pud_t pud)
>>>       return __pte(pud_val(pud));
>>>   }
>>> +static __always_inline bool has_svnapot(void)
>>> +{
>>> +    unsigned int _val;
>>> +
>>> +    ALT_SVNAPOT(_val);
>>> +
>>> +    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);
>>> +}
>>> +
>>> +#else
>>> +
>>> +static inline unsigned long pte_napot(pte_t pte)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>> +#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 res  = __page_val_to_pfn(pte_val(pte));
>>
>> nit: extra space before the =
>
> Nice catch! Will remove this extra space :)
>
>>
>>> +
>>> +    if (has_svnapot() && pte_napot(pte))
>>> +        res = res & (res - 1UL);
>>> +
>>> +    return res;
>>>   }
>>>   #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 bf9dd6764bad..88495f5fcafd 100644
>>> --- a/arch/riscv/kernel/cpu.c
>>> +++ b/arch/riscv/kernel/cpu.c
>>> @@ -165,6 +165,7 @@ static struct riscv_isa_ext_data isa_ext_arr[] = {
>>>       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>>>       __RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
>>>       __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
>>> +    __RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
>>>       __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
>>>       __RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
>>>       __RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>>> diff --git a/arch/riscv/kernel/cpufeature.c
>>> b/arch/riscv/kernel/cpufeature.c
>>> index 694267d1fe81..eeed66c3d497 100644
>>> --- a/arch/riscv/kernel/cpufeature.c
>>> +++ b/arch/riscv/kernel/cpufeature.c
>>> @@ -205,6 +205,7 @@ void __init riscv_fill_hwcap(void)
>>>                   SET_ISA_EXT_MAP("zihintpause",
>>> RISCV_ISA_EXT_ZIHINTPAUSE);
>>>                   SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
>>>                   SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
>>> +                SET_ISA_EXT_MAP("svnapot", RISCV_ISA_EXT_SVNAPOT);
>>>               }
>>>   #undef SET_ISA_EXT_MAP
>>>           }
>>> @@ -252,6 +253,17 @@ void __init riscv_fill_hwcap(void)
>>>   }
>>>   #ifdef CONFIG_RISCV_ALTERNATIVE
>>> +static bool __init_or_module cpufeature_probe_svnapot(unsigned int
>>> stage)
>>> +{
>>> +    if (!IS_ENABLED(CONFIG_RISCV_ISA_SVNAPOT))
>>> +        return false;
>>> +
>>> +    if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
>>> +        return false;
>>> +
>>> +    return riscv_isa_extension_available(NULL, SVNAPOT);
>>> +}
>>> +
>>>   static bool __init_or_module cpufeature_probe_svpbmt(unsigned int
>>> stage)
>>>   {
>>>       if (!IS_ENABLED(CONFIG_RISCV_ISA_SVPBMT))
>>> @@ -289,6 +301,9 @@ static u32 __init_or_module
>>> cpufeature_probe(unsigned int stage)
>>>   {
>>>       u32 cpu_req_feature = 0;
>>> +    if (cpufeature_probe_svnapot(stage))
>>> +        cpu_req_feature |= BIT(CPUFEATURE_SVNAPOT);
>>> +
>>>       if (cpufeature_probe_svpbmt(stage))
>>>           cpu_req_feature |= BIT(CPUFEATURE_SVPBMT);
>>
>> There's a bunch of stuff in this patch that may go away, depending on
>> sequencing just FYI. See [1] for more. I wouldn't rebase on top of that,
>> but just so that you're aware.
>
> Thanks for the information! If this patchset will be merged firstly,
> Jisheng can easily replace implementation of has_svnapot with his
> interface. Otherwise I will do this replacement :)
>
> Thanks,
> Qinglin
>
>>
>> 1 -
>> https://patchwork.kernel.org/project/linux-riscv/cover/20221204174632.3677-1-jszhang@kernel.org/
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv