[PATCH v3 0/3] target/riscv/kvm: reset time changes

[PATCH v3 0/3] target/riscv/kvm: reset time changes

[Daniel Henrique Barboza]

Hi,

In this version I rolled back on the riscv_cpu_reset_hold() changes made
in patch 1. Peter made an argument about keeping the design the same
across architectures and I agreed. Patches 2 and 3 are already taking
care of everything that KVM needs during reset, thus this doesn't incur
in any functional changes.

Drew said it was ok to keep his ack on patch 1 so his ack is kept.

Patches based on alistair/riscv-to-apply.next. All patches acked.

Changes from v2:
- patch 1:
  - do not do an early exit in riscv_cpu_reset_hold() if kvm_enabled()
- v2 link: https://lore.kernel.org/qemu-riscv/20250221122623.495188-1-dbarboza@ventanamicro.com/


Daniel Henrique Barboza (3):
  target/riscv/cpu: remove unneeded !kvm_enabled() check
  target/riscv/kvm: add kvm_riscv_reset_regs_csr()
  target/riscv/kvm: add missing KVM CSRs

 target/riscv/kvm/kvm-cpu.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

-- 
2.48.1

[PATCH v3 1/3] target/riscv/cpu: remove unneeded !kvm_enabled() check

[Daniel Henrique Barboza]

Remove the !kvm_enabled() check in kvm_riscv_reset_vcpu() since the
function is already being gated by kvm_enabled() in
riscv_cpu_reset_hold().

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 471fd554b3..19bb87515b 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1606,9 +1606,6 @@ void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
     CPURISCVState *env = &cpu->env;
     int i;
 
-    if (!kvm_enabled()) {
-        return;
-    }
     for (i = 0; i < 32; i++) {
         env->gpr[i] = 0;
     }
-- 
2.48.1

Re: [PATCH v3 1/3] target/riscv/cpu: remove unneeded !kvm_enabled() check

[Alistair Francis]

On Mon, Feb 24, 2025 at 10:33 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Remove the !kvm_enabled() check in kvm_riscv_reset_vcpu() since the
> function is already being gated by kvm_enabled() in
> riscv_cpu_reset_hold().
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/kvm/kvm-cpu.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 471fd554b3..19bb87515b 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1606,9 +1606,6 @@ void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
>      CPURISCVState *env = &cpu->env;
>      int i;
>
> -    if (!kvm_enabled()) {
> -        return;
> -    }
>      for (i = 0; i < 32; i++) {
>          env->gpr[i] = 0;
>      }
> --
> 2.48.1
>
>

[PATCH v3 2/3] target/riscv/kvm: add kvm_riscv_reset_regs_csr()

[Daniel Henrique Barboza]

We're setting reset vals for KVM csrs during kvm_riscv_reset_vcpu(), but
in no particular order and missing some of them (like env->mstatus).

Create a helper to do that, unclogging reset_vcpu(), and initialize
env->mstatus as well. Keep the regs in the same order they appear in
struct kvm_riscv_csr from the KVM UAPI, similar to what
kvm_riscv_(get|put)_regs_csr are doing. This will make a bit easier to
add new KVM CSRs and to verify which values we're writing back to KVM
during vcpu reset.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 19bb87515b..cabc34b6a2 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -608,6 +608,19 @@ static int kvm_riscv_put_regs_core(CPUState *cs)
     return ret;
 }
 
+static void kvm_riscv_reset_regs_csr(CPURISCVState *env)
+{
+    env->mstatus = 0;
+    env->mie = 0;
+    env->stvec = 0;
+    env->sscratch = 0;
+    env->sepc = 0;
+    env->scause = 0;
+    env->stval = 0;
+    env->mip = 0;
+    env->satp = 0;
+}
+
 static int kvm_riscv_get_regs_csr(CPUState *cs)
 {
     CPURISCVState *env = &RISCV_CPU(cs)->env;
@@ -1612,14 +1625,8 @@ void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
     env->pc = cpu->env.kernel_addr;
     env->gpr[10] = kvm_arch_vcpu_id(CPU(cpu)); /* a0 */
     env->gpr[11] = cpu->env.fdt_addr;          /* a1 */
-    env->satp = 0;
-    env->mie = 0;
-    env->stvec = 0;
-    env->sscratch = 0;
-    env->sepc = 0;
-    env->scause = 0;
-    env->stval = 0;
-    env->mip = 0;
+
+    kvm_riscv_reset_regs_csr(env);
 }
 
 void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level)
-- 
2.48.1

Re: [PATCH v3 2/3] target/riscv/kvm: add kvm_riscv_reset_regs_csr()

[Alistair Francis]

On Mon, Feb 24, 2025 at 10:32 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We're setting reset vals for KVM csrs during kvm_riscv_reset_vcpu(), but
> in no particular order and missing some of them (like env->mstatus).
>
> Create a helper to do that, unclogging reset_vcpu(), and initialize
> env->mstatus as well. Keep the regs in the same order they appear in
> struct kvm_riscv_csr from the KVM UAPI, similar to what
> kvm_riscv_(get|put)_regs_csr are doing. This will make a bit easier to
> add new KVM CSRs and to verify which values we're writing back to KVM
> during vcpu reset.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/kvm/kvm-cpu.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 19bb87515b..cabc34b6a2 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -608,6 +608,19 @@ static int kvm_riscv_put_regs_core(CPUState *cs)
>      return ret;
>  }
>
> +static void kvm_riscv_reset_regs_csr(CPURISCVState *env)
> +{
> +    env->mstatus = 0;
> +    env->mie = 0;
> +    env->stvec = 0;
> +    env->sscratch = 0;
> +    env->sepc = 0;
> +    env->scause = 0;
> +    env->stval = 0;
> +    env->mip = 0;
> +    env->satp = 0;
> +}
> +
>  static int kvm_riscv_get_regs_csr(CPUState *cs)
>  {
>      CPURISCVState *env = &RISCV_CPU(cs)->env;
> @@ -1612,14 +1625,8 @@ void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
>      env->pc = cpu->env.kernel_addr;
>      env->gpr[10] = kvm_arch_vcpu_id(CPU(cpu)); /* a0 */
>      env->gpr[11] = cpu->env.fdt_addr;          /* a1 */
> -    env->satp = 0;
> -    env->mie = 0;
> -    env->stvec = 0;
> -    env->sscratch = 0;
> -    env->sepc = 0;
> -    env->scause = 0;
> -    env->stval = 0;
> -    env->mip = 0;
> +
> +    kvm_riscv_reset_regs_csr(env);
>  }
>
>  void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level)
> --
> 2.48.1
>
>

[PATCH v3 3/3] target/riscv/kvm: add missing KVM CSRs

[Daniel Henrique Barboza]

We're missing scounteren and senvcfg CSRs, both already present in the
KVM UAPI.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index cabc34b6a2..e8c31a32d4 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -619,6 +619,8 @@ static void kvm_riscv_reset_regs_csr(CPURISCVState *env)
     env->stval = 0;
     env->mip = 0;
     env->satp = 0;
+    env->scounteren = 0;
+    env->senvcfg = 0;
 }
 
 static int kvm_riscv_get_regs_csr(CPUState *cs)
@@ -634,6 +636,8 @@ static int kvm_riscv_get_regs_csr(CPUState *cs)
     KVM_RISCV_GET_CSR(cs, env, stval, env->stval);
     KVM_RISCV_GET_CSR(cs, env, sip, env->mip);
     KVM_RISCV_GET_CSR(cs, env, satp, env->satp);
+    KVM_RISCV_GET_CSR(cs, env, scounteren, env->scounteren);
+    KVM_RISCV_GET_CSR(cs, env, senvcfg, env->senvcfg);
 
     return 0;
 }
@@ -651,6 +655,8 @@ static int kvm_riscv_put_regs_csr(CPUState *cs)
     KVM_RISCV_SET_CSR(cs, env, stval, env->stval);
     KVM_RISCV_SET_CSR(cs, env, sip, env->mip);
     KVM_RISCV_SET_CSR(cs, env, satp, env->satp);
+    KVM_RISCV_SET_CSR(cs, env, scounteren, env->scounteren);
+    KVM_RISCV_SET_CSR(cs, env, senvcfg, env->senvcfg);
 
     return 0;
 }
-- 
2.48.1

Re: [PATCH v3 3/3] target/riscv/kvm: add missing KVM CSRs

[Alistair Francis]

On Mon, Feb 24, 2025 at 10:32 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We're missing scounteren and senvcfg CSRs, both already present in the
> KVM UAPI.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/kvm/kvm-cpu.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index cabc34b6a2..e8c31a32d4 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -619,6 +619,8 @@ static void kvm_riscv_reset_regs_csr(CPURISCVState *env)
>      env->stval = 0;
>      env->mip = 0;
>      env->satp = 0;
> +    env->scounteren = 0;
> +    env->senvcfg = 0;
>  }
>
>  static int kvm_riscv_get_regs_csr(CPUState *cs)
> @@ -634,6 +636,8 @@ static int kvm_riscv_get_regs_csr(CPUState *cs)
>      KVM_RISCV_GET_CSR(cs, env, stval, env->stval);
>      KVM_RISCV_GET_CSR(cs, env, sip, env->mip);
>      KVM_RISCV_GET_CSR(cs, env, satp, env->satp);
> +    KVM_RISCV_GET_CSR(cs, env, scounteren, env->scounteren);
> +    KVM_RISCV_GET_CSR(cs, env, senvcfg, env->senvcfg);
>
>      return 0;
>  }
> @@ -651,6 +655,8 @@ static int kvm_riscv_put_regs_csr(CPUState *cs)
>      KVM_RISCV_SET_CSR(cs, env, stval, env->stval);
>      KVM_RISCV_SET_CSR(cs, env, sip, env->mip);
>      KVM_RISCV_SET_CSR(cs, env, satp, env->satp);
> +    KVM_RISCV_SET_CSR(cs, env, scounteren, env->scounteren);
> +    KVM_RISCV_SET_CSR(cs, env, senvcfg, env->senvcfg);
>
>      return 0;
>  }
> --
> 2.48.1
>
>

Re: [PATCH v3 3/3] target/riscv/kvm: add missing KVM CSRs

[Andrea Bolognani]

On Mon, Mar 03, 2025 at 01:46:53PM +1000, Alistair Francis wrote:
> On Mon, Feb 24, 2025 at 10:32 PM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> > We're missing scounteren and senvcfg CSRs, both already present in the
> > KVM UAPI.
> >
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>

This patch seems to have broken KVM acceleration for me:

  $ ./build/qemu-system-riscv64 -display none -M virt,accel=kvm -cpu host
  qemu-system-riscv64: Failed to put registers after init: No such
file or directory

Reverting it makes QEMU work again.

My host is a SiFive HiFive Premier P550 board running Fedora 41. Note
that, since the upstreaming effort for this SoC has just recently
started, I'm using the 6.6-based vendor kernel.

Perhaps the KVM UAPI additions mentioned in the commit message are
more recent than that, and we need to make QEMU's use of them
conditional rather than unconditional?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v3 3/3] target/riscv/kvm: add missing KVM CSRs

[Daniel Henrique Barboza]

On 3/20/25 11:25 AM, Andrea Bolognani wrote:
> On Mon, Mar 03, 2025 at 01:46:53PM +1000, Alistair Francis wrote:
>> On Mon, Feb 24, 2025 at 10:32 PM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
>>> We're missing scounteren and senvcfg CSRs, both already present in the
>>> KVM UAPI.
>>>
>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>>
>> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> 
> This patch seems to have broken KVM acceleration for me:
> 
>    $ ./build/qemu-system-riscv64 -display none -M virt,accel=kvm -cpu host
>    qemu-system-riscv64: Failed to put registers after init: No such
> file or directory
> 
> Reverting it makes QEMU work again.
> 
> My host is a SiFive HiFive Premier P550 board running Fedora 41. Note
> that, since the upstreaming effort for this SoC has just recently
> started, I'm using the 6.6-based vendor kernel.
> 
> Perhaps the KVM UAPI additions mentioned in the commit message are
> more recent than that, and we need to make QEMU's use of them
> conditional rather than unconditional?

Yes, we can't assume that CSRs will be always present.

I'll work on a fix. Thanks for reporting it!


Daniel

> 

Re: [PATCH v3 3/3] target/riscv/kvm: add missing KVM CSRs

[Andrew Jones]

On Thu, Mar 20, 2025 at 07:25:07AM -0700, Andrea Bolognani wrote:
> On Mon, Mar 03, 2025 at 01:46:53PM +1000, Alistair Francis wrote:
> > On Mon, Feb 24, 2025 at 10:32 PM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> > > We're missing scounteren and senvcfg CSRs, both already present in the
> > > KVM UAPI.
> > >
> > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> >
> > Acked-by: Alistair Francis <alistair.francis@wdc.com>
> 
> This patch seems to have broken KVM acceleration for me:
> 
>   $ ./build/qemu-system-riscv64 -display none -M virt,accel=kvm -cpu host
>   qemu-system-riscv64: Failed to put registers after init: No such
> file or directory
> 
> Reverting it makes QEMU work again.
> 
> My host is a SiFive HiFive Premier P550 board running Fedora 41. Note
> that, since the upstreaming effort for this SoC has just recently
> started, I'm using the 6.6-based vendor kernel.

Ancient :-)

> 
> Perhaps the KVM UAPI additions mentioned in the commit message are
> more recent than that, and we need to make QEMU's use of them
> conditional rather than unconditional?

scounteren has been around since the dawn of riscv kvm, but senvcfg has
only been there since 6.7 (just missed your ancient cut-off).

The true fix for this is to start using get-reg-list, which should
hopefully work with the 6.6 kernel too since get-reg-list support has
been around since 6.6.

A quick fix for this is to just drop senvcfg for now since nobody
noticed it was missing before (well, I noticed it was missing, but by
inspection, not test).

Thanks,
drew

Re: [PATCH v3 3/3] target/riscv/kvm: add missing KVM CSRs

[Daniel Henrique Barboza]

On 3/20/25 11:41 AM, Andrew Jones wrote:
> On Thu, Mar 20, 2025 at 07:25:07AM -0700, Andrea Bolognani wrote:
>> On Mon, Mar 03, 2025 at 01:46:53PM +1000, Alistair Francis wrote:
>>> On Mon, Feb 24, 2025 at 10:32 PM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
>>>> We're missing scounteren and senvcfg CSRs, both already present in the
>>>> KVM UAPI.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>>>
>>> Acked-by: Alistair Francis <alistair.francis@wdc.com>
>>
>> This patch seems to have broken KVM acceleration for me:
>>
>>    $ ./build/qemu-system-riscv64 -display none -M virt,accel=kvm -cpu host
>>    qemu-system-riscv64: Failed to put registers after init: No such
>> file or directory
>>
>> Reverting it makes QEMU work again.
>>
>> My host is a SiFive HiFive Premier P550 board running Fedora 41. Note
>> that, since the upstreaming effort for this SoC has just recently
>> started, I'm using the 6.6-based vendor kernel.
> 
> Ancient :-)
> 
>>
>> Perhaps the KVM UAPI additions mentioned in the commit message are
>> more recent than that, and we need to make QEMU's use of them
>> conditional rather than unconditional?
> 
> scounteren has been around since the dawn of riscv kvm, but senvcfg has
> only been there since 6.7 (just missed your ancient cut-off).
> 
> The true fix for this is to start using get-reg-list, which should
> hopefully work with the 6.6 kernel too since get-reg-list support has
> been around since 6.6.

That's the plan. We need to make the same treatment we're already doing with
the extensions with these CSRs.

> 
> A quick fix for this is to just drop senvcfg for now since nobody
> noticed it was missing before (well, I noticed it was missing, but by
> inspection, not test).

We can do that in case the proper fix turns out to be more complex than
we've anticipated and we'll miss the cut for the release. I think we have
time to do it properly, but let's see.

Thanks,

Daniel

> 
> Thanks,
> drew

Re: [PATCH v3 0/3] target/riscv/kvm: reset time changes

[Alistair Francis]

On Mon, Feb 24, 2025 at 10:32 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Hi,
>
> In this version I rolled back on the riscv_cpu_reset_hold() changes made
> in patch 1. Peter made an argument about keeping the design the same
> across architectures and I agreed. Patches 2 and 3 are already taking
> care of everything that KVM needs during reset, thus this doesn't incur
> in any functional changes.
>
> Drew said it was ok to keep his ack on patch 1 so his ack is kept.
>
> Patches based on alistair/riscv-to-apply.next. All patches acked.
>
> Changes from v2:
> - patch 1:
>   - do not do an early exit in riscv_cpu_reset_hold() if kvm_enabled()
> - v2 link: https://lore.kernel.org/qemu-riscv/20250221122623.495188-1-dbarboza@ventanamicro.com/
>
>
> Daniel Henrique Barboza (3):
>   target/riscv/cpu: remove unneeded !kvm_enabled() check
>   target/riscv/kvm: add kvm_riscv_reset_regs_csr()
>   target/riscv/kvm: add missing KVM CSRs

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  target/riscv/kvm/kvm-cpu.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
>
> --
> 2.48.1
>
>