lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMWQL2j++-11jQjFweoZWWxRDjqUYT9CR-motNUrTVibpSxFOQ@mail.gmail.com>
Date: Mon, 22 Jul 2024 10:15:39 +0800
From: Yong-Xuan Wang <yongxuan.wang@...ive.com>
To: Clément Léger <cleger@...osinc.com>
Cc: Alexandre Ghiti <alex@...ti.fr>, linux-kernel@...r.kernel.org, 
	linux-riscv@...ts.infradead.org, kvm-riscv@...ts.infradead.org, 
	kvm@...r.kernel.org, greentime.hu@...ive.com, vincent.chen@...ive.com, 
	Jinyu Tang <tjytimi@....com>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, 
	Andrew Jones <ajones@...tanamicro.com>, Anup Patel <anup@...infault.org>, 
	Conor Dooley <conor.dooley@...rochip.com>, Mayuresh Chitale <mchitale@...tanamicro.com>, 
	Atish Patra <atishp@...osinc.com>, Samuel Ortiz <sameo@...osinc.com>, 
	Daniel Henrique Barboza <dbarboza@...tanamicro.com>, Samuel Holland <samuel.holland@...ive.com>, 
	Evan Green <evan@...osinc.com>, Xiao Wang <xiao.w.wang@...el.com>, 
	Alexandre Ghiti <alexghiti@...osinc.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	Kemeng Shi <shikemeng@...weicloud.com>, "Mike Rapoport (IBM)" <rppt@...nel.org>, 
	Leonardo Bras <leobras@...hat.com>, Charlie Jenkins <charlie@...osinc.com>, 
	"Matthew Wilcox (Oracle)" <willy@...radead.org>, Jisheng Zhang <jszhang@...nel.org>
Subject: Re: [PATCH v7 1/4] RISC-V: Add Svade and Svadu Extensions Support

Hi Clément,

On Fri, Jul 19, 2024 at 3:38 PM Clément Léger <cleger@...osinc.com> wrote:
>
>
>
> 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@....com>
> >> Signed-off-by: Jinyu Tang <tjytimi@....com>
> >> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@...ive.com>
> >> Reviewed-by: Andrew Jones <ajones@...tanamicro.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
>
>

Got it. I will rebase to the for-next branch. Thank you!

Regards,
Yong-Xuan

>
> >>           /*
> >>            * 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@...osinc.com>
> >
> > Thanks,
> >
> > Alex
> >

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ