[PATCH v6 00/20] target/riscv, KVM: fixes and enhancements

[PATCH v6 00/20] target/riscv, KVM: fixes and enhancements

[Daniel Henrique Barboza]

Hi,

This new version has changes suggested by Phil in v5. It also has some
changes I made on the fly. The short summary is:

- use "#ifndef CONFIG_USER_MODE kvm_enabled() #endif" instead of a helper
  that wraps them all up. This was suggested by Phil to be more clear
  about the code that depends on KVM or not. A new patch (15) was added
  to keep the amount of #ifndefs in control in
  riscv_cpu_add_user_properties() due to this design change;

- misa_ext_info_arr[] is now indexed from 0 to 25 instead of 0 from
  potentially 1 << 25. Getters for 'name' and 'description' were added.

It's worth mentioning a change I decided to make that wasn't mentioned
in the v5 review. In patch 12, where we added KVM MISA properties, we
were trying to find the property before proceeding with
object_property_add() to add the remaining properties. I remembered that
we can also use object_property_try_add(). In fact,
object_property_add() is a simple call for object_property_try_add()
that uses &error_fatal. And it turns out that and the only error being
reported by try_add() happens when object_property_find() finds a
property with the same name. This means that we can use
object_property_try_add() with an error pointer and, if an error is
reported, it means that we already have the property. This change makes
us do a single object_property_find() instead of 2. 

Patches 6 and 17 (former 16) had trivial changes, all of them related to
the extinct riscv_running_kvm() helper, and I decided to keep their r-bs
for reviewer convenience.

Series was rebased on top of riscv-to-apply.next.

Patches missing reviews: 1, 11, 12, 14, 15.

Changes from v5:
- trivial changes (R-bs kept):
  - patch 6: use kvm_enabled() instead of riscv_running_kvm()
  - patch 17 (former 16): wrap cpu_set_cfg_unavailable() inside '#ifndef
    CONFIG_USER_MODE'
- patch 1:
  - riscv_running_kvm() removed
  - rename riscv_cpu_realize_functions() to riscv_cpu_realize_tcg()
  - use 'tcg_enabled()' before riscv_cpu_realize_tcg()
  - reword commit msg to indicate that we're checking positive for TCG
    instead of negative for KVM
- patch 11:
  - misa_ext_info_arr[] is now indexed from 0 (A-A) to 25 (Z-A)
  - getter functions for 'name' and 'description' were added
  - misa_ext_info_arr[] is no longer exported in target/riscv/cpu.h
  - 'name' and 'description' of misa_ext_cfg[] must now be populated
    after initialization because misa_ext_info_arr[] is no longer a
    constant array
- patch 12:
  - use object_property_try_add() in riscv_cpu_add_misa_properties().
    It'll set an Error pointer if the property already exists. Use that
    to skip an already created property instead of doing a redundant
    object_property_find() beforehand
  - moved the object_property_find() code that was checking for
    multi-letter properties to patch 14
  - use the new getters() from patch 11 instead of reading the array
    directly
- patch 14:
  - added the code from patch 12 that checks for an existing
    multi-letter property. Add a kvm_enabled() check around it in
    preparation for the mock KVM properties patch later on
- patch 15 (new):
  - create satp_mode properties earlier to avoid an extra #ifndef
    CONFIG_USER_MODE block
- v5 link: https://lists.gnu.org/archive/html/qemu-devel/2023-06/msg05984.html

Daniel Henrique Barboza (20):
  target/riscv: skip features setup for KVM CPUs
  hw/riscv/virt.c: skip 'mmu-type' FDT if satp mode not set
  target/riscv/cpu.c: restrict 'mvendorid' value
  target/riscv/cpu.c: restrict 'mimpid' value
  target/riscv/cpu.c: restrict 'marchid' value
  target/riscv: use KVM scratch CPUs to init KVM properties
  target/riscv: read marchid/mimpid in kvm_riscv_init_machine_ids()
  target/riscv: handle mvendorid/marchid/mimpid for KVM CPUs
  linux-headers: Update to v6.4-rc1
  target/riscv/kvm.c: init 'misa_ext_mask' with scratch CPU
  target/riscv/cpu: add misa_ext_info_arr[]
  target/riscv: add KVM specific MISA properties
  target/riscv/kvm.c: update KVM MISA bits
  target/riscv/kvm.c: add multi-letter extension KVM properties
  target/riscv/cpu.c: add satp_mode properties earlier
  target/riscv/cpu.c: remove priv_ver check from riscv_isa_string_ext()
  target/riscv/cpu.c: create KVM mock properties
  target/riscv: update multi-letter extension KVM properties
  target/riscv/kvm.c: add kvmconfig_get_cfg_addr() helper
  target/riscv/kvm.c: read/write (cbom|cboz)_blocksize in KVM

 hw/riscv/virt.c                               |  14 +-
 include/standard-headers/linux/const.h        |   2 +-
 include/standard-headers/linux/virtio_blk.h   |  18 +-
 .../standard-headers/linux/virtio_config.h    |   6 +
 include/standard-headers/linux/virtio_net.h   |   1 +
 linux-headers/asm-arm64/kvm.h                 |  33 ++
 linux-headers/asm-riscv/kvm.h                 |  53 +-
 linux-headers/asm-riscv/unistd.h              |   9 +
 linux-headers/asm-s390/unistd_32.h            |   1 +
 linux-headers/asm-s390/unistd_64.h            |   1 +
 linux-headers/asm-x86/kvm.h                   |   3 +
 linux-headers/linux/const.h                   |   2 +-
 linux-headers/linux/kvm.h                     |  12 +-
 linux-headers/linux/psp-sev.h                 |   7 +
 linux-headers/linux/userfaultfd.h             |  17 +-
 target/riscv/cpu.c                            | 322 +++++++++--
 target/riscv/cpu.h                            |   7 +-
 target/riscv/kvm.c                            | 499 +++++++++++++++++-
 target/riscv/kvm_riscv.h                      |   1 +
 19 files changed, 917 insertions(+), 91 deletions(-)

-- 
2.41.0

[PATCH v6 01/20] target/riscv: skip features setup for KVM CPUs

[Daniel Henrique Barboza]

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
checking if the CSR_MCOUNTEREN, CSR_MCOUNTINHIBIT and CSR_MENVCFG CSRs
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 to 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_tcg() helper contains all validation logic that
are applicable to TCG CPUs only. riscv_cpu_realize() verifies if we're
running TCG and, if it's the case, proceed with the usual TCG realize()
logic.

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

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index fb8458bf74..bbb201a2b3 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -34,6 +34,7 @@
 #include "migration/vmstate.h"
 #include "fpu/softfloat-helpers.h"
 #include "sysemu/kvm.h"
+#include "sysemu/tcg.h"
 #include "kvm_riscv.h"
 #include "tcg/tcg.h"
 
@@ -1308,20 +1309,12 @@ static void riscv_cpu_validate_misa_priv(CPURISCVState *env, Error **errp)
     }
 }
 
-static void riscv_cpu_realize(DeviceState *dev, Error **errp)
+static void riscv_cpu_realize_tcg(DeviceState *dev, Error **errp)
 {
-    CPUState *cs = CPU(dev);
     RISCVCPU *cpu = RISCV_CPU(dev);
     CPURISCVState *env = &cpu->env;
-    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;
-    }
-
     riscv_cpu_validate_misa_mxl(cpu, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
@@ -1356,7 +1349,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
     }
 
 #ifndef CONFIG_USER_ONLY
-    cs->tcg_cflags |= CF_PCREL;
+    CPU(dev)->tcg_cflags |= CF_PCREL;
 
     if (cpu->cfg.ext_sstc) {
         riscv_timer_init(cpu);
@@ -1369,6 +1362,28 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         }
      }
 #endif
+}
+
+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 (tcg_enabled()) {
+        riscv_cpu_realize_tcg(dev, &local_err);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
 
     riscv_cpu_finalize_features(cpu, &local_err);
     if (local_err != NULL) {
-- 
2.41.0

[PATCH v6 02/20] hw/riscv/virt.c: skip 'mmu-type' FDT if satp mode not set

[Daniel Henrique Barboza]

The absence of a satp mode in riscv_host_cpu_init() is causing the
following 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
**
ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should not be
reached
Bail out! ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should
not be reached
Aborted

The error is triggered from create_fdt_socket_cpus() in hw/riscv/virt.c.
It's trying to get satp_mode_str for a NULL cpu->cfg.satp_mode.map.

For this KVM cpu we would need to inherit the satp supported modes
from the RISC-V host. At this moment this is not possible because the
KVM driver does not support it. And even when it does we can't just let
this broken for every other older kernel.

Since mmu-type is not a required node, according to [1], skip the
'mmu-type' FDT node if there's no satp_mode set. We'll revisit this
logic when we can get satp information from KVM.

[1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/cpu.yaml

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/riscv/virt.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 95708d890e..f025a0fcaf 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -243,13 +243,13 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
             s->soc[socket].hartid_base + cpu);
         qemu_fdt_add_subnode(ms->fdt, cpu_name);
 
-        satp_mode_max = satp_mode_max_from_map(
-            s->soc[socket].harts[cpu].cfg.satp_mode.map);
-        sv_name = g_strdup_printf("riscv,%s",
-                                  satp_mode_str(satp_mode_max, is_32_bit));
-        qemu_fdt_setprop_string(ms->fdt, cpu_name, "mmu-type", sv_name);
-        g_free(sv_name);
-
+        if (cpu_ptr->cfg.satp_mode.supported != 0) {
+            satp_mode_max = satp_mode_max_from_map(cpu_ptr->cfg.satp_mode.map);
+            sv_name = g_strdup_printf("riscv,%s",
+                                      satp_mode_str(satp_mode_max, is_32_bit));
+            qemu_fdt_setprop_string(ms->fdt, cpu_name, "mmu-type", sv_name);
+            g_free(sv_name);
+        }
 
         name = riscv_isa_string(cpu_ptr);
         qemu_fdt_setprop_string(ms->fdt, cpu_name, "riscv,isa", name);
-- 
2.41.0

[PATCH v6 03/20] target/riscv/cpu.c: restrict 'mvendorid' value

[Daniel Henrique Barboza]

We're going to change the handling of mvendorid/marchid/mimpid by the
KVM driver. Since these are always present in all CPUs let's put the
same validation for everyone.

It doesn't make sense to allow 'mvendorid' to be different than it
is already set in named (vendor) CPUs. Generic (dynamic) CPUs can have
any 'mvendorid' they want.

Change 'mvendorid' to be a class property created via
'object_class_property_add', instead of using the DEFINE_PROP_UINT32()
macro. This allow us to define a custom setter for it that will verify,
for named CPUs, if mvendorid is different than it is already set by the
CPU. This is the error thrown for the 'veyron-v1' CPU if 'mvendorid' is
set to an invalid value:

$ qemu-system-riscv64 -M virt -nographic -cpu veyron-v1,mvendorid=2
qemu-system-riscv64: can't apply global veyron-v1-riscv-cpu.mvendorid=2:
    Unable to change veyron-v1-riscv-cpu mvendorid (0x61f)

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index bbb201a2b3..652988db25 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1730,7 +1730,6 @@ static void riscv_cpu_add_user_properties(Object *obj)
 static Property riscv_cpu_properties[] = {
     DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
 
-    DEFINE_PROP_UINT32("mvendorid", RISCVCPU, cfg.mvendorid, 0),
     DEFINE_PROP_UINT64("marchid", RISCVCPU, cfg.marchid, RISCV_CPU_MARCHID),
     DEFINE_PROP_UINT64("mimpid", RISCVCPU, cfg.mimpid, RISCV_CPU_MIMPID),
 
@@ -1817,6 +1816,40 @@ static const struct TCGCPUOps riscv_tcg_ops = {
 #endif /* !CONFIG_USER_ONLY */
 };
 
+static bool riscv_cpu_is_dynamic(Object *cpu_obj)
+{
+    return object_dynamic_cast(cpu_obj, TYPE_RISCV_DYNAMIC_CPU) != NULL;
+}
+
+static void cpu_set_mvendorid(Object *obj, Visitor *v, const char *name,
+                              void *opaque, Error **errp)
+{
+    bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    uint32_t prev_val = cpu->cfg.mvendorid;
+    uint32_t value;
+
+    if (!visit_type_uint32(v, name, &value, errp)) {
+        return;
+    }
+
+    if (!dynamic_cpu && prev_val != value) {
+        error_setg(errp, "Unable to change %s mvendorid (0x%x)",
+                   object_get_typename(obj), prev_val);
+        return;
+    }
+
+    cpu->cfg.mvendorid = value;
+}
+
+static void cpu_get_mvendorid(Object *obj, Visitor *v, const char *name,
+                              void *opaque, Error **errp)
+{
+    bool value = RISCV_CPU(obj)->cfg.mvendorid;
+
+    visit_type_bool(v, name, &value, errp);
+}
+
 static void riscv_cpu_class_init(ObjectClass *c, void *data)
 {
     RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
@@ -1848,6 +1881,9 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
     cc->gdb_get_dynamic_xml = riscv_gdb_get_dynamic_xml;
     cc->tcg_ops = &riscv_tcg_ops;
 
+    object_class_property_add(c, "mvendorid", "uint32", cpu_get_mvendorid,
+                              cpu_set_mvendorid, NULL, NULL);
+
     device_class_set_props(dc, riscv_cpu_properties);
 }
 
-- 
2.41.0

[PATCH v6 04/20] target/riscv/cpu.c: restrict 'mimpid' value

[Daniel Henrique Barboza]

Following the same logic used with 'mvendorid' let's also restrict
'mimpid' for named CPUs. Generic CPUs keep setting the value freely.

Note that we're getting rid of the default RISCV_CPU_MARCHID value. The
reason is that this is not a good default since it's dynamic, changing
with with every QEMU version, regardless of whether the actual
implementation of the CPU changed from one QEMU version to the other.
Named CPU should set it to a meaningful value instead and generic CPUs
can set whatever they want.

This is the error thrown for an invalid 'mimpid' value for the veyron-v1
CPU:

$ ./qemu-system-riscv64 -M virt -nographic -cpu veyron-v1,mimpid=2
qemu-system-riscv64: can't apply global veyron-v1-riscv-cpu.mimpid=2:
    Unable to change veyron-v1-riscv-cpu mimpid (0x111)

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 652988db25..7ca7b7bbfa 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -43,7 +43,6 @@
 #define RISCV_CPU_MARCHID   ((QEMU_VERSION_MAJOR << 16) | \
                              (QEMU_VERSION_MINOR << 8)  | \
                              (QEMU_VERSION_MICRO))
-#define RISCV_CPU_MIMPID    RISCV_CPU_MARCHID
 
 static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
 
@@ -1731,7 +1730,6 @@ static Property riscv_cpu_properties[] = {
     DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
 
     DEFINE_PROP_UINT64("marchid", RISCVCPU, cfg.marchid, RISCV_CPU_MARCHID),
-    DEFINE_PROP_UINT64("mimpid", RISCVCPU, cfg.mimpid, RISCV_CPU_MIMPID),
 
 #ifndef CONFIG_USER_ONLY
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
@@ -1850,6 +1848,35 @@ static void cpu_get_mvendorid(Object *obj, Visitor *v, const char *name,
     visit_type_bool(v, name, &value, errp);
 }
 
+static void cpu_set_mimpid(Object *obj, Visitor *v, const char *name,
+                           void *opaque, Error **errp)
+{
+    bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    uint64_t prev_val = cpu->cfg.mimpid;
+    uint64_t value;
+
+    if (!visit_type_uint64(v, name, &value, errp)) {
+        return;
+    }
+
+    if (!dynamic_cpu && prev_val != value) {
+        error_setg(errp, "Unable to change %s mimpid (0x%lx)",
+                   object_get_typename(obj), prev_val);
+        return;
+    }
+
+    cpu->cfg.mimpid = value;
+}
+
+static void cpu_get_mimpid(Object *obj, Visitor *v, const char *name,
+                           void *opaque, Error **errp)
+{
+    bool value = RISCV_CPU(obj)->cfg.mimpid;
+
+    visit_type_bool(v, name, &value, errp);
+}
+
 static void riscv_cpu_class_init(ObjectClass *c, void *data)
 {
     RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
@@ -1884,6 +1911,9 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
     object_class_property_add(c, "mvendorid", "uint32", cpu_get_mvendorid,
                               cpu_set_mvendorid, NULL, NULL);
 
+    object_class_property_add(c, "mimpid", "uint64", cpu_get_mimpid,
+                              cpu_set_mimpid, NULL, NULL);
+
     device_class_set_props(dc, riscv_cpu_properties);
 }
 
-- 
2.41.0

[PATCH v6 05/20] target/riscv/cpu.c: restrict 'marchid' value

[Daniel Henrique Barboza]

'marchid' shouldn't be set to a different value as previously set for
named CPUs.

For all other CPUs it shouldn't be freely set either - the spec requires
that 'marchid' can't have the MSB (most significant bit) set and every
other bit set to zero, i.e. 0x80000000 is an invalid 'marchid' value for
32 bit CPUs.

As with 'mimpid', setting a default value based on the current QEMU
version is not a good idea because it implies that the CPU
implementation changes from one QEMU version to the other. Named CPUs
should set 'marchid' to a meaningful value instead, and generic CPUs can
set to any valid value.

For the 'veyron-v1' CPU this is the error thrown if 'marchid' is set to
a different val:

$ ./build/qemu-system-riscv64 -M virt -nographic -cpu veyron-v1,marchid=0x80000000
qemu-system-riscv64: can't apply global veyron-v1-riscv-cpu.marchid=0x80000000:
    Unable to change veyron-v1-riscv-cpu marchid (0x8000000000010000)

And, for generics CPUs, this is the error when trying to set to an
invalid val:

$ ./build/qemu-system-riscv64 -M virt -nographic -cpu rv64,marchid=0x8000000000000000
qemu-system-riscv64: can't apply global rv64-riscv-cpu.marchid=0x8000000000000000:
    Unable to set marchid with MSB (64) bit set and the remaining bits zero

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7ca7b7bbfa..51f055b8be 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -39,11 +39,6 @@
 #include "tcg/tcg.h"
 
 /* RISC-V CPU definitions */
-
-#define RISCV_CPU_MARCHID   ((QEMU_VERSION_MAJOR << 16) | \
-                             (QEMU_VERSION_MINOR << 8)  | \
-                             (QEMU_VERSION_MICRO))
-
 static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
 
 struct isa_ext_data {
@@ -1729,8 +1724,6 @@ static void riscv_cpu_add_user_properties(Object *obj)
 static Property riscv_cpu_properties[] = {
     DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
 
-    DEFINE_PROP_UINT64("marchid", RISCVCPU, cfg.marchid, RISCV_CPU_MARCHID),
-
 #ifndef CONFIG_USER_ONLY
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
 #endif
@@ -1877,6 +1870,56 @@ static void cpu_get_mimpid(Object *obj, Visitor *v, const char *name,
     visit_type_bool(v, name, &value, errp);
 }
 
+static void cpu_set_marchid(Object *obj, Visitor *v, const char *name,
+                            void *opaque, Error **errp)
+{
+    bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    uint64_t prev_val = cpu->cfg.marchid;
+    uint64_t value, invalid_val;
+    uint32_t mxlen = 0;
+
+    if (!visit_type_uint64(v, name, &value, errp)) {
+        return;
+    }
+
+    if (!dynamic_cpu && prev_val != value) {
+        error_setg(errp, "Unable to change %s marchid (0x%lx)",
+                   object_get_typename(obj), prev_val);
+        return;
+    }
+
+    switch (riscv_cpu_mxl(&cpu->env)) {
+    case MXL_RV32:
+        mxlen = 32;
+        break;
+    case MXL_RV64:
+    case MXL_RV128:
+        mxlen = 64;
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    invalid_val = 1LL << (mxlen - 1);
+
+    if (value == invalid_val) {
+        error_setg(errp, "Unable to set marchid with MSB (%u) bit set "
+                         "and the remaining bits zero", mxlen);
+        return;
+    }
+
+    cpu->cfg.marchid = value;
+}
+
+static void cpu_get_marchid(Object *obj, Visitor *v, const char *name,
+                           void *opaque, Error **errp)
+{
+    bool value = RISCV_CPU(obj)->cfg.marchid;
+
+    visit_type_bool(v, name, &value, errp);
+}
+
 static void riscv_cpu_class_init(ObjectClass *c, void *data)
 {
     RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
@@ -1914,6 +1957,9 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
     object_class_property_add(c, "mimpid", "uint64", cpu_get_mimpid,
                               cpu_set_mimpid, NULL, NULL);
 
+    object_class_property_add(c, "marchid", "uint64", cpu_get_marchid,
+                              cpu_set_marchid, NULL, NULL);
+
     device_class_set_props(dc, riscv_cpu_properties);
 }
 
-- 
2.41.0

[PATCH v6 06/20] target/riscv: use KVM scratch CPUs to init KVM properties

[Daniel Henrique Barboza]

Certain validations, such as the validations done for the machine IDs
(mvendorid/marchid/mimpid), are done before starting the CPU.
Non-dynamic (named) CPUs tries to match user input with a preset
default. As it is today we can't prefetch a KVM default for these cases
because we're only able to read/write KVM regs after the vcpu is
spinning.

Our target/arm friends use a concept called "scratch CPU", which
consists of creating a vcpu for doing queries and validations and so on,
which is discarded shortly after use [1]. This is a suitable solution
for what we need so let's implement it in target/riscv as well.

kvm_riscv_init_machine_ids() will be used to do any pre-launch setup for
KVM CPUs, via riscv_cpu_add_user_properties(). The function will create
a KVM scratch CPU, fetch KVM regs that work as default values for user
properties, and then discard the scratch CPU afterwards.

We're starting by initializing 'mvendorid'. This concept will be used to
init other KVM specific properties in the next patches as well.

[1] target/arm/kvm.c, kvm_arm_create_scratch_host_vcpu()

Suggested-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c       |  6 +++
 target/riscv/kvm.c       | 85 ++++++++++++++++++++++++++++++++++++++++
 target/riscv/kvm_riscv.h |  1 +
 3 files changed, 92 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 51f055b8be..2485e820f8 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1710,6 +1710,12 @@ static void riscv_cpu_add_user_properties(Object *obj)
     Property *prop;
     DeviceState *dev = DEVICE(obj);
 
+#ifndef CONFIG_USER_ONLY
+    if (kvm_enabled()) {
+        kvm_riscv_init_user_properties(obj);
+    }
+#endif
+
     riscv_cpu_add_misa_properties(obj);
 
     for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index 0f932a5b96..37f0f70794 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -309,6 +309,91 @@ static void kvm_riscv_put_regs_timer(CPUState *cs)
     env->kvm_timer_dirty = false;
 }
 
+typedef struct KVMScratchCPU {
+    int kvmfd;
+    int vmfd;
+    int cpufd;
+} KVMScratchCPU;
+
+/*
+ * Heavily inspired by kvm_arm_create_scratch_host_vcpu()
+ * from target/arm/kvm.c.
+ */
+static bool kvm_riscv_create_scratch_vcpu(KVMScratchCPU *scratch)
+{
+    int kvmfd = -1, vmfd = -1, cpufd = -1;
+
+    kvmfd = qemu_open_old("/dev/kvm", O_RDWR);
+    if (kvmfd < 0) {
+        goto err;
+    }
+    do {
+        vmfd = ioctl(kvmfd, KVM_CREATE_VM, 0);
+    } while (vmfd == -1 && errno == EINTR);
+    if (vmfd < 0) {
+        goto err;
+    }
+    cpufd = ioctl(vmfd, KVM_CREATE_VCPU, 0);
+    if (cpufd < 0) {
+        goto err;
+    }
+
+    scratch->kvmfd =  kvmfd;
+    scratch->vmfd = vmfd;
+    scratch->cpufd = cpufd;
+
+    return true;
+
+ err:
+    if (cpufd >= 0) {
+        close(cpufd);
+    }
+    if (vmfd >= 0) {
+        close(vmfd);
+    }
+    if (kvmfd >= 0) {
+        close(kvmfd);
+    }
+
+    return false;
+}
+
+static void kvm_riscv_destroy_scratch_vcpu(KVMScratchCPU *scratch)
+{
+    close(scratch->cpufd);
+    close(scratch->vmfd);
+    close(scratch->kvmfd);
+}
+
+static void kvm_riscv_init_machine_ids(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
+{
+    CPURISCVState *env = &cpu->env;
+    struct kvm_one_reg reg;
+    int ret;
+
+    reg.id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
+                              KVM_REG_RISCV_CONFIG_REG(mvendorid));
+    reg.addr = (uint64_t)&cpu->cfg.mvendorid;
+    ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
+    if (ret != 0) {
+        error_report("Unable to retrieve mvendorid from host, error %d", ret);
+    }
+}
+
+void kvm_riscv_init_user_properties(Object *cpu_obj)
+{
+    RISCVCPU *cpu = RISCV_CPU(cpu_obj);
+    KVMScratchCPU kvmcpu;
+
+    if (!kvm_riscv_create_scratch_vcpu(&kvmcpu)) {
+        return;
+    }
+
+    kvm_riscv_init_machine_ids(cpu, &kvmcpu);
+
+    kvm_riscv_destroy_scratch_vcpu(&kvmcpu);
+}
+
 const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
     KVM_CAP_LAST_INFO
 };
diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
index ed281bdce0..e3ba935808 100644
--- a/target/riscv/kvm_riscv.h
+++ b/target/riscv/kvm_riscv.h
@@ -19,6 +19,7 @@
 #ifndef QEMU_KVM_RISCV_H
 #define QEMU_KVM_RISCV_H
 
+void kvm_riscv_init_user_properties(Object *cpu_obj);
 void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
 void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);
 
-- 
2.41.0

[PATCH v6 07/20] target/riscv: read marchid/mimpid in kvm_riscv_init_machine_ids()

[Daniel Henrique Barboza]

Allow 'marchid' and 'mimpid' to also be initialized in
kvm_riscv_init_machine_ids().

After this change, the handling of mvendorid/marchid/mimpid for the
'host' CPU type will be equal to what we already have for TCG named
CPUs, i.e. the user is not able to set these values to a different val
than the one that is already preset.

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

diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index 37f0f70794..cd2974c663 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -378,6 +378,22 @@ static void kvm_riscv_init_machine_ids(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
     if (ret != 0) {
         error_report("Unable to retrieve mvendorid from host, error %d", ret);
     }
+
+    reg.id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
+                              KVM_REG_RISCV_CONFIG_REG(marchid));
+    reg.addr = (uint64_t)&cpu->cfg.marchid;
+    ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
+    if (ret != 0) {
+        error_report("Unable to retrieve marchid from host, error %d", ret);
+    }
+
+    reg.id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
+                              KVM_REG_RISCV_CONFIG_REG(mimpid));
+    reg.addr = (uint64_t)&cpu->cfg.mimpid;
+    ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
+    if (ret != 0) {
+        error_report("Unable to retrieve mimpid from host, error %d", ret);
+    }
 }
 
 void kvm_riscv_init_user_properties(Object *cpu_obj)
-- 
2.41.0

[PATCH v6 08/20] target/riscv: handle mvendorid/marchid/mimpid for KVM CPUs

[Daniel Henrique Barboza]

After changing user validation for mvendorid/marchid/mimpid to guarantee
that the value is validated on user input time, coupled with the work in
fetching KVM default values for them by using a scratch CPU, we're
certain that the values in cpu->cfg.(mvendorid|marchid|mimpid) are
already good to be written back to KVM.

There's no need to write the values back for 'host' type CPUs since the
values can't be changed, so let's do that just for generic CPUs.

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

diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index cd2974c663..602727cdfd 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -495,6 +495,33 @@ void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
 
+static int kvm_vcpu_set_machine_ids(RISCVCPU *cpu, CPUState *cs)
+{
+    CPURISCVState *env = &cpu->env;
+    uint64_t id;
+    int ret;
+
+    id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
+                          KVM_REG_RISCV_CONFIG_REG(mvendorid));
+    ret = kvm_set_one_reg(cs, id, &cpu->cfg.mvendorid);
+    if (ret != 0) {
+        return ret;
+    }
+
+    id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
+                          KVM_REG_RISCV_CONFIG_REG(marchid));
+    ret = kvm_set_one_reg(cs, id, &cpu->cfg.marchid);
+    if (ret != 0) {
+        return ret;
+    }
+
+    id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
+                          KVM_REG_RISCV_CONFIG_REG(mimpid));
+    ret = kvm_set_one_reg(cs, id, &cpu->cfg.mimpid);
+
+    return ret;
+}
+
 int kvm_arch_init_vcpu(CPUState *cs)
 {
     int ret = 0;
@@ -513,6 +540,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
     }
     env->misa_ext = isa;
 
+    if (!object_dynamic_cast(OBJECT(cpu), TYPE_RISCV_CPU_HOST)) {
+        ret = kvm_vcpu_set_machine_ids(cpu, cs);
+    }
+
     return ret;
 }
 
-- 
2.41.0

[PATCH v6 09/20] linux-headers: Update to v6.4-rc1

[Daniel Henrique Barboza]

Update to commit ac9a78681b92 ("Linux 6.4-rc1").

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
 include/standard-headers/linux/const.h        |  2 +-
 include/standard-headers/linux/virtio_blk.h   | 18 +++----
 .../standard-headers/linux/virtio_config.h    |  6 +++
 include/standard-headers/linux/virtio_net.h   |  1 +
 linux-headers/asm-arm64/kvm.h                 | 33 ++++++++++++
 linux-headers/asm-riscv/kvm.h                 | 53 ++++++++++++++++++-
 linux-headers/asm-riscv/unistd.h              |  9 ++++
 linux-headers/asm-s390/unistd_32.h            |  1 +
 linux-headers/asm-s390/unistd_64.h            |  1 +
 linux-headers/asm-x86/kvm.h                   |  3 ++
 linux-headers/linux/const.h                   |  2 +-
 linux-headers/linux/kvm.h                     | 12 +++--
 linux-headers/linux/psp-sev.h                 |  7 +++
 linux-headers/linux/userfaultfd.h             | 17 +++++-
 14 files changed, 149 insertions(+), 16 deletions(-)

diff --git a/include/standard-headers/linux/const.h b/include/standard-headers/linux/const.h
index 5e48987251..1eb84b5087 100644
--- a/include/standard-headers/linux/const.h
+++ b/include/standard-headers/linux/const.h
@@ -28,7 +28,7 @@
 #define _BITUL(x)	(_UL(1) << (x))
 #define _BITULL(x)	(_ULL(1) << (x))
 
-#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1)
+#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (__typeof__(x))(a) - 1)
 #define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
 
 #define __KERNEL_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
diff --git a/include/standard-headers/linux/virtio_blk.h b/include/standard-headers/linux/virtio_blk.h
index 7155b1a470..d7be3cf5e4 100644
--- a/include/standard-headers/linux/virtio_blk.h
+++ b/include/standard-headers/linux/virtio_blk.h
@@ -138,11 +138,11 @@ struct virtio_blk_config {
 
 	/* Zoned block device characteristics (if VIRTIO_BLK_F_ZONED) */
 	struct virtio_blk_zoned_characteristics {
-		uint32_t zone_sectors;
-		uint32_t max_open_zones;
-		uint32_t max_active_zones;
-		uint32_t max_append_sectors;
-		uint32_t write_granularity;
+		__virtio32 zone_sectors;
+		__virtio32 max_open_zones;
+		__virtio32 max_active_zones;
+		__virtio32 max_append_sectors;
+		__virtio32 write_granularity;
 		uint8_t model;
 		uint8_t unused2[3];
 	} zoned;
@@ -239,11 +239,11 @@ struct virtio_blk_outhdr {
  */
 struct virtio_blk_zone_descriptor {
 	/* Zone capacity */
-	uint64_t z_cap;
+	__virtio64 z_cap;
 	/* The starting sector of the zone */
-	uint64_t z_start;
+	__virtio64 z_start;
 	/* Zone write pointer position in sectors */
-	uint64_t z_wp;
+	__virtio64 z_wp;
 	/* Zone type */
 	uint8_t z_type;
 	/* Zone state */
@@ -252,7 +252,7 @@ struct virtio_blk_zone_descriptor {
 };
 
 struct virtio_blk_zone_report {
-	uint64_t nr_zones;
+	__virtio64 nr_zones;
 	uint8_t reserved[56];
 	struct virtio_blk_zone_descriptor zones[];
 };
diff --git a/include/standard-headers/linux/virtio_config.h b/include/standard-headers/linux/virtio_config.h
index 965ee6ae23..8a7d0dc8b0 100644
--- a/include/standard-headers/linux/virtio_config.h
+++ b/include/standard-headers/linux/virtio_config.h
@@ -97,6 +97,12 @@
  */
 #define VIRTIO_F_SR_IOV			37
 
+/*
+ * This feature indicates that the driver passes extra data (besides
+ * identifying the virtqueue) in its device notifications.
+ */
+#define VIRTIO_F_NOTIFICATION_DATA	38
+
 /*
  * This feature indicates that the driver can reset a queue individually.
  */
diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
index c0e797067a..2325485f2c 100644
--- a/include/standard-headers/linux/virtio_net.h
+++ b/include/standard-headers/linux/virtio_net.h
@@ -61,6 +61,7 @@
 #define VIRTIO_NET_F_GUEST_USO6	55	/* Guest can handle USOv6 in. */
 #define VIRTIO_NET_F_HOST_USO	56	/* Host can handle USO in. */
 #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports hash report */
+#define VIRTIO_NET_F_GUEST_HDRLEN  59	/* Guest provides the exact hdr_len value. */
 #define VIRTIO_NET_F_RSS	  60	/* Supports RSS RX steering */
 #define VIRTIO_NET_F_RSC_EXT	  61	/* extended coalescing info */
 #define VIRTIO_NET_F_STANDBY	  62	/* Act as standby for another device
diff --git a/linux-headers/asm-arm64/kvm.h b/linux-headers/asm-arm64/kvm.h
index d7e7bb885e..38e5957526 100644
--- a/linux-headers/asm-arm64/kvm.h
+++ b/linux-headers/asm-arm64/kvm.h
@@ -198,6 +198,15 @@ struct kvm_arm_copy_mte_tags {
 	__u64 reserved[2];
 };
 
+/*
+ * Counter/Timer offset structure. Describe the virtual/physical offset.
+ * To be used with KVM_ARM_SET_COUNTER_OFFSET.
+ */
+struct kvm_arm_counter_offset {
+	__u64 counter_offset;
+	__u64 reserved;
+};
+
 #define KVM_ARM_TAGS_TO_GUEST		0
 #define KVM_ARM_TAGS_FROM_GUEST		1
 
@@ -363,6 +372,10 @@ enum {
 	KVM_REG_ARM_VENDOR_HYP_BIT_PTP		= 1,
 };
 
+/* Device Control API on vm fd */
+#define KVM_ARM_VM_SMCCC_CTRL		0
+#define   KVM_ARM_VM_SMCCC_FILTER	0
+
 /* Device Control API: ARM VGIC */
 #define KVM_DEV_ARM_VGIC_GRP_ADDR	0
 #define KVM_DEV_ARM_VGIC_GRP_DIST_REGS	1
@@ -402,6 +415,8 @@ enum {
 #define KVM_ARM_VCPU_TIMER_CTRL		1
 #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
 #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
+#define   KVM_ARM_VCPU_TIMER_IRQ_HVTIMER	2
+#define   KVM_ARM_VCPU_TIMER_IRQ_HPTIMER	3
 #define KVM_ARM_VCPU_PVTIME_CTRL	2
 #define   KVM_ARM_VCPU_PVTIME_IPA	0
 
@@ -458,6 +473,24 @@ enum {
 /* run->fail_entry.hardware_entry_failure_reason codes. */
 #define KVM_EXIT_FAIL_ENTRY_CPU_UNSUPPORTED	(1ULL << 0)
 
+enum kvm_smccc_filter_action {
+	KVM_SMCCC_FILTER_HANDLE = 0,
+	KVM_SMCCC_FILTER_DENY,
+	KVM_SMCCC_FILTER_FWD_TO_USER,
+
+};
+
+struct kvm_smccc_filter {
+	__u32 base;
+	__u32 nr_functions;
+	__u8 action;
+	__u8 pad[15];
+};
+
+/* arm64-specific KVM_EXIT_HYPERCALL flags */
+#define KVM_HYPERCALL_EXIT_SMC		(1U << 0)
+#define KVM_HYPERCALL_EXIT_16BIT	(1U << 1)
+
 #endif
 
 #endif /* __ARM_KVM_H__ */
diff --git a/linux-headers/asm-riscv/kvm.h b/linux-headers/asm-riscv/kvm.h
index 92af6f3f05..f92790c948 100644
--- a/linux-headers/asm-riscv/kvm.h
+++ b/linux-headers/asm-riscv/kvm.h
@@ -12,6 +12,7 @@
 #ifndef __ASSEMBLY__
 
 #include <linux/types.h>
+#include <asm/bitsperlong.h>
 #include <asm/ptrace.h>
 
 #define __KVM_HAVE_READONLY_MEM
@@ -52,6 +53,7 @@ struct kvm_riscv_config {
 	unsigned long mvendorid;
 	unsigned long marchid;
 	unsigned long mimpid;
+	unsigned long zicboz_block_size;
 };
 
 /* CORE registers for KVM_GET_ONE_REG and KVM_SET_ONE_REG */
@@ -64,7 +66,7 @@ struct kvm_riscv_core {
 #define KVM_RISCV_MODE_S	1
 #define KVM_RISCV_MODE_U	0
 
-/* CSR registers for KVM_GET_ONE_REG and KVM_SET_ONE_REG */
+/* General CSR registers for KVM_GET_ONE_REG and KVM_SET_ONE_REG */
 struct kvm_riscv_csr {
 	unsigned long sstatus;
 	unsigned long sie;
@@ -78,6 +80,17 @@ struct kvm_riscv_csr {
 	unsigned long scounteren;
 };
 
+/* AIA CSR registers for KVM_GET_ONE_REG and KVM_SET_ONE_REG */
+struct kvm_riscv_aia_csr {
+	unsigned long siselect;
+	unsigned long iprio1;
+	unsigned long iprio2;
+	unsigned long sieh;
+	unsigned long siph;
+	unsigned long iprio1h;
+	unsigned long iprio2h;
+};
+
 /* TIMER registers for KVM_GET_ONE_REG and KVM_SET_ONE_REG */
 struct kvm_riscv_timer {
 	__u64 frequency;
@@ -105,9 +118,29 @@ enum KVM_RISCV_ISA_EXT_ID {
 	KVM_RISCV_ISA_EXT_SVINVAL,
 	KVM_RISCV_ISA_EXT_ZIHINTPAUSE,
 	KVM_RISCV_ISA_EXT_ZICBOM,
+	KVM_RISCV_ISA_EXT_ZICBOZ,
+	KVM_RISCV_ISA_EXT_ZBB,
+	KVM_RISCV_ISA_EXT_SSAIA,
 	KVM_RISCV_ISA_EXT_MAX,
 };
 
+/*
+ * SBI extension IDs specific to KVM. This is not the same as the SBI
+ * extension IDs defined by the RISC-V SBI specification.
+ */
+enum KVM_RISCV_SBI_EXT_ID {
+	KVM_RISCV_SBI_EXT_V01 = 0,
+	KVM_RISCV_SBI_EXT_TIME,
+	KVM_RISCV_SBI_EXT_IPI,
+	KVM_RISCV_SBI_EXT_RFENCE,
+	KVM_RISCV_SBI_EXT_SRST,
+	KVM_RISCV_SBI_EXT_HSM,
+	KVM_RISCV_SBI_EXT_PMU,
+	KVM_RISCV_SBI_EXT_EXPERIMENTAL,
+	KVM_RISCV_SBI_EXT_VENDOR,
+	KVM_RISCV_SBI_EXT_MAX,
+};
+
 /* Possible states for kvm_riscv_timer */
 #define KVM_RISCV_TIMER_STATE_OFF	0
 #define KVM_RISCV_TIMER_STATE_ON	1
@@ -118,6 +151,8 @@ enum KVM_RISCV_ISA_EXT_ID {
 /* If you need to interpret the index values, here is the key: */
 #define KVM_REG_RISCV_TYPE_MASK		0x00000000FF000000
 #define KVM_REG_RISCV_TYPE_SHIFT	24
+#define KVM_REG_RISCV_SUBTYPE_MASK	0x0000000000FF0000
+#define KVM_REG_RISCV_SUBTYPE_SHIFT	16
 
 /* Config registers are mapped as type 1 */
 #define KVM_REG_RISCV_CONFIG		(0x01 << KVM_REG_RISCV_TYPE_SHIFT)
@@ -131,8 +166,12 @@ enum KVM_RISCV_ISA_EXT_ID {
 
 /* Control and status registers are mapped as type 3 */
 #define KVM_REG_RISCV_CSR		(0x03 << KVM_REG_RISCV_TYPE_SHIFT)
+#define KVM_REG_RISCV_CSR_GENERAL	(0x0 << KVM_REG_RISCV_SUBTYPE_SHIFT)
+#define KVM_REG_RISCV_CSR_AIA		(0x1 << KVM_REG_RISCV_SUBTYPE_SHIFT)
 #define KVM_REG_RISCV_CSR_REG(name)	\
 		(offsetof(struct kvm_riscv_csr, name) / sizeof(unsigned long))
+#define KVM_REG_RISCV_CSR_AIA_REG(name)	\
+	(offsetof(struct kvm_riscv_aia_csr, name) / sizeof(unsigned long))
 
 /* Timer registers are mapped as type 4 */
 #define KVM_REG_RISCV_TIMER		(0x04 << KVM_REG_RISCV_TYPE_SHIFT)
@@ -152,6 +191,18 @@ enum KVM_RISCV_ISA_EXT_ID {
 /* ISA Extension registers are mapped as type 7 */
 #define KVM_REG_RISCV_ISA_EXT		(0x07 << KVM_REG_RISCV_TYPE_SHIFT)
 
+/* SBI extension registers are mapped as type 8 */
+#define KVM_REG_RISCV_SBI_EXT		(0x08 << KVM_REG_RISCV_TYPE_SHIFT)
+#define KVM_REG_RISCV_SBI_SINGLE	(0x0 << KVM_REG_RISCV_SUBTYPE_SHIFT)
+#define KVM_REG_RISCV_SBI_MULTI_EN	(0x1 << KVM_REG_RISCV_SUBTYPE_SHIFT)
+#define KVM_REG_RISCV_SBI_MULTI_DIS	(0x2 << KVM_REG_RISCV_SUBTYPE_SHIFT)
+#define KVM_REG_RISCV_SBI_MULTI_REG(__ext_id)	\
+		((__ext_id) / __BITS_PER_LONG)
+#define KVM_REG_RISCV_SBI_MULTI_MASK(__ext_id)	\
+		(1UL << ((__ext_id) % __BITS_PER_LONG))
+#define KVM_REG_RISCV_SBI_MULTI_REG_LAST	\
+		KVM_REG_RISCV_SBI_MULTI_REG(KVM_RISCV_SBI_EXT_MAX - 1)
+
 #endif
 
 #endif /* __LINUX_KVM_RISCV_H */
diff --git a/linux-headers/asm-riscv/unistd.h b/linux-headers/asm-riscv/unistd.h
index 73d7cdd2ec..950ab3fd44 100644
--- a/linux-headers/asm-riscv/unistd.h
+++ b/linux-headers/asm-riscv/unistd.h
@@ -43,3 +43,12 @@
 #define __NR_riscv_flush_icache (__NR_arch_specific_syscall + 15)
 #endif
 __SYSCALL(__NR_riscv_flush_icache, sys_riscv_flush_icache)
+
+/*
+ * Allows userspace to query the kernel for CPU architecture and
+ * microarchitecture details across a given set of CPUs.
+ */
+#ifndef __NR_riscv_hwprobe
+#define __NR_riscv_hwprobe (__NR_arch_specific_syscall + 14)
+#endif
+__SYSCALL(__NR_riscv_hwprobe, sys_riscv_hwprobe)
diff --git a/linux-headers/asm-s390/unistd_32.h b/linux-headers/asm-s390/unistd_32.h
index 8e644d65f5..800f3adb20 100644
--- a/linux-headers/asm-s390/unistd_32.h
+++ b/linux-headers/asm-s390/unistd_32.h
@@ -419,6 +419,7 @@
 #define __NR_landlock_create_ruleset 444
 #define __NR_landlock_add_rule 445
 #define __NR_landlock_restrict_self 446
+#define __NR_memfd_secret 447
 #define __NR_process_mrelease 448
 #define __NR_futex_waitv 449
 #define __NR_set_mempolicy_home_node 450
diff --git a/linux-headers/asm-s390/unistd_64.h b/linux-headers/asm-s390/unistd_64.h
index 51da542fec..399a605901 100644
--- a/linux-headers/asm-s390/unistd_64.h
+++ b/linux-headers/asm-s390/unistd_64.h
@@ -367,6 +367,7 @@
 #define __NR_landlock_create_ruleset 444
 #define __NR_landlock_add_rule 445
 #define __NR_landlock_restrict_self 446
+#define __NR_memfd_secret 447
 #define __NR_process_mrelease 448
 #define __NR_futex_waitv 449
 #define __NR_set_mempolicy_home_node 450
diff --git a/linux-headers/asm-x86/kvm.h b/linux-headers/asm-x86/kvm.h
index 2937e7bf69..2b3a8f7bd2 100644
--- a/linux-headers/asm-x86/kvm.h
+++ b/linux-headers/asm-x86/kvm.h
@@ -557,4 +557,7 @@ struct kvm_pmu_event_filter {
 #define KVM_VCPU_TSC_CTRL 0 /* control group for the timestamp counter (TSC) */
 #define   KVM_VCPU_TSC_OFFSET 0 /* attribute for the TSC offset */
 
+/* x86-specific KVM_EXIT_HYPERCALL flags. */
+#define KVM_EXIT_HYPERCALL_LONG_MODE	BIT(0)
+
 #endif /* _ASM_X86_KVM_H */
diff --git a/linux-headers/linux/const.h b/linux-headers/linux/const.h
index 5e48987251..1eb84b5087 100644
--- a/linux-headers/linux/const.h
+++ b/linux-headers/linux/const.h
@@ -28,7 +28,7 @@
 #define _BITUL(x)	(_UL(1) << (x))
 #define _BITULL(x)	(_ULL(1) << (x))
 
-#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1)
+#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (__typeof__(x))(a) - 1)
 #define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
 
 #define __KERNEL_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 599de3c6e3..65b145b317 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -341,8 +341,11 @@ struct kvm_run {
 			__u64 nr;
 			__u64 args[6];
 			__u64 ret;
-			__u32 longmode;
-			__u32 pad;
+
+			union {
+				__u32 longmode;
+				__u64 flags;
+			};
 		} hypercall;
 		/* KVM_EXIT_TPR_ACCESS */
 		struct {
@@ -1182,6 +1185,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_S390_PROTECTED_ASYNC_DISABLE 224
 #define KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP 225
 #define KVM_CAP_PMU_EVENT_MASKED_EVENTS 226
+#define KVM_CAP_COUNTER_OFFSET 227
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1449,7 +1453,7 @@ struct kvm_vfio_spapr_tce {
 #define KVM_CREATE_VCPU           _IO(KVMIO,   0x41)
 #define KVM_GET_DIRTY_LOG         _IOW(KVMIO,  0x42, struct kvm_dirty_log)
 #define KVM_SET_NR_MMU_PAGES      _IO(KVMIO,   0x44)
-#define KVM_GET_NR_MMU_PAGES      _IO(KVMIO,   0x45)
+#define KVM_GET_NR_MMU_PAGES      _IO(KVMIO,   0x45)  /* deprecated */
 #define KVM_SET_USER_MEMORY_REGION _IOW(KVMIO, 0x46, \
 					struct kvm_userspace_memory_region)
 #define KVM_SET_TSS_ADDR          _IO(KVMIO,   0x47)
@@ -1541,6 +1545,8 @@ struct kvm_s390_ucas_mapping {
 #define KVM_SET_PMU_EVENT_FILTER  _IOW(KVMIO,  0xb2, struct kvm_pmu_event_filter)
 #define KVM_PPC_SVM_OFF		  _IO(KVMIO,  0xb3)
 #define KVM_ARM_MTE_COPY_TAGS	  _IOR(KVMIO,  0xb4, struct kvm_arm_copy_mte_tags)
+/* Available with KVM_CAP_COUNTER_OFFSET */
+#define KVM_ARM_SET_COUNTER_OFFSET _IOW(KVMIO,  0xb5, struct kvm_arm_counter_offset)
 
 /* ioctl for vm fd */
 #define KVM_CREATE_DEVICE	  _IOWR(KVMIO,  0xe0, struct kvm_create_device)
diff --git a/linux-headers/linux/psp-sev.h b/linux-headers/linux/psp-sev.h
index 51d8b3940e..12ccb70099 100644
--- a/linux-headers/linux/psp-sev.h
+++ b/linux-headers/linux/psp-sev.h
@@ -36,6 +36,13 @@ enum {
  * SEV Firmware status code
  */
 typedef enum {
+	/*
+	 * This error code is not in the SEV spec. Its purpose is to convey that
+	 * there was an error that prevented the SEV firmware from being called.
+	 * The SEV API error codes are 16 bits, so the -1 value will not overlap
+	 * with possible values from the specification.
+	 */
+	SEV_RET_NO_FW_CALL = -1,
 	SEV_RET_SUCCESS = 0,
 	SEV_RET_INVALID_PLATFORM_STATE,
 	SEV_RET_INVALID_GUEST_STATE,
diff --git a/linux-headers/linux/userfaultfd.h b/linux-headers/linux/userfaultfd.h
index ba5d0df52f..14e402263a 100644
--- a/linux-headers/linux/userfaultfd.h
+++ b/linux-headers/linux/userfaultfd.h
@@ -38,7 +38,8 @@
 			   UFFD_FEATURE_MINOR_HUGETLBFS |	\
 			   UFFD_FEATURE_MINOR_SHMEM |		\
 			   UFFD_FEATURE_EXACT_ADDRESS |		\
-			   UFFD_FEATURE_WP_HUGETLBFS_SHMEM)
+			   UFFD_FEATURE_WP_HUGETLBFS_SHMEM |	\
+			   UFFD_FEATURE_WP_UNPOPULATED)
 #define UFFD_API_IOCTLS				\
 	((__u64)1 << _UFFDIO_REGISTER |		\
 	 (__u64)1 << _UFFDIO_UNREGISTER |	\
@@ -203,6 +204,12 @@ struct uffdio_api {
 	 *
 	 * UFFD_FEATURE_WP_HUGETLBFS_SHMEM indicates that userfaultfd
 	 * write-protection mode is supported on both shmem and hugetlbfs.
+	 *
+	 * UFFD_FEATURE_WP_UNPOPULATED indicates that userfaultfd
+	 * write-protection mode will always apply to unpopulated pages
+	 * (i.e. empty ptes).  This will be the default behavior for shmem
+	 * & hugetlbfs, so this flag only affects anonymous memory behavior
+	 * when userfault write-protection mode is registered.
 	 */
 #define UFFD_FEATURE_PAGEFAULT_FLAG_WP		(1<<0)
 #define UFFD_FEATURE_EVENT_FORK			(1<<1)
@@ -217,6 +224,7 @@ struct uffdio_api {
 #define UFFD_FEATURE_MINOR_SHMEM		(1<<10)
 #define UFFD_FEATURE_EXACT_ADDRESS		(1<<11)
 #define UFFD_FEATURE_WP_HUGETLBFS_SHMEM		(1<<12)
+#define UFFD_FEATURE_WP_UNPOPULATED		(1<<13)
 	__u64 features;
 
 	__u64 ioctls;
@@ -297,6 +305,13 @@ struct uffdio_writeprotect {
 struct uffdio_continue {
 	struct uffdio_range range;
 #define UFFDIO_CONTINUE_MODE_DONTWAKE		((__u64)1<<0)
+	/*
+	 * UFFDIO_CONTINUE_MODE_WP will map the page write protected on
+	 * the fly.  UFFDIO_CONTINUE_MODE_WP is available only if the
+	 * write protected ioctl is implemented for the range
+	 * according to the uffdio_register.ioctls.
+	 */
+#define UFFDIO_CONTINUE_MODE_WP			((__u64)1<<1)
 	__u64 mode;
 
 	/*
-- 
2.41.0

[PATCH v6 10/20] target/riscv/kvm.c: init 'misa_ext_mask' with scratch CPU

[Daniel Henrique Barboza]

At this moment we're retrieving env->misa_ext during
kvm_arch_init_cpu(), leaving env->misa_ext_mask behind.

We want to set env->misa_ext_mask, and we want to set it as early as
possible. The reason is that we're going to use it in the validation
process of the KVM MISA properties we're going to add next. Setting it
during arch_init_cpu() is too late for user validation.

Move the code to a new helper that is going to be called during init()
time, via kvm_riscv_init_user_properties(), like we're already doing for
the machine ID properties. Set both misa_ext and misa_ext_mask to the
same value retrieved by the 'isa' config reg.

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

diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index 602727cdfd..4d0808cb9a 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -396,6 +396,28 @@ static void kvm_riscv_init_machine_ids(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
     }
 }
 
+static void kvm_riscv_init_misa_ext_mask(RISCVCPU *cpu,
+                                         KVMScratchCPU *kvmcpu)
+{
+    CPURISCVState *env = &cpu->env;
+    struct kvm_one_reg reg;
+    int ret;
+
+    reg.id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
+                              KVM_REG_RISCV_CONFIG_REG(isa));
+    reg.addr = (uint64_t)&env->misa_ext_mask;
+    ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
+
+    if (ret) {
+        error_report("Unable to fetch ISA register from KVM, "
+                     "error %d", ret);
+        kvm_riscv_destroy_scratch_vcpu(kvmcpu);
+        exit(EXIT_FAILURE);
+    }
+
+    env->misa_ext = env->misa_ext_mask;
+}
+
 void kvm_riscv_init_user_properties(Object *cpu_obj)
 {
     RISCVCPU *cpu = RISCV_CPU(cpu_obj);
@@ -406,6 +428,7 @@ void kvm_riscv_init_user_properties(Object *cpu_obj)
     }
 
     kvm_riscv_init_machine_ids(cpu, &kvmcpu);
+    kvm_riscv_init_misa_ext_mask(cpu, &kvmcpu);
 
     kvm_riscv_destroy_scratch_vcpu(&kvmcpu);
 }
@@ -525,21 +548,10 @@ static int kvm_vcpu_set_machine_ids(RISCVCPU *cpu, CPUState *cs)
 int kvm_arch_init_vcpu(CPUState *cs)
 {
     int ret = 0;
-    target_ulong isa;
     RISCVCPU *cpu = RISCV_CPU(cs);
-    CPURISCVState *env = &cpu->env;
-    uint64_t id;
 
     qemu_add_vm_change_state_handler(kvm_riscv_vm_state_change, cs);
 
-    id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
-                          KVM_REG_RISCV_CONFIG_REG(isa));
-    ret = kvm_get_one_reg(cs, id, &isa);
-    if (ret) {
-        return ret;
-    }
-    env->misa_ext = isa;
-
     if (!object_dynamic_cast(OBJECT(cpu), TYPE_RISCV_CPU_HOST)) {
         ret = kvm_vcpu_set_machine_ids(cpu, cs);
     }
-- 
2.41.0

[PATCH v6 11/20] target/riscv/cpu: add misa_ext_info_arr[]

[Daniel Henrique Barboza]

Next patch will add KVM specific user properties for both MISA and
multi-letter extensions. For MISA extensions we want to make use of what
is already available in misa_ext_cfgs[] to avoid code repetition.

misa_ext_info_arr[] array will hold name and description for each MISA
extension that misa_ext_cfgs[] is declaring. We'll then use this new
array in KVM code to avoid duplicating strings.

There's nothing holding us back from doing the same with multi-letter
extensions. For now doing just with MISA extensions is enough.

Suggested-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++----------------
 target/riscv/cpu.h |  7 +++-
 2 files changed, 61 insertions(+), 29 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 2485e820f8..90dd2078ae 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
     visit_type_bool(v, name, &value, errp);
 }
 
-static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
-    {.name = "a", .description = "Atomic instructions",
-     .misa_bit = RVA, .enabled = true},
-    {.name = "c", .description = "Compressed instructions",
-     .misa_bit = RVC, .enabled = true},
-    {.name = "d", .description = "Double-precision float point",
-     .misa_bit = RVD, .enabled = true},
-    {.name = "f", .description = "Single-precision float point",
-     .misa_bit = RVF, .enabled = true},
-    {.name = "i", .description = "Base integer instruction set",
-     .misa_bit = RVI, .enabled = true},
-    {.name = "e", .description = "Base integer instruction set (embedded)",
-     .misa_bit = RVE, .enabled = false},
-    {.name = "m", .description = "Integer multiplication and division",
-     .misa_bit = RVM, .enabled = true},
-    {.name = "s", .description = "Supervisor-level instructions",
-     .misa_bit = RVS, .enabled = true},
-    {.name = "u", .description = "User-level instructions",
-     .misa_bit = RVU, .enabled = true},
-    {.name = "h", .description = "Hypervisor",
-     .misa_bit = RVH, .enabled = true},
-    {.name = "x-j", .description = "Dynamic translated languages",
-     .misa_bit = RVJ, .enabled = false},
-    {.name = "v", .description = "Vector operations",
-     .misa_bit = RVV, .enabled = false},
-    {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)",
-     .misa_bit = RVG, .enabled = false},
+typedef struct misa_ext_info {
+    const char *name;
+    const char *description;
+} MISAExtInfo;
+
+#define MISA_EXT_INFO(_idx, _propname, _descr) \
+    [(_idx - 'A')] = {.name = _propname, .description = _descr}
+
+static const MISAExtInfo misa_ext_info_arr[] = {
+    MISA_EXT_INFO('A', "a", "Atomic instructions"),
+    MISA_EXT_INFO('C', "c", "Compressed instructions"),
+    MISA_EXT_INFO('D', "d", "Double-precision float point"),
+    MISA_EXT_INFO('F', "f", "Single-precision float point"),
+    MISA_EXT_INFO('I', "i", "Base integer instruction set"),
+    MISA_EXT_INFO('E', "e", "Base integer instruction set (embedded)"),
+    MISA_EXT_INFO('M', "m", "Integer multiplication and division"),
+    MISA_EXT_INFO('S', "s", "Supervisor-level instructions"),
+    MISA_EXT_INFO('U', "u", "User-level instructions"),
+    MISA_EXT_INFO('H', "h", "Hypervisor"),
+    MISA_EXT_INFO('J', "x-j", "Dynamic translated languages"),
+    MISA_EXT_INFO('V', "v", "Vector operations"),
+    MISA_EXT_INFO('G', "g", "General purpose (IMAFD_Zicsr_Zifencei)"),
+};
+
+const char *riscv_get_misa_ext_name(uint32_t bit)
+{
+    return misa_ext_info_arr[ctz32(bit)].name;
+}
+
+const char *riscv_get_misa_ext_descr(uint32_t bit)
+{
+    return misa_ext_info_arr[ctz32(bit)].description;
+}
+
+#define MISA_CFG(_bit, _enabled) \
+    {.misa_bit = _bit, .enabled = _enabled}
+
+static RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
+    MISA_CFG(RVA, true),
+    MISA_CFG(RVC, true),
+    MISA_CFG(RVD, true),
+    MISA_CFG(RVF, true),
+    MISA_CFG(RVI, true),
+    MISA_CFG(RVE, false),
+    MISA_CFG(RVM, true),
+    MISA_CFG(RVS, true),
+    MISA_CFG(RVU, true),
+    MISA_CFG(RVH, true),
+    MISA_CFG(RVJ, false),
+    MISA_CFG(RVV, false),
+    MISA_CFG(RVG, false),
 };
 
 static void riscv_cpu_add_misa_properties(Object *cpu_obj)
@@ -1592,7 +1616,10 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
     int i;
 
     for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
-        const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
+        RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
+
+        misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit);
+        misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit);
 
         object_property_add(cpu_obj, misa_cfg->name, "bool",
                             cpu_get_misa_ext_cfg,
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index cc20ee25a7..acae118b9b 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -41,7 +41,10 @@
 
 #define RV(x) ((target_ulong)1 << (x - 'A'))
 
-/* Consider updating misa_ext_cfgs[] when adding new MISA bits here */
+/*
+ * Consider updating misa_ext_info_arr[] and misa_ext_cfgs[]
+ * when adding new MISA bits here.
+ */
 #define RVI RV('I')
 #define RVE RV('E') /* E and I are mutually exclusive */
 #define RVM RV('M')
@@ -56,6 +59,8 @@
 #define RVJ RV('J')
 #define RVG RV('G')
 
+const char *riscv_get_misa_ext_name(uint32_t bit);
+const char *riscv_get_misa_ext_descr(uint32_t bit);
 
 /* Privileged specification version */
 enum {
-- 
2.41.0

[PATCH v6 12/20] target/riscv: add KVM specific MISA properties

[Daniel Henrique Barboza]

Using all TCG user properties in KVM is tricky. First because KVM
supports only a small subset of what TCG provides, so most of the
cpu->cfg flags do nothing for KVM.

Second, and more important, we don't have a way of telling if any given
value is an user input or not. For TCG this has a small impact since we
just validating everything and error out if needed. But for KVM it would
be good to know if a given value was set by the user or if it's a value
already provided by KVM. Otherwise we don't know how to handle failed
kvm_set_one_regs() when writing the configurations back.

These characteristics make it overly complicated to use the same user
facing flags for both KVM and TCG. A simpler approach is to create KVM
specific properties that have specialized logic, forking KVM and TCG use
cases for those cases only. Fully separating KVM/TCG properties is
unneeded at this point - in fact we want the user experience to be as
equal as possible, regardless of the acceleration chosen.

We'll start this fork with the MISA properties, adding the MISA bits
that the KVM driver currently supports. A new KVMCPUConfig type is
introduced. It'll hold general information about an extension. For MISA
extensions we're going to use the newly created getters of
misa_ext_infos[] to populate their name and description. 'offset' holds
the MISA bit (RVA, RVC, ...). We're calling it 'offset' instead of
'misa_bit' because this same KVMCPUConfig struct will be used to
multi-letter extensions later on.

This new type also holds a 'user_set' flag. This flag will be set when
the user set an option that's different than what is already configured
in the host, requiring KVM intervention to write the regs back during
kvm_arch_init_vcpu(). Similar mechanics will be implemented for
multi-letter extensions as well.

There is no need to duplicate more code than necessary, so we're going
to use the existing kvm_riscv_init_user_properties() to add the KVM
specific properties. Any code that is adding a TCG user prop is then
changed slightly to verify first if there's a KVM prop with the same
name already added.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c | 13 +++++---
 target/riscv/kvm.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 90dd2078ae..f4b1868466 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1617,14 +1617,19 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
 
     for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
         RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
+        Error *local_err = NULL;
 
         misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit);
         misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit);
 
-        object_property_add(cpu_obj, misa_cfg->name, "bool",
-                            cpu_get_misa_ext_cfg,
-                            cpu_set_misa_ext_cfg,
-                            NULL, (void *)misa_cfg);
+        object_property_try_add(cpu_obj, misa_cfg->name, "bool",
+                                cpu_get_misa_ext_cfg, cpu_set_misa_ext_cfg,
+                                NULL, (void *)misa_cfg, &local_err);
+        if (local_err) {
+            /* Someone (KVM) already created the property */
+            continue;
+        }
+
         object_property_set_description(cpu_obj, misa_cfg->name,
                                         misa_cfg->description);
         object_property_set_bool(cpu_obj, misa_cfg->name,
diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index 4d0808cb9a..0fb63cced3 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -22,8 +22,10 @@
 #include <linux/kvm.h>
 
 #include "qemu/timer.h"
+#include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
+#include "qapi/visitor.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "sysemu/kvm_int.h"
@@ -105,6 +107,81 @@ static uint64_t kvm_riscv_reg_id(CPURISCVState *env, uint64_t type,
         } \
     } while (0)
 
+typedef struct KVMCPUConfig {
+    const char *name;
+    const char *description;
+    target_ulong offset;
+    int kvm_reg_id;
+    bool user_set;
+} KVMCPUConfig;
+
+#define KVM_MISA_CFG(_bit, _reg_id) \
+    {.offset = _bit, .kvm_reg_id = _reg_id}
+
+/* KVM ISA extensions */
+static KVMCPUConfig kvm_misa_ext_cfgs[] = {
+    KVM_MISA_CFG(RVA, KVM_RISCV_ISA_EXT_A),
+    KVM_MISA_CFG(RVC, KVM_RISCV_ISA_EXT_C),
+    KVM_MISA_CFG(RVD, KVM_RISCV_ISA_EXT_D),
+    KVM_MISA_CFG(RVF, KVM_RISCV_ISA_EXT_F),
+    KVM_MISA_CFG(RVH, KVM_RISCV_ISA_EXT_H),
+    KVM_MISA_CFG(RVI, KVM_RISCV_ISA_EXT_I),
+    KVM_MISA_CFG(RVM, KVM_RISCV_ISA_EXT_M),
+};
+
+static void kvm_cpu_set_misa_ext_cfg(Object *obj, Visitor *v,
+                                     const char *name,
+                                     void *opaque, Error **errp)
+{
+    KVMCPUConfig *misa_ext_cfg = opaque;
+    target_ulong misa_bit = misa_ext_cfg->offset;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    CPURISCVState *env = &cpu->env;
+    bool value, host_bit;
+
+    if (!visit_type_bool(v, name, &value, errp)) {
+        return;
+    }
+
+    host_bit = env->misa_ext_mask & misa_bit;
+
+    if (value == host_bit) {
+        return;
+    }
+
+    if (!value) {
+        misa_ext_cfg->user_set = true;
+        return;
+    }
+
+    /*
+     * Forbid users to enable extensions that aren't
+     * available in the hart.
+     */
+    error_setg(errp, "Enabling MISA bit '%s' is not allowed: it's not "
+               "enabled in the host", misa_ext_cfg->name);
+}
+
+static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(kvm_misa_ext_cfgs); i++) {
+        KVMCPUConfig *misa_cfg = &kvm_misa_ext_cfgs[i];
+        int bit = misa_cfg->offset;
+
+        misa_cfg->name = riscv_get_misa_ext_name(bit);
+        misa_cfg->description = riscv_get_misa_ext_descr(bit);
+
+        object_property_add(cpu_obj, misa_cfg->name, "bool",
+                            NULL,
+                            kvm_cpu_set_misa_ext_cfg,
+                            NULL, misa_cfg);
+        object_property_set_description(cpu_obj, misa_cfg->name,
+                                        misa_cfg->description);
+    }
+}
+
 static int kvm_riscv_get_regs_core(CPUState *cs)
 {
     int ret = 0;
@@ -427,6 +504,7 @@ void kvm_riscv_init_user_properties(Object *cpu_obj)
         return;
     }
 
+    kvm_riscv_add_cpu_user_properties(cpu_obj);
     kvm_riscv_init_machine_ids(cpu, &kvmcpu);
     kvm_riscv_init_misa_ext_mask(cpu, &kvmcpu);
 
-- 
2.41.0

[PATCH v6 13/20] target/riscv/kvm.c: update KVM MISA bits

[Daniel Henrique Barboza]

Our design philosophy with KVM properties can be resumed in two main
decisions based on KVM interface availability and what the user wants to
do:

- if the user disables an extension that the host KVM module doesn't
know about (i.e. it doesn't implement the kvm_get_one_reg() interface),
keep booting the CPU. This will avoid users having to deal with issues
with older KVM versions while disabling features they don't care;

- for any other case we're going to error out immediately. If the user
wants to enable a feature that KVM doesn't know about this a problem that
is worth aborting - the user must know that the feature wasn't enabled
in the hart. Likewise, if KVM knows about the extension, the user wants
to enable/disable it, and we fail to do it so, that's also a problem we
can't shrug it off.

In the case of MISA bits we won't even try enabling bits that aren't
already available in the host. The ioctl() is so likely to fail that
it's not worth trying. This check is already done in the previous patch,
in kvm_cpu_set_misa_ext_cfg(), thus we don't need to worry about it now.

In kvm_riscv_update_cpu_misa_ext() we'll go through every potential user
option and do as follows:

- if the user didn't set the property or set to the same value of the
host, do nothing;

- Disable the given extension in KVM. Error out if anything goes wrong.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/kvm.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index 0fb63cced3..ba97b0cbed 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -162,6 +162,41 @@ static void kvm_cpu_set_misa_ext_cfg(Object *obj, Visitor *v,
                "enabled in the host", misa_ext_cfg->name);
 }
 
+static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs)
+{
+    CPURISCVState *env = &cpu->env;
+    uint64_t id, reg;
+    int i, ret;
+
+    for (i = 0; i < ARRAY_SIZE(kvm_misa_ext_cfgs); i++) {
+        KVMCPUConfig *misa_cfg = &kvm_misa_ext_cfgs[i];
+        target_ulong misa_bit = misa_cfg->offset;
+
+        if (!misa_cfg->user_set) {
+            continue;
+        }
+
+        /* If we're here we're going to disable the MISA bit */
+        reg = 0;
+        id = kvm_riscv_reg_id(env, KVM_REG_RISCV_ISA_EXT,
+                              misa_cfg->kvm_reg_id);
+        ret = kvm_set_one_reg(cs, id, &reg);
+        if (ret != 0) {
+            /*
+             * We're not checking for -EINVAL because if the bit is about
+             * to be disabled, it means that it was already enabled by
+             * KVM. We determined that by fetching the 'isa' register
+             * during init() time. Any error at this point is worth
+             * aborting.
+             */
+            error_report("Unable to set KVM reg %s, error %d",
+                         misa_cfg->name, ret);
+            exit(EXIT_FAILURE);
+        }
+        env->misa_ext &= ~misa_bit;
+    }
+}
+
 static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
 {
     int i;
@@ -632,8 +667,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
 
     if (!object_dynamic_cast(OBJECT(cpu), TYPE_RISCV_CPU_HOST)) {
         ret = kvm_vcpu_set_machine_ids(cpu, cs);
+        if (ret != 0) {
+            return ret;
+        }
     }
 
+    kvm_riscv_update_cpu_misa_ext(cpu, cs);
+
     return ret;
 }
 
-- 
2.41.0

[PATCH v6 14/20] target/riscv/kvm.c: add multi-letter extension KVM properties

[Daniel Henrique Barboza]

Let's add KVM user properties for the multi-letter extensions that KVM
currently supports: zicbom, zicboz, zihintpause, zbb, ssaia, sstc,
svinval and svpbmt.

As with MISA extensions, we're using the KVMCPUConfig type to hold
information about the state of each extension. However, multi-letter
extensions have more cases to cover than MISA extensions, so we're
adding an extra 'supported' flag as well. This flag will reflect if a
given extension is supported by KVM, i.e. KVM knows how to handle it.
This is determined during KVM extension discovery in
kvm_riscv_init_multiext_cfg(), where we test for EINVAL errors. Any
other error different from EINVAL will cause an abort.

The use of the 'user_set' is similar to what we already do with MISA
extensions: the flag set only if the user is changing the extension
state.

The 'supported' flag will be used later on to make an exception for
users that are disabling multi-letter extensions that are unknown to
KVM.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c |   8 +++
 target/riscv/kvm.c | 119 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 127 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f4b1868466..5428402cfa 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1751,6 +1751,14 @@ static void riscv_cpu_add_user_properties(Object *obj)
     riscv_cpu_add_misa_properties(obj);
 
     for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
+#ifndef CONFIG_USER_ONLY
+        if (kvm_enabled()) {
+            /* Check if KVM created the property already */
+            if (object_property_find(obj, prop->name)) {
+                continue;
+            }
+        }
+#endif
         qdev_property_add_static(dev, prop);
     }
 
diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index ba97b0cbed..68f6538b04 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -113,6 +113,7 @@ typedef struct KVMCPUConfig {
     target_ulong offset;
     int kvm_reg_id;
     bool user_set;
+    bool supported;
 } KVMCPUConfig;
 
 #define KVM_MISA_CFG(_bit, _reg_id) \
@@ -197,6 +198,81 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs)
     }
 }
 
+#define CPUCFG(_prop) offsetof(struct RISCVCPUConfig, _prop)
+
+#define KVM_EXT_CFG(_name, _prop, _reg_id) \
+    {.name = _name, .offset = CPUCFG(_prop), \
+     .kvm_reg_id = _reg_id}
+
+static KVMCPUConfig kvm_multi_ext_cfgs[] = {
+    KVM_EXT_CFG("zicbom", ext_icbom, KVM_RISCV_ISA_EXT_ZICBOM),
+    KVM_EXT_CFG("zicboz", ext_icboz, KVM_RISCV_ISA_EXT_ZICBOZ),
+    KVM_EXT_CFG("zihintpause", ext_zihintpause, KVM_RISCV_ISA_EXT_ZIHINTPAUSE),
+    KVM_EXT_CFG("zbb", ext_zbb, KVM_RISCV_ISA_EXT_ZBB),
+    KVM_EXT_CFG("ssaia", ext_ssaia, KVM_RISCV_ISA_EXT_SSAIA),
+    KVM_EXT_CFG("sstc", ext_sstc, KVM_RISCV_ISA_EXT_SSTC),
+    KVM_EXT_CFG("svinval", ext_svinval, KVM_RISCV_ISA_EXT_SVINVAL),
+    KVM_EXT_CFG("svpbmt", ext_svpbmt, KVM_RISCV_ISA_EXT_SVPBMT),
+};
+
+static void kvm_cpu_cfg_set(RISCVCPU *cpu, KVMCPUConfig *multi_ext,
+                            uint32_t val)
+{
+    int cpu_cfg_offset = multi_ext->offset;
+    bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
+
+    *ext_enabled = val;
+}
+
+static uint32_t kvm_cpu_cfg_get(RISCVCPU *cpu,
+                                KVMCPUConfig *multi_ext)
+{
+    int cpu_cfg_offset = multi_ext->offset;
+    bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
+
+    return *ext_enabled;
+}
+
+static void kvm_cpu_set_multi_ext_cfg(Object *obj, Visitor *v,
+                                      const char *name,
+                                      void *opaque, Error **errp)
+{
+    KVMCPUConfig *multi_ext_cfg = opaque;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    bool value, host_val;
+
+    if (!visit_type_bool(v, name, &value, errp)) {
+        return;
+    }
+
+    host_val = kvm_cpu_cfg_get(cpu, multi_ext_cfg);
+
+    /*
+     * Ignore if the user is setting the same value
+     * as the host.
+     */
+    if (value == host_val) {
+        return;
+    }
+
+    if (!multi_ext_cfg->supported) {
+        /*
+         * Error out if the user is trying to enable an
+         * extension that KVM doesn't support. Ignore
+         * option otherwise.
+         */
+        if (value) {
+            error_setg(errp, "KVM does not support disabling extension %s",
+                       multi_ext_cfg->name);
+        }
+
+        return;
+    }
+
+    multi_ext_cfg->user_set = true;
+    kvm_cpu_cfg_set(cpu, multi_ext_cfg, value);
+}
+
 static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
 {
     int i;
@@ -215,6 +291,15 @@ static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
         object_property_set_description(cpu_obj, misa_cfg->name,
                                         misa_cfg->description);
     }
+
+    for (i = 0; i < ARRAY_SIZE(kvm_multi_ext_cfgs); i++) {
+        KVMCPUConfig *multi_cfg = &kvm_multi_ext_cfgs[i];
+
+        object_property_add(cpu_obj, multi_cfg->name, "bool",
+                            NULL,
+                            kvm_cpu_set_multi_ext_cfg,
+                            NULL, multi_cfg);
+    }
 }
 
 static int kvm_riscv_get_regs_core(CPUState *cs)
@@ -530,6 +615,39 @@ static void kvm_riscv_init_misa_ext_mask(RISCVCPU *cpu,
     env->misa_ext = env->misa_ext_mask;
 }
 
+static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
+{
+    CPURISCVState *env = &cpu->env;
+    uint64_t val;
+    int i, ret;
+
+    for (i = 0; i < ARRAY_SIZE(kvm_multi_ext_cfgs); i++) {
+        KVMCPUConfig *multi_ext_cfg = &kvm_multi_ext_cfgs[i];
+        struct kvm_one_reg reg;
+
+        reg.id = kvm_riscv_reg_id(env, KVM_REG_RISCV_ISA_EXT,
+                                  multi_ext_cfg->kvm_reg_id);
+        reg.addr = (uint64_t)&val;
+        ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
+        if (ret != 0) {
+            if (ret == -EINVAL) {
+                /* Silently default to 'false' if KVM does not support it. */
+                multi_ext_cfg->supported = false;
+                val = false;
+            } else {
+                error_report("Unable to read ISA_EXT KVM register %s, "
+                             "error %d", multi_ext_cfg->name, ret);
+                kvm_riscv_destroy_scratch_vcpu(kvmcpu);
+                exit(EXIT_FAILURE);
+            }
+        } else {
+            multi_ext_cfg->supported = true;
+        }
+
+        kvm_cpu_cfg_set(cpu, multi_ext_cfg, val);
+    }
+}
+
 void kvm_riscv_init_user_properties(Object *cpu_obj)
 {
     RISCVCPU *cpu = RISCV_CPU(cpu_obj);
@@ -542,6 +660,7 @@ void kvm_riscv_init_user_properties(Object *cpu_obj)
     kvm_riscv_add_cpu_user_properties(cpu_obj);
     kvm_riscv_init_machine_ids(cpu, &kvmcpu);
     kvm_riscv_init_misa_ext_mask(cpu, &kvmcpu);
+    kvm_riscv_init_multiext_cfg(cpu, &kvmcpu);
 
     kvm_riscv_destroy_scratch_vcpu(&kvmcpu);
 }
-- 
2.41.0

[PATCH v6 15/20] target/riscv/cpu.c: add satp_mode properties earlier

[Daniel Henrique Barboza]

riscv_cpu_add_user_properties() ended up with an excess of "#ifndef
CONFIG_USER_ONLY" blocks after changes that added KVM properties
handling.

KVM specific properties are required to be created earlier than their
TCG counterparts, but the remaining props can be created at any order.
Move riscv_add_satp_mode_properties() to the start of the function,
inside the !CONFIG_USER_ONLY block already present there, to remove the
last ifndef block.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 5428402cfa..b4a6fd8bab 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1743,6 +1743,8 @@ static void riscv_cpu_add_user_properties(Object *obj)
     DeviceState *dev = DEVICE(obj);
 
 #ifndef CONFIG_USER_ONLY
+    riscv_add_satp_mode_properties(obj);
+
     if (kvm_enabled()) {
         kvm_riscv_init_user_properties(obj);
     }
@@ -1761,10 +1763,6 @@ static void riscv_cpu_add_user_properties(Object *obj)
 #endif
         qdev_property_add_static(dev, prop);
     }
-
-#ifndef CONFIG_USER_ONLY
-    riscv_add_satp_mode_properties(obj);
-#endif
 }
 
 static Property riscv_cpu_properties[] = {
-- 
2.41.0

[PATCH v6 16/20] target/riscv/cpu.c: remove priv_ver check from riscv_isa_string_ext()

[Daniel Henrique Barboza]

riscv_isa_string_ext() is being used by riscv_isa_string(), which is
then used by boards to retrieve the 'riscv,isa' string to be written in
the FDT. All this happens after riscv_cpu_realize(), meaning that we're
already past riscv_cpu_validate_set_extensions() and, more important,
riscv_cpu_disable_priv_spec_isa_exts().

This means that all extensions that needed to be disabled due to
priv_spec mismatch are already disabled. Checking this again during
riscv_isa_string_ext() is unneeded. Remove it.

As a bonus, riscv_isa_string_ext() can now be used with the 'host'
KVM-only CPU type since it doesn't have a env->priv_ver assigned and it
would fail this check for no good reason.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/cpu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index b4a6fd8bab..79c8ffe6b7 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -2015,8 +2015,7 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
     int i;
 
     for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
-        if (cpu->env.priv_ver >= isa_edata_arr[i].min_version &&
-            isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
+        if (isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
             new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
             g_free(old);
             old = new;
-- 
2.41.0

[PATCH v6 17/20] target/riscv/cpu.c: create KVM mock properties

[Daniel Henrique Barboza]

KVM-specific properties are being created inside target/riscv/kvm.c. But
at this moment we're gathering all the remaining properties from TCG and
adding them as is when running KVM. This creates a situation where
non-KVM properties are setting flags to 'true' due to its default
settings (e.g.  Zawrs). Users can also freely enable them via command
line.

This doesn't impact runtime per se because KVM doesn't care about these
flags, but code such as riscv_isa_string_ext() take those flags into
account. The result is that, for a KVM guest, setting non-KVM properties
will make them appear in the riscv,isa DT.

We want to keep the same API for both TCG and KVM and at the same time,
when running KVM, forbid non-KVM extensions to be enabled internally. We
accomplish both by changing riscv_cpu_add_user_properties() to add a
mock boolean property for every non-KVM extension in
riscv_cpu_extensions[]. Then, when running KVM, users are still free to
set extensions at will, but we'll error out if a non-KVM extension is
enabled. Setting such extension to 'false' will be ignored.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 79c8ffe6b7..6d7a0bc4ae 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1731,6 +1731,26 @@ static Property riscv_cpu_extensions[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+
+#ifndef CONFIG_USER_ONLY
+static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
+                                    const char *name,
+                                    void *opaque, Error **errp)
+{
+    const char *propname = opaque;
+    bool value;
+
+    if (!visit_type_bool(v, name, &value, errp)) {
+        return;
+    }
+
+    if (value) {
+        error_setg(errp, "extension %s is not available with KVM",
+                   propname);
+    }
+}
+#endif
+
 /*
  * Add CPU properties with user-facing flags.
  *
@@ -1759,6 +1779,22 @@ static void riscv_cpu_add_user_properties(Object *obj)
             if (object_property_find(obj, prop->name)) {
                 continue;
             }
+
+            /*
+             * Set the default to disabled for every extension
+             * unknown to KVM and error out if the user attempts
+             * to enable any of them.
+             *
+             * We're giving a pass for non-bool properties since they're
+             * not related to the availability of extensions and can be
+             * safely ignored as is.
+             */
+            if (prop->info == &qdev_prop_bool) {
+                object_property_add(obj, prop->name, "bool",
+                                    NULL, cpu_set_cfg_unavailable,
+                                    NULL, (void *)prop->name);
+                continue;
+            }
         }
 #endif
         qdev_property_add_static(dev, prop);
-- 
2.41.0

[PATCH v6 18/20] target/riscv: update multi-letter extension KVM properties

[Daniel Henrique Barboza]

We're now ready to update the multi-letter extensions status for KVM.

kvm_riscv_update_cpu_cfg_isa_ext() is called called during vcpu creation
time to verify which user options changes host defaults (via the 'user_set'
flag) and tries to write them back to KVM.

Failure to commit a change to KVM is only ignored in case KVM doesn't
know about the extension (-EINVAL error code) and the user wanted to
disable the given extension. Otherwise we're going to abort the boot
process.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/kvm.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index 68f6538b04..aa4edc8a56 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -273,6 +273,32 @@ static void kvm_cpu_set_multi_ext_cfg(Object *obj, Visitor *v,
     kvm_cpu_cfg_set(cpu, multi_ext_cfg, value);
 }
 
+static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
+{
+    CPURISCVState *env = &cpu->env;
+    uint64_t id, reg;
+    int i, ret;
+
+    for (i = 0; i < ARRAY_SIZE(kvm_multi_ext_cfgs); i++) {
+        KVMCPUConfig *multi_ext_cfg = &kvm_multi_ext_cfgs[i];
+
+        if (!multi_ext_cfg->user_set) {
+            continue;
+        }
+
+        id = kvm_riscv_reg_id(env, KVM_REG_RISCV_ISA_EXT,
+                              multi_ext_cfg->kvm_reg_id);
+        reg = kvm_cpu_cfg_get(cpu, multi_ext_cfg);
+        ret = kvm_set_one_reg(cs, id, &reg);
+        if (ret != 0) {
+            error_report("Unable to %s extension %s in KVM, error %d",
+                         reg ? "enable" : "disable",
+                         multi_ext_cfg->name, ret);
+            exit(EXIT_FAILURE);
+        }
+    }
+}
+
 static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
 {
     int i;
@@ -792,6 +818,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
     }
 
     kvm_riscv_update_cpu_misa_ext(cpu, cs);
+    kvm_riscv_update_cpu_cfg_isa_ext(cpu, cs);
 
     return ret;
 }
-- 
2.41.0

[PATCH v6 19/20] target/riscv/kvm.c: add kvmconfig_get_cfg_addr() helper

[Daniel Henrique Barboza]

There are 2 places in which we need to get a pointer to a certain
property of the cpu->cfg struct based on property offset. Next patch
will add a couple more.

Create a helper to avoid repeating this code over and over.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/kvm.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index aa4edc8a56..d1bf518893 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -215,11 +215,15 @@ static KVMCPUConfig kvm_multi_ext_cfgs[] = {
     KVM_EXT_CFG("svpbmt", ext_svpbmt, KVM_RISCV_ISA_EXT_SVPBMT),
 };
 
+static void *kvmconfig_get_cfg_addr(RISCVCPU *cpu, KVMCPUConfig *kvmcfg)
+{
+    return (void *)&cpu->cfg + kvmcfg->offset;
+}
+
 static void kvm_cpu_cfg_set(RISCVCPU *cpu, KVMCPUConfig *multi_ext,
                             uint32_t val)
 {
-    int cpu_cfg_offset = multi_ext->offset;
-    bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
+    bool *ext_enabled = kvmconfig_get_cfg_addr(cpu, multi_ext);
 
     *ext_enabled = val;
 }
@@ -227,8 +231,7 @@ static void kvm_cpu_cfg_set(RISCVCPU *cpu, KVMCPUConfig *multi_ext,
 static uint32_t kvm_cpu_cfg_get(RISCVCPU *cpu,
                                 KVMCPUConfig *multi_ext)
 {
-    int cpu_cfg_offset = multi_ext->offset;
-    bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
+    bool *ext_enabled = kvmconfig_get_cfg_addr(cpu, multi_ext);
 
     return *ext_enabled;
 }
-- 
2.41.0

[PATCH v6 20/20] target/riscv/kvm.c: read/write (cbom|cboz)_blocksize in KVM

[Daniel Henrique Barboza]

If we don't set a proper cbom_blocksize|cboz_blocksize in the FDT the
Linux Kernel will fail to detect the availability of the CBOM/CBOZ
extensions, regardless of the contents of the 'riscv,isa' DT prop.

The FDT is being written using the cpu->cfg.cbom|z_blocksize attributes,
so let's expose them as user properties like it is already done with
TCG.

This will also require us to determine proper blocksize values during
init() time since the FDT is already created during realize(). We'll
take a ride in kvm_riscv_init_multiext_cfg() to do it. Note that we
don't need to fetch both cbom and cboz blocksizes every time: check for
their parent extensions (icbom and icboz) and only read the blocksizes
if needed.

In contrast with cbom/z_blocksize properties from TCG, the user is not
able to set any value that is different from the 'host' value when
running KVM. KVM can be particularly harsh dealing with it: a ENOTSUPP
can be thrown for the mere attempt of executing kvm_set_one_reg() for
these 2 regs.

Hopefully, we don't need to call kvm_set_one_reg() for these regs.
We'll check if the user input matches the host value in
kvm_cpu_set_cbomz_blksize(), the set() accessor for both blocksize
properties. We'll fail fast since it's already known to not be
supported.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/kvm.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index d1bf518893..e535884830 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -276,6 +276,42 @@ static void kvm_cpu_set_multi_ext_cfg(Object *obj, Visitor *v,
     kvm_cpu_cfg_set(cpu, multi_ext_cfg, value);
 }
 
+static KVMCPUConfig kvm_cbom_blocksize = {
+    .name = "cbom_blocksize",
+    .offset = CPUCFG(cbom_blocksize),
+    .kvm_reg_id = KVM_REG_RISCV_CONFIG_REG(zicbom_block_size)
+};
+
+static KVMCPUConfig kvm_cboz_blocksize = {
+    .name = "cboz_blocksize",
+    .offset = CPUCFG(cboz_blocksize),
+    .kvm_reg_id = KVM_REG_RISCV_CONFIG_REG(zicboz_block_size)
+};
+
+static void kvm_cpu_set_cbomz_blksize(Object *obj, Visitor *v,
+                                      const char *name,
+                                      void *opaque, Error **errp)
+{
+    KVMCPUConfig *cbomz_cfg = opaque;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    uint16_t value, *host_val;
+
+    if (!visit_type_uint16(v, name, &value, errp)) {
+        return;
+    }
+
+    host_val = kvmconfig_get_cfg_addr(cpu, cbomz_cfg);
+
+    if (value != *host_val) {
+        error_report("Unable to set %s to a different value than "
+                     "the host (%u)",
+                     cbomz_cfg->name, *host_val);
+        exit(EXIT_FAILURE);
+    }
+
+    cbomz_cfg->user_set = true;
+}
+
 static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
 {
     CPURISCVState *env = &cpu->env;
@@ -329,6 +365,14 @@ static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
                             kvm_cpu_set_multi_ext_cfg,
                             NULL, multi_cfg);
     }
+
+    object_property_add(cpu_obj, "cbom_blocksize", "uint16",
+                        NULL, kvm_cpu_set_cbomz_blksize,
+                        NULL, &kvm_cbom_blocksize);
+
+    object_property_add(cpu_obj, "cboz_blocksize", "uint16",
+                        NULL, kvm_cpu_set_cbomz_blksize,
+                        NULL, &kvm_cboz_blocksize);
 }
 
 static int kvm_riscv_get_regs_core(CPUState *cs)
@@ -644,6 +688,24 @@ static void kvm_riscv_init_misa_ext_mask(RISCVCPU *cpu,
     env->misa_ext = env->misa_ext_mask;
 }
 
+static void kvm_riscv_read_cbomz_blksize(RISCVCPU *cpu, KVMScratchCPU *kvmcpu,
+                                         KVMCPUConfig *cbomz_cfg)
+{
+    CPURISCVState *env = &cpu->env;
+    struct kvm_one_reg reg;
+    int ret;
+
+    reg.id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
+                              cbomz_cfg->kvm_reg_id);
+    reg.addr = (uint64_t)kvmconfig_get_cfg_addr(cpu, cbomz_cfg);
+    ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
+    if (ret != 0) {
+        error_report("Unable to read KVM reg %s, error %d",
+                     cbomz_cfg->name, ret);
+        exit(EXIT_FAILURE);
+    }
+}
+
 static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
 {
     CPURISCVState *env = &cpu->env;
@@ -675,6 +737,14 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
 
         kvm_cpu_cfg_set(cpu, multi_ext_cfg, val);
     }
+
+    if (cpu->cfg.ext_icbom) {
+        kvm_riscv_read_cbomz_blksize(cpu, kvmcpu, &kvm_cbom_blocksize);
+    }
+
+    if (cpu->cfg.ext_icboz) {
+        kvm_riscv_read_cbomz_blksize(cpu, kvmcpu, &kvm_cboz_blocksize);
+    }
 }
 
 void kvm_riscv_init_user_properties(Object *cpu_obj)
-- 
2.41.0

Re: [PATCH v6 11/20] target/riscv/cpu: add misa_ext_info_arr[]

[Philippe Mathieu-Daudé]

On 28/6/23 23:30, Daniel Henrique Barboza wrote:
> Next patch will add KVM specific user properties for both MISA and
> multi-letter extensions. For MISA extensions we want to make use of what
> is already available in misa_ext_cfgs[] to avoid code repetition.
> 
> misa_ext_info_arr[] array will hold name and description for each MISA
> extension that misa_ext_cfgs[] is declaring. We'll then use this new
> array in KVM code to avoid duplicating strings.
> 
> There's nothing holding us back from doing the same with multi-letter
> extensions. For now doing just with MISA extensions is enough.
> 
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++----------------
>   target/riscv/cpu.h |  7 +++-
>   2 files changed, 61 insertions(+), 29 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 2485e820f8..90dd2078ae 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
>       visit_type_bool(v, name, &value, errp);
>   }
>   
> -static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
> -    {.name = "a", .description = "Atomic instructions",
> -     .misa_bit = RVA, .enabled = true},
> -    {.name = "c", .description = "Compressed instructions",
> -     .misa_bit = RVC, .enabled = true},
> -    {.name = "d", .description = "Double-precision float point",
> -     .misa_bit = RVD, .enabled = true},
> -    {.name = "f", .description = "Single-precision float point",
> -     .misa_bit = RVF, .enabled = true},
> -    {.name = "i", .description = "Base integer instruction set",
> -     .misa_bit = RVI, .enabled = true},
> -    {.name = "e", .description = "Base integer instruction set (embedded)",
> -     .misa_bit = RVE, .enabled = false},
> -    {.name = "m", .description = "Integer multiplication and division",
> -     .misa_bit = RVM, .enabled = true},
> -    {.name = "s", .description = "Supervisor-level instructions",
> -     .misa_bit = RVS, .enabled = true},
> -    {.name = "u", .description = "User-level instructions",
> -     .misa_bit = RVU, .enabled = true},
> -    {.name = "h", .description = "Hypervisor",
> -     .misa_bit = RVH, .enabled = true},
> -    {.name = "x-j", .description = "Dynamic translated languages",
> -     .misa_bit = RVJ, .enabled = false},
> -    {.name = "v", .description = "Vector operations",
> -     .misa_bit = RVV, .enabled = false},
> -    {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)",
> -     .misa_bit = RVG, .enabled = false},
> +typedef struct misa_ext_info {
> +    const char *name;
> +    const char *description;
> +} MISAExtInfo;
> +
> +#define MISA_EXT_INFO(_idx, _propname, _descr) \
> +    [(_idx - 'A')] = {.name = _propname, .description = _descr}
> +
> +static const MISAExtInfo misa_ext_info_arr[] = {
> +    MISA_EXT_INFO('A', "a", "Atomic instructions"),
> +    MISA_EXT_INFO('C', "c", "Compressed instructions"),
> +    MISA_EXT_INFO('D', "d", "Double-precision float point"),
> +    MISA_EXT_INFO('F', "f", "Single-precision float point"),
> +    MISA_EXT_INFO('I', "i", "Base integer instruction set"),
> +    MISA_EXT_INFO('E', "e", "Base integer instruction set (embedded)"),
> +    MISA_EXT_INFO('M', "m", "Integer multiplication and division"),
> +    MISA_EXT_INFO('S', "s", "Supervisor-level instructions"),
> +    MISA_EXT_INFO('U', "u", "User-level instructions"),
> +    MISA_EXT_INFO('H', "h", "Hypervisor"),
> +    MISA_EXT_INFO('J', "x-j", "Dynamic translated languages"),
> +    MISA_EXT_INFO('V', "v", "Vector operations"),
> +    MISA_EXT_INFO('G', "g", "General purpose (IMAFD_Zicsr_Zifencei)"),
> +};
> +
> +const char *riscv_get_misa_ext_name(uint32_t bit)
> +{

Is that OK to return NULL, or should we assert for
unimplemented bit/feature?

> +    return misa_ext_info_arr[ctz32(bit)].name;
> +}
> +
> +const char *riscv_get_misa_ext_descr(uint32_t bit)
> +{
> +    return misa_ext_info_arr[ctz32(bit)].description;

Ditto.

> +}
> +
> +#define MISA_CFG(_bit, _enabled) \
> +    {.misa_bit = _bit, .enabled = _enabled}
> +
> +static RISCVCPUMisaExtConfig misa_ext_cfgs[] = {

'const'

> +    MISA_CFG(RVA, true),
> +    MISA_CFG(RVC, true),
> +    MISA_CFG(RVD, true),
> +    MISA_CFG(RVF, true),
> +    MISA_CFG(RVI, true),
> +    MISA_CFG(RVE, false),
> +    MISA_CFG(RVM, true),
> +    MISA_CFG(RVS, true),
> +    MISA_CFG(RVU, true),
> +    MISA_CFG(RVH, true),
> +    MISA_CFG(RVJ, false),
> +    MISA_CFG(RVV, false),
> +    MISA_CFG(RVG, false),
>   };
>   
>   static void riscv_cpu_add_misa_properties(Object *cpu_obj)
> @@ -1592,7 +1616,10 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>       int i;
>   
>       for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
> -        const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
> +        RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];

const

> +
> +        misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit);
> +        misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit);

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v6 15/20] target/riscv/cpu.c: add satp_mode properties earlier

[Philippe Mathieu-Daudé]

On 28/6/23 23:30, Daniel Henrique Barboza wrote:
> riscv_cpu_add_user_properties() ended up with an excess of "#ifndef
> CONFIG_USER_ONLY" blocks after changes that added KVM properties
> handling.
> 
> KVM specific properties are required to be created earlier than their
> TCG counterparts, but the remaining props can be created at any order.
> Move riscv_add_satp_mode_properties() to the start of the function,
> inside the !CONFIG_USER_ONLY block already present there, to remove the
> last ifndef block.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v6 01/20] target/riscv: skip features setup for KVM CPUs

[Andrew Jones]

On Wed, Jun 28, 2023 at 06:30:14PM -0300, 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
> checking if the CSR_MCOUNTEREN, CSR_MCOUNTINHIBIT and CSR_MENVCFG CSRs
> 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 to 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_tcg() helper contains all validation logic that
> are applicable to TCG CPUs only. riscv_cpu_realize() verifies if we're
> running TCG and, if it's the case, proceed with the usual TCG realize()
> logic.
> 
> [1] lib/sbi/sbi_hart.c, hart_detect_features()
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c | 35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index fb8458bf74..bbb201a2b3 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -34,6 +34,7 @@
>  #include "migration/vmstate.h"
>  #include "fpu/softfloat-helpers.h"
>  #include "sysemu/kvm.h"
> +#include "sysemu/tcg.h"
>  #include "kvm_riscv.h"
>  #include "tcg/tcg.h"
>  
> @@ -1308,20 +1309,12 @@ static void riscv_cpu_validate_misa_priv(CPURISCVState *env, Error **errp)
>      }
>  }
>  
> -static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> +static void riscv_cpu_realize_tcg(DeviceState *dev, Error **errp)
>  {
> -    CPUState *cs = CPU(dev);
>      RISCVCPU *cpu = RISCV_CPU(dev);
>      CPURISCVState *env = &cpu->env;
> -    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;
> -    }
> -
>      riscv_cpu_validate_misa_mxl(cpu, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
> @@ -1356,7 +1349,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>      }
>  
>  #ifndef CONFIG_USER_ONLY
> -    cs->tcg_cflags |= CF_PCREL;
> +    CPU(dev)->tcg_cflags |= CF_PCREL;
>  
>      if (cpu->cfg.ext_sstc) {
>          riscv_timer_init(cpu);
> @@ -1369,6 +1362,28 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>          }
>       }
>  #endif
> +}
> +
> +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 (tcg_enabled()) {
> +        riscv_cpu_realize_tcg(dev, &local_err);
> +        if (local_err != NULL) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }
>  
>      riscv_cpu_finalize_features(cpu, &local_err);
>      if (local_err != NULL) {
> -- 
> 2.41.0
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH v6 11/20] target/riscv/cpu: add misa_ext_info_arr[]

[Andrew Jones]

On Wed, Jun 28, 2023 at 06:30:24PM -0300, Daniel Henrique Barboza wrote:
> Next patch will add KVM specific user properties for both MISA and
> multi-letter extensions. For MISA extensions we want to make use of what
> is already available in misa_ext_cfgs[] to avoid code repetition.
> 
> misa_ext_info_arr[] array will hold name and description for each MISA
> extension that misa_ext_cfgs[] is declaring. We'll then use this new
> array in KVM code to avoid duplicating strings.
> 
> There's nothing holding us back from doing the same with multi-letter
> extensions. For now doing just with MISA extensions is enough.
> 
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++----------------
>  target/riscv/cpu.h |  7 +++-
>  2 files changed, 61 insertions(+), 29 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 2485e820f8..90dd2078ae 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
>      visit_type_bool(v, name, &value, errp);
>  }
>  
> -static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
> -    {.name = "a", .description = "Atomic instructions",
> -     .misa_bit = RVA, .enabled = true},
> -    {.name = "c", .description = "Compressed instructions",
> -     .misa_bit = RVC, .enabled = true},
> -    {.name = "d", .description = "Double-precision float point",
> -     .misa_bit = RVD, .enabled = true},
> -    {.name = "f", .description = "Single-precision float point",
> -     .misa_bit = RVF, .enabled = true},
> -    {.name = "i", .description = "Base integer instruction set",
> -     .misa_bit = RVI, .enabled = true},
> -    {.name = "e", .description = "Base integer instruction set (embedded)",
> -     .misa_bit = RVE, .enabled = false},
> -    {.name = "m", .description = "Integer multiplication and division",
> -     .misa_bit = RVM, .enabled = true},
> -    {.name = "s", .description = "Supervisor-level instructions",
> -     .misa_bit = RVS, .enabled = true},
> -    {.name = "u", .description = "User-level instructions",
> -     .misa_bit = RVU, .enabled = true},
> -    {.name = "h", .description = "Hypervisor",
> -     .misa_bit = RVH, .enabled = true},
> -    {.name = "x-j", .description = "Dynamic translated languages",
> -     .misa_bit = RVJ, .enabled = false},
> -    {.name = "v", .description = "Vector operations",
> -     .misa_bit = RVV, .enabled = false},
> -    {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)",
> -     .misa_bit = RVG, .enabled = false},
> +typedef struct misa_ext_info {
> +    const char *name;
> +    const char *description;
> +} MISAExtInfo;
> +
> +#define MISA_EXT_INFO(_idx, _propname, _descr) \
> +    [(_idx - 'A')] = {.name = _propname, .description = _descr}

We don't have to give up on passing RV* to this macro. Directly
using __builtin_ctz() with a constant should work, i.e.

 #define MISA_EXT_INFO(_bit, _propname, _descr) \
     [__builtin_ctz(_bit)] = {.name = _propname, .description = _descr}

and then

 MISA_EXT_INFO(RVA, "a", "Atomic instructions"),
 MISA_EXT_INFO(RVD, "d", "Double-precision float point"),
 ...

(We don't need the ctz32() wrapper since we know we'll never input zero to
__builtin_ctz().)

> +
> +static const MISAExtInfo misa_ext_info_arr[] = {
> +    MISA_EXT_INFO('A', "a", "Atomic instructions"),
> +    MISA_EXT_INFO('C', "c", "Compressed instructions"),
> +    MISA_EXT_INFO('D', "d", "Double-precision float point"),
> +    MISA_EXT_INFO('F', "f", "Single-precision float point"),
> +    MISA_EXT_INFO('I', "i", "Base integer instruction set"),
> +    MISA_EXT_INFO('E', "e", "Base integer instruction set (embedded)"),
> +    MISA_EXT_INFO('M', "m", "Integer multiplication and division"),
> +    MISA_EXT_INFO('S', "s", "Supervisor-level instructions"),
> +    MISA_EXT_INFO('U', "u", "User-level instructions"),
> +    MISA_EXT_INFO('H', "h", "Hypervisor"),
> +    MISA_EXT_INFO('J', "x-j", "Dynamic translated languages"),
> +    MISA_EXT_INFO('V', "v", "Vector operations"),
> +    MISA_EXT_INFO('G', "g", "General purpose (IMAFD_Zicsr_Zifencei)"),
> +};
> +
> +const char *riscv_get_misa_ext_name(uint32_t bit)
> +{
> +    return misa_ext_info_arr[ctz32(bit)].name;
> +}
> +
> +const char *riscv_get_misa_ext_descr(uint32_t bit)

nit: I'd prefer riscv_get_misa_ext_description() for the name.

> +{
> +    return misa_ext_info_arr[ctz32(bit)].description;
> +}
> +
> +#define MISA_CFG(_bit, _enabled) \
> +    {.misa_bit = _bit, .enabled = _enabled}
> +
> +static RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
> +    MISA_CFG(RVA, true),
> +    MISA_CFG(RVC, true),
> +    MISA_CFG(RVD, true),
> +    MISA_CFG(RVF, true),
> +    MISA_CFG(RVI, true),
> +    MISA_CFG(RVE, false),
> +    MISA_CFG(RVM, true),
> +    MISA_CFG(RVS, true),
> +    MISA_CFG(RVU, true),
> +    MISA_CFG(RVH, true),
> +    MISA_CFG(RVJ, false),
> +    MISA_CFG(RVV, false),
> +    MISA_CFG(RVG, false),
>  };
>  
>  static void riscv_cpu_add_misa_properties(Object *cpu_obj)
> @@ -1592,7 +1616,10 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>      int i;
>  
>      for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
> -        const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
> +        RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
> +
> +        misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit);
> +        misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit);

This might be necessary for the KVM side, but we should be able to avoid
this runtime setting here where the compiler can be certain everything is
a constant. We just need the old MISA_CFG() back, slightly tweaked to use
__builtin_ctz().

>  
>          object_property_add(cpu_obj, misa_cfg->name, "bool",
>                              cpu_get_misa_ext_cfg,
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index cc20ee25a7..acae118b9b 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -41,7 +41,10 @@
>  
>  #define RV(x) ((target_ulong)1 << (x - 'A'))
>  
> -/* Consider updating misa_ext_cfgs[] when adding new MISA bits here */
> +/*
> + * Consider updating misa_ext_info_arr[] and misa_ext_cfgs[]
> + * when adding new MISA bits here.
> + */
>  #define RVI RV('I')
>  #define RVE RV('E') /* E and I are mutually exclusive */
>  #define RVM RV('M')
> @@ -56,6 +59,8 @@
>  #define RVJ RV('J')
>  #define RVG RV('G')
>  
> +const char *riscv_get_misa_ext_name(uint32_t bit);
> +const char *riscv_get_misa_ext_descr(uint32_t bit);
>  
>  /* Privileged specification version */
>  enum {
> -- 
> 2.41.0
>

Thanks,
drew

Re: [PATCH v6 12/20] target/riscv: add KVM specific MISA properties

[Andrew Jones]

On Wed, Jun 28, 2023 at 06:30:25PM -0300, Daniel Henrique Barboza wrote:
> Using all TCG user properties in KVM is tricky. First because KVM
> supports only a small subset of what TCG provides, so most of the
> cpu->cfg flags do nothing for KVM.
> 
> Second, and more important, we don't have a way of telling if any given
> value is an user input or not. For TCG this has a small impact since we
> just validating everything and error out if needed. But for KVM it would
> be good to know if a given value was set by the user or if it's a value
> already provided by KVM. Otherwise we don't know how to handle failed
> kvm_set_one_regs() when writing the configurations back.
> 
> These characteristics make it overly complicated to use the same user
> facing flags for both KVM and TCG. A simpler approach is to create KVM
> specific properties that have specialized logic, forking KVM and TCG use
> cases for those cases only. Fully separating KVM/TCG properties is
> unneeded at this point - in fact we want the user experience to be as
> equal as possible, regardless of the acceleration chosen.
> 
> We'll start this fork with the MISA properties, adding the MISA bits
> that the KVM driver currently supports. A new KVMCPUConfig type is
> introduced. It'll hold general information about an extension. For MISA
> extensions we're going to use the newly created getters of
> misa_ext_infos[] to populate their name and description. 'offset' holds
> the MISA bit (RVA, RVC, ...). We're calling it 'offset' instead of
> 'misa_bit' because this same KVMCPUConfig struct will be used to
> multi-letter extensions later on.
> 
> This new type also holds a 'user_set' flag. This flag will be set when
> the user set an option that's different than what is already configured
> in the host, requiring KVM intervention to write the regs back during
> kvm_arch_init_vcpu(). Similar mechanics will be implemented for
> multi-letter extensions as well.
> 
> There is no need to duplicate more code than necessary, so we're going
> to use the existing kvm_riscv_init_user_properties() to add the KVM
> specific properties. Any code that is adding a TCG user prop is then
> changed slightly to verify first if there's a KVM prop with the same
> name already added.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c | 13 +++++---
>  target/riscv/kvm.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+), 4 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 90dd2078ae..f4b1868466 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1617,14 +1617,19 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>  
>      for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
>          RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
> +        Error *local_err = NULL;
>  
>          misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit);
>          misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit);
>  
> -        object_property_add(cpu_obj, misa_cfg->name, "bool",
> -                            cpu_get_misa_ext_cfg,
> -                            cpu_set_misa_ext_cfg,
> -                            NULL, (void *)misa_cfg);
> +        object_property_try_add(cpu_obj, misa_cfg->name, "bool",
> +                                cpu_get_misa_ext_cfg, cpu_set_misa_ext_cfg,
> +                                NULL, (void *)misa_cfg, &local_err);
> +        if (local_err) {
> +            /* Someone (KVM) already created the property */
> +            continue;
> +        }

This assumes object_property_try_add() only fails when it detects
duplicate properties. That's currently true, but it's not documented,
so I'm not sure we should count on it. Also, if we do want to assume
only duplicate properties generate errors, then we can pass NULL for
errp and just check the return value of the call, as it'll be NULL on
failure.

> +
>          object_property_set_description(cpu_obj, misa_cfg->name,
>                                          misa_cfg->description);
>          object_property_set_bool(cpu_obj, misa_cfg->name,
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 4d0808cb9a..0fb63cced3 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -22,8 +22,10 @@
>  #include <linux/kvm.h>
>  
>  #include "qemu/timer.h"
> +#include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "qemu/main-loop.h"
> +#include "qapi/visitor.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/kvm.h"
>  #include "sysemu/kvm_int.h"
> @@ -105,6 +107,81 @@ static uint64_t kvm_riscv_reg_id(CPURISCVState *env, uint64_t type,
>          } \
>      } while (0)
>  
> +typedef struct KVMCPUConfig {
> +    const char *name;
> +    const char *description;
> +    target_ulong offset;
> +    int kvm_reg_id;
> +    bool user_set;
> +} KVMCPUConfig;
> +
> +#define KVM_MISA_CFG(_bit, _reg_id) \
> +    {.offset = _bit, .kvm_reg_id = _reg_id}
> +
> +/* KVM ISA extensions */
> +static KVMCPUConfig kvm_misa_ext_cfgs[] = {
> +    KVM_MISA_CFG(RVA, KVM_RISCV_ISA_EXT_A),
> +    KVM_MISA_CFG(RVC, KVM_RISCV_ISA_EXT_C),
> +    KVM_MISA_CFG(RVD, KVM_RISCV_ISA_EXT_D),
> +    KVM_MISA_CFG(RVF, KVM_RISCV_ISA_EXT_F),
> +    KVM_MISA_CFG(RVH, KVM_RISCV_ISA_EXT_H),
> +    KVM_MISA_CFG(RVI, KVM_RISCV_ISA_EXT_I),
> +    KVM_MISA_CFG(RVM, KVM_RISCV_ISA_EXT_M),
> +};
> +
> +static void kvm_cpu_set_misa_ext_cfg(Object *obj, Visitor *v,
> +                                     const char *name,
> +                                     void *opaque, Error **errp)
> +{
> +    KVMCPUConfig *misa_ext_cfg = opaque;
> +    target_ulong misa_bit = misa_ext_cfg->offset;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    CPURISCVState *env = &cpu->env;
> +    bool value, host_bit;
> +
> +    if (!visit_type_bool(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    host_bit = env->misa_ext_mask & misa_bit;
> +
> +    if (value == host_bit) {
> +        return;
> +    }
> +
> +    if (!value) {
> +        misa_ext_cfg->user_set = true;
> +        return;
> +    }
> +
> +    /*
> +     * Forbid users to enable extensions that aren't
> +     * available in the hart.
> +     */
> +    error_setg(errp, "Enabling MISA bit '%s' is not allowed: it's not "
> +               "enabled in the host", misa_ext_cfg->name);
> +}
> +
> +static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(kvm_misa_ext_cfgs); i++) {
> +        KVMCPUConfig *misa_cfg = &kvm_misa_ext_cfgs[i];
> +        int bit = misa_cfg->offset;
> +
> +        misa_cfg->name = riscv_get_misa_ext_name(bit);
> +        misa_cfg->description = riscv_get_misa_ext_descr(bit);
> +
> +        object_property_add(cpu_obj, misa_cfg->name, "bool",
> +                            NULL,
> +                            kvm_cpu_set_misa_ext_cfg,
> +                            NULL, misa_cfg);
> +        object_property_set_description(cpu_obj, misa_cfg->name,
> +                                        misa_cfg->description);
> +    }
> +}
> +
>  static int kvm_riscv_get_regs_core(CPUState *cs)
>  {
>      int ret = 0;
> @@ -427,6 +504,7 @@ void kvm_riscv_init_user_properties(Object *cpu_obj)
>          return;
>      }
>  
> +    kvm_riscv_add_cpu_user_properties(cpu_obj);
>      kvm_riscv_init_machine_ids(cpu, &kvmcpu);
>      kvm_riscv_init_misa_ext_mask(cpu, &kvmcpu);
>  
> -- 
> 2.41.0
>

Thanks,
drew

Re: [PATCH v6 14/20] target/riscv/kvm.c: add multi-letter extension KVM properties

[Andrew Jones]

On Wed, Jun 28, 2023 at 06:30:27PM -0300, Daniel Henrique Barboza wrote:
> Let's add KVM user properties for the multi-letter extensions that KVM
> currently supports: zicbom, zicboz, zihintpause, zbb, ssaia, sstc,
> svinval and svpbmt.
> 
> As with MISA extensions, we're using the KVMCPUConfig type to hold
> information about the state of each extension. However, multi-letter
> extensions have more cases to cover than MISA extensions, so we're
> adding an extra 'supported' flag as well. This flag will reflect if a
> given extension is supported by KVM, i.e. KVM knows how to handle it.
> This is determined during KVM extension discovery in
> kvm_riscv_init_multiext_cfg(), where we test for EINVAL errors. Any
> other error different from EINVAL will cause an abort.
> 
> The use of the 'user_set' is similar to what we already do with MISA
> extensions: the flag set only if the user is changing the extension
> state.
> 
> The 'supported' flag will be used later on to make an exception for
> users that are disabling multi-letter extensions that are unknown to
> KVM.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c |   8 +++
>  target/riscv/kvm.c | 119 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 127 insertions(+)
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH v6 15/20] target/riscv/cpu.c: add satp_mode properties earlier

[Andrew Jones]

On Wed, Jun 28, 2023 at 06:30:28PM -0300, Daniel Henrique Barboza wrote:
> riscv_cpu_add_user_properties() ended up with an excess of "#ifndef
> CONFIG_USER_ONLY" blocks after changes that added KVM properties
> handling.
> 
> KVM specific properties are required to be created earlier than their
> TCG counterparts, but the remaining props can be created at any order.
> Move riscv_add_satp_mode_properties() to the start of the function,
> inside the !CONFIG_USER_ONLY block already present there, to remove the
> last ifndef block.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 5428402cfa..b4a6fd8bab 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1743,6 +1743,8 @@ static void riscv_cpu_add_user_properties(Object *obj)
>      DeviceState *dev = DEVICE(obj);
>  
>  #ifndef CONFIG_USER_ONLY
> +    riscv_add_satp_mode_properties(obj);
> +
>      if (kvm_enabled()) {
>          kvm_riscv_init_user_properties(obj);
>      }
> @@ -1761,10 +1763,6 @@ static void riscv_cpu_add_user_properties(Object *obj)
>  #endif
>          qdev_property_add_static(dev, prop);
>      }
> -
> -#ifndef CONFIG_USER_ONLY
> -    riscv_add_satp_mode_properties(obj);
> -#endif
>  }
>  
>  static Property riscv_cpu_properties[] = {
> -- 
> 2.41.0
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH v6 17/20] target/riscv/cpu.c: create KVM mock properties

[Andrew Jones]

On Wed, Jun 28, 2023 at 06:30:30PM -0300, Daniel Henrique Barboza wrote:
> KVM-specific properties are being created inside target/riscv/kvm.c. But
> at this moment we're gathering all the remaining properties from TCG and
> adding them as is when running KVM. This creates a situation where
> non-KVM properties are setting flags to 'true' due to its default
> settings (e.g.  Zawrs). Users can also freely enable them via command
> line.
> 
> This doesn't impact runtime per se because KVM doesn't care about these
> flags, but code such as riscv_isa_string_ext() take those flags into
> account. The result is that, for a KVM guest, setting non-KVM properties
> will make them appear in the riscv,isa DT.
> 
> We want to keep the same API for both TCG and KVM and at the same time,
> when running KVM, forbid non-KVM extensions to be enabled internally. We
> accomplish both by changing riscv_cpu_add_user_properties() to add a
> mock boolean property for every non-KVM extension in
> riscv_cpu_extensions[]. Then, when running KVM, users are still free to
> set extensions at will, but we'll error out if a non-KVM extension is
> enabled. Setting such extension to 'false' will be ignored.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 79c8ffe6b7..6d7a0bc4ae 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1731,6 +1731,26 @@ static Property riscv_cpu_extensions[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +
> +#ifndef CONFIG_USER_ONLY
> +static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
> +                                    const char *name,
> +                                    void *opaque, Error **errp)
> +{
> +    const char *propname = opaque;
> +    bool value;
> +
> +    if (!visit_type_bool(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    if (value) {
> +        error_setg(errp, "extension %s is not available with KVM",
> +                   propname);
> +    }
> +}
> +#endif
> +
>  /*
>   * Add CPU properties with user-facing flags.
>   *
> @@ -1759,6 +1779,22 @@ static void riscv_cpu_add_user_properties(Object *obj)
>              if (object_property_find(obj, prop->name)) {
>                  continue;
>              }
> +
> +            /*
> +             * Set the default to disabled for every extension
> +             * unknown to KVM and error out if the user attempts
> +             * to enable any of them.
> +             *
> +             * We're giving a pass for non-bool properties since they're
> +             * not related to the availability of extensions and can be
> +             * safely ignored as is.
> +             */
> +            if (prop->info == &qdev_prop_bool) {
> +                object_property_add(obj, prop->name, "bool",
> +                                    NULL, cpu_set_cfg_unavailable,
> +                                    NULL, (void *)prop->name);
> +                continue;
> +            }
>          }
>  #endif
>          qdev_property_add_static(dev, prop);
> -- 
> 2.41.0
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH v6 11/20] target/riscv/cpu: add misa_ext_info_arr[]

[Daniel Henrique Barboza]

On 6/29/23 04:26, Philippe Mathieu-Daudé wrote:
> On 28/6/23 23:30, Daniel Henrique Barboza wrote:
>> Next patch will add KVM specific user properties for both MISA and
>> multi-letter extensions. For MISA extensions we want to make use of what
>> is already available in misa_ext_cfgs[] to avoid code repetition.
>>
>> misa_ext_info_arr[] array will hold name and description for each MISA
>> extension that misa_ext_cfgs[] is declaring. We'll then use this new
>> array in KVM code to avoid duplicating strings.
>>
>> There's nothing holding us back from doing the same with multi-letter
>> extensions. For now doing just with MISA extensions is enough.
>>
>> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++----------------
>>   target/riscv/cpu.h |  7 +++-
>>   2 files changed, 61 insertions(+), 29 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 2485e820f8..90dd2078ae 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
>>       visit_type_bool(v, name, &value, errp);
>>   }
>> -static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
>> -    {.name = "a", .description = "Atomic instructions",
>> -     .misa_bit = RVA, .enabled = true},
>> -    {.name = "c", .description = "Compressed instructions",
>> -     .misa_bit = RVC, .enabled = true},
>> -    {.name = "d", .description = "Double-precision float point",
>> -     .misa_bit = RVD, .enabled = true},
>> -    {.name = "f", .description = "Single-precision float point",
>> -     .misa_bit = RVF, .enabled = true},
>> -    {.name = "i", .description = "Base integer instruction set",
>> -     .misa_bit = RVI, .enabled = true},
>> -    {.name = "e", .description = "Base integer instruction set (embedded)",
>> -     .misa_bit = RVE, .enabled = false},
>> -    {.name = "m", .description = "Integer multiplication and division",
>> -     .misa_bit = RVM, .enabled = true},
>> -    {.name = "s", .description = "Supervisor-level instructions",
>> -     .misa_bit = RVS, .enabled = true},
>> -    {.name = "u", .description = "User-level instructions",
>> -     .misa_bit = RVU, .enabled = true},
>> -    {.name = "h", .description = "Hypervisor",
>> -     .misa_bit = RVH, .enabled = true},
>> -    {.name = "x-j", .description = "Dynamic translated languages",
>> -     .misa_bit = RVJ, .enabled = false},
>> -    {.name = "v", .description = "Vector operations",
>> -     .misa_bit = RVV, .enabled = false},
>> -    {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)",
>> -     .misa_bit = RVG, .enabled = false},
>> +typedef struct misa_ext_info {
>> +    const char *name;
>> +    const char *description;
>> +} MISAExtInfo;
>> +
>> +#define MISA_EXT_INFO(_idx, _propname, _descr) \
>> +    [(_idx - 'A')] = {.name = _propname, .description = _descr}
>> +
>> +static const MISAExtInfo misa_ext_info_arr[] = {
>> +    MISA_EXT_INFO('A', "a", "Atomic instructions"),
>> +    MISA_EXT_INFO('C', "c", "Compressed instructions"),
>> +    MISA_EXT_INFO('D', "d", "Double-precision float point"),
>> +    MISA_EXT_INFO('F', "f", "Single-precision float point"),
>> +    MISA_EXT_INFO('I', "i", "Base integer instruction set"),
>> +    MISA_EXT_INFO('E', "e", "Base integer instruction set (embedded)"),
>> +    MISA_EXT_INFO('M', "m", "Integer multiplication and division"),
>> +    MISA_EXT_INFO('S', "s", "Supervisor-level instructions"),
>> +    MISA_EXT_INFO('U', "u", "User-level instructions"),
>> +    MISA_EXT_INFO('H', "h", "Hypervisor"),
>> +    MISA_EXT_INFO('J', "x-j", "Dynamic translated languages"),
>> +    MISA_EXT_INFO('V', "v", "Vector operations"),
>> +    MISA_EXT_INFO('G', "g", "General purpose (IMAFD_Zicsr_Zifencei)"),
>> +};
>> +
>> +const char *riscv_get_misa_ext_name(uint32_t bit)
>> +{
> 
> Is that OK to return NULL, or should we assert for
> unimplemented bit/feature?
> 
>> +    return misa_ext_info_arr[ctz32(bit)].name;
>> +}
>> +
>> +const char *riscv_get_misa_ext_descr(uint32_t bit)
>> +{
>> +    return misa_ext_info_arr[ctz32(bit)].description;
> 
> Ditto.
> 
>> +}
>> +
>> +#define MISA_CFG(_bit, _enabled) \
>> +    {.misa_bit = _bit, .enabled = _enabled}
>> +
>> +static RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
> 
> 'const'

Not sure why I got rid of 'const' here. I'll reintroduce it.


Daniel

> 
>> +    MISA_CFG(RVA, true),
>> +    MISA_CFG(RVC, true),
>> +    MISA_CFG(RVD, true),
>> +    MISA_CFG(RVF, true),
>> +    MISA_CFG(RVI, true),
>> +    MISA_CFG(RVE, false),
>> +    MISA_CFG(RVM, true),
>> +    MISA_CFG(RVS, true),
>> +    MISA_CFG(RVU, true),
>> +    MISA_CFG(RVH, true),
>> +    MISA_CFG(RVJ, false),
>> +    MISA_CFG(RVV, false),
>> +    MISA_CFG(RVG, false),
>>   };
>>   static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>> @@ -1592,7 +1616,10 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>>       int i;
>>       for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
>> -        const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
>> +        RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
> 
> const
> 
>> +
>> +        misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit);
>> +        misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit);
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

Re: [PATCH v6 11/20] target/riscv/cpu: add misa_ext_info_arr[]

[Daniel Henrique Barboza]

On 6/29/23 05:59, Andrew Jones wrote:
> On Wed, Jun 28, 2023 at 06:30:24PM -0300, Daniel Henrique Barboza wrote:
>> Next patch will add KVM specific user properties for both MISA and
>> multi-letter extensions. For MISA extensions we want to make use of what
>> is already available in misa_ext_cfgs[] to avoid code repetition.
>>
>> misa_ext_info_arr[] array will hold name and description for each MISA
>> extension that misa_ext_cfgs[] is declaring. We'll then use this new
>> array in KVM code to avoid duplicating strings.
>>
>> There's nothing holding us back from doing the same with multi-letter
>> extensions. For now doing just with MISA extensions is enough.
>>
>> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++----------------
>>   target/riscv/cpu.h |  7 +++-
>>   2 files changed, 61 insertions(+), 29 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 2485e820f8..90dd2078ae 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
>>       visit_type_bool(v, name, &value, errp);
>>   }
>>   
>> -static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
>> -    {.name = "a", .description = "Atomic instructions",
>> -     .misa_bit = RVA, .enabled = true},
>> -    {.name = "c", .description = "Compressed instructions",
>> -     .misa_bit = RVC, .enabled = true},
>> -    {.name = "d", .description = "Double-precision float point",
>> -     .misa_bit = RVD, .enabled = true},
>> -    {.name = "f", .description = "Single-precision float point",
>> -     .misa_bit = RVF, .enabled = true},
>> -    {.name = "i", .description = "Base integer instruction set",
>> -     .misa_bit = RVI, .enabled = true},
>> -    {.name = "e", .description = "Base integer instruction set (embedded)",
>> -     .misa_bit = RVE, .enabled = false},
>> -    {.name = "m", .description = "Integer multiplication and division",
>> -     .misa_bit = RVM, .enabled = true},
>> -    {.name = "s", .description = "Supervisor-level instructions",
>> -     .misa_bit = RVS, .enabled = true},
>> -    {.name = "u", .description = "User-level instructions",
>> -     .misa_bit = RVU, .enabled = true},
>> -    {.name = "h", .description = "Hypervisor",
>> -     .misa_bit = RVH, .enabled = true},
>> -    {.name = "x-j", .description = "Dynamic translated languages",
>> -     .misa_bit = RVJ, .enabled = false},
>> -    {.name = "v", .description = "Vector operations",
>> -     .misa_bit = RVV, .enabled = false},
>> -    {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)",
>> -     .misa_bit = RVG, .enabled = false},
>> +typedef struct misa_ext_info {
>> +    const char *name;
>> +    const char *description;
>> +} MISAExtInfo;
>> +
>> +#define MISA_EXT_INFO(_idx, _propname, _descr) \
>> +    [(_idx - 'A')] = {.name = _propname, .description = _descr}
> 
> We don't have to give up on passing RV* to this macro. Directly
> using __builtin_ctz() with a constant should work, i.e.
> 
>   #define MISA_EXT_INFO(_bit, _propname, _descr) \
>       [__builtin_ctz(_bit)] = {.name = _propname, .description = _descr}
> 
> and then
> 
>   MISA_EXT_INFO(RVA, "a", "Atomic instructions"),
>   MISA_EXT_INFO(RVD, "d", "Double-precision float point"),
>   ...
> 
> (We don't need the ctz32() wrapper since we know we'll never input zero to
> __builtin_ctz().)

I'll give it a try. I'll spare us from having to assign name and descr during
runtime (at least for this file).

Daniel

> 
>> +
>> +static const MISAExtInfo misa_ext_info_arr[] = {
>> +    MISA_EXT_INFO('A', "a", "Atomic instructions"),
>> +    MISA_EXT_INFO('C', "c", "Compressed instructions"),
>> +    MISA_EXT_INFO('D', "d", "Double-precision float point"),
>> +    MISA_EXT_INFO('F', "f", "Single-precision float point"),
>> +    MISA_EXT_INFO('I', "i", "Base integer instruction set"),
>> +    MISA_EXT_INFO('E', "e", "Base integer instruction set (embedded)"),
>> +    MISA_EXT_INFO('M', "m", "Integer multiplication and division"),
>> +    MISA_EXT_INFO('S', "s", "Supervisor-level instructions"),
>> +    MISA_EXT_INFO('U', "u", "User-level instructions"),
>> +    MISA_EXT_INFO('H', "h", "Hypervisor"),
>> +    MISA_EXT_INFO('J', "x-j", "Dynamic translated languages"),
>> +    MISA_EXT_INFO('V', "v", "Vector operations"),
>> +    MISA_EXT_INFO('G', "g", "General purpose (IMAFD_Zicsr_Zifencei)"),
>> +};
>> +
>> +const char *riscv_get_misa_ext_name(uint32_t bit)
>> +{
>> +    return misa_ext_info_arr[ctz32(bit)].name;
>> +}
>> +
>> +const char *riscv_get_misa_ext_descr(uint32_t bit)
> 
> nit: I'd prefer riscv_get_misa_ext_description() for the name.
> 
>> +{
>> +    return misa_ext_info_arr[ctz32(bit)].description;
>> +}
>> +
>> +#define MISA_CFG(_bit, _enabled) \
>> +    {.misa_bit = _bit, .enabled = _enabled}
>> +
>> +static RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
>> +    MISA_CFG(RVA, true),
>> +    MISA_CFG(RVC, true),
>> +    MISA_CFG(RVD, true),
>> +    MISA_CFG(RVF, true),
>> +    MISA_CFG(RVI, true),
>> +    MISA_CFG(RVE, false),
>> +    MISA_CFG(RVM, true),
>> +    MISA_CFG(RVS, true),
>> +    MISA_CFG(RVU, true),
>> +    MISA_CFG(RVH, true),
>> +    MISA_CFG(RVJ, false),
>> +    MISA_CFG(RVV, false),
>> +    MISA_CFG(RVG, false),
>>   };
>>   
>>   static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>> @@ -1592,7 +1616,10 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>>       int i;
>>   
>>       for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
>> -        const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
>> +        RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
>> +
>> +        misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit);
>> +        misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit);
> 
> This might be necessary for the KVM side, but we should be able to avoid
> this runtime setting here where the compiler can be certain everything is
> a constant. We just need the old MISA_CFG() back, slightly tweaked to use
> __builtin_ctz().
> 
>>   
>>           object_property_add(cpu_obj, misa_cfg->name, "bool",
>>                               cpu_get_misa_ext_cfg,
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index cc20ee25a7..acae118b9b 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -41,7 +41,10 @@
>>   
>>   #define RV(x) ((target_ulong)1 << (x - 'A'))
>>   
>> -/* Consider updating misa_ext_cfgs[] when adding new MISA bits here */
>> +/*
>> + * Consider updating misa_ext_info_arr[] and misa_ext_cfgs[]
>> + * when adding new MISA bits here.
>> + */
>>   #define RVI RV('I')
>>   #define RVE RV('E') /* E and I are mutually exclusive */
>>   #define RVM RV('M')
>> @@ -56,6 +59,8 @@
>>   #define RVJ RV('J')
>>   #define RVG RV('G')
>>   
>> +const char *riscv_get_misa_ext_name(uint32_t bit);
>> +const char *riscv_get_misa_ext_descr(uint32_t bit);
>>   
>>   /* Privileged specification version */
>>   enum {
>> -- 
>> 2.41.0
>>
> 
> Thanks,
> drew

Re: [PATCH v6 11/20] target/riscv/cpu: add misa_ext_info_arr[]

[Daniel Henrique Barboza]

On 6/29/23 08:36, Daniel Henrique Barboza wrote:
> 
> 
> On 6/29/23 04:26, Philippe Mathieu-Daudé wrote:
>> On 28/6/23 23:30, Daniel Henrique Barboza wrote:
>>> Next patch will add KVM specific user properties for both MISA and
>>> multi-letter extensions. For MISA extensions we want to make use of what
>>> is already available in misa_ext_cfgs[] to avoid code repetition.
>>>
>>> misa_ext_info_arr[] array will hold name and description for each MISA
>>> extension that misa_ext_cfgs[] is declaring. We'll then use this new
>>> array in KVM code to avoid duplicating strings.
>>>
>>> There's nothing holding us back from doing the same with multi-letter
>>> extensions. For now doing just with MISA extensions is enough.
>>>
>>> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> ---
>>>   target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++----------------
>>>   target/riscv/cpu.h |  7 +++-
>>>   2 files changed, 61 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>> index 2485e820f8..90dd2078ae 100644
>>> --- a/target/riscv/cpu.c
>>> +++ b/target/riscv/cpu.c
>>> @@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
>>>       visit_type_bool(v, name, &value, errp);
>>>   }
>>> -static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
>>> -    {.name = "a", .description = "Atomic instructions",
>>> -     .misa_bit = RVA, .enabled = true},
>>> -    {.name = "c", .description = "Compressed instructions",
>>> -     .misa_bit = RVC, .enabled = true},
>>> -    {.name = "d", .description = "Double-precision float point",
>>> -     .misa_bit = RVD, .enabled = true},
>>> -    {.name = "f", .description = "Single-precision float point",
>>> -     .misa_bit = RVF, .enabled = true},
>>> -    {.name = "i", .description = "Base integer instruction set",
>>> -     .misa_bit = RVI, .enabled = true},
>>> -    {.name = "e", .description = "Base integer instruction set (embedded)",
>>> -     .misa_bit = RVE, .enabled = false},
>>> -    {.name = "m", .description = "Integer multiplication and division",
>>> -     .misa_bit = RVM, .enabled = true},
>>> -    {.name = "s", .description = "Supervisor-level instructions",
>>> -     .misa_bit = RVS, .enabled = true},
>>> -    {.name = "u", .description = "User-level instructions",
>>> -     .misa_bit = RVU, .enabled = true},
>>> -    {.name = "h", .description = "Hypervisor",
>>> -     .misa_bit = RVH, .enabled = true},
>>> -    {.name = "x-j", .description = "Dynamic translated languages",
>>> -     .misa_bit = RVJ, .enabled = false},
>>> -    {.name = "v", .description = "Vector operations",
>>> -     .misa_bit = RVV, .enabled = false},
>>> -    {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)",
>>> -     .misa_bit = RVG, .enabled = false},
>>> +typedef struct misa_ext_info {
>>> +    const char *name;
>>> +    const char *description;
>>> +} MISAExtInfo;
>>> +
>>> +#define MISA_EXT_INFO(_idx, _propname, _descr) \
>>> +    [(_idx - 'A')] = {.name = _propname, .description = _descr}
>>> +
>>> +static const MISAExtInfo misa_ext_info_arr[] = {
>>> +    MISA_EXT_INFO('A', "a", "Atomic instructions"),
>>> +    MISA_EXT_INFO('C', "c", "Compressed instructions"),
>>> +    MISA_EXT_INFO('D', "d", "Double-precision float point"),
>>> +    MISA_EXT_INFO('F', "f", "Single-precision float point"),
>>> +    MISA_EXT_INFO('I', "i", "Base integer instruction set"),
>>> +    MISA_EXT_INFO('E', "e", "Base integer instruction set (embedded)"),
>>> +    MISA_EXT_INFO('M', "m", "Integer multiplication and division"),
>>> +    MISA_EXT_INFO('S', "s", "Supervisor-level instructions"),
>>> +    MISA_EXT_INFO('U', "u", "User-level instructions"),
>>> +    MISA_EXT_INFO('H', "h", "Hypervisor"),
>>> +    MISA_EXT_INFO('J', "x-j", "Dynamic translated languages"),
>>> +    MISA_EXT_INFO('V', "v", "Vector operations"),
>>> +    MISA_EXT_INFO('G', "g", "General purpose (IMAFD_Zicsr_Zifencei)"),
>>> +};
>>> +
>>> +const char *riscv_get_misa_ext_name(uint32_t bit)
>>> +{
>>
>> Is that OK to return NULL, or should we assert for
>> unimplemented bit/feature?

It's easier to assert out if name or description is NULL (meaning that we don't
implement the bit).


>>
>>> +    return misa_ext_info_arr[ctz32(bit)].name;
>>> +}
>>> +
>>> +const char *riscv_get_misa_ext_descr(uint32_t bit)
>>> +{
>>> +    return misa_ext_info_arr[ctz32(bit)].description;
>>
>> Ditto.
>>
>>> +}
>>> +
>>> +#define MISA_CFG(_bit, _enabled) \
>>> +    {.misa_bit = _bit, .enabled = _enabled}
>>> +
>>> +static RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
>>
>> 'const'
> 
> Not sure why I got rid of 'const' here. I'll reintroduce it.

Just remembered why. 'name' and 'description' are being initialized during runtime, so
the array can't be 'const'.

If we managed to init everything in the macro like Drew suggested we can keep it 'const'.


Daniel

> 
> 
> Daniel
> 
>>
>>> +    MISA_CFG(RVA, true),
>>> +    MISA_CFG(RVC, true),
>>> +    MISA_CFG(RVD, true),
>>> +    MISA_CFG(RVF, true),
>>> +    MISA_CFG(RVI, true),
>>> +    MISA_CFG(RVE, false),
>>> +    MISA_CFG(RVM, true),
>>> +    MISA_CFG(RVS, true),
>>> +    MISA_CFG(RVU, true),
>>> +    MISA_CFG(RVH, true),
>>> +    MISA_CFG(RVJ, false),
>>> +    MISA_CFG(RVV, false),
>>> +    MISA_CFG(RVG, false),
>>>   };
>>>   static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>>> @@ -1592,7 +1616,10 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>>>       int i;
>>>       for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
>>> -        const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
>>> +        RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
>>
>> const
>>
>>> +
>>> +        misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit);
>>> +        misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit);
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>

Re: [PATCH v6 12/20] target/riscv: add KVM specific MISA properties

[Daniel Henrique Barboza]

On 6/29/23 06:12, Andrew Jones wrote:
> On Wed, Jun 28, 2023 at 06:30:25PM -0300, Daniel Henrique Barboza wrote:
>> Using all TCG user properties in KVM is tricky. First because KVM
>> supports only a small subset of what TCG provides, so most of the
>> cpu->cfg flags do nothing for KVM.
>>
>> Second, and more important, we don't have a way of telling if any given
>> value is an user input or not. For TCG this has a small impact since we
>> just validating everything and error out if needed. But for KVM it would
>> be good to know if a given value was set by the user or if it's a value
>> already provided by KVM. Otherwise we don't know how to handle failed
>> kvm_set_one_regs() when writing the configurations back.
>>
>> These characteristics make it overly complicated to use the same user
>> facing flags for both KVM and TCG. A simpler approach is to create KVM
>> specific properties that have specialized logic, forking KVM and TCG use
>> cases for those cases only. Fully separating KVM/TCG properties is
>> unneeded at this point - in fact we want the user experience to be as
>> equal as possible, regardless of the acceleration chosen.
>>
>> We'll start this fork with the MISA properties, adding the MISA bits
>> that the KVM driver currently supports. A new KVMCPUConfig type is
>> introduced. It'll hold general information about an extension. For MISA
>> extensions we're going to use the newly created getters of
>> misa_ext_infos[] to populate their name and description. 'offset' holds
>> the MISA bit (RVA, RVC, ...). We're calling it 'offset' instead of
>> 'misa_bit' because this same KVMCPUConfig struct will be used to
>> multi-letter extensions later on.
>>
>> This new type also holds a 'user_set' flag. This flag will be set when
>> the user set an option that's different than what is already configured
>> in the host, requiring KVM intervention to write the regs back during
>> kvm_arch_init_vcpu(). Similar mechanics will be implemented for
>> multi-letter extensions as well.
>>
>> There is no need to duplicate more code than necessary, so we're going
>> to use the existing kvm_riscv_init_user_properties() to add the KVM
>> specific properties. Any code that is adding a TCG user prop is then
>> changed slightly to verify first if there's a KVM prop with the same
>> name already added.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c | 13 +++++---
>>   target/riscv/kvm.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 87 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 90dd2078ae..f4b1868466 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1617,14 +1617,19 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>>   
>>       for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
>>           RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
>> +        Error *local_err = NULL;
>>   
>>           misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit);
>>           misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit);
>>   
>> -        object_property_add(cpu_obj, misa_cfg->name, "bool",
>> -                            cpu_get_misa_ext_cfg,
>> -                            cpu_set_misa_ext_cfg,
>> -                            NULL, (void *)misa_cfg);
>> +        object_property_try_add(cpu_obj, misa_cfg->name, "bool",
>> +                                cpu_get_misa_ext_cfg, cpu_set_misa_ext_cfg,
>> +                                NULL, (void *)misa_cfg, &local_err);
>> +        if (local_err) {
>> +            /* Someone (KVM) already created the property */
>> +            continue;
>> +        }
> 
> This assumes object_property_try_add() only fails when it detects
> duplicate properties. That's currently true, but it's not documented,
> so I'm not sure we should count on it. Also, if we do want to assume
> only duplicate properties generate errors, then we can pass NULL for
> errp and just check the return value of the call, as it'll be NULL on
> failure.

At this moment only a duplicate property returns NULL, so this works. But then, if this
function changes in the future and more conditions return NULL, we might not be sure if
we should ignore the NULL return instead of erroring out.

If we want to be on the safe side I believe we should give up this idea and go back to
what it was before. We'll safely ignore if there's a duplicate because we'll test for
it, and then object_property_add() can error_fatal in any other error.

Thanks,

Daniel

> 
>> +
>>           object_property_set_description(cpu_obj, misa_cfg->name,
>>                                           misa_cfg->description);
>>           object_property_set_bool(cpu_obj, misa_cfg->name,
>> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
>> index 4d0808cb9a..0fb63cced3 100644
>> --- a/target/riscv/kvm.c
>> +++ b/target/riscv/kvm.c
>> @@ -22,8 +22,10 @@
>>   #include <linux/kvm.h>
>>   
>>   #include "qemu/timer.h"
>> +#include "qapi/error.h"
>>   #include "qemu/error-report.h"
>>   #include "qemu/main-loop.h"
>> +#include "qapi/visitor.h"
>>   #include "sysemu/sysemu.h"
>>   #include "sysemu/kvm.h"
>>   #include "sysemu/kvm_int.h"
>> @@ -105,6 +107,81 @@ static uint64_t kvm_riscv_reg_id(CPURISCVState *env, uint64_t type,
>>           } \
>>       } while (0)
>>   
>> +typedef struct KVMCPUConfig {
>> +    const char *name;
>> +    const char *description;
>> +    target_ulong offset;
>> +    int kvm_reg_id;
>> +    bool user_set;
>> +} KVMCPUConfig;
>> +
>> +#define KVM_MISA_CFG(_bit, _reg_id) \
>> +    {.offset = _bit, .kvm_reg_id = _reg_id}
>> +
>> +/* KVM ISA extensions */
>> +static KVMCPUConfig kvm_misa_ext_cfgs[] = {
>> +    KVM_MISA_CFG(RVA, KVM_RISCV_ISA_EXT_A),
>> +    KVM_MISA_CFG(RVC, KVM_RISCV_ISA_EXT_C),
>> +    KVM_MISA_CFG(RVD, KVM_RISCV_ISA_EXT_D),
>> +    KVM_MISA_CFG(RVF, KVM_RISCV_ISA_EXT_F),
>> +    KVM_MISA_CFG(RVH, KVM_RISCV_ISA_EXT_H),
>> +    KVM_MISA_CFG(RVI, KVM_RISCV_ISA_EXT_I),
>> +    KVM_MISA_CFG(RVM, KVM_RISCV_ISA_EXT_M),
>> +};
>> +
>> +static void kvm_cpu_set_misa_ext_cfg(Object *obj, Visitor *v,
>> +                                     const char *name,
>> +                                     void *opaque, Error **errp)
>> +{
>> +    KVMCPUConfig *misa_ext_cfg = opaque;
>> +    target_ulong misa_bit = misa_ext_cfg->offset;
>> +    RISCVCPU *cpu = RISCV_CPU(obj);
>> +    CPURISCVState *env = &cpu->env;
>> +    bool value, host_bit;
>> +
>> +    if (!visit_type_bool(v, name, &value, errp)) {
>> +        return;
>> +    }
>> +
>> +    host_bit = env->misa_ext_mask & misa_bit;
>> +
>> +    if (value == host_bit) {
>> +        return;
>> +    }
>> +
>> +    if (!value) {
>> +        misa_ext_cfg->user_set = true;
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * Forbid users to enable extensions that aren't
>> +     * available in the hart.
>> +     */
>> +    error_setg(errp, "Enabling MISA bit '%s' is not allowed: it's not "
>> +               "enabled in the host", misa_ext_cfg->name);
>> +}
>> +
>> +static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(kvm_misa_ext_cfgs); i++) {
>> +        KVMCPUConfig *misa_cfg = &kvm_misa_ext_cfgs[i];
>> +        int bit = misa_cfg->offset;
>> +
>> +        misa_cfg->name = riscv_get_misa_ext_name(bit);
>> +        misa_cfg->description = riscv_get_misa_ext_descr(bit);
>> +
>> +        object_property_add(cpu_obj, misa_cfg->name, "bool",
>> +                            NULL,
>> +                            kvm_cpu_set_misa_ext_cfg,
>> +                            NULL, misa_cfg);
>> +        object_property_set_description(cpu_obj, misa_cfg->name,
>> +                                        misa_cfg->description);
>> +    }
>> +}
>> +
>>   static int kvm_riscv_get_regs_core(CPUState *cs)
>>   {
>>       int ret = 0;
>> @@ -427,6 +504,7 @@ void kvm_riscv_init_user_properties(Object *cpu_obj)
>>           return;
>>       }
>>   
>> +    kvm_riscv_add_cpu_user_properties(cpu_obj);
>>       kvm_riscv_init_machine_ids(cpu, &kvmcpu);
>>       kvm_riscv_init_misa_ext_mask(cpu, &kvmcpu);
>>   
>> -- 
>> 2.41.0
>>
> 
> Thanks,
> drew

Re: [PATCH v6 11/20] target/riscv/cpu: add misa_ext_info_arr[]

[Daniel Henrique Barboza]

Drew,

On 6/29/23 05:59, Andrew Jones wrote:
> On Wed, Jun 28, 2023 at 06:30:24PM -0300, Daniel Henrique Barboza wrote:
>> Next patch will add KVM specific user properties for both MISA and
>> multi-letter extensions. For MISA extensions we want to make use of what
>> is already available in misa_ext_cfgs[] to avoid code repetition.
>>
>> misa_ext_info_arr[] array will hold name and description for each MISA
>> extension that misa_ext_cfgs[] is declaring. We'll then use this new
>> array in KVM code to avoid duplicating strings.
>>
>> There's nothing holding us back from doing the same with multi-letter
>> extensions. For now doing just with MISA extensions is enough.
>>
>> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++----------------
>>   target/riscv/cpu.h |  7 +++-
>>   2 files changed, 61 insertions(+), 29 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 2485e820f8..90dd2078ae 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
>>       visit_type_bool(v, name, &value, errp);
>>   }
>>   
>> -static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
>> -    {.name = "a", .description = "Atomic instructions",
>> -     .misa_bit = RVA, .enabled = true},
>> -    {.name = "c", .description = "Compressed instructions",
>> -     .misa_bit = RVC, .enabled = true},
>> -    {.name = "d", .description = "Double-precision float point",
>> -     .misa_bit = RVD, .enabled = true},
>> -    {.name = "f", .description = "Single-precision float point",
>> -     .misa_bit = RVF, .enabled = true},
>> -    {.name = "i", .description = "Base integer instruction set",
>> -     .misa_bit = RVI, .enabled = true},
>> -    {.name = "e", .description = "Base integer instruction set (embedded)",
>> -     .misa_bit = RVE, .enabled = false},
>> -    {.name = "m", .description = "Integer multiplication and division",
>> -     .misa_bit = RVM, .enabled = true},
>> -    {.name = "s", .description = "Supervisor-level instructions",
>> -     .misa_bit = RVS, .enabled = true},
>> -    {.name = "u", .description = "User-level instructions",
>> -     .misa_bit = RVU, .enabled = true},
>> -    {.name = "h", .description = "Hypervisor",
>> -     .misa_bit = RVH, .enabled = true},
>> -    {.name = "x-j", .description = "Dynamic translated languages",
>> -     .misa_bit = RVJ, .enabled = false},
>> -    {.name = "v", .description = "Vector operations",
>> -     .misa_bit = RVV, .enabled = false},
>> -    {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)",
>> -     .misa_bit = RVG, .enabled = false},
>> +typedef struct misa_ext_info {
>> +    const char *name;
>> +    const char *description;
>> +} MISAExtInfo;
>> +
>> +#define MISA_EXT_INFO(_idx, _propname, _descr) \
>> +    [(_idx - 'A')] = {.name = _propname, .description = _descr}
> 
> We don't have to give up on passing RV* to this macro. Directly
> using __builtin_ctz() with a constant should work, i.e.
> 
>   #define MISA_EXT_INFO(_bit, _propname, _descr) \
>       [__builtin_ctz(_bit)] = {.name = _propname, .description = _descr}
> 
> and then
> 
>   MISA_EXT_INFO(RVA, "a", "Atomic instructions"),
>   MISA_EXT_INFO(RVD, "d", "Double-precision float point"),
>   ...
> 
> (We don't need the ctz32() wrapper since we know we'll never input zero to
> __builtin_ctz().)

I run the series through gitlab because I got worried about this change in different
compilers and so on. And in fact I fear that we break 'clang-user' with it:

https://gitlab.com/danielhb/qemu/-/jobs/4569265199

u.c.o -c ../target/riscv/cpu.c
../target/riscv/cpu.c:1624:5: error: initializer element is not a compile-time constant
     MISA_CFG(RVA, true),
     ^~~~~~~~~~~~~~~~~~~
../target/riscv/cpu.c:1619:53: note: expanded from macro 'MISA_CFG'
     {.name = misa_ext_info_arr[MISA_INFO_IDX(_bit)].name, \
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
1 error generated.
[1503/2619] Compiling C object libqemu-ppc64le-linux-user.fa.p/linux-user_syscall.c.o


Which is a shame because gcc and everyone else is okay with it, but 'clang-user' and
'tsan-build' runners are complaining about it.

Unless there's a directive to make clang accept this code (I didn't find any) we'll
need to keep updating name and description during runtime, and we'll have to keep
removing 'const' from misa_ext_cfgs[].


Thanks,


Daniel


> 
>> +
>> +static const MISAExtInfo misa_ext_info_arr[] = {
>> +    MISA_EXT_INFO('A', "a", "Atomic instructions"),
>> +    MISA_EXT_INFO('C', "c", "Compressed instructions"),
>> +    MISA_EXT_INFO('D', "d", "Double-precision float point"),
>> +    MISA_EXT_INFO('F', "f", "Single-precision float point"),
>> +    MISA_EXT_INFO('I', "i", "Base integer instruction set"),
>> +    MISA_EXT_INFO('E', "e", "Base integer instruction set (embedded)"),
>> +    MISA_EXT_INFO('M', "m", "Integer multiplication and division"),
>> +    MISA_EXT_INFO('S', "s", "Supervisor-level instructions"),
>> +    MISA_EXT_INFO('U', "u", "User-level instructions"),
>> +    MISA_EXT_INFO('H', "h", "Hypervisor"),
>> +    MISA_EXT_INFO('J', "x-j", "Dynamic translated languages"),
>> +    MISA_EXT_INFO('V', "v", "Vector operations"),
>> +    MISA_EXT_INFO('G', "g", "General purpose (IMAFD_Zicsr_Zifencei)"),
>> +};
>> +
>> +const char *riscv_get_misa_ext_name(uint32_t bit)
>> +{
>> +    return misa_ext_info_arr[ctz32(bit)].name;
>> +}
>> +
>> +const char *riscv_get_misa_ext_descr(uint32_t bit)
> 
> nit: I'd prefer riscv_get_misa_ext_description() for the name.
> 
>> +{
>> +    return misa_ext_info_arr[ctz32(bit)].description;
>> +}
>> +
>> +#define MISA_CFG(_bit, _enabled) \
>> +    {.misa_bit = _bit, .enabled = _enabled}
>> +
>> +static RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
>> +    MISA_CFG(RVA, true),
>> +    MISA_CFG(RVC, true),
>> +    MISA_CFG(RVD, true),
>> +    MISA_CFG(RVF, true),
>> +    MISA_CFG(RVI, true),
>> +    MISA_CFG(RVE, false),
>> +    MISA_CFG(RVM, true),
>> +    MISA_CFG(RVS, true),
>> +    MISA_CFG(RVU, true),
>> +    MISA_CFG(RVH, true),
>> +    MISA_CFG(RVJ, false),
>> +    MISA_CFG(RVV, false),
>> +    MISA_CFG(RVG, false),
>>   };
>>   
>>   static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>> @@ -1592,7 +1616,10 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>>       int i;
>>   
>>       for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
>> -        const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
>> +        RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
>> +
>> +        misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit);
>> +        misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit);
> 
> This might be necessary for the KVM side, but we should be able to avoid
> this runtime setting here where the compiler can be certain everything is
> a constant. We just need the old MISA_CFG() back, slightly tweaked to use
> __builtin_ctz().
> 
>>   
>>           object_property_add(cpu_obj, misa_cfg->name, "bool",
>>                               cpu_get_misa_ext_cfg,
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index cc20ee25a7..acae118b9b 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -41,7 +41,10 @@
>>   
>>   #define RV(x) ((target_ulong)1 << (x - 'A'))
>>   
>> -/* Consider updating misa_ext_cfgs[] when adding new MISA bits here */
>> +/*
>> + * Consider updating misa_ext_info_arr[] and misa_ext_cfgs[]
>> + * when adding new MISA bits here.
>> + */
>>   #define RVI RV('I')
>>   #define RVE RV('E') /* E and I are mutually exclusive */
>>   #define RVM RV('M')
>> @@ -56,6 +59,8 @@
>>   #define RVJ RV('J')
>>   #define RVG RV('G')
>>   
>> +const char *riscv_get_misa_ext_name(uint32_t bit);
>> +const char *riscv_get_misa_ext_descr(uint32_t bit);
>>   
>>   /* Privileged specification version */
>>   enum {
>> -- 
>> 2.41.0
>>
> 
> Thanks,
> drew

Re: [PATCH v6 11/20] target/riscv/cpu: add misa_ext_info_arr[]

[Andrew Jones]

On Thu, Jun 29, 2023 at 07:04:28PM -0300, Daniel Henrique Barboza wrote:
> Drew,
> 
> On 6/29/23 05:59, Andrew Jones wrote:
> > On Wed, Jun 28, 2023 at 06:30:24PM -0300, Daniel Henrique Barboza wrote:
> > > Next patch will add KVM specific user properties for both MISA and
> > > multi-letter extensions. For MISA extensions we want to make use of what
> > > is already available in misa_ext_cfgs[] to avoid code repetition.
> > > 
> > > misa_ext_info_arr[] array will hold name and description for each MISA
> > > extension that misa_ext_cfgs[] is declaring. We'll then use this new
> > > array in KVM code to avoid duplicating strings.
> > > 
> > > There's nothing holding us back from doing the same with multi-letter
> > > extensions. For now doing just with MISA extensions is enough.
> > > 
> > > Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > > ---
> > >   target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++----------------
> > >   target/riscv/cpu.h |  7 +++-
> > >   2 files changed, 61 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 2485e820f8..90dd2078ae 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
> > >       visit_type_bool(v, name, &value, errp);
> > >   }
> > > -static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
> > > -    {.name = "a", .description = "Atomic instructions",
> > > -     .misa_bit = RVA, .enabled = true},
> > > -    {.name = "c", .description = "Compressed instructions",
> > > -     .misa_bit = RVC, .enabled = true},
> > > -    {.name = "d", .description = "Double-precision float point",
> > > -     .misa_bit = RVD, .enabled = true},
> > > -    {.name = "f", .description = "Single-precision float point",
> > > -     .misa_bit = RVF, .enabled = true},
> > > -    {.name = "i", .description = "Base integer instruction set",
> > > -     .misa_bit = RVI, .enabled = true},
> > > -    {.name = "e", .description = "Base integer instruction set (embedded)",
> > > -     .misa_bit = RVE, .enabled = false},
> > > -    {.name = "m", .description = "Integer multiplication and division",
> > > -     .misa_bit = RVM, .enabled = true},
> > > -    {.name = "s", .description = "Supervisor-level instructions",
> > > -     .misa_bit = RVS, .enabled = true},
> > > -    {.name = "u", .description = "User-level instructions",
> > > -     .misa_bit = RVU, .enabled = true},
> > > -    {.name = "h", .description = "Hypervisor",
> > > -     .misa_bit = RVH, .enabled = true},
> > > -    {.name = "x-j", .description = "Dynamic translated languages",
> > > -     .misa_bit = RVJ, .enabled = false},
> > > -    {.name = "v", .description = "Vector operations",
> > > -     .misa_bit = RVV, .enabled = false},
> > > -    {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)",
> > > -     .misa_bit = RVG, .enabled = false},
> > > +typedef struct misa_ext_info {
> > > +    const char *name;
> > > +    const char *description;
> > > +} MISAExtInfo;
> > > +
> > > +#define MISA_EXT_INFO(_idx, _propname, _descr) \
> > > +    [(_idx - 'A')] = {.name = _propname, .description = _descr}
> > 
> > We don't have to give up on passing RV* to this macro. Directly
> > using __builtin_ctz() with a constant should work, i.e.
> > 
> >   #define MISA_EXT_INFO(_bit, _propname, _descr) \
> >       [__builtin_ctz(_bit)] = {.name = _propname, .description = _descr}
> > 
> > and then
> > 
> >   MISA_EXT_INFO(RVA, "a", "Atomic instructions"),
> >   MISA_EXT_INFO(RVD, "d", "Double-precision float point"),
> >   ...
> > 
> > (We don't need the ctz32() wrapper since we know we'll never input zero to
> > __builtin_ctz().)
> 
> I run the series through gitlab because I got worried about this change in different
> compilers and so on. And in fact I fear that we break 'clang-user' with it:
> 
> https://gitlab.com/danielhb/qemu/-/jobs/4569265199
> 
> u.c.o -c ../target/riscv/cpu.c
> ../target/riscv/cpu.c:1624:5: error: initializer element is not a compile-time constant
>     MISA_CFG(RVA, true),
>     ^~~~~~~~~~~~~~~~~~~
> ../target/riscv/cpu.c:1619:53: note: expanded from macro 'MISA_CFG'
>     {.name = misa_ext_info_arr[MISA_INFO_IDX(_bit)].name, \
>              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> 1 error generated.
> [1503/2619] Compiling C object libqemu-ppc64le-linux-user.fa.p/linux-user_syscall.c.o
> 
> 
> Which is a shame because gcc and everyone else is okay with it, but 'clang-user' and
> 'tsan-build' runners are complaining about it.
> 
> Unless there's a directive to make clang accept this code (I didn't find any) we'll
> need to keep updating name and description during runtime, and we'll have to keep
> removing 'const' from misa_ext_cfgs[].
>

Yeah, that's a pity, and odd that any compiler wouldn't be able to
identify that a constant input to a builtin linear function produces
another constant...

Oh well, we can only be as good as the tools we use...

Thanks,
drew

Re: [PATCH v6 02/20] hw/riscv/virt.c: skip 'mmu-type' FDT if satp mode not set

[Michael Tokarev]

29.06.2023 00:30, Daniel Henrique Barboza wrote:
> The absence of a satp mode in riscv_host_cpu_init() is causing the
> following 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
> **
> ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should not be
> reached
> Bail out! ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should
> not be reached
> Aborted

Hi!

Not a review/comment for the change itself, but a question about your
work environment.

Why do you run qemu with sudo?  Is it just because your user lacks access
to /dev/kvm device node (which is fixed by adding it to kvm group) ?

I find it a bit worrying to see people run development as root and the
recipes to run it as root ens up in even in commit messages.  I think
it's not good practice to do it like this, but more important is to
teach users to do it this way. And this is more serious than one might
think.

Thanks,

/mjt

Re: [PATCH v6 02/20] hw/riscv/virt.c: skip 'mmu-type' FDT if satp mode not set

[Daniel Henrique Barboza]

On 6/30/23 04:36, Michael Tokarev wrote:
> 29.06.2023 00:30, Daniel Henrique Barboza wrote:
>> The absence of a satp mode in riscv_host_cpu_init() is causing the
>> following 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
>> **
>> ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should not be
>> reached
>> Bail out! ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should
>> not be reached
>> Aborted
> 
> Hi!
> 
> Not a review/comment for the change itself, but a question about your
> work environment.
> 
> Why do you run qemu with sudo?  Is it just because your user lacks access
> to /dev/kvm device node (which is fixed by adding it to kvm group) ?

Yes, it's because of /dev/kvm device access. These KVM tests were done in
an emulated environment that don't have UAC properly set.

> 
> I find it a bit worrying to see people run development as root and the
> recipes to run it as root ens up in even in commit messages.  I think
> it's not good practice to do it like this, but more important is to
> teach users to do it this way. And this is more serious than one might
> think.

Just removed all 'sudo' references from commit msgs for the next version.


Daniel

> 
> Thanks,
> 
> /mjt

Re: [PATCH v6 02/20] hw/riscv/virt.c: skip 'mmu-type' FDT if satp mode not set

[Michael Tokarev]

30.06.2023 10:46, Daniel Henrique Barboza wrote:

> On 6/30/23 04:36, Michael Tokarev wrote:
..
>> I find it a bit worrying to see people run development as root and the
>> recipes to run it as root ens up in even in commit messages.  I think
>> it's not good practice to do it like this, but more important is to
>> teach users to do it this way. And this is more serious than one might
>> think.
> 
> Just removed all 'sudo' references from commit msgs for the next version.

Thank you Daniel!

/mjt