[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: Philippe Mathieu-Daudé
Subject: Re: [PATCH v5 01/19] target/riscv: skip features setup for KVM CPUs
Date: Wed, 28 Jun 2023 08:39:59 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.12.0

On 28/6/23 01:24, Daniel Henrique Barboza wrote:

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 **errp)
+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.

I'd rather that, but as you prefer (can be cleaned later).

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

Ah. This comes from a not obvious debugging on ARM where we were
checking for '!kvm_enabled()' instead of 'tcg_enabled()', and QEMU
was misbehaving when using the HVF accelerator.

So better check for the correct accelerator from scratch, in case
some company switch to riscv in few years and add HVF implementation
for riscv ;)

reply via email to

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