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

[Peter Maydell <peter.maydell@linaro.org>]

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