Re: [PATCH v2 1/4] riscv, mm: detect svnapot cpu support at runtime

[<Conor.Dooley@microchip.com>]

On 16/07/2022 13:35, Jisheng Zhang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Sat, Jul 16, 2022 at 04:56:45PM +0800, panqinglin2020@iscas.ac.cn wrote:
>> From: Qinglin Pan <panqinglin2020@iscas.ac.cn>
>>
>> This patch add 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 patch 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.
>>
>> Yours,
>> Qinglin
> 
> hmm, this "Yours ..." should be removed in commit msg

And while they're at it, they can drop the "This patch".

I might've said this on a v1 (I know I said it on someone's
v1 recently) that having "Also, this patch" makes it sound
like this should actually be two patches and not one.

Thanks,
Conor.

>>
>> Signed-off-by: Qinglin Pan <panqinglin2020@iscas.ac.cn>
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index b17fd4666b0c..c5e1629a6033 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -385,6 +385,13 @@ config FPU
>>
>>         If you don't know what to do here, say Y.
>>
>> +config SVNAPOT
>> +     bool "Svnapot support"
> 
> Do we add an config opition for each isa extension? That's not
> necessary. I think we'd better remove this CONFIG option and keep
> unified Image in mind.
> 
>> +     default n
>> +     help
>> +       Select if your CPU supports Svnapot and you want to enable it when
>> +       kernel is booting.
>> +
>>  endmenu # "Platform type"
>>  menu "Kernel features"
>> diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
>> index 9e2888dbb5b1..84ad32075637 100644
>> --- a/arch/riscv/include/asm/errata_list.h
>> +++ b/arch/riscv/include/asm/errata_list.h
>> @@ -20,7 +20,8 @@
>>  #endif
>>
>>  #define      CPUFEATURE_SVPBMT 0
>> -#define      CPUFEATURE_NUMBER 1
>> +#define      CPUFEATURE_SVNAPOT 1
>> +#define      CPUFEATURE_NUMBER 2
>>
>>  #ifdef __ASSEMBLY__
>>
>> @@ -93,6 +94,27 @@ asm volatile(ALTERNATIVE(                                          \
>>  #define ALT_THEAD_PMA(_val)
>>  #endif
>>
>> +#define ALT_SVNAPOT(_val)                                            \
> 
> I believe SVNAPOT can be supported w/o ALTERNATIVE, static key mechanism
> would be much simpler.
> 
>> +asm(ALTERNATIVE("li %0, 0", "li %0, 1", 0,                           \
>> +             CPUFEATURE_SVNAPOT, CONFIG_SVNAPOT)                     \
>> +             : "=r"(_val) :)
>> +
>> +#define ALT_SVNAPOT_PTE_PFN(_val, _napot_shift, _pfn_mask, _pfn_shift)       \
>> +asm(ALTERNATIVE("and %0, %1, %2\n\t"                                 \
>> +             "srli %0, %0, %3\n\t"                                   \
>> +             "nop\n\tnop\n\tnop",                                    \
>> +             "srli t3, %1, %4\n\t"                                   \
>> +             "and %0, %1, %2\n\t"                                    \
>> +             "srli %0, %0, %3\n\t"                                   \
>> +             "sub  t4, %0, t3\n\t"                                   \
>> +             "and  %0, %0, t4",                                      \
>> +             0, CPUFEATURE_SVNAPOT, CONFIG_SVNAPOT)                  \
>> +             : "+r"(_val)                                            \
>> +             : "r"(_val),                                            \
>> +               "r"(_pfn_mask),                                       \
>> +               "i"(_pfn_shift),                                      \
>> +               "i"(_napot_shift))
>> +
>>  #endif /* __ASSEMBLY__ */
>>
>>  #endif
>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>> index e48eebdd2631..292ab93321e3 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_SVPBMT,
>> +     RISCV_ISA_EXT_SVNAPOT,
>>       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 5c2aba5efbd0..e8463515a46c 100644
>> --- a/arch/riscv/include/asm/pgtable-64.h
>> +++ b/arch/riscv/include/asm/pgtable-64.h
>> @@ -74,6 +74,20 @@ 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      (1UL << _PAGE_NAPOT_SHIFT)
>> +#define NAPOT_CONT64KB_ORDER 4UL
>> +#define NAPOT_CONT64KB_SHIFT (NAPOT_CONT64KB_ORDER + PAGE_SHIFT)
>> +#define NAPOT_CONT64KB_SIZE (1UL << NAPOT_CONT64KB_SHIFT)
>> +#define NAPOT_CONT64KB_MASK (NAPOT_CONT64KB_SIZE - 1)
>> +#define NAPOT_64KB_PTE_NUM (1UL << NAPOT_CONT64KB_ORDER)
>> +#define NAPOT_64KB_MASK (7UL << _PAGE_PFN_SHIFT)
>> +
>>  /*
>>   * [62:61] Svpbmt Memory Type definitions:
>>   *
>> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
>> index 1d1be9d9419c..34c4be9de79e 100644
>> --- a/arch/riscv/include/asm/pgtable.h
>> +++ b/arch/riscv/include/asm/pgtable.h
>> @@ -284,10 +284,38 @@ 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);
>> +     return _val;
>> +}
>> +
>> +#ifdef CONFIG_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)
>> +{
>> +     unsigned long napot_bits = (1UL << (order - 1)) << _PAGE_PFN_SHIFT;
>> +     unsigned long lower_prot =
>> +             pte_val(pte) & ((1UL << _PAGE_PFN_SHIFT) - 1UL);
>> +     unsigned long upper_prot = (pte_val(pte) >> _PAGE_PFN_SHIFT)
>> +                                << _PAGE_PFN_SHIFT;
>> +
>> +     return __pte(upper_prot | napot_bits | lower_prot | _PAGE_NAPOT);
>> +}
>> +#endif /* CONFIG_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 fba9e9f46a8c..9f1113fa2b96 100644
>> --- a/arch/riscv/kernel/cpu.c
>> +++ b/arch/riscv/kernel/cpu.c
>> @@ -89,6 +89,7 @@ int riscv_of_parent_hartid(struct device_node *node)
>>  static struct riscv_isa_ext_data isa_ext_arr[] = {
>>       __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>>       __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
>> +     __RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
>>       __RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
>>  };
>>
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index 1b3ec44e25f5..9f38b7d02f2a 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -198,6 +198,7 @@ void __init riscv_fill_hwcap(void)
>>                       } else {
>>                               SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
>>                               SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
>> +                             SET_ISA_EXT_MAP("svnapot", RISCV_ISA_EXT_SVNAPOT);
>>                       }
>>  #undef SET_ISA_EXT_MAP
>>               }
>> @@ -259,6 +260,20 @@ static bool __init_or_module cpufeature_probe_svpbmt(unsigned int stage)
>>       return false;
>>  }
>>
>> +static bool __init_or_module cpufeature_probe_svnapot(unsigned int stage)
>> +{
>> +#ifdef CONFIG_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.
>>   *
>> @@ -273,6 +288,9 @@ static u32 __init_or_module cpufeature_probe(unsigned int stage)
>>       if (cpufeature_probe_svpbmt(stage))
>>               cpu_req_feature |= (1U << CPUFEATURE_SVPBMT);
>>
>> +     if (cpufeature_probe_svnapot(stage))
>> +             cpu_req_feature |= (1U << CPUFEATURE_SVNAPOT);
>> +
>>       return cpu_req_feature;
>>  }
>>
>> --
>> 2.35.1
>>
>>
>> _______________________________________________
>> 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

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