[PATCH v7 1/4] RISC-V: Add Svade and Svadu Extensions Support

[Clément Léger <cleger@rivosinc.com>]

Hi Yong-Xuan,


On 18/07/2024 18:43, Alexandre Ghiti wrote:
> Hi Yong-Xuan,
> 
> On 12/07/2024 10:38, Yong-Xuan Wang wrote:
>> Svade and Svadu extensions represent two schemes for managing the PTE A/D
>> bits. When the PTE A/D bits need to be set, Svade extension intdicates
>> that a related page fault will be raised. In contrast, the Svadu
>> extension
>> supports hardware updating of PTE A/D bits. Since the Svade extension is
>> mandatory and the Svadu extension is optional in RVA23 profile, by
>> default
>> the M-mode firmware will enable the Svadu extension in the menvcfg CSR
>> when only Svadu is present in DT.
>>
>> This patch detects Svade and Svadu extensions from DT and adds
>> arch_has_hw_pte_young() to enable optimization in MGLRU and
>> __wp_page_copy_user() when we have the PTE A/D bits hardware updating
>> support.
>>
>> Co-developed-by: Jinyu Tang <tjytimi@163.com>
>> Signed-off-by: Jinyu Tang <tjytimi@163.com>
>> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>> ---
>> ? arch/riscv/Kconfig?????????????? |? 1 +
>> ? arch/riscv/include/asm/csr.h???? |? 1 +
>> ? arch/riscv/include/asm/hwcap.h?? |? 2 ++
>> ? arch/riscv/include/asm/pgtable.h | 13 ++++++++++++-
>> ? arch/riscv/kernel/cpufeature.c?? | 32 ++++++++++++++++++++++++++++++++
>> ? 5 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index 0525ee2d63c7..3d705e28ff85 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -36,6 +36,7 @@ config RISCV
>> ????? select ARCH_HAS_PMEM_API
>> ????? select ARCH_HAS_PREPARE_SYNC_CORE_CMD
>> ????? select ARCH_HAS_PTE_SPECIAL
>> +??? select ARCH_HAS_HW_PTE_YOUNG
>> ????? select ARCH_HAS_SET_DIRECT_MAP if MMU
>> ????? select ARCH_HAS_SET_MEMORY if MMU
>> ????? select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
>> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
>> index 25966995da04..524cd4131c71 100644
>> --- a/arch/riscv/include/asm/csr.h
>> +++ b/arch/riscv/include/asm/csr.h
>> @@ -195,6 +195,7 @@
>> ? /* xENVCFG flags */
>> ? #define ENVCFG_STCE??????????? (_AC(1, ULL) << 63)
>> ? #define ENVCFG_PBMTE??????????? (_AC(1, ULL) << 62)
>> +#define ENVCFG_ADUE??????????? (_AC(1, ULL) << 61)
>> ? #define ENVCFG_CBZE??????????? (_AC(1, UL) << 7)
>> ? #define ENVCFG_CBCFE??????????? (_AC(1, UL) << 6)
>> ? #define ENVCFG_CBIE_SHIFT??????? 4
>> diff --git a/arch/riscv/include/asm/hwcap.h
>> b/arch/riscv/include/asm/hwcap.h
>> index e17d0078a651..35d7aa49785d 100644
>> --- a/arch/riscv/include/asm/hwcap.h
>> +++ b/arch/riscv/include/asm/hwcap.h
>> @@ -81,6 +81,8 @@
>> ? #define RISCV_ISA_EXT_ZTSO??????? 72
>> ? #define RISCV_ISA_EXT_ZACAS??????? 73
>> ? #define RISCV_ISA_EXT_XANDESPMU??????? 74
>> +#define RISCV_ISA_EXT_SVADE???????????? 75
>> +#define RISCV_ISA_EXT_SVADU??????? 76
>> ? ? #define RISCV_ISA_EXT_XLINUXENVCFG??? 127
>> ? diff --git a/arch/riscv/include/asm/pgtable.h
>> b/arch/riscv/include/asm/pgtable.h
>> index aad8b8ca51f1..ec0cdacd7da0 100644
>> --- a/arch/riscv/include/asm/pgtable.h
>> +++ b/arch/riscv/include/asm/pgtable.h
>> @@ -120,6 +120,7 @@
>> ? #include <asm/tlbflush.h>
>> ? #include <linux/mm_types.h>
>> ? #include <asm/compat.h>
>> +#include <asm/cpufeature.h>
>> ? ? #define __page_val_to_pfn(_val)? (((_val) & _PAGE_PFN_MASK) >>
>> _PAGE_PFN_SHIFT)
>> ? @@ -288,7 +289,6 @@ static inline pte_t pud_pte(pud_t pud)
>> ? }
>> ? ? #ifdef CONFIG_RISCV_ISA_SVNAPOT
>> -#include <asm/cpufeature.h>
>> ? ? static __always_inline bool has_svnapot(void)
>> ? {
>> @@ -624,6 +624,17 @@ static inline pgprot_t
>> pgprot_writecombine(pgprot_t _prot)
>> ????? return __pgprot(prot);
>> ? }
>> ? +/*
>> + * Both Svade and Svadu control the hardware behavior when the PTE
>> A/D bits need to be set. By
>> + * default the M-mode firmware enables the hardware updating scheme
>> when only Svadu is present in
>> + * DT.
>> + */
>> +#define arch_has_hw_pte_young arch_has_hw_pte_young
>> +static inline bool arch_has_hw_pte_young(void)
>> +{
>> +??? return riscv_has_extension_unlikely(RISCV_ISA_EXT_SVADU);
>> +}
>> +
>> ? /*
>> ?? * THP functions
>> ?? */
>> diff --git a/arch/riscv/kernel/cpufeature.c
>> b/arch/riscv/kernel/cpufeature.c
>> index 5ef48cb20ee1..b2c3fe945e89 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -301,6 +301,8 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>> ????? __RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
>> ????? __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>> ????? __RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
>> +??? __RISCV_ISA_EXT_DATA(svade, RISCV_ISA_EXT_SVADE),
>> +??? __RISCV_ISA_EXT_DATA(svadu, RISCV_ISA_EXT_SVADU),
>> ????? __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),
>> @@ -554,6 +556,21 @@ static void __init
>> riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>> ????????????? clear_bit(RISCV_ISA_EXT_v, isainfo->isa);
>> ????????? }
>> ? +??????? /*
>> +???????? * When neither Svade nor Svadu present in DT, it is technically
>> +???????? * unknown whether the platform uses Svade or Svadu.
>> Supervisor may
>> +???????? * assume Svade to be present and enabled or it can discover
>> based
>> +???????? * on mvendorid, marchid, and mimpid. When both Svade and
>> Svadu present
>> +???????? * in DT, supervisor must assume Svadu turned-off at boot
>> time. To use
>> +???????? * Svadu, supervisor must explicitly enable it using the SBI
>> FWFT extension.
>> +???????? */
>> +??????? if (!test_bit(RISCV_ISA_EXT_SVADE, isainfo->isa) &&
>> +??????????? !test_bit(RISCV_ISA_EXT_SVADU, isainfo->isa))
>> +??????????? set_bit(RISCV_ISA_EXT_SVADE, isainfo->isa);
>> +??????? else if (test_bit(RISCV_ISA_EXT_SVADE, isainfo->isa) &&
>> +???????????? test_bit(RISCV_ISA_EXT_SVADU, isainfo->isa))
>> +??????????? clear_bit(RISCV_ISA_EXT_SVADU, isainfo->isa);
>> +
>> ????????? /*
>> ?????????? * All "okay" hart should have same isa. Set HWCAP based on
>> ?????????? * common capabilities of every "okay" hart, in case they don't
>> @@ -619,6 +636,21 @@ static int __init
>> riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
>> ? ????????? of_node_put(cpu_node);
>> ? +??????? /*
>> +???????? * When neither Svade nor Svadu present in DT, it is technically
>> +???????? * unknown whether the platform uses Svade or Svadu.
>> Supervisor may
>> +???????? * assume Svade to be present and enabled or it can discover
>> based
>> +???????? * on mvendorid, marchid, and mimpid. When both Svade and
>> Svadu present
>> +???????? * in DT, supervisor must assume Svadu turned-off at boot
>> time. To use
>> +???????? * Svadu, supervisor must explicitly enable it using the SBI
>> FWFT extension.
>> +???????? */
>> +??????? if (!test_bit(RISCV_ISA_EXT_SVADE, isainfo->isa) &&
>> +??????????? !test_bit(RISCV_ISA_EXT_SVADU, isainfo->isa))
>> +??????????? set_bit(RISCV_ISA_EXT_SVADE, isainfo->isa);
>> +??????? else if (test_bit(RISCV_ISA_EXT_SVADE, isainfo->isa) &&
>> +???????????? test_bit(RISCV_ISA_EXT_SVADU, isainfo->isa))
>> +??????????? clear_bit(RISCV_ISA_EXT_SVADU, isainfo->isa);
>> +

This is a duplicate of the previous chunk of code. Moreover, now that we
have a .validate callback for ISA extension (in for-next), I would
prefer this to be based on that support rather that having duplicated
extension specific handling code.

I think this could be translated (almost) using the following
.validate() callback for SVADU/SVADE extension:

static int riscv_ext_svadu_validate(const struct riscv_isa_ext_data *data,
				  const unsigned long *isa_bitmap)
{
	/* SVADE has already been detected, use SVADE only */
	if (__riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_SVADE))
		return -ENOTSUPP;

	return 0;
}

static int riscv_ext_svade_validate(const struct riscv_isa_ext_data *data,
				  const unsigned long *isa_bitmap)
{
	/* Clear SVADU, it will be enable using the FWFT extension if present */
	clear_bit(RISCV_ISA_EXT_SVADU, isa_bitmap);

	return 0;
}

However, this will not enable SVADE if neither SVADU/SVADE are set (as
done by your patch) but since SVADE does not seems to be used explicitly
in your patch series, I think it is sane to keep it like that.

Thanks,

Cl?ment



>> ????????? /*
>> ?????????? * All "okay" harts should have same isa. Set HWCAP based on
>> ?????????? * common capabilities of every "okay" hart, in case they
>> don't.
> 
> 
> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> 
> Thanks,
> 
> Alex
>