[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v5 01/19] target/riscv: skip features setup for KVM CPUs

From: Daniel Henrique Barboza
Subject: Re: [PATCH v5 01/19] target/riscv: skip features setup for KVM CPUs
Date: Tue, 27 Jun 2023 20:24:24 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0

On 6/27/23 18:19, Philippe Mathieu-Daudé wrote:
Hi Daniel,

On 27/6/23 18:31, Daniel Henrique Barboza wrote:
As it is today it's not possible to use '-cpu host' if the RISC-V host
has RVH enabled. This is the resulting error:

$ sudo ./qemu/build/qemu-system-riscv64 \
     -machine virt,accel=kvm -m 2G -smp 1 \
     -nographic -snapshot -kernel ./guest_imgs/Image  \
     -initrd ./guest_imgs/rootfs_kvm_riscv64.img \
     -append "earlycon=sbi root=/dev/ram rw" \
     -cpu host
qemu-system-riscv64: H extension requires priv spec 1.12.0

This happens because we're checking for priv spec for all CPUs, and
since we're not setting  env->priv_ver for the 'host' CPU, it's being
default to zero (i.e. PRIV_SPEC_1_10_0).

In reality env->priv_ver does not make sense when running with the KVM
'host' CPU. It's used to gate certain CSRs/extensions during translation
to make them unavailable if the hart declares an older spec version. It
doesn't have any other use. E.g. OpenSBI version 1.2 retrieves the spec
are available [1].

'priv_ver' is just one example. We're doing a lot of feature validation
and setup during riscv_cpu_realize() that it doesn't apply KVM CPUs.
Validating the feature set for those CPUs is a KVM problem that should
be handled in KVM specific code.

The new riscv_cpu_realize_features() helper contains all validation
logic that are not applicable to KVM CPUs. riscv_cpu_realize() verifies
if we're dealing with a KVM CPU and, if not, execute the new helper to
proceed with the usual realize() logic for all other CPUs.

[1] lib/sbi/sbi_hart.c, hart_detect_features()

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
  target/riscv/cpu.c | 43 +++++++++++++++++++++++++++++++++----------
  1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index fb8458bf74..e515dde208 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -331,6 +331,15 @@ static void set_satp_mode_default_map(RISCVCPU *cpu)
+static bool riscv_running_kvm(void)
+    return kvm_enabled();
+    return false;

Instead of this, ...

@@ -1369,6 +1370,28 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
+static void riscv_cpu_realize(DeviceState *dev, Error **errp)
+    CPUState *cs = CPU(dev);
+    RISCVCPU *cpu = RISCV_CPU(dev);
+    RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
+    Error *local_err = NULL;
+    cpu_exec_realizefn(cs, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    if (!riscv_running_kvm()) {

... why not simply do:

    #ifndef CONFIG_USER_ONLY

        if (!kvm_enabled()) {

+        riscv_cpu_realize_features(dev, &local_err);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }



Because a month back, when I first wrote this series, I was getting a lot of
failed linux-user builds because kvm_enabled() was being called in a linux-user
context. I got a little tired of it and created this wrapper that included
the #ifndef.

After all patches are applied we have 3 places where this function is called.
There's a chance that some of them are being called in a sysemu emulation
only and we could make it do with just a kvm_enabled(). I guess it doesn't
hurt to remove this function and handle each case one by one.

If riscv_cpu_realize_features() is for all but KVM, then the
name isn't ideal.

I guess we could rename it to riscv_cpu_realize_tcg(). We don't have any other
accelerators aside from KVM and TCG, and there's a high chance that only TCG 
care about this code even with other acccelerators in place.

In fact, this check could also be made with "if (tcg_enabled())" instead of
!kvm_enabled().This goes in line with a change you posted in qemu-ppc a few
days ago.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]