On 2/24/25 6:59 AM, Peter Maydell wrote:
On Thu, 20 Feb 2025 at 16:14, Daniel Henrique Barboza
<dbarb...@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 <dbarb...@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
