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

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

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