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

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

[Daniel Henrique Barboza]

Hi,

This is a series that does a lot of design changes w.r.t KVM support in
the RISC-V machines. 

The most straightforward patches are patches 1 and 2, which are bug fixes
that allows the KVM 'host' CPU to boot.

For the remaining 14 patches I'll describe in text form the changes
intended:

- machine_ids (mvendorid, marchid, mimpid) changes

  - for named CPUs: can't be changed from the value previously set by
    their cpu_init()
  - for the KVM 'host' CPU: can't be changed from the host value
  - for generic CPUs (both TCG and KVM): can be changed at will, as
    long as marchid has a valid value


- KVM extension discovery during init

  We want extensions to be validated in user option callbacks to
  alleaviate the work we need to do during kvm_arch_init_vcpu().

  This can only be done if we discover host capabilities sooner than
  realize() time. We're borrowing a solution from ARM called 'scratch
  CPU', where we initialize KVM fds to be able to read host capabilities
  and discard the CPU afterwards. This scratch CPU is spinned during
  cpu_init() time to fill in default values for KVM extensions. It's
  also possible to error out right away depending on the errors found
  during this process.

- KVM specific user options

  The current design of user options for TCG is not enough to implement
  error handling in KVM because we don't know whether the user set an
  specific option or not. This is crucial to determine whether we should
  error out or keep going when facing a KVM side error.

  KVM options have a 'user_set' flag to determine if the user changed
  that specific config option or not. They also have specific KVM
  attributes such as kvm_reg_id and 'supported' that are also used for
  error handling.

- KVM extensions error handling changes

  We determine if a case is serious enough to error out or not based on
  two factors: user input and KVM support. KVM support is determined by
  the error code that KVM might thrown our way. RISC-V KVM uses 'EINVAL'
  to tell that a given reg is not available to be read/written by the
  kvm_get/set_one_reg() ioctl. Searching for the EINVAL error allow us
  to determine if a given extension is not supported during the KVM
  feature discovery step.

  As a rule we don't error out in two scenarios:

  - The user is setting an extension to the same value as present in the
    host. In this case we just ignore the option altogether;
  - The user is disabling an extension (i.e. turning off) that is not
    supported by KVM. E.g the user sets 'zbb=false' and KVM is not able
    to read/write 'zbb'. We don't error out because this is a common
    case that can happen when running QEMU with older KVM versions.

  We error out in the following scenariosn:

  - All errors that happens when user tries to enable an extension.
    Failing to enable an extension is bad because there are user
    expectations with enabled extensions and how the guest will behave; 

  - User disables an extension that KVM support and KVM throws an error
    when trying to disable it. We rrror out because, unlike the case
    where KVM is alien to the extension, this indicates either a KVM
    limitation or any other error that the user should be aware of. For
    instance, as of Linux 6.4 rc1, RISC-V KVM doesn't allow 'ssaia',
    'sstc', 'svinval', 'zihintpause' and 'zbb' to be disabled. Thus a
    failure in disabling 'zbb' is something that is worth aborting the
    launch.

- Difference between single-letter and multi-letter extensions handling

  We don't support enabling a single-letter extension that is not
  already enabled in the host. In other words we just support disabling
  existing single-letter extensions.

  The same doesn't apply for multi-letter extensions because, although
  it's not the case for KVM at this moment, KVM is likely to have cases
  where certain extensions are disabled by vendors but can be enabled by
  users that want to try it out. We're already supporting this case to
  avoid changing the design later.


Things that are known to be missing/pending:

- named CPU KVM support: I want to forbid this use case entirely, but
  refrained to do it in this series because it's a bit too overloaded
  with changes as is;

- documentation changes: it would be nice to have these design decisions
  documented. I'll work in it as a follow-up.


Daniel Henrique Barboza (16):
  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: 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: adapt 'riscv_isa_string' for KVM
  target/riscv: update multi-letter extension KVM properties
  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                            | 171 +++++-
 target/riscv/kvm.c                            | 553 +++++++++++++++++-
 target/riscv/kvm_riscv.h                      |   3 +
 18 files changed, 855 insertions(+), 51 deletions(-)

-- 
2.40.1

[PATCH 01/16] 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 KVM CPUs.
Validating the feature set for those CPUs is a KVM problem that should
be handled in KVM specific code.

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

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

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 938c7bd87b..72f5433776 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -331,6 +331,15 @@ static void set_satp_mode_default_map(RISCVCPU *cpu)
 }
 #endif
 
+static bool riscv_running_KVM(void)
+{
+#ifndef CONFIG_USER_ONLY
+    return kvm_enabled();
+#else
+    return false;
+#endif
+}
+
 static void riscv_any_cpu_init(Object *obj)
 {
     RISCVCPU *cpu = RISCV_CPU(obj);
@@ -1295,20 +1304,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_features(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);
@@ -1354,6 +1355,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 (!riscv_running_KVM()) {
+        riscv_cpu_realize_features(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.40.1

[PATCH 02/16] 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.

Skip the 'mmu-type' FDT node if there's no satp_mode set.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 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 4e3efbee16..8aa907e81f 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.40.1

[PATCH 03/16] 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>
---
 target/riscv/cpu.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 72f5433776..bcd69bb032 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1723,7 +1723,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),
 
@@ -1810,6 +1809,32 @@ 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 riscv_cpu_class_init(ObjectClass *c, void *data)
 {
     RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
@@ -1841,6 +1866,10 @@ 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", NULL,
+                              cpu_set_mvendorid,
+                              NULL, NULL);
+
     device_class_set_props(dc, riscv_cpu_properties);
 }
 
-- 
2.40.1

[PATCH 04/16] 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>
---
 target/riscv/cpu.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index bcd69bb032..ed3b33343c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -42,7 +42,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";
 
@@ -1724,7 +1723,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),
@@ -1835,6 +1833,27 @@ static void cpu_set_mvendorid(Object *obj, Visitor *v, const char *name,
     cpu->cfg.mvendorid = value;
 }
 
+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 riscv_cpu_class_init(ObjectClass *c, void *data)
 {
     RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
@@ -1870,6 +1889,10 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
                               cpu_set_mvendorid,
                               NULL, NULL);
 
+    object_class_property_add(c, "mimpid", "uint64", NULL,
+                              cpu_set_mimpid,
+                              NULL, NULL);
+
     device_class_set_props(dc, riscv_cpu_properties);
 }
 
-- 
2.40.1

[PATCH 05/16] 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>
---
 target/riscv/cpu.c | 53 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 7 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index ed3b33343c..d6e23bfd83 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -38,11 +38,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 {
@@ -1722,8 +1717,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
@@ -1854,6 +1847,48 @@ static void cpu_set_mimpid(Object *obj, Visitor *v, const char *name,
     cpu->cfg.mimpid = value;
 }
 
+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 riscv_cpu_class_init(ObjectClass *c, void *data)
 {
     RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
@@ -1893,6 +1928,10 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
                               cpu_set_mimpid,
                               NULL, NULL);
 
+    object_class_property_add(c, "marchid", "uint64", NULL,
+                              cpu_set_marchid,
+                              NULL, NULL);
+
     device_class_set_props(dc, riscv_cpu_properties);
 }
 
-- 
2.40.1

[PATCH 06/16] 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>
---
 target/riscv/cpu.c       |  4 ++
 target/riscv/kvm.c       | 85 ++++++++++++++++++++++++++++++++++++++++
 target/riscv/kvm_riscv.h |  1 +
 3 files changed, 90 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index d6e23bfd83..749d8bf5eb 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1703,6 +1703,10 @@ static void riscv_cpu_add_user_properties(Object *obj)
     Property *prop;
     DeviceState *dev = DEVICE(obj);
 
+    if (riscv_running_KVM()) {
+        kvm_riscv_init_user_properties(obj);
+    }
+
     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.40.1

[PATCH 07/16] 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>
---
 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.40.1

[PATCH 08/16] 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>
---
 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.40.1

[PATCH 09/16] 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>
---
 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.40.1

[PATCH 10/16] 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>
---
 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.40.1

[PATCH 11/16] 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. The KVM version of
RISCVCPUMisaExtConfig and kvm_misa_ext_cfgs[] are inspired by the
existing RISCVCPUMisaExtConfig and misa_ext_cfgs[] from
target/riscv/cpu.c. For KVM  we're adding an extra oomph in
RISCVCPUMisaExtConfig with the 'user_set' boolean. 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().

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 | 10 ++++++
 target/riscv/kvm.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 749d8bf5eb..3c348049a3 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1587,6 +1587,11 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
     for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
         const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
 
+        /* Check if KVM didn't create the property already */
+        if (object_property_find(cpu_obj, misa_cfg->name)) {
+            continue;
+        }
+
         object_property_add(cpu_obj, misa_cfg->name, "bool",
                             cpu_get_misa_ext_cfg,
                             cpu_set_misa_ext_cfg,
@@ -1710,6 +1715,11 @@ static void riscv_cpu_add_user_properties(Object *obj)
     riscv_cpu_add_misa_properties(obj);
 
     for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
+        /* Check if KVM didn't create the property already */
+        if (object_property_find(obj, prop->name)) {
+            continue;
+        }
+
         qdev_property_add_static(dev, prop);
     }
 
diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index 4d0808cb9a..6afd56cda5 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 RISCVCPUMisaExtConfig {
+    const char *name;
+    const char *description;
+    target_ulong misa_bit;
+    int kvm_reg_id;
+    bool user_set;
+} RISCVCPUMisaExtConfig;
+
+/* KVM ISA extensions */
+static RISCVCPUMisaExtConfig kvm_misa_ext_cfgs[] = {
+    {.name = "a", .description = "Atomic instructions",
+     .misa_bit = RVA, .kvm_reg_id = KVM_RISCV_ISA_EXT_A},
+    {.name = "c", .description = "Compressed instructions",
+     .misa_bit = RVC, .kvm_reg_id = KVM_RISCV_ISA_EXT_C},
+    {.name = "d", .description = "Double-precision float point",
+     .misa_bit = RVD, .kvm_reg_id = KVM_RISCV_ISA_EXT_D},
+    {.name = "f", .description = "Single-precision float point",
+     .misa_bit = RVF, .kvm_reg_id = KVM_RISCV_ISA_EXT_F},
+    {.name = "h", .description = "Hypervisor",
+     .misa_bit = RVH, .kvm_reg_id = KVM_RISCV_ISA_EXT_H},
+    {.name = "i", .description = "Base integer instruction set",
+     .misa_bit = RVI, .kvm_reg_id = KVM_RISCV_ISA_EXT_I},
+    {.name = "m", .description = "Integer multiplication and division",
+     .misa_bit = RVM, .kvm_reg_id = KVM_RISCV_ISA_EXT_M},
+};
+
+static void kvm_cpu_set_misa_ext_cfg(Object *obj, Visitor *v,
+                                     const char *name,
+                                     void *opaque, Error **errp)
+{
+    RISCVCPUMisaExtConfig *misa_ext_cfg = opaque;
+    target_ulong misa_bit = misa_ext_cfg->misa_bit;
+    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++) {
+        RISCVCPUMisaExtConfig *misa_cfg = &kvm_misa_ext_cfgs[i];
+
+        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.40.1

[PATCH 12/16] 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.

For MISA bits we're going to be a little more conservative: we won't
even try enabling bits that aren't already available in the host. The
ioctl() is so likely to fail that'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. If it fails we need to verify the
error code. -EINVAL indicates that KVM doesn't know about the reg, so
re-enable the extension in env->misa_ext and keep booting. If it fails
for any other reason we're going to exit out.

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

diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index 6afd56cda5..bb1dafe263 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -166,6 +166,42 @@ 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++) {
+        RISCVCPUMisaExtConfig *misa_cfg = &kvm_misa_ext_cfgs[i];
+
+        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) {
+            if (ret == -EINVAL) {
+                /*
+                 * KVM doesn't know how to handle this bit. Since
+                 * it's an extension that the user wants to disable,
+                 * do not error out.
+                 */
+                continue;
+            } else {
+                error_report("Unable to set KVM reg %s, error %d",
+                             misa_cfg->name, ret);
+                exit(EXIT_FAILURE);
+            }
+        }
+        env->misa_ext &= ~misa_cfg->misa_bit;
+    }
+}
+
 static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
 {
     int i;
@@ -632,8 +668,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.40.1

[PATCH 13/16] 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 the recently added MISA properties we're also going to add a
'user_set' flag in each of them. The flag will be set only if the user
chose an option that's different from the host and will require extra
handling from the KVM driver.

However, multi-letter CPUs 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 'supported' flag will then be used later on to give 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/kvm.c | 136 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 136 insertions(+)

diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index bb1dafe263..b4193a10d8 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -202,6 +202,99 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs)
     }
 }
 
+typedef struct RISCVCPUMultiExtConfig {
+    const char *name;
+    int kvm_reg_id;
+    int cpu_cfg_offset;
+    bool supported;
+    bool user_set;
+} RISCVCPUMultiExtConfig;
+
+#define CPUCFG(_prop) offsetof(struct RISCVCPUConfig, _prop)
+
+/*
+ * KVM ISA Multi-letter extensions. We care about the order
+ * since it'll be used to create the ISA string later on.
+ * We follow the same ordering rules of isa_edata_arr[]
+ * from target/riscv/cpu.c.
+ */
+static RISCVCPUMultiExtConfig kvm_multi_ext_cfgs[] = {
+    {.name = "zicbom", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZICBOM,
+     .cpu_cfg_offset = CPUCFG(ext_icbom)},
+    {.name = "zicboz", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZICBOZ,
+     .cpu_cfg_offset = CPUCFG(ext_icboz)},
+    {.name = "zihintpause", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZIHINTPAUSE,
+     .cpu_cfg_offset = CPUCFG(ext_zihintpause)},
+    {.name = "zbb", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZBB,
+     .cpu_cfg_offset = CPUCFG(ext_zbb)},
+    {.name = "ssaia", .kvm_reg_id = KVM_RISCV_ISA_EXT_SSAIA,
+     .cpu_cfg_offset = CPUCFG(ext_ssaia)},
+    {.name = "sstc", .kvm_reg_id = KVM_RISCV_ISA_EXT_SSTC,
+     .cpu_cfg_offset = CPUCFG(ext_sstc)},
+    {.name = "svinval", .kvm_reg_id = KVM_RISCV_ISA_EXT_SVINVAL,
+     .cpu_cfg_offset = CPUCFG(ext_svinval)},
+    {.name = "svpbmt", .kvm_reg_id = KVM_RISCV_ISA_EXT_SVPBMT,
+     .cpu_cfg_offset = CPUCFG(ext_svpbmt)},
+};
+
+static void kvm_cpu_cfg_set(RISCVCPU *cpu, RISCVCPUMultiExtConfig *multi_ext,
+                            uint32_t val)
+{
+    int cpu_cfg_offset = multi_ext->cpu_cfg_offset;
+    bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
+
+    *ext_enabled = val;
+}
+
+static uint32_t kvm_cpu_cfg_get(RISCVCPU *cpu,
+                                RISCVCPUMultiExtConfig *multi_ext)
+{
+    int cpu_cfg_offset = multi_ext->cpu_cfg_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)
+{
+    RISCVCPUMultiExtConfig *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;
@@ -216,6 +309,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++) {
+        RISCVCPUMultiExtConfig *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)
@@ -531,6 +633,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++) {
+        RISCVCPUMultiExtConfig *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);
@@ -543,6 +678,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.40.1

[PATCH 14/16] target/riscv: adapt 'riscv_isa_string' for KVM

[Daniel Henrique Barboza]

KVM is not using the same attributes as TCG, i.e. it doesn't use
isa_edata_arr[]. Add a new kvm_riscv_isa_string_ext() helper that does
basically the same thing, but using KVM internals instead.

The decision to add this helper target/riscv/kvm.c is to foster the
separation between KVM and TCG logic, while still using
riscv_isa_string_ext() from target/riscv/cpu.c to retrieve the string
to not overcomplicate things.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c       |  5 +++++
 target/riscv/kvm.c       | 19 +++++++++++++++++++
 target/riscv/kvm_riscv.h |  2 ++
 3 files changed, 26 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 3c348049a3..ec1d0c621a 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1956,6 +1956,11 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
     char *new = *isa_str;
     int i;
 
+    if (riscv_running_KVM()) {
+        kvm_riscv_isa_string_ext(cpu, isa_str, max_str_len);
+        return;
+    }
+
     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])) {
diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index b4193a10d8..675e18df3b 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -320,6 +320,25 @@ static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
     }
 }
 
+void kvm_riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
+                              int max_str_len)
+{
+    char *old = *isa_str;
+    char *new = *isa_str;
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(kvm_multi_ext_cfgs); i++) {
+        RISCVCPUMultiExtConfig *multi_ext_cfg = &kvm_multi_ext_cfgs[i];
+        if (kvm_cpu_cfg_get(cpu, multi_ext_cfg)) {
+            new = g_strconcat(old, "_", multi_ext_cfg->name, NULL);
+            g_free(old);
+            old = new;
+        }
+    }
+
+    *isa_str = new;
+}
+
 static int kvm_riscv_get_regs_core(CPUState *cs)
 {
     int ret = 0;
diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
index e3ba935808..1a12efa8db 100644
--- a/target/riscv/kvm_riscv.h
+++ b/target/riscv/kvm_riscv.h
@@ -20,6 +20,8 @@
 #define QEMU_KVM_RISCV_H
 
 void kvm_riscv_init_user_properties(Object *cpu_obj);
+void kvm_riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
+                              int max_str_len);
 void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
 void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);
 
-- 
2.40.1

[PATCH 15/16] 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>
---
 target/riscv/kvm.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index 675e18df3b..92b99fe261 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -295,6 +295,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++) {
+        RISCVCPUMultiExtConfig *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;
@@ -829,6 +855,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.40.1

[PATCH 16/16] 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 use them. We'll also expose them as user flags like it is
already done with TCG.

However, in contrast with what happens with TCG, the user is not able to
set any value that is different from the 'host' value. And KVM can be
harsh dealing with it: a ENOTSUPP can be thrown for the mere attempt of
executing kvm_set_one_reg() for these 2 regs.

We'll read the 'host' value and use it to set these values, regardless of
user choice. If the user happened to chose a different value, error out.
We'll also error out if we failed to read the block sizes.

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

diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index 92b99fe261..7789d835e5 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -241,8 +241,16 @@ static void kvm_cpu_cfg_set(RISCVCPU *cpu, RISCVCPUMultiExtConfig *multi_ext,
                             uint32_t val)
 {
     int cpu_cfg_offset = multi_ext->cpu_cfg_offset;
-    bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
+    uint16_t *blocksize;
+    bool *ext_enabled;
 
+    if (strstr(multi_ext->name, "blocksize")) {
+        blocksize = (void *)&cpu->cfg + cpu_cfg_offset;
+        *blocksize = val;
+        return;
+    }
+
+    ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
     *ext_enabled = val;
 }
 
@@ -250,8 +258,15 @@ static uint32_t kvm_cpu_cfg_get(RISCVCPU *cpu,
                                 RISCVCPUMultiExtConfig *multi_ext)
 {
     int cpu_cfg_offset = multi_ext->cpu_cfg_offset;
-    bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
+    uint16_t *blocksize;
+    bool *ext_enabled;
 
+    if (strstr(multi_ext->name, "blocksize")) {
+        blocksize = (void *)&cpu->cfg + cpu_cfg_offset;
+        return *blocksize;
+    }
+
+    ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
     return *ext_enabled;
 }
 
@@ -295,6 +310,33 @@ static void kvm_cpu_set_multi_ext_cfg(Object *obj, Visitor *v,
     kvm_cpu_cfg_set(cpu, multi_ext_cfg, value);
 }
 
+/*
+ * We'll avoid extra complexity by always assuming this
+ * array order with cbom first.
+ */
+static RISCVCPUMultiExtConfig kvm_cbomz_blksize_cfgs[] = {
+    {.name = "cbom_blocksize", .cpu_cfg_offset = CPUCFG(cbom_blocksize),
+     .kvm_reg_id = KVM_REG_RISCV_CONFIG_REG(zicbom_block_size)},
+    {.name = "cboz_blocksize", .cpu_cfg_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)
+{
+    RISCVCPUMultiExtConfig *cbomz_size_cfg = opaque;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    uint16_t value;
+
+    if (!visit_type_uint16(v, name, &value, errp)) {
+        return;
+    }
+
+    cbomz_size_cfg->user_set = true;
+    kvm_cpu_cfg_set(cpu, cbomz_size_cfg, value);
+}
+
 static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
 {
     CPURISCVState *env = &cpu->env;
@@ -321,6 +363,45 @@ static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
     }
 }
 
+static void kvm_riscv_finalize_features(RISCVCPU *cpu, CPUState *cs)
+{
+    CPURISCVState *env = &cpu->env;
+    uint64_t id, reg;
+    int i, ret;
+
+    for (i = 0; i < ARRAY_SIZE(kvm_cbomz_blksize_cfgs); i++) {
+        RISCVCPUMultiExtConfig *cbomz_cfg = &kvm_cbomz_blksize_cfgs[i];
+        uint64_t host_val;
+
+        if ((i == 0 && !cpu->cfg.ext_icbom) ||
+            (i == 1 && !cpu->cfg.ext_icboz)) {
+            continue;
+        }
+
+        id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
+                              cbomz_cfg->kvm_reg_id);
+
+        ret = kvm_get_one_reg(cs, id, &host_val);
+        if (ret != 0) {
+            error_report("Unable to read KVM reg val %s, error %d",
+                         cbomz_cfg->name, ret);
+            exit(EXIT_FAILURE);
+        }
+
+        if (cbomz_cfg->user_set) {
+            reg = kvm_cpu_cfg_get(cpu, cbomz_cfg);
+            if (reg != host_val) {
+                error_report("Unable to set %s to a different value than "
+                             "the host (%lu)",
+                             cbomz_cfg->name, host_val);
+                exit(EXIT_FAILURE);
+            }
+        }
+
+        kvm_cpu_cfg_set(cpu, cbomz_cfg, host_val);
+    }
+}
+
 static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
 {
     int i;
@@ -344,6 +425,14 @@ static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
                             kvm_cpu_set_multi_ext_cfg,
                             NULL, multi_cfg);
     }
+
+    for (i = 0; i < ARRAY_SIZE(kvm_cbomz_blksize_cfgs); i++) {
+        RISCVCPUMultiExtConfig *cbomz_size_cfg = &kvm_cbomz_blksize_cfgs[i];
+
+        object_property_add(cpu_obj, cbomz_size_cfg->name, "uint16",
+                            NULL, kvm_cpu_set_cbomz_blksize,
+                            NULL, cbomz_size_cfg);
+    }
 }
 
 void kvm_riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
@@ -856,6 +945,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);
+    kvm_riscv_finalize_features(cpu, cs);
 
     return ret;
 }
-- 
2.40.1

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

[Alistair Francis]

On Wed, May 31, 2023 at 5:49 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> 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 KVM CPUs.
> Validating the feature set for those CPUs is a KVM problem that should
> be handled in KVM specific code.
>
> The new riscv_cpu_realize_features() helper contains all validation
> logic that are not applicable to KVM CPUs. riscv_cpu_realize() verifies
> if we're dealing with a KVM CPU and, if not, execute the new helper to
> proceed with the usual realize() logic for all other CPUs.
>
> [1] lib/sbi/sbi_hart.c, hart_detect_features()
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.c | 41 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 938c7bd87b..72f5433776 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -331,6 +331,15 @@ static void set_satp_mode_default_map(RISCVCPU *cpu)
>  }
>  #endif
>
> +static bool riscv_running_KVM(void)
> +{
> +#ifndef CONFIG_USER_ONLY
> +    return kvm_enabled();
> +#else
> +    return false;
> +#endif
> +}
> +
>  static void riscv_any_cpu_init(Object *obj)
>  {
>      RISCVCPU *cpu = RISCV_CPU(obj);
> @@ -1295,20 +1304,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_features(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);
> @@ -1354,6 +1355,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 (!riscv_running_KVM()) {
> +        riscv_cpu_realize_features(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.40.1
>
>

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

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:08PM -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 KVM CPUs.
> Validating the feature set for those CPUs is a KVM problem that should
> be handled in KVM specific code.
> 
> The new riscv_cpu_realize_features() helper contains all validation
> logic that are not applicable to KVM CPUs. riscv_cpu_realize() verifies
> if we're dealing with a KVM CPU and, if not, execute the new helper to
> proceed with the usual realize() logic for all other CPUs.
> 
> [1] lib/sbi/sbi_hart.c, hart_detect_features()
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c | 41 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 938c7bd87b..72f5433776 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -331,6 +331,15 @@ static void set_satp_mode_default_map(RISCVCPU *cpu)
>  }
>  #endif
>  
> +static bool riscv_running_KVM(void)

KVM should be lowercase

> +{
> +#ifndef CONFIG_USER_ONLY
> +    return kvm_enabled();
> +#else
> +    return false;
> +#endif
> +}
> +
>  static void riscv_any_cpu_init(Object *obj)
>  {
>      RISCVCPU *cpu = RISCV_CPU(obj);
> @@ -1295,20 +1304,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_features(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);
> @@ -1354,6 +1355,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 (!riscv_running_KVM()) {
> +        riscv_cpu_realize_features(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.40.1
> 
>

Otherwise,

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

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

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:09PM -0300, 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
> 
> 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.
> 
> Skip the 'mmu-type' FDT node if there's no satp_mode set.

This seems reasonable, since mmu-type is not a required node, according to
[1], and there's nothing we can put in it, which would be correct, until
we can get the 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>
> ---
>  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 4e3efbee16..8aa907e81f 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.40.1
> 
>

Adding a sentence, like what I wrote above, to the commit message in order
to provide better justification might be nice, but either way

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

Thanks,
drew

Re: [PATCH 03/16] target/riscv/cpu.c: restrict 'mvendorid' value

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:10PM -0300, Daniel Henrique Barboza wrote:
> 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>
> ---
>  target/riscv/cpu.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 72f5433776..bcd69bb032 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1723,7 +1723,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),
>  
> @@ -1810,6 +1809,32 @@ 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 riscv_cpu_class_init(ObjectClass *c, void *data)
>  {
>      RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> @@ -1841,6 +1866,10 @@ 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", NULL,
> +                              cpu_set_mvendorid,
> +                              NULL, NULL);
> +

Shouldn't we provide a get function as well?

>      device_class_set_props(dc, riscv_cpu_properties);
>  }
>  
> -- 
> 2.40.1
> 
> 

Otherwise,

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

Re: [PATCH 04/16] target/riscv/cpu.c: restrict 'mimpid' value

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:11PM -0300, Daniel Henrique Barboza wrote:
> 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>
> ---
>  target/riscv/cpu.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index bcd69bb032..ed3b33343c 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -42,7 +42,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";
>  
> @@ -1724,7 +1723,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),
> @@ -1835,6 +1833,27 @@ static void cpu_set_mvendorid(Object *obj, Visitor *v, const char *name,
>      cpu->cfg.mvendorid = value;
>  }
>  
> +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 riscv_cpu_class_init(ObjectClass *c, void *data)
>  {
>      RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> @@ -1870,6 +1889,10 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
>                                cpu_set_mvendorid,
>                                NULL, NULL);
>  
> +    object_class_property_add(c, "mimpid", "uint64", NULL,
> +                              cpu_set_mimpid,
> +                              NULL, NULL);
> +

Same, shouldn't we also define 'get' comment as the last patch.

>      device_class_set_props(dc, riscv_cpu_properties);
>  }
>  
> -- 
> 2.40.1
> 
>

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

Re: [PATCH 05/16] target/riscv/cpu.c: restrict 'marchid' value

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:12PM -0300, Daniel Henrique Barboza wrote:
> '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>
> ---
>  target/riscv/cpu.c | 53 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index ed3b33343c..d6e23bfd83 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -38,11 +38,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 {
> @@ -1722,8 +1717,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
> @@ -1854,6 +1847,48 @@ static void cpu_set_mimpid(Object *obj, Visitor *v, const char *name,
>      cpu->cfg.mimpid = value;
>  }
>  
> +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 riscv_cpu_class_init(ObjectClass *c, void *data)
>  {
>      RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> @@ -1893,6 +1928,10 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
>                                cpu_set_mimpid,
>                                NULL, NULL);
>  
> +    object_class_property_add(c, "marchid", "uint64", NULL,
> +                              cpu_set_marchid,
> +                              NULL, NULL);
> +

get?

>      device_class_set_props(dc, riscv_cpu_properties);
>  }
>  
> -- 
> 2.40.1
> 
>

Otherwise,

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

Re: [PATCH 06/16] target/riscv: use KVM scratch CPUs to init KVM properties

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:13PM -0300, Daniel Henrique Barboza wrote:
> 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>
> ---
>  target/riscv/cpu.c       |  4 ++
>  target/riscv/kvm.c       | 85 ++++++++++++++++++++++++++++++++++++++++
>  target/riscv/kvm_riscv.h |  1 +
>  3 files changed, 90 insertions(+)
>

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

Re: [PATCH 07/16] target/riscv: read marchid/mimpid in kvm_riscv_init_machine_ids()

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:14PM -0300, Daniel Henrique Barboza wrote:
> 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>
> ---
>  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.40.1
> 
>

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

Re: [PATCH 08/16] target/riscv: handle mvendorid/marchid/mimpid for KVM CPUs

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:15PM -0300, Daniel Henrique Barboza wrote:
> 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>
> ---
>  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.40.1
> 
>

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

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

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:17PM -0300, Daniel Henrique Barboza wrote:
> 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>
> ---
>  target/riscv/kvm.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
>

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

Re: [PATCH 03/16] target/riscv/cpu.c: restrict 'mvendorid' value

[Daniel Henrique Barboza]

On 6/6/23 10:19, Andrew Jones wrote:
> On Tue, May 30, 2023 at 04:46:10PM -0300, Daniel Henrique Barboza wrote:
>> 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>
>> ---
>>   target/riscv/cpu.c | 31 ++++++++++++++++++++++++++++++-
>>   1 file changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 72f5433776..bcd69bb032 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1723,7 +1723,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),
>>   
>> @@ -1810,6 +1809,32 @@ 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 riscv_cpu_class_init(ObjectClass *c, void *data)
>>   {
>>       RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
>> @@ -1841,6 +1866,10 @@ 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", NULL,
>> +                              cpu_set_mvendorid,
>> +                              NULL, NULL);
>> +
> 
> Shouldn't we provide a get function as well?

We can. I refrain from adding a get() interface because I didn't add new code that
access mvendorid via object_property_get_uint(OBJECT(cpu), "mvendorid", errp). The
code that access this value uses cpu->cfg.mvendorid directly.

It would be interesting to add a get() interface if cpu->cfg.mvendorid was a value
that was read in a different manner that it's stored. It's not the case ATM, so
I only added the set() interface.


Thanks,


Daniel



> 
>>       device_class_set_props(dc, riscv_cpu_properties);
>>   }
>>   
>> -- 
>> 2.40.1
>>
>>
> 
> Otherwise,
> 
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

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

[Daniel Henrique Barboza]

On 6/6/23 10:13, Andrew Jones wrote:
> On Tue, May 30, 2023 at 04:46:09PM -0300, 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
>>
>> 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.
>>
>> Skip the 'mmu-type' FDT node if there's no satp_mode set.
> 
> This seems reasonable, since mmu-type is not a required node, according to
> [1], and there's nothing we can put in it, which would be correct, until
> we can get the 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>
>> ---
>>   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 4e3efbee16..8aa907e81f 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.40.1
>>
>>
> 
> Adding a sentence, like what I wrote above, to the commit message in order
> to provide better justification might be nice, but either way
> 
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks! Commit message amended for v2 as follows:


(....)

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




Daniel


> 
> Thanks,
> drew

Re: [PATCH 11/16] target/riscv: add KVM specific MISA properties

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:18PM -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. The KVM version of
> RISCVCPUMisaExtConfig and kvm_misa_ext_cfgs[] are inspired by the
> existing RISCVCPUMisaExtConfig and misa_ext_cfgs[] from
> target/riscv/cpu.c. For KVM  we're adding an extra oomph in
> RISCVCPUMisaExtConfig with the 'user_set' boolean. 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().
> 
> 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 | 10 ++++++
>  target/riscv/kvm.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 88 insertions(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 749d8bf5eb..3c348049a3 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1587,6 +1587,11 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>      for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
>          const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
>  
> +        /* Check if KVM didn't create the property already */
> +        if (object_property_find(cpu_obj, misa_cfg->name)) {
> +            continue;
> +        }
> +
>          object_property_add(cpu_obj, misa_cfg->name, "bool",
>                              cpu_get_misa_ext_cfg,
>                              cpu_set_misa_ext_cfg,
> @@ -1710,6 +1715,11 @@ static void riscv_cpu_add_user_properties(Object *obj)
>      riscv_cpu_add_misa_properties(obj);
>  
>      for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
> +        /* Check if KVM didn't create the property already */
> +        if (object_property_find(obj, prop->name)) {
> +            continue;
> +        }
> +
>          qdev_property_add_static(dev, prop);
>      }
>  
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 4d0808cb9a..6afd56cda5 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 RISCVCPUMisaExtConfig {

I'd give this a name with KVM in it.

> +    const char *name;
> +    const char *description;
> +    target_ulong misa_bit;
> +    int kvm_reg_id;
> +    bool user_set;
> +} RISCVCPUMisaExtConfig;
> +
> +/* KVM ISA extensions */
> +static RISCVCPUMisaExtConfig kvm_misa_ext_cfgs[] = {
> +    {.name = "a", .description = "Atomic instructions",
> +     .misa_bit = RVA, .kvm_reg_id = KVM_RISCV_ISA_EXT_A},
> +    {.name = "c", .description = "Compressed instructions",
> +     .misa_bit = RVC, .kvm_reg_id = KVM_RISCV_ISA_EXT_C},
> +    {.name = "d", .description = "Double-precision float point",
> +     .misa_bit = RVD, .kvm_reg_id = KVM_RISCV_ISA_EXT_D},
> +    {.name = "f", .description = "Single-precision float point",
> +     .misa_bit = RVF, .kvm_reg_id = KVM_RISCV_ISA_EXT_F},
> +    {.name = "h", .description = "Hypervisor",
> +     .misa_bit = RVH, .kvm_reg_id = KVM_RISCV_ISA_EXT_H},
> +    {.name = "i", .description = "Base integer instruction set",
> +     .misa_bit = RVI, .kvm_reg_id = KVM_RISCV_ISA_EXT_I},
> +    {.name = "m", .description = "Integer multiplication and division",
> +     .misa_bit = RVM, .kvm_reg_id = KVM_RISCV_ISA_EXT_M},
> +};

I'm not a huge fan of duplicating the name and description strings. Maybe
we should put them in their own array, indexed by misa bit, in order to
share them.

 struct misa_ext_cfg_name {
     const char *name;
     const char *description;
 };

static const struct misa_ext_cfg_name misa_ext_cfg_names[] = {
    [RVA] = { "a", "Atomic instructions", },
    [RVC] = { "c", "Compressed instructions", },
    ...

#define MISA_CFG(_bit, _enabled) \
    {.name = misa_ext_cfg_names[_bit].name, \
     .description = misa_ext_cfg_names[_bit].description, \
     .misa_bit = _bit, .enabled = _enabled}

static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
    MISA_CFG(RVA, true),
    MISA_CFG(RVC, true),
    ...

#define KVM_MISA_CFG(_bit, _reg_id) \
    {.name = misa_ext_cfg_names[_bit].name,
     .description = misa_ext_cfg_names[_bit].description, \
     .misa_bit = _bit, .kvm_reg_id = _reg_id}

static const RISCVCPUKVMMisaExtConfig kvm_misa_ext_cfgs[] = {
    KVM_MISA_CFG(RVA, KVM_RISCV_ISA_EXT_A),
    KVM_MISA_CFG(RVC, KVM_RISCV_ISA_EXT_C),
    ...

> +
> +static void kvm_cpu_set_misa_ext_cfg(Object *obj, Visitor *v,
> +                                     const char *name,
> +                                     void *opaque, Error **errp)
> +{
> +    RISCVCPUMisaExtConfig *misa_ext_cfg = opaque;
> +    target_ulong misa_bit = misa_ext_cfg->misa_bit;
> +    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++) {
> +        RISCVCPUMisaExtConfig *misa_cfg = &kvm_misa_ext_cfgs[i];
> +
> +        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.40.1
> 
>

Otherwise, LGTM.

Thanks,
drew

Re: [PATCH 13/16] target/riscv/kvm.c: add multi-letter extension KVM properties

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:20PM -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 the recently added MISA properties we're also going to add a
> 'user_set' flag in each of them. The flag will be set only if the user
> chose an option that's different from the host and will require extra
> handling from the KVM driver.
> 
> However, multi-letter CPUs 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.

I wish that was ENOENT, but I suppose that ship sailed.

> 
> The 'supported' flag will then be used later on to give 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/kvm.c | 136 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 136 insertions(+)
> 
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index bb1dafe263..b4193a10d8 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -202,6 +202,99 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs)
>      }
>  }
>  
> +typedef struct RISCVCPUMultiExtConfig {
> +    const char *name;

No description? I'd prefer we use the same cfg struct for single-letter
and multi-letter extensions. We can use a union to overlap cpu_cfg_offset
and misa_bit.

> +    int kvm_reg_id;
> +    int cpu_cfg_offset;
> +    bool supported;
> +    bool user_set;
> +} RISCVCPUMultiExtConfig;
> +
> +#define CPUCFG(_prop) offsetof(struct RISCVCPUConfig, _prop)
> +
> +/*
> + * KVM ISA Multi-letter extensions. We care about the order
> + * since it'll be used to create the ISA string later on.
> + * We follow the same ordering rules of isa_edata_arr[]
> + * from target/riscv/cpu.c.
> + */
> +static RISCVCPUMultiExtConfig kvm_multi_ext_cfgs[] = {
> +    {.name = "zicbom", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZICBOM,
> +     .cpu_cfg_offset = CPUCFG(ext_icbom)},
> +    {.name = "zicboz", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZICBOZ,
> +     .cpu_cfg_offset = CPUCFG(ext_icboz)},
> +    {.name = "zihintpause", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZIHINTPAUSE,
> +     .cpu_cfg_offset = CPUCFG(ext_zihintpause)},
> +    {.name = "zbb", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZBB,
> +     .cpu_cfg_offset = CPUCFG(ext_zbb)},
> +    {.name = "ssaia", .kvm_reg_id = KVM_RISCV_ISA_EXT_SSAIA,
> +     .cpu_cfg_offset = CPUCFG(ext_ssaia)},
> +    {.name = "sstc", .kvm_reg_id = KVM_RISCV_ISA_EXT_SSTC,
> +     .cpu_cfg_offset = CPUCFG(ext_sstc)},
> +    {.name = "svinval", .kvm_reg_id = KVM_RISCV_ISA_EXT_SVINVAL,
> +     .cpu_cfg_offset = CPUCFG(ext_svinval)},
> +    {.name = "svpbmt", .kvm_reg_id = KVM_RISCV_ISA_EXT_SVPBMT,
> +     .cpu_cfg_offset = CPUCFG(ext_svpbmt)},

As pointed out in the last patch, it'd be nice to share names (and
descriptions) with TCG.

> +};
> +
> +static void kvm_cpu_cfg_set(RISCVCPU *cpu, RISCVCPUMultiExtConfig *multi_ext,
> +                            uint32_t val)
> +{
> +    int cpu_cfg_offset = multi_ext->cpu_cfg_offset;
> +    bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
> +
> +    *ext_enabled = val;
> +}
> +
> +static uint32_t kvm_cpu_cfg_get(RISCVCPU *cpu,
> +                                RISCVCPUMultiExtConfig *multi_ext)
> +{
> +    int cpu_cfg_offset = multi_ext->cpu_cfg_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)
> +{
> +    RISCVCPUMultiExtConfig *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;
> @@ -216,6 +309,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++) {
> +        RISCVCPUMultiExtConfig *multi_cfg = &kvm_multi_ext_cfgs[i];
> +
> +        object_property_add(cpu_obj, multi_cfg->name, "bool",
> +                            NULL,

You have a getter function, so we might as well set it here, no?

> +                            kvm_cpu_set_multi_ext_cfg,
> +                            NULL, multi_cfg);
> +    }
>  }
>  
>  static int kvm_riscv_get_regs_core(CPUState *cs)
> @@ -531,6 +633,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++) {
> +        RISCVCPUMultiExtConfig *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);

I don't think we usually bother cleaning up when exiting, we just let exit
do it, but OK.

> +                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);
> @@ -543,6 +678,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.40.1
> 
>

Thanks,
drew

Re: [PATCH 12/16] target/riscv/kvm.c: update KVM MISA bits

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:19PM -0300, Daniel Henrique Barboza wrote:
> 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.
> 
> For MISA bits we're going to be a little more conservative: we won't
> even try enabling bits that aren't already available in the host. The
> ioctl() is so likely to fail that'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. If it fails we need to verify the
> error code. -EINVAL indicates that KVM doesn't know about the reg, so
> re-enable the extension in env->misa_ext and keep booting. If it fails

We shouldn't "re-enable the extension in env->misa_ext..." when the
extension isn't supported by KVM for any reason. But, assuming EINVAL
is only returned when KVM doesn't support the extension (I wish it
returned ENOENT instead), then we'll never get EINVAL here in update
anyway. env->misa_ext is initialized to what KVM supports, so it
wouldn't have had this unsupported extension bit set in the first
place, meaning 'user_set' wouldn't get set at property setting time
either.

> for any other reason we're going to exit out.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/kvm.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 6afd56cda5..bb1dafe263 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -166,6 +166,42 @@ 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++) {
> +        RISCVCPUMisaExtConfig *misa_cfg = &kvm_misa_ext_cfgs[i];
> +
> +        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) {
> +            if (ret == -EINVAL) {
> +                /*
> +                 * KVM doesn't know how to handle this bit. Since
> +                 * it's an extension that the user wants to disable,
> +                 * do not error out.
> +                 */
> +                continue;

This case can be replaced with a comment explaining we don't ever
expect EINVAL here at update time, since user_set will never be
true for user-disabled extensions which KVM doesn't support.

> +            } else {
> +                error_report("Unable to set KVM reg %s, error %d",
> +                             misa_cfg->name, ret);
> +                exit(EXIT_FAILURE);
> +            }
> +        }
> +        env->misa_ext &= ~misa_cfg->misa_bit;
> +    }
> +}
> +
>  static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
>  {
>      int i;
> @@ -632,8 +668,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.40.1
> 
>

Thanks,
drew

Re: [PATCH 14/16] target/riscv: adapt 'riscv_isa_string' for KVM

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:21PM -0300, Daniel Henrique Barboza wrote:
> KVM is not using the same attributes as TCG, i.e. it doesn't use
> isa_edata_arr[]. Add a new kvm_riscv_isa_string_ext() helper that does
> basically the same thing, but using KVM internals instead.
> 
> The decision to add this helper target/riscv/kvm.c is to foster the
> separation between KVM and TCG logic, while still using
> riscv_isa_string_ext() from target/riscv/cpu.c to retrieve the string
> to not overcomplicate things.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c       |  5 +++++
>  target/riscv/kvm.c       | 19 +++++++++++++++++++
>  target/riscv/kvm_riscv.h |  2 ++
>  3 files changed, 26 insertions(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 3c348049a3..ec1d0c621a 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1956,6 +1956,11 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
>      char *new = *isa_str;
>      int i;
>  
> +    if (riscv_running_KVM()) {
> +        kvm_riscv_isa_string_ext(cpu, isa_str, max_str_len);
> +        return;
> +    }
> +
>      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])) {
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index b4193a10d8..675e18df3b 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -320,6 +320,25 @@ static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
>      }
>  }
>  
> +void kvm_riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
> +                              int max_str_len)
> +{
> +    char *old = *isa_str;
> +    char *new = *isa_str;
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(kvm_multi_ext_cfgs); i++) {
> +        RISCVCPUMultiExtConfig *multi_ext_cfg = &kvm_multi_ext_cfgs[i];
> +        if (kvm_cpu_cfg_get(cpu, multi_ext_cfg)) {
> +            new = g_strconcat(old, "_", multi_ext_cfg->name, NULL);
> +            g_free(old);
> +            old = new;
> +        }
> +    }
> +
> +    *isa_str = new;
> +}
> +
>  static int kvm_riscv_get_regs_core(CPUState *cs)
>  {
>      int ret = 0;
> diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
> index e3ba935808..1a12efa8db 100644
> --- a/target/riscv/kvm_riscv.h
> +++ b/target/riscv/kvm_riscv.h
> @@ -20,6 +20,8 @@
>  #define QEMU_KVM_RISCV_H
>  
>  void kvm_riscv_init_user_properties(Object *cpu_obj);
> +void kvm_riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
> +                              int max_str_len);
>  void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
>  void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);
>  
> -- 
> 2.40.1
> 
>

Hmm, more duplication. I think we need an abstraction which can support
both TCG and KVM extension lists. Allowing functions like
riscv_isa_string_ext() to be shared for them.

Thanks,
drew

Re: [PATCH 15/16] target/riscv: update multi-letter extension KVM properties

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:22PM -0300, Daniel Henrique Barboza wrote:
> 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>
> ---
>  target/riscv/kvm.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 675e18df3b..92b99fe261 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -295,6 +295,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++) {
> +        RISCVCPUMultiExtConfig *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;
> @@ -829,6 +855,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.40.1
> 
>

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

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

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:23PM -0300, Daniel Henrique Barboza wrote:
> 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 use them. We'll also expose them as user flags like it is
> already done with TCG.
> 
> However, in contrast with what happens with TCG, the user is not able to
> set any value that is different from the 'host' value. And KVM can be
> harsh dealing with it: a ENOTSUPP can be thrown for the mere attempt of
> executing kvm_set_one_reg() for these 2 regs.
> 
> We'll read the 'host' value and use it to set these values, regardless of
> user choice. If the user happened to chose a different value, error out.
> We'll also error out if we failed to read the block sizes.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/kvm.c | 94 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 92 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 92b99fe261..7789d835e5 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -241,8 +241,16 @@ static void kvm_cpu_cfg_set(RISCVCPU *cpu, RISCVCPUMultiExtConfig *multi_ext,
>                              uint32_t val)
>  {
>      int cpu_cfg_offset = multi_ext->cpu_cfg_offset;
> -    bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
> +    uint16_t *blocksize;
> +    bool *ext_enabled;
>  
> +    if (strstr(multi_ext->name, "blocksize")) {
> +        blocksize = (void *)&cpu->cfg + cpu_cfg_offset;
> +        *blocksize = val;
> +        return;
> +    }

We should add 'get' accessors to each property and then always use those
accessors to get the values. Trying to share a single accessor across
properties, using the names to determine their sizes, is basically trying
to reinvent 'get' without the function pointer.

> +
> +    ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
>      *ext_enabled = val;
>  }
>  
> @@ -250,8 +258,15 @@ static uint32_t kvm_cpu_cfg_get(RISCVCPU *cpu,
>                                  RISCVCPUMultiExtConfig *multi_ext)
>  {
>      int cpu_cfg_offset = multi_ext->cpu_cfg_offset;
> -    bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
> +    uint16_t *blocksize;
> +    bool *ext_enabled;
>  
> +    if (strstr(multi_ext->name, "blocksize")) {
> +        blocksize = (void *)&cpu->cfg + cpu_cfg_offset;
> +        return *blocksize;
> +    }
> +
> +    ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
>      return *ext_enabled;
>  }
>  
> @@ -295,6 +310,33 @@ static void kvm_cpu_set_multi_ext_cfg(Object *obj, Visitor *v,
>      kvm_cpu_cfg_set(cpu, multi_ext_cfg, value);
>  }
>  
> +/*
> + * We'll avoid extra complexity by always assuming this
> + * array order with cbom first.
> + */
> +static RISCVCPUMultiExtConfig kvm_cbomz_blksize_cfgs[] = {

Hmm, yet another cfg struct type, and this one is specific to block sizes.
I'd rather we find a way to keep cfg definitions more general and then use
the same struct for all.

> +    {.name = "cbom_blocksize", .cpu_cfg_offset = CPUCFG(cbom_blocksize),
> +     .kvm_reg_id = KVM_REG_RISCV_CONFIG_REG(zicbom_block_size)},
> +    {.name = "cboz_blocksize", .cpu_cfg_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)
> +{
> +    RISCVCPUMultiExtConfig *cbomz_size_cfg = opaque;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    uint16_t value;
> +
> +    if (!visit_type_uint16(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    cbomz_size_cfg->user_set = true;
> +    kvm_cpu_cfg_set(cpu, cbomz_size_cfg, value);
> +}
> +
>  static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
>  {
>      CPURISCVState *env = &cpu->env;
> @@ -321,6 +363,45 @@ static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
>      }
>  }
>  
> +static void kvm_riscv_finalize_features(RISCVCPU *cpu, CPUState *cs)
> +{
> +    CPURISCVState *env = &cpu->env;
> +    uint64_t id, reg;
> +    int i, ret;
> +
> +    for (i = 0; i < ARRAY_SIZE(kvm_cbomz_blksize_cfgs); i++) {
> +        RISCVCPUMultiExtConfig *cbomz_cfg = &kvm_cbomz_blksize_cfgs[i];
> +        uint64_t host_val;
> +
> +        if ((i == 0 && !cpu->cfg.ext_icbom) ||
> +            (i == 1 && !cpu->cfg.ext_icboz)) {

Rather than the required array order and this magic index stuff, we can
just save the offset of the ext_* boolean in the cfg structure, like we
already do for the *_blocksize, and then check it here.

Also, I think we want to warn here if cbomz_cfg->user_set is set. If the
user set some block size, but disabled the extension, then they should be
told that the block size will be ignored. Letting them know it's ignored
is particularly good to do since we're not validating it. I.e. the user
shouldn't assume the block size they put there is worth anything at all.

> +            continue;
> +        }
> +
> +        id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
> +                              cbomz_cfg->kvm_reg_id);
> +
> +        ret = kvm_get_one_reg(cs, id, &host_val);
> +        if (ret != 0) {
> +            error_report("Unable to read KVM reg val %s, error %d",
> +                         cbomz_cfg->name, ret);
> +            exit(EXIT_FAILURE);
> +        }
> +
> +        if (cbomz_cfg->user_set) {
> +            reg = kvm_cpu_cfg_get(cpu, cbomz_cfg);
> +            if (reg != host_val) {
> +                error_report("Unable to set %s to a different value than "
> +                             "the host (%lu)",
> +                             cbomz_cfg->name, host_val);
> +                exit(EXIT_FAILURE);
> +            }
> +        }
> +
> +        kvm_cpu_cfg_set(cpu, cbomz_cfg, host_val);
> +    }
> +}
> +
>  static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
>  {
>      int i;
> @@ -344,6 +425,14 @@ static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
>                              kvm_cpu_set_multi_ext_cfg,
>                              NULL, multi_cfg);
>      }
> +
> +    for (i = 0; i < ARRAY_SIZE(kvm_cbomz_blksize_cfgs); i++) {
> +        RISCVCPUMultiExtConfig *cbomz_size_cfg = &kvm_cbomz_blksize_cfgs[i];
> +
> +        object_property_add(cpu_obj, cbomz_size_cfg->name, "uint16",
> +                            NULL, kvm_cpu_set_cbomz_blksize,
> +                            NULL, cbomz_size_cfg);
> +    }
>  }
>  
>  void kvm_riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
> @@ -856,6 +945,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);
> +    kvm_riscv_finalize_features(cpu, cs);
>  
>      return ret;
>  }
> -- 
> 2.40.1
> 
>

Thanks,
drew

Re: [PATCH 13/16] target/riscv/kvm.c: add multi-letter extension KVM properties

[Daniel Henrique Barboza]

On 6/7/23 08:48, Andrew Jones wrote:
> On Tue, May 30, 2023 at 04:46:20PM -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 the recently added MISA properties we're also going to add a
>> 'user_set' flag in each of them. The flag will be set only if the user
>> chose an option that's different from the host and will require extra
>> handling from the KVM driver.
>>
>> However, multi-letter CPUs 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.
> 
> I wish that was ENOENT, but I suppose that ship sailed.
> 
>>
>> The 'supported' flag will then be used later on to give 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/kvm.c | 136 +++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 136 insertions(+)
>>
>> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
>> index bb1dafe263..b4193a10d8 100644
>> --- a/target/riscv/kvm.c
>> +++ b/target/riscv/kvm.c
>> @@ -202,6 +202,99 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs)
>>       }
>>   }
>>   
>> +typedef struct RISCVCPUMultiExtConfig {
>> +    const char *name;
> 
> No description? I'd prefer we use the same cfg struct for single-letter
> and multi-letter extensions. We can use a union to overlap cpu_cfg_offset
> and misa_bit.

multi-letter extensions don't have a 'description' field in TCG. Nothing
prevents us from adding for KVM though.

And yes, I'll create a single struct to handle both MISA and multi-letter
extensions. We just need to have different getters/setters for each
category.

> 
>> +    int kvm_reg_id;
>> +    int cpu_cfg_offset;
>> +    bool supported;
>> +    bool user_set;
>> +} RISCVCPUMultiExtConfig;
>> +
>> +#define CPUCFG(_prop) offsetof(struct RISCVCPUConfig, _prop)
>> +
>> +/*
>> + * KVM ISA Multi-letter extensions. We care about the order
>> + * since it'll be used to create the ISA string later on.
>> + * We follow the same ordering rules of isa_edata_arr[]
>> + * from target/riscv/cpu.c.
>> + */
>> +static RISCVCPUMultiExtConfig kvm_multi_ext_cfgs[] = {
>> +    {.name = "zicbom", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZICBOM,
>> +     .cpu_cfg_offset = CPUCFG(ext_icbom)},
>> +    {.name = "zicboz", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZICBOZ,
>> +     .cpu_cfg_offset = CPUCFG(ext_icboz)},
>> +    {.name = "zihintpause", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZIHINTPAUSE,
>> +     .cpu_cfg_offset = CPUCFG(ext_zihintpause)},
>> +    {.name = "zbb", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZBB,
>> +     .cpu_cfg_offset = CPUCFG(ext_zbb)},
>> +    {.name = "ssaia", .kvm_reg_id = KVM_RISCV_ISA_EXT_SSAIA,
>> +     .cpu_cfg_offset = CPUCFG(ext_ssaia)},
>> +    {.name = "sstc", .kvm_reg_id = KVM_RISCV_ISA_EXT_SSTC,
>> +     .cpu_cfg_offset = CPUCFG(ext_sstc)},
>> +    {.name = "svinval", .kvm_reg_id = KVM_RISCV_ISA_EXT_SVINVAL,
>> +     .cpu_cfg_offset = CPUCFG(ext_svinval)},
>> +    {.name = "svpbmt", .kvm_reg_id = KVM_RISCV_ISA_EXT_SVPBMT,
>> +     .cpu_cfg_offset = CPUCFG(ext_svpbmt)},
> 
> As pointed out in the last patch, it'd be nice to share names (and
> descriptions) with TCG.

I believe it's ok to do that for the MISA bits since it's a handful of entries.

But I'd rather deal with a little code duplication for multi-letter extensions
for now. We have 73 multi-letters extensions in TCG. Adding a description for
each one, change how the properties are being created and so on seems too
much for this series.

> 
>> +};
>> +
>> +static void kvm_cpu_cfg_set(RISCVCPU *cpu, RISCVCPUMultiExtConfig *multi_ext,
>> +                            uint32_t val)
>> +{
>> +    int cpu_cfg_offset = multi_ext->cpu_cfg_offset;
>> +    bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
>> +
>> +    *ext_enabled = val;
>> +}
>> +
>> +static uint32_t kvm_cpu_cfg_get(RISCVCPU *cpu,
>> +                                RISCVCPUMultiExtConfig *multi_ext)
>> +{
>> +    int cpu_cfg_offset = multi_ext->cpu_cfg_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)
>> +{
>> +    RISCVCPUMultiExtConfig *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;
>> @@ -216,6 +309,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++) {
>> +        RISCVCPUMultiExtConfig *multi_cfg = &kvm_multi_ext_cfgs[i];
>> +
>> +        object_property_add(cpu_obj, multi_cfg->name, "bool",
>> +                            NULL,
> 
> You have a getter function, so we might as well set it here, no?

Yep, might as well set the get() callback in the property.


Thanks,


Daniel

> 
>> +                            kvm_cpu_set_multi_ext_cfg,
>> +                            NULL, multi_cfg);
>> +    }
>>   }
>>   
>>   static int kvm_riscv_get_regs_core(CPUState *cs)
>> @@ -531,6 +633,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++) {
>> +        RISCVCPUMultiExtConfig *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);
> 
> I don't think we usually bother cleaning up when exiting, we just let exit
> do it, but OK.
> 
>> +                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);
>> @@ -543,6 +678,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.40.1
>>
>>
> 
> Thanks,
> drew

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

[Daniel Henrique Barboza]

On 6/7/23 10:01, Andrew Jones wrote:
> On Tue, May 30, 2023 at 04:46:23PM -0300, Daniel Henrique Barboza wrote:
>> 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 use them. We'll also expose them as user flags like it is
>> already done with TCG.
>>
>> However, in contrast with what happens with TCG, the user is not able to
>> set any value that is different from the 'host' value. And KVM can be
>> harsh dealing with it: a ENOTSUPP can be thrown for the mere attempt of
>> executing kvm_set_one_reg() for these 2 regs.
>>
>> We'll read the 'host' value and use it to set these values, regardless of
>> user choice. If the user happened to chose a different value, error out.
>> We'll also error out if we failed to read the block sizes.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/kvm.c | 94 +++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 92 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
>> index 92b99fe261..7789d835e5 100644
>> --- a/target/riscv/kvm.c
>> +++ b/target/riscv/kvm.c
>> @@ -241,8 +241,16 @@ static void kvm_cpu_cfg_set(RISCVCPU *cpu, RISCVCPUMultiExtConfig *multi_ext,
>>                               uint32_t val)
>>   {
>>       int cpu_cfg_offset = multi_ext->cpu_cfg_offset;
>> -    bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
>> +    uint16_t *blocksize;
>> +    bool *ext_enabled;
>>   
>> +    if (strstr(multi_ext->name, "blocksize")) {
>> +        blocksize = (void *)&cpu->cfg + cpu_cfg_offset;
>> +        *blocksize = val;
>> +        return;
>> +    }
> 
> We should add 'get' accessors to each property and then always use those
> accessors to get the values. Trying to share a single accessor across
> properties, using the names to determine their sizes, is basically trying
> to reinvent 'get' without the function pointer.

To be honest we don't need all this machinery for the blocksize attributes.
We check them only in a few cases and could access them directly via cpu->cfg.

I'll change this up in v2.


Daniel

> 
>> +
>> +    ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
>>       *ext_enabled = val;
>>   }
>>   
>> @@ -250,8 +258,15 @@ static uint32_t kvm_cpu_cfg_get(RISCVCPU *cpu,
>>                                   RISCVCPUMultiExtConfig *multi_ext)
>>   {
>>       int cpu_cfg_offset = multi_ext->cpu_cfg_offset;
>> -    bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
>> +    uint16_t *blocksize;
>> +    bool *ext_enabled;
>>   
>> +    if (strstr(multi_ext->name, "blocksize")) {
>> +        blocksize = (void *)&cpu->cfg + cpu_cfg_offset;
>> +        return *blocksize;
>> +    }
>> +
>> +    ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
>>       return *ext_enabled;
>>   }
>>   
>> @@ -295,6 +310,33 @@ static void kvm_cpu_set_multi_ext_cfg(Object *obj, Visitor *v,
>>       kvm_cpu_cfg_set(cpu, multi_ext_cfg, value);
>>   }
>>   
>> +/*
>> + * We'll avoid extra complexity by always assuming this
>> + * array order with cbom first.
>> + */
>> +static RISCVCPUMultiExtConfig kvm_cbomz_blksize_cfgs[] = {
> 
> Hmm, yet another cfg struct type, and this one is specific to block sizes.
> I'd rather we find a way to keep cfg definitions more general and then use
> the same struct for all.
> 
>> +    {.name = "cbom_blocksize", .cpu_cfg_offset = CPUCFG(cbom_blocksize),
>> +     .kvm_reg_id = KVM_REG_RISCV_CONFIG_REG(zicbom_block_size)},
>> +    {.name = "cboz_blocksize", .cpu_cfg_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)
>> +{
>> +    RISCVCPUMultiExtConfig *cbomz_size_cfg = opaque;
>> +    RISCVCPU *cpu = RISCV_CPU(obj);
>> +    uint16_t value;
>> +
>> +    if (!visit_type_uint16(v, name, &value, errp)) {
>> +        return;
>> +    }
>> +
>> +    cbomz_size_cfg->user_set = true;
>> +    kvm_cpu_cfg_set(cpu, cbomz_size_cfg, value);
>> +}
>> +
>>   static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
>>   {
>>       CPURISCVState *env = &cpu->env;
>> @@ -321,6 +363,45 @@ static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
>>       }
>>   }
>>   
>> +static void kvm_riscv_finalize_features(RISCVCPU *cpu, CPUState *cs)
>> +{
>> +    CPURISCVState *env = &cpu->env;
>> +    uint64_t id, reg;
>> +    int i, ret;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(kvm_cbomz_blksize_cfgs); i++) {
>> +        RISCVCPUMultiExtConfig *cbomz_cfg = &kvm_cbomz_blksize_cfgs[i];
>> +        uint64_t host_val;
>> +
>> +        if ((i == 0 && !cpu->cfg.ext_icbom) ||
>> +            (i == 1 && !cpu->cfg.ext_icboz)) {
> 
> Rather than the required array order and this magic index stuff, we can
> just save the offset of the ext_* boolean in the cfg structure, like we
> already do for the *_blocksize, and then check it here.
> 
> Also, I think we want to warn here if cbomz_cfg->user_set is set. If the
> user set some block size, but disabled the extension, then they should be
> told that the block size will be ignored. Letting them know it's ignored
> is particularly good to do since we're not validating it. I.e. the user
> shouldn't assume the block size they put there is worth anything at all.
> 
>> +            continue;
>> +        }
>> +
>> +        id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
>> +                              cbomz_cfg->kvm_reg_id);
>> +
>> +        ret = kvm_get_one_reg(cs, id, &host_val);
>> +        if (ret != 0) {
>> +            error_report("Unable to read KVM reg val %s, error %d",
>> +                         cbomz_cfg->name, ret);
>> +            exit(EXIT_FAILURE);
>> +        }
>> +
>> +        if (cbomz_cfg->user_set) {
>> +            reg = kvm_cpu_cfg_get(cpu, cbomz_cfg);
>> +            if (reg != host_val) {
>> +                error_report("Unable to set %s to a different value than "
>> +                             "the host (%lu)",
>> +                             cbomz_cfg->name, host_val);
>> +                exit(EXIT_FAILURE);
>> +            }
>> +        }
>> +
>> +        kvm_cpu_cfg_set(cpu, cbomz_cfg, host_val);
>> +    }
>> +}
>> +
>>   static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
>>   {
>>       int i;
>> @@ -344,6 +425,14 @@ static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
>>                               kvm_cpu_set_multi_ext_cfg,
>>                               NULL, multi_cfg);
>>       }
>> +
>> +    for (i = 0; i < ARRAY_SIZE(kvm_cbomz_blksize_cfgs); i++) {
>> +        RISCVCPUMultiExtConfig *cbomz_size_cfg = &kvm_cbomz_blksize_cfgs[i];
>> +
>> +        object_property_add(cpu_obj, cbomz_size_cfg->name, "uint16",
>> +                            NULL, kvm_cpu_set_cbomz_blksize,
>> +                            NULL, cbomz_size_cfg);
>> +    }
>>   }
>>   
>>   void kvm_riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
>> @@ -856,6 +945,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);
>> +    kvm_riscv_finalize_features(cpu, cs);
>>   
>>       return ret;
>>   }
>> -- 
>> 2.40.1
>>
>>
> 
> Thanks,
> drew

Re: [PATCH 13/16] target/riscv/kvm.c: add multi-letter extension KVM properties

[Andrew Jones]

On Wed, Jun 07, 2023 at 04:59:02PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 6/7/23 08:48, Andrew Jones wrote:
> > On Tue, May 30, 2023 at 04:46:20PM -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 the recently added MISA properties we're also going to add a
> > > 'user_set' flag in each of them. The flag will be set only if the user
> > > chose an option that's different from the host and will require extra
> > > handling from the KVM driver.
> > > 
> > > However, multi-letter CPUs 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.
> > 
> > I wish that was ENOENT, but I suppose that ship sailed.
> > 
> > > 
> > > The 'supported' flag will then be used later on to give 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/kvm.c | 136 +++++++++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 136 insertions(+)
> > > 
> > > diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> > > index bb1dafe263..b4193a10d8 100644
> > > --- a/target/riscv/kvm.c
> > > +++ b/target/riscv/kvm.c
> > > @@ -202,6 +202,99 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs)
> > >       }
> > >   }
> > > +typedef struct RISCVCPUMultiExtConfig {
> > > +    const char *name;
> > 
> > No description? I'd prefer we use the same cfg struct for single-letter
> > and multi-letter extensions. We can use a union to overlap cpu_cfg_offset
> > and misa_bit.
> 
> multi-letter extensions don't have a 'description' field in TCG. Nothing
> prevents us from adding for KVM though.

KVM can wait for descriptions until after adding them to TCG, and then,
hopefully, figuring out how to share them with KVM.

Thanks,
drew

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

[Andrew Jones]

On Wed, Jun 07, 2023 at 05:37:16PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 6/7/23 10:01, Andrew Jones wrote:
> > On Tue, May 30, 2023 at 04:46:23PM -0300, Daniel Henrique Barboza wrote:
> > > 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 use them. We'll also expose them as user flags like it is
> > > already done with TCG.
> > > 
> > > However, in contrast with what happens with TCG, the user is not able to
> > > set any value that is different from the 'host' value. And KVM can be
> > > harsh dealing with it: a ENOTSUPP can be thrown for the mere attempt of
> > > executing kvm_set_one_reg() for these 2 regs.
> > > 
> > > We'll read the 'host' value and use it to set these values, regardless of
> > > user choice. If the user happened to chose a different value, error out.
> > > We'll also error out if we failed to read the block sizes.
> > > 
> > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > > ---
> > >   target/riscv/kvm.c | 94 +++++++++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 92 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> > > index 92b99fe261..7789d835e5 100644
> > > --- a/target/riscv/kvm.c
> > > +++ b/target/riscv/kvm.c
> > > @@ -241,8 +241,16 @@ static void kvm_cpu_cfg_set(RISCVCPU *cpu, RISCVCPUMultiExtConfig *multi_ext,
> > >                               uint32_t val)
> > >   {
> > >       int cpu_cfg_offset = multi_ext->cpu_cfg_offset;
> > > -    bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
> > > +    uint16_t *blocksize;
> > > +    bool *ext_enabled;
> > > +    if (strstr(multi_ext->name, "blocksize")) {
> > > +        blocksize = (void *)&cpu->cfg + cpu_cfg_offset;
> > > +        *blocksize = val;
> > > +        return;
> > > +    }
> > 
> > We should add 'get' accessors to each property and then always use those
> > accessors to get the values. Trying to share a single accessor across
> > properties, using the names to determine their sizes, is basically trying
> > to reinvent 'get' without the function pointer.
> 
> To be honest we don't need all this machinery for the blocksize attributes.
> We check them only in a few cases and could access them directly via cpu->cfg.
>

OK

A bit off-topic, but thinking about this some more, we're doing block
sizes wrong. We should be using boolean properties for cpu features in
order to simplify qmp_query_cpu_model_expansion(), which we'll want to add
to riscv soon. For CBO block sizes, that means to create multiple possible
block size booleans, e.g. cbom64, cbom128, ..., which are all mutually
exclusive. Using booleans also avoids the need to validate that the block
size a user puts on the command line is a power-of-2, which TCG isn't
currently doing, but should be.

Changing how we input block sizes, with all the deprecation and what not,
would be a separate series though, and I think it can go after this
series, because the block sizes would still ultimately be stored in the
same way, as uint16s, for convenience of use.

Thanks,
drew

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

[Alistair Francis]

On Wed, May 31, 2023 at 5:48 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> 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
>
> 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.
>
> Skip the 'mmu-type' FDT node if there's no satp_mode set.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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 4e3efbee16..8aa907e81f 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.40.1
>
>

Re: [PATCH 03/16] target/riscv/cpu.c: restrict 'mvendorid' value

[Alistair Francis]

On Wed, May 31, 2023 at 5:49 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 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)

Is this something we want to enforce? What if someone wanted to test
using the veyron-v1 CPU but they wanted to change some properties. I
don't see an advantage in not letting them do that

Alistair

>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 72f5433776..bcd69bb032 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1723,7 +1723,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),
>
> @@ -1810,6 +1809,32 @@ 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 riscv_cpu_class_init(ObjectClass *c, void *data)
>  {
>      RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> @@ -1841,6 +1866,10 @@ 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", NULL,
> +                              cpu_set_mvendorid,
> +                              NULL, NULL);
> +
>      device_class_set_props(dc, riscv_cpu_properties);
>  }
>
> --
> 2.40.1
>
>

Re: [PATCH 06/16] target/riscv: use KVM scratch CPUs to init KVM properties

[Alistair Francis]

On Wed, May 31, 2023 at 5:50 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 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>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.c       |  4 ++
>  target/riscv/kvm.c       | 85 ++++++++++++++++++++++++++++++++++++++++
>  target/riscv/kvm_riscv.h |  1 +
>  3 files changed, 90 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index d6e23bfd83..749d8bf5eb 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1703,6 +1703,10 @@ static void riscv_cpu_add_user_properties(Object *obj)
>      Property *prop;
>      DeviceState *dev = DEVICE(obj);
>
> +    if (riscv_running_KVM()) {
> +        kvm_riscv_init_user_properties(obj);
> +    }
> +
>      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.40.1
>
>

Re: [PATCH 07/16] target/riscv: read marchid/mimpid in kvm_riscv_init_machine_ids()

[Alistair Francis]

On Wed, May 31, 2023 at 5:48 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 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>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

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

Re: [PATCH 03/16] target/riscv/cpu.c: restrict 'mvendorid' value

[Daniel Henrique Barboza]

On 6/12/23 00:56, Alistair Francis wrote:
> On Wed, May 31, 2023 at 5:49 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> 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)
> 
> Is this something we want to enforce? What if someone wanted to test
> using the veyron-v1 CPU but they wanted to change some properties. I
> don't see an advantage in not letting them do that

The idea is to keep things simpler for us. As it is today we forbid users to
enable/disable extensions for vendor CPUs. Doing the same thing for
mvendorid/marchid/mimpid seems consistent with what we're already doing.

Also, guest software might rely on vendor IDs from the CPU to take certain
actions, and if the user is free to change the CPU ID from vendor CPUs the
software will misbehave and the user can claim "I created a veyron-v1 CPU and
the guest it's like like it's not". Allowing mvendorid and friends to be changed
doesn't do much for users (we forbid enabling/disabling extensions, so what's
to gain from changing machine IDs?) and it can be a potential source of bugs.



Thanks,


Daniel


> 
> Alistair
> 
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c | 31 ++++++++++++++++++++++++++++++-
>>   1 file changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 72f5433776..bcd69bb032 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1723,7 +1723,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),
>>
>> @@ -1810,6 +1809,32 @@ 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 riscv_cpu_class_init(ObjectClass *c, void *data)
>>   {
>>       RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
>> @@ -1841,6 +1866,10 @@ 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", NULL,
>> +                              cpu_set_mvendorid,
>> +                              NULL, NULL);
>> +
>>       device_class_set_props(dc, riscv_cpu_properties);
>>   }
>>
>> --
>> 2.40.1
>>
>>

Re: [PATCH 13/16] target/riscv/kvm.c: add multi-letter extension KVM properties

[Daniel Henrique Barboza]

On 6/7/23 08:48, Andrew Jones wrote:
> On Tue, May 30, 2023 at 04:46:20PM -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 the recently added MISA properties we're also going to add a
>> 'user_set' flag in each of them. The flag will be set only if the user
>> chose an option that's different from the host and will require extra
>> handling from the KVM driver.
>>
>> However, multi-letter CPUs 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.
> 
> I wish that was ENOENT, but I suppose that ship sailed.
> 
>>
>> The 'supported' flag will then be used later on to give 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/kvm.c | 136 +++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 136 insertions(+)
>>
>> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
>> index bb1dafe263..b4193a10d8 100644
>> --- a/target/riscv/kvm.c
>> +++ b/target/riscv/kvm.c
>> @@ -202,6 +202,99 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs)
>>       }
>>   }
>>   
>> +typedef struct RISCVCPUMultiExtConfig {
>> +    const char *name;
> 
> No description? I'd prefer we use the same cfg struct for single-letter
> and multi-letter extensions. We can use a union to overlap cpu_cfg_offset
> and misa_bit.
> 
>> +    int kvm_reg_id;
>> +    int cpu_cfg_offset;
>> +    bool supported;
>> +    bool user_set;
>> +} RISCVCPUMultiExtConfig;
>> +
>> +#define CPUCFG(_prop) offsetof(struct RISCVCPUConfig, _prop)
>> +
>> +/*
>> + * KVM ISA Multi-letter extensions. We care about the order
>> + * since it'll be used to create the ISA string later on.
>> + * We follow the same ordering rules of isa_edata_arr[]
>> + * from target/riscv/cpu.c.
>> + */
>> +static RISCVCPUMultiExtConfig kvm_multi_ext_cfgs[] = {
>> +    {.name = "zicbom", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZICBOM,
>> +     .cpu_cfg_offset = CPUCFG(ext_icbom)},
>> +    {.name = "zicboz", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZICBOZ,
>> +     .cpu_cfg_offset = CPUCFG(ext_icboz)},
>> +    {.name = "zihintpause", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZIHINTPAUSE,
>> +     .cpu_cfg_offset = CPUCFG(ext_zihintpause)},
>> +    {.name = "zbb", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZBB,
>> +     .cpu_cfg_offset = CPUCFG(ext_zbb)},
>> +    {.name = "ssaia", .kvm_reg_id = KVM_RISCV_ISA_EXT_SSAIA,
>> +     .cpu_cfg_offset = CPUCFG(ext_ssaia)},
>> +    {.name = "sstc", .kvm_reg_id = KVM_RISCV_ISA_EXT_SSTC,
>> +     .cpu_cfg_offset = CPUCFG(ext_sstc)},
>> +    {.name = "svinval", .kvm_reg_id = KVM_RISCV_ISA_EXT_SVINVAL,
>> +     .cpu_cfg_offset = CPUCFG(ext_svinval)},
>> +    {.name = "svpbmt", .kvm_reg_id = KVM_RISCV_ISA_EXT_SVPBMT,
>> +     .cpu_cfg_offset = CPUCFG(ext_svpbmt)},
> 
> As pointed out in the last patch, it'd be nice to share names (and
> descriptions) with TCG.
> 
>> +};
>> +
>> +static void kvm_cpu_cfg_set(RISCVCPU *cpu, RISCVCPUMultiExtConfig *multi_ext,
>> +                            uint32_t val)
>> +{
>> +    int cpu_cfg_offset = multi_ext->cpu_cfg_offset;
>> +    bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
>> +
>> +    *ext_enabled = val;
>> +}
>> +
>> +static uint32_t kvm_cpu_cfg_get(RISCVCPU *cpu,
>> +                                RISCVCPUMultiExtConfig *multi_ext)
>> +{
>> +    int cpu_cfg_offset = multi_ext->cpu_cfg_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)
>> +{
>> +    RISCVCPUMultiExtConfig *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;
>> @@ -216,6 +309,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++) {
>> +        RISCVCPUMultiExtConfig *multi_cfg = &kvm_multi_ext_cfgs[i];
>> +
>> +        object_property_add(cpu_obj, multi_cfg->name, "bool",
>> +                            NULL,
> 
> You have a getter function, so we might as well set it here, no?

I was going back to do this change then realized that we don't have a get() for
this case. We have kvm_cpu_cfg_get(), which is a way of retrieving the value of
cpu->cfg.<attr>. It's not the same as a get() impl for object_property_add().


Thanks,


Daniel



> 
>> +                            kvm_cpu_set_multi_ext_cfg,
>> +                            NULL, multi_cfg);
>> +    }
>>   }
>>   
>>   static int kvm_riscv_get_regs_core(CPUState *cs)
>> @@ -531,6 +633,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++) {
>> +        RISCVCPUMultiExtConfig *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);
> 
> I don't think we usually bother cleaning up when exiting, we just let exit
> do it, but OK.
> 
>> +                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);
>> @@ -543,6 +678,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.40.1
>>
>>
> 
> Thanks,
> drew

Re: [PATCH 03/16] target/riscv/cpu.c: restrict 'mvendorid' value

[Alistair Francis]

On Tue, Jun 13, 2023 at 4:52 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 6/12/23 00:56, Alistair Francis wrote:
> > On Wed, May 31, 2023 at 5:49 AM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> 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)
> >
> > Is this something we want to enforce? What if someone wanted to test
> > using the veyron-v1 CPU but they wanted to change some properties. I
> > don't see an advantage in not letting them do that
>
> The idea is to keep things simpler for us. As it is today we forbid users to
> enable/disable extensions for vendor CPUs. Doing the same thing for
> mvendorid/marchid/mimpid seems consistent with what we're already doing.
>
> Also, guest software might rely on vendor IDs from the CPU to take certain
> actions, and if the user is free to change the CPU ID from vendor CPUs the
> software will misbehave and the user can claim "I created a veyron-v1 CPU and
> the guest it's like like it's not". Allowing mvendorid and friends to be changed
> doesn't do much for users (we forbid enabling/disabling extensions, so what's
> to gain from changing machine IDs?) and it can be a potential source of bugs.

Fair points. Ok, fine with me then :)

Alistair

>
>
>
> Thanks,
>
>
> Daniel
>
>
> >
> > Alistair
> >
> >>
> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >> ---
> >>   target/riscv/cpu.c | 31 ++++++++++++++++++++++++++++++-
> >>   1 file changed, 30 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index 72f5433776..bcd69bb032 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -1723,7 +1723,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),
> >>
> >> @@ -1810,6 +1809,32 @@ 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 riscv_cpu_class_init(ObjectClass *c, void *data)
> >>   {
> >>       RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> >> @@ -1841,6 +1866,10 @@ 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", NULL,
> >> +                              cpu_set_mvendorid,
> >> +                              NULL, NULL);
> >> +
> >>       device_class_set_props(dc, riscv_cpu_properties);
> >>   }
> >>
> >> --
> >> 2.40.1
> >>
> >>

Re: [PATCH 14/16] target/riscv: adapt 'riscv_isa_string' for KVM

[Daniel Henrique Barboza]

On 6/7/23 09:21, Andrew Jones wrote:
> On Tue, May 30, 2023 at 04:46:21PM -0300, Daniel Henrique Barboza wrote:
>> KVM is not using the same attributes as TCG, i.e. it doesn't use
>> isa_edata_arr[]. Add a new kvm_riscv_isa_string_ext() helper that does
>> basically the same thing, but using KVM internals instead.
>>
>> The decision to add this helper target/riscv/kvm.c is to foster the
>> separation between KVM and TCG logic, while still using
>> riscv_isa_string_ext() from target/riscv/cpu.c to retrieve the string
>> to not overcomplicate things.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c       |  5 +++++
>>   target/riscv/kvm.c       | 19 +++++++++++++++++++
>>   target/riscv/kvm_riscv.h |  2 ++
>>   3 files changed, 26 insertions(+)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 3c348049a3..ec1d0c621a 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1956,6 +1956,11 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
>>       char *new = *isa_str;
>>       int i;
>>   
>> +    if (riscv_running_KVM()) {
>> +        kvm_riscv_isa_string_ext(cpu, isa_str, max_str_len);
>> +        return;
>> +    }
>> +
>>       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])) {
>> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
>> index b4193a10d8..675e18df3b 100644
>> --- a/target/riscv/kvm.c
>> +++ b/target/riscv/kvm.c
>> @@ -320,6 +320,25 @@ static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
>>       }
>>   }
>>   
>> +void kvm_riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
>> +                              int max_str_len)
>> +{
>> +    char *old = *isa_str;
>> +    char *new = *isa_str;
>> +    int i;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(kvm_multi_ext_cfgs); i++) {
>> +        RISCVCPUMultiExtConfig *multi_ext_cfg = &kvm_multi_ext_cfgs[i];
>> +        if (kvm_cpu_cfg_get(cpu, multi_ext_cfg)) {
>> +            new = g_strconcat(old, "_", multi_ext_cfg->name, NULL);
>> +            g_free(old);
>> +            old = new;
>> +        }
>> +    }
>> +
>> +    *isa_str = new;
>> +}
>> +
>>   static int kvm_riscv_get_regs_core(CPUState *cs)
>>   {
>>       int ret = 0;
>> diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
>> index e3ba935808..1a12efa8db 100644
>> --- a/target/riscv/kvm_riscv.h
>> +++ b/target/riscv/kvm_riscv.h
>> @@ -20,6 +20,8 @@
>>   #define QEMU_KVM_RISCV_H
>>   
>>   void kvm_riscv_init_user_properties(Object *cpu_obj);
>> +void kvm_riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
>> +                              int max_str_len);
>>   void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
>>   void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);
>>   
>> -- 
>> 2.40.1
>>
>>
> 
> Hmm, more duplication. I think we need an abstraction which can support
> both TCG and KVM extension lists. Allowing functions like
> riscv_isa_string_ext() to be shared for them.

I tried to play around a bit and didn't manage to find a solution that covers
both.

The root cause is that the TCG only options are being ignored by KVM, but they
are still around. I made an attempt of re-using the existing isa_string()
function with KVM by changing all TCG-only extensions default to 'false'. This
doesn't change the fact that, with KVM, I can do:

sudo ./qemu/build/qemu-system-riscv64  -machine virt,accel=kvm \
-cpu host,zhinx=true,zhinxmin=true (...)

Note that zhinx and zhinxmin are TCG-only. And the ISA string showed these 2
extensions:

# cat /proc/device-tree/cpus/cpu@0/riscv,isa
rv64imafdc_zicbom_zicboz_zbb_zhinx_zhinxmin_sstc


Alternatives would be to change TCG code to allow for extra fields for KVM (e.g. the
'supported' flag) to allow the isa_string() function to ignore the TCG-only extensions.
Bear in mind that TCG has 63 extensions, so we would do 63 ioctls for each CPU in this
extension discovery and KVM only 8 support extensions ATM.

Another idea is to make the existing isa_string() compare isa_edata_arr[] with the
KVM counterpart kvm_multi_ext_cfgs[] and, if running KVM, check if the extension
in isa_edata_arr[] is also in the KVM array. This also seems a bit inefficient since
we're adding a search loop for 55 extensions when creating the string.

Another alternative is to exclude all TCG-only extensions from the command line when
running KVM. We would fork the API though, which is something that we're wanting to
avoid.

Duplicating this code as we're doing here guarantees that the KVM isa string won't
have anything that KVM doesn't know about, regardless of the user input. I am not a
fan of duplication, but at this moment it seems plausible to keep it. At least until
we sort a way of unifying both TCG and KVM options in a satisfying manner.

I mean, at least as far as a I can see. Suggestions always welcome.


Thanks,


Daniel




> 
> Thanks,
> drew

Re: [PATCH 14/16] target/riscv: adapt 'riscv_isa_string' for KVM

[Daniel Henrique Barboza]

On 6/13/23 07:29, Daniel Henrique Barboza wrote:
> 
> 
> On 6/7/23 09:21, Andrew Jones wrote:
>> On Tue, May 30, 2023 at 04:46:21PM -0300, Daniel Henrique Barboza wrote:
>>> KVM is not using the same attributes as TCG, i.e. it doesn't use
>>> isa_edata_arr[]. Add a new kvm_riscv_isa_string_ext() helper that does
>>> basically the same thing, but using KVM internals instead.
>>>
>>> The decision to add this helper target/riscv/kvm.c is to foster the
>>> separation between KVM and TCG logic, while still using
>>> riscv_isa_string_ext() from target/riscv/cpu.c to retrieve the string
>>> to not overcomplicate things.
>>>
>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> ---
>>>   target/riscv/cpu.c       |  5 +++++
>>>   target/riscv/kvm.c       | 19 +++++++++++++++++++
>>>   target/riscv/kvm_riscv.h |  2 ++
>>>   3 files changed, 26 insertions(+)
>>>
>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>> index 3c348049a3..ec1d0c621a 100644
>>> --- a/target/riscv/cpu.c
>>> +++ b/target/riscv/cpu.c
>>> @@ -1956,6 +1956,11 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
>>>       char *new = *isa_str;
>>>       int i;
>>> +    if (riscv_running_KVM()) {
>>> +        kvm_riscv_isa_string_ext(cpu, isa_str, max_str_len);
>>> +        return;
>>> +    }
>>> +
>>>       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])) {
>>> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
>>> index b4193a10d8..675e18df3b 100644
>>> --- a/target/riscv/kvm.c
>>> +++ b/target/riscv/kvm.c
>>> @@ -320,6 +320,25 @@ static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
>>>       }
>>>   }
>>> +void kvm_riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
>>> +                              int max_str_len)
>>> +{
>>> +    char *old = *isa_str;
>>> +    char *new = *isa_str;
>>> +    int i;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(kvm_multi_ext_cfgs); i++) {
>>> +        RISCVCPUMultiExtConfig *multi_ext_cfg = &kvm_multi_ext_cfgs[i];
>>> +        if (kvm_cpu_cfg_get(cpu, multi_ext_cfg)) {
>>> +            new = g_strconcat(old, "_", multi_ext_cfg->name, NULL);
>>> +            g_free(old);
>>> +            old = new;
>>> +        }
>>> +    }
>>> +
>>> +    *isa_str = new;
>>> +}
>>> +
>>>   static int kvm_riscv_get_regs_core(CPUState *cs)
>>>   {
>>>       int ret = 0;
>>> diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
>>> index e3ba935808..1a12efa8db 100644
>>> --- a/target/riscv/kvm_riscv.h
>>> +++ b/target/riscv/kvm_riscv.h
>>> @@ -20,6 +20,8 @@
>>>   #define QEMU_KVM_RISCV_H
>>>   void kvm_riscv_init_user_properties(Object *cpu_obj);
>>> +void kvm_riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
>>> +                              int max_str_len);
>>>   void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
>>>   void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);
>>> -- 
>>> 2.40.1
>>>
>>>
>>
>> Hmm, more duplication. I think we need an abstraction which can support
>> both TCG and KVM extension lists. Allowing functions like
>> riscv_isa_string_ext() to be shared for them.
> 
> I tried to play around a bit and didn't manage to find a solution that covers
> both.
> 
> The root cause is that the TCG only options are being ignored by KVM, but they
> are still around. I made an attempt of re-using the existing isa_string()
> function with KVM by changing all TCG-only extensions default to 'false'. This
> doesn't change the fact that, with KVM, I can do:
> 
> sudo ./qemu/build/qemu-system-riscv64  -machine virt,accel=kvm \
> -cpu host,zhinx=true,zhinxmin=true (...)
> 
> Note that zhinx and zhinxmin are TCG-only. And the ISA string showed these 2
> extensions:
> 
> # cat /proc/device-tree/cpus/cpu@0/riscv,isa
> rv64imafdc_zicbom_zicboz_zbb_zhinx_zhinxmin_sstc
> 
> 
> Alternatives would be to change TCG code to allow for extra fields for KVM (e.g. the
> 'supported' flag) to allow the isa_string() function to ignore the TCG-only extensions.
> Bear in mind that TCG has 63 extensions, so we would do 63 ioctls for each CPU in this
> extension discovery and KVM only 8 support extensions ATM.

... or we can add an extra flag in isa_edata_arr[] 'kvm_available' to indicate if a
given extension is also present in KVM, set it manually for each KVM-capable
entry of the array and check for that during riscv_isa_string_ext().

This will avoid the code duplication while not allowing TCG-only extensions
to appear in the riscv,isa when running KVM.

I made this change in v2. I'll send it up shortly. Thanks,


Daniel

> 
> Another idea is to make the existing isa_string() compare isa_edata_arr[] with the
> KVM counterpart kvm_multi_ext_cfgs[] and, if running KVM, check if the extension
> in isa_edata_arr[] is also in the KVM array. This also seems a bit inefficient since
> we're adding a search loop for 55 extensions when creating the string.
> 
> Another alternative is to exclude all TCG-only extensions from the command line when
> running KVM. We would fork the API though, which is something that we're wanting to
> avoid.
> 
> Duplicating this code as we're doing here guarantees that the KVM isa string won't
> have anything that KVM doesn't know about, regardless of the user input. I am not a
> fan of duplication, but at this moment it seems plausible to keep it. At least until
> we sort a way of unifying both TCG and KVM options in a satisfying manner.
> 
> I mean, at least as far as a I can see. Suggestions always welcome.
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> 
> 
>>
>> Thanks,
>> drew