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 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