[v3] target/riscv/kvm: reset time changes

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

Posted by Daniel Henrique Barboza 1 month, 1 week ago

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

Posted by Alistair Francis 1 month ago

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

Posted by Andrea Bolognani 1 week, 6 days ago

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

Posted by Daniel Henrique Barboza 1 week, 6 days ago


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

Posted by Andrew Jones 1 week, 6 days ago

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

Posted by Daniel Henrique Barboza 1 week, 6 days ago


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