linux-kernel - Re: [PATCH V4 RESEND 3/3] RISC-V: KVM: Prevent HGATP_MODE

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: <CAAhSdy2yFQYbrp8npzBUwtviJYVQ=vv1F_k3jybYBvheYMgaZQ@mail.gmail.com>
Date: Fri, 5 Sep 2025 12:21:20 +0530
From: Anup Patel <anup@...infault.org>
To: guoren@...nel.org
Cc: troy.mitchell@...ux.dev, alex@...ti.fr, aou@...s.berkeley.edu, 
	atish.patra@...ux.dev, fangyu.yu@...ux.alibaba.com, guoren@...ux.alibaba.com, 
	kvm-riscv@...ts.infradead.org, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	palmer@...belt.com, paul.walmsley@...ive.com, 
	Nutty Liu <nutty.liu@...mail.com>
Subject: Re: [PATCH V4 RESEND 3/3] RISC-V: KVM: Prevent HGATP_MODE_BARE passed

On Thu, Aug 21, 2025 at 7:56 PM <guoren@...nel.org> wrote:
>
> From: "Guo Ren (Alibaba DAMO Academy)" <guoren@...nel.org>
>
> urrent kvm_riscv_gstage_mode_detect() assumes H-extension must

s/urrent/Current/

> have HGATP_MODE_SV39X4/SV32X4 at least, but the spec allows
> H-extension with HGATP_MODE_BARE alone. The KVM depends on
> !HGATP_MODE_BARE at least, so enhance the gstage-mode-detect
> to block HGATP_MODE_BARE.
>
> Move gstage-mode-check closer to gstage-mode-detect to prevent
> unnecessary init.
>
> Reviewed-by: Troy Mitchell <troy.mitchell@...ux.dev>
> Reviewed-by: Nutty Liu <nutty.liu@...mail.com>
> Signed-off-by: Guo Ren (Alibaba DAMO Academy) <guoren@...nel.org>
> ---
>  arch/riscv/kvm/gstage.c | 27 ++++++++++++++++++++++++---
>  arch/riscv/kvm/main.c   | 35 +++++++++++++++++------------------
>  2 files changed, 41 insertions(+), 21 deletions(-)
>
> diff --git a/arch/riscv/kvm/gstage.c b/arch/riscv/kvm/gstage.c
> index 24c270d6d0e2..b67d60d722c2 100644
> --- a/arch/riscv/kvm/gstage.c
> +++ b/arch/riscv/kvm/gstage.c
> @@ -321,7 +321,7 @@ void __init kvm_riscv_gstage_mode_detect(void)
>         if ((csr_read(CSR_HGATP) >> HGATP_MODE_SHIFT) == HGATP_MODE_SV57X4) {
>                 kvm_riscv_gstage_mode = HGATP_MODE_SV57X4;
>                 kvm_riscv_gstage_pgd_levels = 5;
> -               goto skip_sv48x4_test;
> +               goto done;
>         }
>
>         /* Try Sv48x4 G-stage mode */
> @@ -329,10 +329,31 @@ void __init kvm_riscv_gstage_mode_detect(void)
>         if ((csr_read(CSR_HGATP) >> HGATP_MODE_SHIFT) == HGATP_MODE_SV48X4) {
>                 kvm_riscv_gstage_mode = HGATP_MODE_SV48X4;
>                 kvm_riscv_gstage_pgd_levels = 4;
> +               goto done;
>         }
> -skip_sv48x4_test:
>
> +       /* Try Sv39x4 G-stage mode */
> +       csr_write(CSR_HGATP, HGATP_MODE_SV39X4 << HGATP_MODE_SHIFT);
> +       if ((csr_read(CSR_HGATP) >> HGATP_MODE_SHIFT) == HGATP_MODE_SV39X4) {
> +               kvm_riscv_gstage_mode = HGATP_MODE_SV39X4;
> +               kvm_riscv_gstage_pgd_levels = 3;
> +               goto done;
> +       }
> +#else /* CONFIG_32BIT */
> +       /* Try Sv32x4 G-stage mode */
> +       csr_write(CSR_HGATP, HGATP_MODE_SV32X4 << HGATP_MODE_SHIFT);
> +       if ((csr_read(CSR_HGATP) >> HGATP_MODE_SHIFT) == HGATP_MODE_SV32X4) {
> +               kvm_riscv_gstage_mode = HGATP_MODE_SV32X4;
> +               kvm_riscv_gstage_pgd_levels = 2;
> +               goto done;
> +       }
> +#endif
> +
> +       /* KVM depends on !HGATP_MODE_OFF */
> +       kvm_riscv_gstage_mode = HGATP_MODE_OFF;
> +       kvm_riscv_gstage_pgd_levels = 0;
> +
> +done:
>         csr_write(CSR_HGATP, 0);
>         kvm_riscv_local_hfence_gvma_all();
> -#endif
>  }
> diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
> index 67c876de74ef..8ee7aaa74ddc 100644
> --- a/arch/riscv/kvm/main.c
> +++ b/arch/riscv/kvm/main.c
> @@ -93,6 +93,23 @@ static int __init riscv_kvm_init(void)
>                 return rc;
>
>         kvm_riscv_gstage_mode_detect();
> +       switch (kvm_riscv_gstage_mode) {
> +       case HGATP_MODE_SV32X4:
> +               str = "Sv32x4";
> +               break;
> +       case HGATP_MODE_SV39X4:
> +               str = "Sv39x4";
> +               break;
> +       case HGATP_MODE_SV48X4:
> +               str = "Sv48x4";
> +               break;
> +       case HGATP_MODE_SV57X4:
> +               str = "Sv57x4";
> +               break;
> +       default:

Need kvm_riscv_nacl_exit() here.

> +               return -ENODEV;
> +       }
> +       kvm_info("using %s G-stage page table format\n", str);

Moving the kvm_info() over here now prints G-stage mode
before announcing availablity of h-extension which looks odd.
It's better to keep kvm_info() in the same location and only
move the switch-case.

>
>         kvm_riscv_gstage_vmid_detect();
>
> @@ -135,24 +152,6 @@ static int __init riscv_kvm_init(void)
>                          (rc) ? slist : "no features");
>         }
>
> -       switch (kvm_riscv_gstage_mode) {
> -       case HGATP_MODE_SV32X4:
> -               str = "Sv32x4";
> -               break;
> -       case HGATP_MODE_SV39X4:
> -               str = "Sv39x4";
> -               break;
> -       case HGATP_MODE_SV48X4:
> -               str = "Sv48x4";
> -               break;
> -       case HGATP_MODE_SV57X4:
> -               str = "Sv57x4";
> -               break;
> -       default:
> -               return -ENODEV;
> -       }
> -       kvm_info("using %s G-stage page table format\n", str);
> -
>         kvm_info("VMID %ld bits available\n", kvm_riscv_gstage_vmid_bits());
>
>         if (kvm_riscv_aia_available())
> --
> 2.40.1
>

Otherwise, this looks good to me.

I will take care of minor comments mentioned above at the
time of merging this series.

Regards,
Anup