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

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

[Daniel Henrique Barboza]

Hi,

These patches were done in the context of gitlab #2573 [1]. The gitlab
entry per se will probably be closed as a guest software bug, but while
working on it I noticed that we're writing a TCG-initialized
env->mstatus in KVM.

This is happening because riscv_cpu_reset_hold() is doing all TCG
related initialization first, and then calling kvm_riscv_reset_vcpu() in
the end. For example, we're writing '0xa0000000' in 'sstatus' because
TCG is setting env->mstatus = 0xa0000000.

First patch separates KVM vcpu initialization from TCG, centering all
KVM reset procedure into kvm_riscv_reset_vcpu(). Patches 2 and 3 are
small improvements made around get/put KVM csr regs. 

[1] https://gitlab.com/qemu-project/qemu/-/issues/2573

Daniel Henrique Barboza (3):
  target/riscv/cpu: ignore TCG init for KVM CPUs in reset_hold
  target/riscv/kvm: use env->sie to read/write 'sie' CSR
  target/riscv/kvm: reset all available KVM CSRs in kvm_reset()

 target/riscv/cpu.c         |  9 +++++----
 target/riscv/kvm/kvm-cpu.c | 15 ++++++++++-----
 2 files changed, 15 insertions(+), 9 deletions(-)

-- 
2.48.1

[PATCH 1/3] target/riscv/cpu: ignore TCG init for KVM CPUs in reset_hold

[Daniel Henrique Barboza]

riscv_cpu_reset_hold() does a lot of TCG-related initializations that
aren't relevant for KVM, but nevertheless are impacting the reset state
of KVM vcpus.

When running a KVM guest, kvm_riscv_reset_vcpu() is called at the end of
reset_hold(). At that point env->mstatus is initialized to a non-zero
value, and it will be use to write 'sstatus' in the vcpu
(kvm_arch_put_registers() then kvm_riscv_put_regs_csr()).

Do an early exit in riscv_cpu_reset_hold() if we're running KVM. All the
KVM reset procedure will be centered in kvm_riscv_reset_vcpu().

While we're at it, remove the kvm_enabled() check in
kvm_riscv_reset_vcpu() since it's already being gated in
riscv_cpu_reset_hold().

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c         | 9 +++++----
 target/riscv/kvm/kvm-cpu.c | 3 ---
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 522d6584e4..8e6e629ec4 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1050,6 +1050,11 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
         mcc->parent_phases.hold(obj, type);
     }
 #ifndef CONFIG_USER_ONLY
+    if (kvm_enabled()) {
+        kvm_riscv_reset_vcpu(cpu);
+        return;
+    }
+
     env->misa_mxl = mcc->misa_mxl_max;
     env->priv = PRV_M;
     env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
@@ -1146,10 +1151,6 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
         env->rnmip = 0;
         env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, false);
     }
-
-    if (kvm_enabled()) {
-        kvm_riscv_reset_vcpu(cpu);
-    }
 #endif
 }
 
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 23ce779359..484b6afe7c 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1603,9 +1603,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 2/3] target/riscv/kvm: use env->sie to read/write 'sie' CSR

[Daniel Henrique Barboza]

Using env->sie is clearer than using env->mie.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 484b6afe7c..fea03f3657 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -610,7 +610,7 @@ static int kvm_riscv_get_regs_csr(CPUState *cs)
     CPURISCVState *env = &RISCV_CPU(cs)->env;
 
     KVM_RISCV_GET_CSR(cs, env, sstatus, env->mstatus);
-    KVM_RISCV_GET_CSR(cs, env, sie, env->mie);
+    KVM_RISCV_GET_CSR(cs, env, sie, env->sie);
     KVM_RISCV_GET_CSR(cs, env, stvec, env->stvec);
     KVM_RISCV_GET_CSR(cs, env, sscratch, env->sscratch);
     KVM_RISCV_GET_CSR(cs, env, sepc, env->sepc);
@@ -627,7 +627,7 @@ static int kvm_riscv_put_regs_csr(CPUState *cs)
     CPURISCVState *env = &RISCV_CPU(cs)->env;
 
     KVM_RISCV_SET_CSR(cs, env, sstatus, env->mstatus);
-    KVM_RISCV_SET_CSR(cs, env, sie, env->mie);
+    KVM_RISCV_SET_CSR(cs, env, sie, env->sie);
     KVM_RISCV_SET_CSR(cs, env, stvec, env->stvec);
     KVM_RISCV_SET_CSR(cs, env, sscratch, env->sscratch);
     KVM_RISCV_SET_CSR(cs, env, sepc, env->sepc);
-- 
2.48.1

[PATCH 3/3] target/riscv/kvm: reset all available KVM CSRs in kvm_reset()

[Daniel Henrique Barboza]

Explictly reset env->mstatus and env->sie. Add a comment about env->mip
being read/written into KVM 'sip' CSR.

We're also not read/writing 'scounteren' which is available in the KVM
UAPI. Add it in kvm_reset() and get/put_regs_csr().

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

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index fea03f3657..ee7a9295b4 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -618,6 +618,7 @@ 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);
 
     return 0;
 }
@@ -635,6 +636,7 @@ 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);
 
     return 0;
 }
@@ -1609,6 +1611,10 @@ 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 */
+
+    /* sstatus is read/written into mstatus */
+    env->mstatus = 0;
+    env->sie = 0;
     env->satp = 0;
     env->mie = 0;
     env->stvec = 0;
@@ -1616,7 +1622,9 @@ void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
     env->sepc = 0;
     env->scause = 0;
     env->stval = 0;
+    /* sip is read/written into mip */
     env->mip = 0;
+    env->scounteren = 0;
 }
 
 void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level)
-- 
2.48.1

Re: [PATCH 1/3] target/riscv/cpu: ignore TCG init for KVM CPUs in reset_hold

[Andrew Jones]

On Thu, Feb 20, 2025 at 01:13:11PM -0300, Daniel Henrique Barboza wrote:
> riscv_cpu_reset_hold() does a lot of TCG-related initializations that
> aren't relevant for KVM, but nevertheless are impacting the reset state
> of KVM vcpus.
> 
> When running a KVM guest, kvm_riscv_reset_vcpu() is called at the end of
> reset_hold(). At that point env->mstatus is initialized to a non-zero
> value, and it will be use to write 'sstatus' in the vcpu
> (kvm_arch_put_registers() then kvm_riscv_put_regs_csr()).
> 
> Do an early exit in riscv_cpu_reset_hold() if we're running KVM. All the
> KVM reset procedure will be centered in kvm_riscv_reset_vcpu().
> 
> While we're at it, remove the kvm_enabled() check in
> kvm_riscv_reset_vcpu() since it's already being gated in
> riscv_cpu_reset_hold().
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c         | 9 +++++----
>  target/riscv/kvm/kvm-cpu.c | 3 ---
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 522d6584e4..8e6e629ec4 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1050,6 +1050,11 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
>          mcc->parent_phases.hold(obj, type);
>      }
>  #ifndef CONFIG_USER_ONLY
> +    if (kvm_enabled()) {
> +        kvm_riscv_reset_vcpu(cpu);
> +        return;
> +    }
> +
>      env->misa_mxl = mcc->misa_mxl_max;
>      env->priv = PRV_M;
>      env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> @@ -1146,10 +1151,6 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
>          env->rnmip = 0;
>          env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, false);
>      }
> -
> -    if (kvm_enabled()) {
> -        kvm_riscv_reset_vcpu(cpu);
> -    }
>  #endif
>  }
>  
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 23ce779359..484b6afe7c 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1603,9 +1603,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
> 
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH 2/3] target/riscv/kvm: use env->sie to read/write 'sie' CSR

[Andrew Jones]

On Thu, Feb 20, 2025 at 01:13:12PM -0300, Daniel Henrique Barboza wrote:
> Using env->sie is clearer than using env->mie.

Maybe? Just as sstatus is a subset of mstatus, sip and sie can be
subsets of mip and mie. However, the AIA can change sip/sie so they
no longer alias mip/mie, which is why we have 'mvip' an 'sie' members
in CPURISCVState. In the end, for KVM, it doesn't really matter since
this is just s/r storage. I'd probably just drop this patch and keep
using mie. Otherwise, what about mip?

Thanks,
drew

> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/kvm/kvm-cpu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 484b6afe7c..fea03f3657 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -610,7 +610,7 @@ static int kvm_riscv_get_regs_csr(CPUState *cs)
>      CPURISCVState *env = &RISCV_CPU(cs)->env;
>  
>      KVM_RISCV_GET_CSR(cs, env, sstatus, env->mstatus);
> -    KVM_RISCV_GET_CSR(cs, env, sie, env->mie);
> +    KVM_RISCV_GET_CSR(cs, env, sie, env->sie);
>      KVM_RISCV_GET_CSR(cs, env, stvec, env->stvec);
>      KVM_RISCV_GET_CSR(cs, env, sscratch, env->sscratch);
>      KVM_RISCV_GET_CSR(cs, env, sepc, env->sepc);
> @@ -627,7 +627,7 @@ static int kvm_riscv_put_regs_csr(CPUState *cs)
>      CPURISCVState *env = &RISCV_CPU(cs)->env;
>  
>      KVM_RISCV_SET_CSR(cs, env, sstatus, env->mstatus);
> -    KVM_RISCV_SET_CSR(cs, env, sie, env->mie);
> +    KVM_RISCV_SET_CSR(cs, env, sie, env->sie);
>      KVM_RISCV_SET_CSR(cs, env, stvec, env->stvec);
>      KVM_RISCV_SET_CSR(cs, env, sscratch, env->sscratch);
>      KVM_RISCV_SET_CSR(cs, env, sepc, env->sepc);
> -- 
> 2.48.1
> 
> 

Re: [PATCH 3/3] target/riscv/kvm: reset all available KVM CSRs in kvm_reset()

[Andrew Jones]

On Thu, Feb 20, 2025 at 01:13:13PM -0300, Daniel Henrique Barboza wrote:
> Explictly reset env->mstatus and env->sie.

mie was already getting set to zero, so that should have just been renamed
in the last patch, but I still think we should drop the last patch.

> Add a comment about env->mip
> being read/written into KVM 'sip' CSR.
> 
> We're also not read/writing 'scounteren' which is available in the KVM
> UAPI. Add it in kvm_reset() and get/put_regs_csr().
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/kvm/kvm-cpu.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index fea03f3657..ee7a9295b4 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -618,6 +618,7 @@ 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);

senvcfg is also missing.

>  
>      return 0;
>  }
> @@ -635,6 +636,7 @@ 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);
>  
>      return 0;
>  }
> @@ -1609,6 +1611,10 @@ 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 */
> +
> +    /* sstatus is read/written into mstatus */

How about just a single comment above this function stating that we
reset all registers that we will s/r with csr get/put. Interested
parties can go look at get or put to see the mappings.

> +    env->mstatus = 0;
> +    env->sie = 0;
>      env->satp = 0;
>      env->mie = 0;
>      env->stvec = 0;
> @@ -1616,7 +1622,9 @@ void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
>      env->sepc = 0;
>      env->scause = 0;
>      env->stval = 0;
> +    /* sip is read/written into mip */
>      env->mip = 0;
> +    env->scounteren = 0;
>  }
>  
>  void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level)
> -- 
> 2.48.1
> 
>

Thanks,
drew

Re: [PATCH 3/3] target/riscv/kvm: reset all available KVM CSRs in kvm_reset()

[Andrew Jones]

On Fri, Feb 21, 2025 at 09:45:35AM +0100, Andrew Jones wrote:
> On Thu, Feb 20, 2025 at 01:13:13PM -0300, Daniel Henrique Barboza wrote:
> > Explictly reset env->mstatus and env->sie.
> 
> mie was already getting set to zero, so that should have just been renamed
> in the last patch, but I still think we should drop the last patch.
> 
> > Add a comment about env->mip
> > being read/written into KVM 'sip' CSR.
> > 
> > We're also not read/writing 'scounteren' which is available in the KVM
> > UAPI. Add it in kvm_reset() and get/put_regs_csr().
> > 
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >  target/riscv/kvm/kvm-cpu.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> > index fea03f3657..ee7a9295b4 100644
> > --- a/target/riscv/kvm/kvm-cpu.c
> > +++ b/target/riscv/kvm/kvm-cpu.c
> > @@ -618,6 +618,7 @@ 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);
> 
> senvcfg is also missing.
> 
> >  
> >      return 0;
> >  }
> > @@ -635,6 +636,7 @@ 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);
> >  
> >      return 0;
> >  }
> > @@ -1609,6 +1611,10 @@ 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 */
> > +
> > +    /* sstatus is read/written into mstatus */
> 
> How about just a single comment above this function stating that we
> reset all registers that we will s/r with csr get/put. Interested
> parties can go look at get or put to see the mappings.
> 
> > +    env->mstatus = 0;
> > +    env->sie = 0;
> >      env->satp = 0;
> >      env->mie = 0;
> >      env->stvec = 0;
> > @@ -1616,7 +1622,9 @@ void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
> >      env->sepc = 0;
> >      env->scause = 0;
> >      env->stval = 0;
> > +    /* sip is read/written into mip */
> >      env->mip = 0;
> > +    env->scounteren = 0;

It'd be nice to put all the above register assignments in the order of
struct kvm_riscv_csr, like get/put do.

Thanks,
drew


> >  }
> >  
> >  void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level)
> > -- 
> > 2.48.1
> > 
> >
> 
> Thanks,
> drew

Re: [PATCH 2/3] target/riscv/kvm: use env->sie to read/write 'sie' CSR

[Daniel Henrique Barboza]

On 2/21/25 5:37 AM, Andrew Jones wrote:
> On Thu, Feb 20, 2025 at 01:13:12PM -0300, Daniel Henrique Barboza wrote:
>> Using env->sie is clearer than using env->mie.
> 
> Maybe? Just as sstatus is a subset of mstatus, sip and sie can be
> subsets of mip and mie. However, the AIA can change sip/sie so they
> no longer alias mip/mie, which is why we have 'mvip' an 'sie' members
> in CPURISCVState. In the end, for KVM, it doesn't really matter since
> this is just s/r storage. I'd probably just drop this patch and keep
> using mie. Otherwise, what about mip?

We don't have env->sip so it would fall on the same case as using env->mstatus
to get/put sstatus.

But yeah this change isn't that big of a deal and can be replaced with a comment
in reset_vcpu like you said in v3. Or maybe we could put those assignments in a
kvm_riscv_reset_regs_csr(), and the name of the function itself is an indication
that we follow the same order as kvm_riscv_(get/put)_regs_csr().

I think I'll use patch 2 to do that and patch 3 to add the missing KVM CSRs
(scounteren and senvcfg).


Thanks,

Daniel

> 
> Thanks,
> drew
> 
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/kvm/kvm-cpu.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
>> index 484b6afe7c..fea03f3657 100644
>> --- a/target/riscv/kvm/kvm-cpu.c
>> +++ b/target/riscv/kvm/kvm-cpu.c
>> @@ -610,7 +610,7 @@ static int kvm_riscv_get_regs_csr(CPUState *cs)
>>       CPURISCVState *env = &RISCV_CPU(cs)->env;
>>   
>>       KVM_RISCV_GET_CSR(cs, env, sstatus, env->mstatus);
>> -    KVM_RISCV_GET_CSR(cs, env, sie, env->mie);
>> +    KVM_RISCV_GET_CSR(cs, env, sie, env->sie);
>>       KVM_RISCV_GET_CSR(cs, env, stvec, env->stvec);
>>       KVM_RISCV_GET_CSR(cs, env, sscratch, env->sscratch);
>>       KVM_RISCV_GET_CSR(cs, env, sepc, env->sepc);
>> @@ -627,7 +627,7 @@ static int kvm_riscv_put_regs_csr(CPUState *cs)
>>       CPURISCVState *env = &RISCV_CPU(cs)->env;
>>   
>>       KVM_RISCV_SET_CSR(cs, env, sstatus, env->mstatus);
>> -    KVM_RISCV_SET_CSR(cs, env, sie, env->mie);
>> +    KVM_RISCV_SET_CSR(cs, env, sie, env->sie);
>>       KVM_RISCV_SET_CSR(cs, env, stvec, env->stvec);
>>       KVM_RISCV_SET_CSR(cs, env, sscratch, env->sscratch);
>>       KVM_RISCV_SET_CSR(cs, env, sepc, env->sepc);
>> -- 
>> 2.48.1
>>
>>

Re: [PATCH 1/3] target/riscv/cpu: ignore TCG init for KVM CPUs in reset_hold

[Alistair Francis]

On Fri, Feb 21, 2025 at 2:14 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> riscv_cpu_reset_hold() does a lot of TCG-related initializations that
> aren't relevant for KVM, but nevertheless are impacting the reset state
> of KVM vcpus.
>
> When running a KVM guest, kvm_riscv_reset_vcpu() is called at the end of
> reset_hold(). At that point env->mstatus is initialized to a non-zero
> value, and it will be use to write 'sstatus' in the vcpu
> (kvm_arch_put_registers() then kvm_riscv_put_regs_csr()).
>
> Do an early exit in riscv_cpu_reset_hold() if we're running KVM. All the
> KVM reset procedure will be centered in kvm_riscv_reset_vcpu().
>
> While we're at it, remove the kvm_enabled() check in
> kvm_riscv_reset_vcpu() since it's already being gated in
> riscv_cpu_reset_hold().
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         | 9 +++++----
>  target/riscv/kvm/kvm-cpu.c | 3 ---
>  2 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 522d6584e4..8e6e629ec4 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1050,6 +1050,11 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
>          mcc->parent_phases.hold(obj, type);
>      }
>  #ifndef CONFIG_USER_ONLY
> +    if (kvm_enabled()) {
> +        kvm_riscv_reset_vcpu(cpu);
> +        return;
> +    }
> +
>      env->misa_mxl = mcc->misa_mxl_max;
>      env->priv = PRV_M;
>      env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> @@ -1146,10 +1151,6 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
>          env->rnmip = 0;
>          env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, false);
>      }
> -
> -    if (kvm_enabled()) {
> -        kvm_riscv_reset_vcpu(cpu);
> -    }
>  #endif
>  }
>
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 23ce779359..484b6afe7c 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1603,9 +1603,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 1/3] target/riscv/cpu: ignore TCG init for KVM CPUs in reset_hold

[Peter Maydell]

On Thu, 20 Feb 2025 at 16:14, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> riscv_cpu_reset_hold() does a lot of TCG-related initializations that
> aren't relevant for KVM, but nevertheless are impacting the reset state
> of KVM vcpus.
>
> When running a KVM guest, kvm_riscv_reset_vcpu() is called at the end of
> reset_hold(). At that point env->mstatus is initialized to a non-zero
> value, and it will be use to write 'sstatus' in the vcpu
> (kvm_arch_put_registers() then kvm_riscv_put_regs_csr()).
>
> Do an early exit in riscv_cpu_reset_hold() if we're running KVM. All the
> KVM reset procedure will be centered in kvm_riscv_reset_vcpu().
>
> While we're at it, remove the kvm_enabled() check in
> kvm_riscv_reset_vcpu() since it's already being gated in
> riscv_cpu_reset_hold().
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c         | 9 +++++----
>  target/riscv/kvm/kvm-cpu.c | 3 ---
>  2 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 522d6584e4..8e6e629ec4 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1050,6 +1050,11 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
>          mcc->parent_phases.hold(obj, type);
>      }
>  #ifndef CONFIG_USER_ONLY
> +    if (kvm_enabled()) {
> +        kvm_riscv_reset_vcpu(cpu);
> +        return;
> +    }
> +
>      env->misa_mxl = mcc->misa_mxl_max;
>      env->priv = PRV_M;
>      env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> @@ -1146,10 +1151,6 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
>          env->rnmip = 0;
>          env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, false);
>      }
> -
> -    if (kvm_enabled()) {
> -        kvm_riscv_reset_vcpu(cpu);
> -    }
>  #endif
>  }

This looks super odd, from an "I don't know anything about
riscv specifics" position. Generally the idea is:
 * reset in QEMU should reset the CPU state
 * what a reset CPU looks like doesn't differ between
   accelerators
 * when we start the KVM CPU, we copy the state from QEMU
   to the kernel, and then the kernel's idea of the reset state
   matches

This patch looks like it's entirely skipping basically
all of the QEMU CPU state reset code specifically for KVM.
So now you'll have two different pieces of code controlling
reset for different accelerators, and the resulting CPU
state won't be consistent between them...

thanks
-- PMM

Re: [PATCH 1/3] target/riscv/cpu: ignore TCG init for KVM CPUs in reset_hold

[Daniel Henrique Barboza]

On 2/24/25 6:59 AM, Peter Maydell wrote:
> On Thu, 20 Feb 2025 at 16:14, Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> riscv_cpu_reset_hold() does a lot of TCG-related initializations that
>> aren't relevant for KVM, but nevertheless are impacting the reset state
>> of KVM vcpus.
>>
>> When running a KVM guest, kvm_riscv_reset_vcpu() is called at the end of
>> reset_hold(). At that point env->mstatus is initialized to a non-zero
>> value, and it will be use to write 'sstatus' in the vcpu
>> (kvm_arch_put_registers() then kvm_riscv_put_regs_csr()).
>>
>> Do an early exit in riscv_cpu_reset_hold() if we're running KVM. All the
>> KVM reset procedure will be centered in kvm_riscv_reset_vcpu().
>>
>> While we're at it, remove the kvm_enabled() check in
>> kvm_riscv_reset_vcpu() since it's already being gated in
>> riscv_cpu_reset_hold().
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c         | 9 +++++----
>>   target/riscv/kvm/kvm-cpu.c | 3 ---
>>   2 files changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 522d6584e4..8e6e629ec4 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1050,6 +1050,11 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
>>           mcc->parent_phases.hold(obj, type);
>>       }
>>   #ifndef CONFIG_USER_ONLY
>> +    if (kvm_enabled()) {
>> +        kvm_riscv_reset_vcpu(cpu);
>> +        return;
>> +    }
>> +
>>       env->misa_mxl = mcc->misa_mxl_max;
>>       env->priv = PRV_M;
>>       env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
>> @@ -1146,10 +1151,6 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
>>           env->rnmip = 0;
>>           env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, false);
>>       }
>> -
>> -    if (kvm_enabled()) {
>> -        kvm_riscv_reset_vcpu(cpu);
>> -    }
>>   #endif
>>   }
> 
> This looks super odd, from an "I don't know anything about
> riscv specifics" position. Generally the idea is:
>   * reset in QEMU should reset the CPU state
>   * what a reset CPU looks like doesn't differ between
>     accelerators
>   * when we start the KVM CPU, we copy the state from QEMU
>     to the kernel, and then the kernel's idea of the reset state
>     matches
> 
> This patch looks like it's entirely skipping basically
> all of the QEMU CPU state reset code specifically for KVM.

Not sure I understood what you said here.

Without this patch, riscv_cpu_reset_hold() is doing initializations that are TCG
based, both for user mode and system mode, and in the end is calling the kvm
specific reset function if we're running KVM. This patch is simply skipping
all the TCG related reset procedures if we're running KVM. So the patch isn't
skipping the KVM specific QEMU CPU reset code, it is skipping the TCG specific
reset code if we're running KVM.

Granted, after applying patches 2 and 3 we could discard this patch because
now we're resetting all that KVM needs in kvm_reset_vcpu(), but why go
through the reset steps for TCG if we're going to overwrite them later during
kvm_reset_vcpu()?


> So now you'll have two different pieces of code controlling
> reset for different accelerators, and the resulting CPU
> state won't be consistent between them...

That is already the case even without this patch, doesn't it? If we have to call
a specific kvm reset function during cpu reset_hold then we're already in a point
where the reset procedure is differing between accelerators. I won't say that
this is a good design but I don't see it as a problem.

For instance, going to a code you're probably more familiar with, target/arm/cpu.c,
arm_cpu_reset_hold(), is doing something similar to what riscv_cpu_reset_hold() is
doing without this patch: a lot of TCG setups are made, then kvm_arm_reset_vcpu() is
called in the end if kvm_enabled(). kvm_arm_reset_vcpu() then overwrites at least some
of the TCG specific setup that was done before:

     /* Re-init VCPU so that all registers are set to
      * their respective reset values.
      */
     ret = kvm_arm_vcpu_init(cpu);

kvm_arm_vcpu_init() is doing a KVM_ARM_VCPU_INIT ioctl that will populate the CPU object
with the kernel specific feature bitmask and so on. Note that this is not copying the TCG
setup into the kernel, it is in fact doing the opposite.

Note that my intention here isn't to make a case that the ARM KVM cpu doesn't need anything
that is being done in arm_cpu_reset_hold(). My point here is that KVM and TCG CPUs will have
different reset setups for some archs. For RISC-V I can say that KVM CPU does not rely on
anything that the TCG reset code is doing, hence why I sent this patch to make it official.


Thanks,

Daniel

> 
> thanks
> -- PMM

Re: [PATCH 1/3] target/riscv/cpu: ignore TCG init for KVM CPUs in reset_hold

[Peter Maydell]

On Mon, 24 Feb 2025 at 11:29, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 2/24/25 6:59 AM, Peter Maydell wrote:
> > On Thu, 20 Feb 2025 at 16:14, Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> riscv_cpu_reset_hold() does a lot of TCG-related initializations that
> >> aren't relevant for KVM, but nevertheless are impacting the reset state
> >> of KVM vcpus.
> >>
> >> When running a KVM guest, kvm_riscv_reset_vcpu() is called at the end of
> >> reset_hold(). At that point env->mstatus is initialized to a non-zero
> >> value, and it will be use to write 'sstatus' in the vcpu
> >> (kvm_arch_put_registers() then kvm_riscv_put_regs_csr()).
> >>
> >> Do an early exit in riscv_cpu_reset_hold() if we're running KVM. All the
> >> KVM reset procedure will be centered in kvm_riscv_reset_vcpu().
> >>
> >> While we're at it, remove the kvm_enabled() check in
> >> kvm_riscv_reset_vcpu() since it's already being gated in
> >> riscv_cpu_reset_hold().

> > This looks super odd, from an "I don't know anything about
> > riscv specifics" position. Generally the idea is:
> >   * reset in QEMU should reset the CPU state
> >   * what a reset CPU looks like doesn't differ between
> >     accelerators
> >   * when we start the KVM CPU, we copy the state from QEMU
> >     to the kernel, and then the kernel's idea of the reset state
> >     matches
> >
> > This patch looks like it's entirely skipping basically
> > all of the QEMU CPU state reset code specifically for KVM.
>
> Not sure I understood what you said here.
>
> Without this patch, riscv_cpu_reset_hold() is doing initializations that are TCG
> based, both for user mode and system mode, and in the end is calling the kvm
> specific reset function if we're running KVM. This patch is simply skipping
> all the TCG related reset procedures if we're running KVM. So the patch isn't
> skipping the KVM specific QEMU CPU reset code, it is skipping the TCG specific
> reset code if we're running KVM.

What I'm saying is that you shouldn't have "TCG reset" and
"KVM reset" that are totally different code paths, but that the
reset function should be doing "reset the CPU", and then the
KVM codepath makes specific decisions about "for KVM these
particular things should be the kernel's reset settings" and
then passes that state over to the kernel.

> Granted, after applying patches 2 and 3 we could discard this patch because
> now we're resetting all that KVM needs in kvm_reset_vcpu(), but why go
> through the reset steps for TCG if we're going to overwrite them later during
> kvm_reset_vcpu()?

The idea is that you only overwrite specific state where
you've decided "actually the kernel is the authoritative
source for what the reset state for these registers is".

> > So now you'll have two different pieces of code controlling
> > reset for different accelerators, and the resulting CPU
> > state won't be consistent between them...
>
> That is already the case even without this patch, doesn't it? If we have to call
> a specific kvm reset function during cpu reset_hold then we're already in a point
> where the reset procedure is differing between accelerators. I won't say that
> this is a good design but I don't see it as a problem.

> For instance, going to a code you're probably more familiar with, target/arm/cpu.c,
> arm_cpu_reset_hold(), is doing something similar to what riscv_cpu_reset_hold() is
> doing without this patch: a lot of TCG setups are made, then kvm_arm_reset_vcpu() is
> called in the end if kvm_enabled(). kvm_arm_reset_vcpu() then overwrites at least some
> of the TCG specific setup that was done before:
>
>      /* Re-init VCPU so that all registers are set to
>       * their respective reset values.
>       */
>      ret = kvm_arm_vcpu_init(cpu);
>
> kvm_arm_vcpu_init() is doing a KVM_ARM_VCPU_INIT ioctl that will populate the CPU object
> with the kernel specific feature bitmask and so on. Note that this is not copying the TCG
> setup into the kernel, it is in fact doing the opposite.

The way this code path in Arm works is:
 * we reset all the QEMU-side CPU state struct in arm_cpu_reset_hold()
 * kvm_arm_reset_vcpu() calls kvm_arm_vcpu_init() which inits
   the KVM side vcpu
 * kvm_arm_reset_vcpu() calls the sequence write_kvmstate_to_list()
   followed by write_list_to_cpustate() which does "for the system
   registers specifically, read the values (and which registers we
   have) from KVM, and write them to the QEMU CPU state struct".
   We do this because on Arm we've said that the system registers
   in particular have the kernel as their authoritative source.
   (IIRC x86 makes QEMU the authority for its similar registers,
   so it has to do even less in its kvm_arch_reset_vcpu().)
 * the generic CPU core code will call kvm_arch_put_registers()
   before starting the vCPU, which copies whatever is in the QEMU-side
   CPU state struct into KVM

The upshot is that QEMU is the authority and arm_cpu_reset_hold()
defines the reset value for all CPU state by default. (For instance,
reset values for general purpose registers are set there.) But for
specific parts of the state where we want KVM to be the authority,
kvm_arm_reset_vcpu() gets to override that by filling in the CPU
state struct by asking the kernel what its values are.

> Note that my intention here isn't to make a case that the ARM KVM cpu doesn't need anything
> that is being done in arm_cpu_reset_hold(). My point here is that KVM and TCG CPUs will have
> different reset setups for some archs. For RISC-V I can say that KVM CPU does not rely on
> anything that the TCG reset code is doing, hence why I sent this patch to make it official.

What I'm trying to suggest here is that we don't want different
architectures to end up doing things differently. There are
multiple different design patterns that will work, but it will
be easier to work on QEMU if we can standardise on doing it
the same way across architectures.

thanks
-- PMM

Re: [PATCH 1/3] target/riscv/cpu: ignore TCG init for KVM CPUs in reset_hold

[Daniel Henrique Barboza]

On 2/24/25 8:47 AM, Peter Maydell wrote:
> On Mon, 24 Feb 2025 at 11:29, Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>>
>> On 2/24/25 6:59 AM, Peter Maydell wrote:
>>> On Thu, 20 Feb 2025 at 16:14, Daniel Henrique Barboza
>>> <dbarboza@ventanamicro.com> wrote:
>>>>
>>>> riscv_cpu_reset_hold() does a lot of TCG-related initializations that
>>>> aren't relevant for KVM, but nevertheless are impacting the reset state
>>>> of KVM vcpus.
>>>>
>>>> When running a KVM guest, kvm_riscv_reset_vcpu() is called at the end of
>>>> reset_hold(). At that point env->mstatus is initialized to a non-zero
>>>> value, and it will be use to write 'sstatus' in the vcpu
>>>> (kvm_arch_put_registers() then kvm_riscv_put_regs_csr()).
>>>>
>>>> Do an early exit in riscv_cpu_reset_hold() if we're running KVM. All the
>>>> KVM reset procedure will be centered in kvm_riscv_reset_vcpu().
>>>>
>>>> While we're at it, remove the kvm_enabled() check in
>>>> kvm_riscv_reset_vcpu() since it's already being gated in
>>>> riscv_cpu_reset_hold().
> 
>>> This looks super odd, from an "I don't know anything about
>>> riscv specifics" position. Generally the idea is:
>>>    * reset in QEMU should reset the CPU state
>>>    * what a reset CPU looks like doesn't differ between
>>>      accelerators
>>>    * when we start the KVM CPU, we copy the state from QEMU
>>>      to the kernel, and then the kernel's idea of the reset state
>>>      matches
>>>
>>> This patch looks like it's entirely skipping basically
>>> all of the QEMU CPU state reset code specifically for KVM.
>>
>> Not sure I understood what you said here.
>>
>> Without this patch, riscv_cpu_reset_hold() is doing initializations that are TCG
>> based, both for user mode and system mode, and in the end is calling the kvm
>> specific reset function if we're running KVM. This patch is simply skipping
>> all the TCG related reset procedures if we're running KVM. So the patch isn't
>> skipping the KVM specific QEMU CPU reset code, it is skipping the TCG specific
>> reset code if we're running KVM.
> 
> What I'm saying is that you shouldn't have "TCG reset" and
> "KVM reset" that are totally different code paths, but that the
> reset function should be doing "reset the CPU", and then the
> KVM codepath makes specific decisions about "for KVM these
> particular things should be the kernel's reset settings" and
> then passes that state over to the kernel.
> 
>> Granted, after applying patches 2 and 3 we could discard this patch because
>> now we're resetting all that KVM needs in kvm_reset_vcpu(), but why go
>> through the reset steps for TCG if we're going to overwrite them later during
>> kvm_reset_vcpu()?
> 
> The idea is that you only overwrite specific state where
> you've decided "actually the kernel is the authoritative
> source for what the reset state for these registers is".
> 
>>> So now you'll have two different pieces of code controlling
>>> reset for different accelerators, and the resulting CPU
>>> state won't be consistent between them...
>>
>> That is already the case even without this patch, doesn't it? If we have to call
>> a specific kvm reset function during cpu reset_hold then we're already in a point
>> where the reset procedure is differing between accelerators. I won't say that
>> this is a good design but I don't see it as a problem.
> 
>> For instance, going to a code you're probably more familiar with, target/arm/cpu.c,
>> arm_cpu_reset_hold(), is doing something similar to what riscv_cpu_reset_hold() is
>> doing without this patch: a lot of TCG setups are made, then kvm_arm_reset_vcpu() is
>> called in the end if kvm_enabled(). kvm_arm_reset_vcpu() then overwrites at least some
>> of the TCG specific setup that was done before:
>>
>>       /* Re-init VCPU so that all registers are set to
>>        * their respective reset values.
>>        */
>>       ret = kvm_arm_vcpu_init(cpu);
>>
>> kvm_arm_vcpu_init() is doing a KVM_ARM_VCPU_INIT ioctl that will populate the CPU object
>> with the kernel specific feature bitmask and so on. Note that this is not copying the TCG
>> setup into the kernel, it is in fact doing the opposite.
> 
> The way this code path in Arm works is:
>   * we reset all the QEMU-side CPU state struct in arm_cpu_reset_hold()
>   * kvm_arm_reset_vcpu() calls kvm_arm_vcpu_init() which inits
>     the KVM side vcpu
>   * kvm_arm_reset_vcpu() calls the sequence write_kvmstate_to_list()
>     followed by write_list_to_cpustate() which does "for the system
>     registers specifically, read the values (and which registers we
>     have) from KVM, and write them to the QEMU CPU state struct".
>     We do this because on Arm we've said that the system registers
>     in particular have the kernel as their authoritative source.
>     (IIRC x86 makes QEMU the authority for its similar registers,
>     so it has to do even less in its kvm_arch_reset_vcpu().)
>   * the generic CPU core code will call kvm_arch_put_registers()
>     before starting the vCPU, which copies whatever is in the QEMU-side
>     CPU state struct into KVM
> 
> The upshot is that QEMU is the authority and arm_cpu_reset_hold()
> defines the reset value for all CPU state by default. (For instance,
> reset values for general purpose registers are set there.) But for
> specific parts of the state where we want KVM to be the authority,
> kvm_arm_reset_vcpu() gets to override that by filling in the CPU
> state struct by asking the kernel what its values are.
> 
>> Note that my intention here isn't to make a case that the ARM KVM cpu doesn't need anything
>> that is being done in arm_cpu_reset_hold(). My point here is that KVM and TCG CPUs will have
>> different reset setups for some archs. For RISC-V I can say that KVM CPU does not rely on
>> anything that the TCG reset code is doing, hence why I sent this patch to make it official.
> 
> What I'm trying to suggest here is that we don't want different
> architectures to end up doing things differently. There are
> multiple different design patterns that will work, but it will
> be easier to work on QEMU if we can standardise on doing it
> the same way across architectures.

I can get behind this argument.

For this patch I'll just remove the redundant !kvm_enabled() check in kvm_riscv_reset_vcpu().
Let's keep the kvm_riscv_reset_vcpu() call at the end of riscv_cpu_reset_hold() to keep
the design where we have a single reset procedure, overwriting the needed state where
we want KVM to be the authority instead of QEMU.


Thanks,

Daniel

> 
> thanks
> -- PMM