Re: [PATCH 7/7] target/riscv/kvm: reset 'scounteren' with host val
On 4/23/25 12:46 PM, Andrew Jones wrote:
On Thu, Apr 17, 2025 at 09:48:39AM -0300, Daniel Henrique Barboza wrote:
The Linux kernel, up until at least version 6.12, has issues with a KVM
guest setting scounteren to 0 during reset. No error will be thrown
during boot, but trying to use rdtime in the guest (by executing 'ping'
for example) will result in a stack trace and an illegal instruction
error:
/ # ping 8.8.8.8
PING 3.33.130.190 (8.8.8.8): 56 data bytes
[ 19.464484] ping[56]: unhandled signal 4 code 0x1 at 0x7fffae0665f4
[ 19.493332] CPU: 0 PID: 56 Comm: ping Not tainted 6.9.0-rc3-dbarboza #7
[ 19.523249] Hardware name: riscv-virtio,qemu (DT)
[ 19.546641] epc : 7fffae0665f4 ra : 000c6316 sp :
7fffc7cfd9f0
[ 19.576214] gp : 00172408 tp : 001767a0 t0 :
[ 19.606719] t1 : 0020 t2 : s0 :
7fffc7cfda00
[ 19.640938] s1 : 7fffc7cfda30 a0 : 0001 a1 :
7fffc7cfda30
[ 19.676698] a2 : a3 : 09ee a4 :
7fffae064000
[ 19.721036] a5 : 0001 a6 : a7 :
001784d0
[ 19.765061] s2 : 001784d0 s3 : 0011ca38 s4 :
0011d000
[ 19.801985] s5 : 0001 s6 : s7 :
[ 19.841235] s8 : 00177788 s9 : 00176828 s10:
[ 19.882479] s11: 001777a8 t3 : t4 :
00172f60
[ 19.923651] t5 : 0020 t6 : 000a
[ 19.950491] status: 00024020 badaddr: c01027f3 cause:
0002
[ 19.995864] Code: 431c f613 0017 869b 0007 ea59 000f 0220 435c cfbd (27f3)
c010
Illegal instruction
/ #
Reading the host scounteren val and using it during reset, instead of
zeroing it, will prevent this error. It is worth noting that scounteren
is a WARL reg according to the RISC-V ISA spec, and in theory the kernel
should accept a zero val and convert it to something that won't break
the guest.
0 is legal, so the kernel (KVM) shouldn't change what userspace selects.
Userspace, which includes user policy knowledge, knows best and indeed 0
is the best selection when no other policy is provided. Changing from 0
to whatever KVM has put there is wrong.
We can't tell from userspace if the host kernel handles scounteren = 0
during reset accordingly, so to prevent this error we'll assume that it
won't. Read the reg from the host and use it as reset value.
It's not the host kernel that needs to change how it handles things. It's
the guest kernel. When the guest uses ping, which likely calls gtod, which
uses rdtime, or if the guest uses anything that results in an rdtime use,
then it'll hit this issue if the host doesn't support sscofpmf (which the
QEMU rv64 cpu type doesn't). The reason is because KVM won't expose the
SBI PMU without sscofpmf and then the guest kernel pmu driver won't run,
and currently the only place scounteren is getting set up is in the pmu
driver. The guest kernel should unconditionally set up scounteren to
match what it expects userspace to use (like enable rdtime, since the
guest kernel actually implements gtod in vdso with it)
We'll end up repeating code from kvm_riscv_get_regs_csr() to do that.
Create a helper that will get a single CSR and use it in get_regs_csr()
and in kvm_riscv_reset_regs_csr() to avoid code duplication.
Fixes: 4db19d5b21 ("target/riscv/kvm: add missing KVM CSRs")
This isn't the right tag since that is already fixed by checking
get-reg-list in a previous patch. This patch is trying to fix a guest
kernel bug, which it shouldn't be doing, at least not without some user
toggle allowing the workaround to be turned on/off.
Signed-off-by: Daniel Henrique Barboza
---
target/riscv/kvm/kvm-cpu.c | 73 --
1 file changed, 55 insertions(+), 18 deletions(-)
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index a91a87b175..918fe51257 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -634,29 +634,40 @@ static int kvm_riscv_put_regs_core(CPUState *cs)
return ret;
}
-static int kvm_riscv_get_regs_csr(CPUState *cs)
+static int kvm_riscv_get_reg_csr(CPUState *cs,
+ KVMCPUConfig *csr_cfg)
{
RISCVCPU *cpu = RISCV_CPU(cs);
uint64_t reg;
-int i, ret;
+int ret;
-for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
-KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
+if (!csr_cfg->supported) {
+return 0;
+}
-if (!csr_cfg->supported) {
-continue;
-}
+ret = kvm_get_one_reg(cs, csr_cfg->kvm_reg_id, ®);
+if (ret) {
+return ret;
+}
+
+if (csr_cfg->prop_size == sizeof(uint32_t)) {
+kvm_cpu_csr_set_u32(cpu, csr_cfg, reg);
+} else {
+kvm_cpu_csr_set_u64(cpu, csr_cfg, reg);
+}
+
+return 0;
+}
+
+static int
Re: [PATCH 7/7] target/riscv/kvm: reset 'scounteren' with host val
On 4/23/25 3:06 PM, Andrea Bolognani wrote: On Wed, Apr 23, 2025 at 05:46:16PM +0200, Andrew Jones wrote: I would just drop this patch and make the default 'virt' cpu type 'max', then nobody will hit the issue. FWIW virt-manager has recently started doing just that: https://github.com/virt-manager/virt-manager/pull/784 We have a patch in the QEMU ML that does that: "[PATCH 2/2] hw/riscv/virt.c: change default CPU to 'max'" https://lore.kernel.org/qemu-riscv/[email protected]/ Any updates in that patch is appreciated, in particular if it comes from the tooling side. Thanks, Daniel
Re: [PATCH 7/7] target/riscv/kvm: reset 'scounteren' with host val
On Wed, Apr 23, 2025 at 05:46:16PM +0200, Andrew Jones wrote: > I would just drop this patch and make the default 'virt' cpu type 'max', > then nobody will hit the issue. FWIW virt-manager has recently started doing just that: https://github.com/virt-manager/virt-manager/pull/784 -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 7/7] target/riscv/kvm: reset 'scounteren' with host val
On Thu, Apr 17, 2025 at 09:48:39AM -0300, Daniel Henrique Barboza wrote:
> The Linux kernel, up until at least version 6.12, has issues with a KVM
> guest setting scounteren to 0 during reset. No error will be thrown
> during boot, but trying to use rdtime in the guest (by executing 'ping'
> for example) will result in a stack trace and an illegal instruction
> error:
>
> / # ping 8.8.8.8
> PING 3.33.130.190 (8.8.8.8): 56 data bytes
> [ 19.464484] ping[56]: unhandled signal 4 code 0x1 at 0x7fffae0665f4
> [ 19.493332] CPU: 0 PID: 56 Comm: ping Not tainted 6.9.0-rc3-dbarboza #7
> [ 19.523249] Hardware name: riscv-virtio,qemu (DT)
> [ 19.546641] epc : 7fffae0665f4 ra : 000c6316 sp :
> 7fffc7cfd9f0
> [ 19.576214] gp : 00172408 tp : 001767a0 t0 :
>
> [ 19.606719] t1 : 0020 t2 : s0 :
> 7fffc7cfda00
> [ 19.640938] s1 : 7fffc7cfda30 a0 : 0001 a1 :
> 7fffc7cfda30
> [ 19.676698] a2 : a3 : 09ee a4 :
> 7fffae064000
> [ 19.721036] a5 : 0001 a6 : a7 :
> 001784d0
> [ 19.765061] s2 : 001784d0 s3 : 0011ca38 s4 :
> 0011d000
> [ 19.801985] s5 : 0001 s6 : s7 :
>
> [ 19.841235] s8 : 00177788 s9 : 00176828 s10:
>
> [ 19.882479] s11: 001777a8 t3 : t4 :
> 00172f60
> [ 19.923651] t5 : 0020 t6 : 000a
> [ 19.950491] status: 00024020 badaddr: c01027f3 cause:
> 0002
> [ 19.995864] Code: 431c f613 0017 869b 0007 ea59 000f 0220 435c cfbd (27f3)
> c010
> Illegal instruction
> / #
>
> Reading the host scounteren val and using it during reset, instead of
> zeroing it, will prevent this error. It is worth noting that scounteren
> is a WARL reg according to the RISC-V ISA spec, and in theory the kernel
> should accept a zero val and convert it to something that won't break
> the guest.
0 is legal, so the kernel (KVM) shouldn't change what userspace selects.
Userspace, which includes user policy knowledge, knows best and indeed 0
is the best selection when no other policy is provided. Changing from 0
to whatever KVM has put there is wrong.
>
> We can't tell from userspace if the host kernel handles scounteren = 0
> during reset accordingly, so to prevent this error we'll assume that it
> won't. Read the reg from the host and use it as reset value.
It's not the host kernel that needs to change how it handles things. It's
the guest kernel. When the guest uses ping, which likely calls gtod, which
uses rdtime, or if the guest uses anything that results in an rdtime use,
then it'll hit this issue if the host doesn't support sscofpmf (which the
QEMU rv64 cpu type doesn't). The reason is because KVM won't expose the
SBI PMU without sscofpmf and then the guest kernel pmu driver won't run,
and currently the only place scounteren is getting set up is in the pmu
driver. The guest kernel should unconditionally set up scounteren to
match what it expects userspace to use (like enable rdtime, since the
guest kernel actually implements gtod in vdso with it)
>
> We'll end up repeating code from kvm_riscv_get_regs_csr() to do that.
> Create a helper that will get a single CSR and use it in get_regs_csr()
> and in kvm_riscv_reset_regs_csr() to avoid code duplication.
>
> Fixes: 4db19d5b21 ("target/riscv/kvm: add missing KVM CSRs")
This isn't the right tag since that is already fixed by checking
get-reg-list in a previous patch. This patch is trying to fix a guest
kernel bug, which it shouldn't be doing, at least not without some user
toggle allowing the workaround to be turned on/off.
> Signed-off-by: Daniel Henrique Barboza
> ---
> target/riscv/kvm/kvm-cpu.c | 73 --
> 1 file changed, 55 insertions(+), 18 deletions(-)
>
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index a91a87b175..918fe51257 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -634,29 +634,40 @@ static int kvm_riscv_put_regs_core(CPUState *cs)
> return ret;
> }
>
> -static int kvm_riscv_get_regs_csr(CPUState *cs)
> +static int kvm_riscv_get_reg_csr(CPUState *cs,
> + KVMCPUConfig *csr_cfg)
> {
> RISCVCPU *cpu = RISCV_CPU(cs);
> uint64_t reg;
> -int i, ret;
> +int ret;
>
> -for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
> -KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
> +if (!csr_cfg->supported) {
> +return 0;
> +}
>
> -if (!csr_cfg->supported) {
> -continue;
> -}
> +ret = kvm_get_one_reg(cs, csr_cfg->kvm_reg_id, ®);
> +if (ret) {
> +return ret;
> +}
> +
> +if (csr_cfg->prop_size == sizeof(uint32_t)) {
> +kvm_cpu_csr_set_u32(cpu,
