* [PATCH 0/7] target/riscv: store max SATP mode as a single integer in RISCVCPUConfig
@ 2025-02-18 16:57 Paolo Bonzini
2025-02-18 16:57 ` [PATCH 1/7] hw/riscv: acpi: only create RHCT MMU entry for supported types Paolo Bonzini
` (6 more replies)
0 siblings, 7 replies; 23+ messages in thread
From: Paolo Bonzini @ 2025-02-18 16:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Alistair Francis
The maximum available SATP mode implies all the shorter virtual address sizes.
Simplify the handling of the satp_mode field in RISCVCPUConfig:
- store a single byte in RISCVCPUConfig for the maximum supported size,
and adjust it to the maximum requested size based on QOM properties
- move satp_mode.{map,init} out of RISCVCPUConfig since they are
only needed to implement the user-friendly properties for -cpu
The benefit is that code outside target/riscv/ does not need to call
satp_mode_max_from_map() anymore, it can just check cpu->cfg.max_satp_mode.
The first three patches are independent bugfixes.
This series is a spin off of "target/riscv: declarative CPU definitions"
(https://lore.kernel.org/qemu-devel/20250206182711.2420505-1-pbonzini@redhat.com/T/#t).
Paolo
Paolo Bonzini (7):
hw/riscv: acpi: only create RHCT MMU entry for supported types
target/riscv: env->misa_mxl is a constant
target/riscv: assert argument to set_satp_mode_max_supported is valid
target/riscv: cpu: store max SATP mode as a single integer
target/riscv: update max_satp_mode based on QOM properties
target/riscv: remove supported from RISCVSATPMap
target/riscv: move satp_mode.{map,init} out of CPUConfig
target/riscv/cpu.h | 15 +++++-
target/riscv/cpu_cfg.h | 17 +------
hw/riscv/virt-acpi-build.c | 15 +++---
hw/riscv/virt.c | 5 +-
target/riscv/cpu.c | 98 +++++++++++++++++++++-----------------
target/riscv/csr.c | 9 +++-
target/riscv/machine.c | 13 +++++
target/riscv/tcg/tcg-cpu.c | 3 +-
8 files changed, 100 insertions(+), 75 deletions(-)
--
2.48.1
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/7] hw/riscv: acpi: only create RHCT MMU entry for supported types
2025-02-18 16:57 [PATCH 0/7] target/riscv: store max SATP mode as a single integer in RISCVCPUConfig Paolo Bonzini
@ 2025-02-18 16:57 ` Paolo Bonzini
2025-03-06 1:13 ` Alistair Francis
2025-02-18 16:57 ` [PATCH 2/7] target/riscv: env->misa_mxl is a constant Paolo Bonzini
` (5 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2025-02-18 16:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Alistair Francis
Do not create the RHCT MMU type entry for RV32 CPUs, since it
only has definitions for SV39/SV48/SV57. Likewise, check that
satp_mode_max_from_map() will actually return a valid value, skipping
the MMU type entry if all MMU types were disabled on the command line.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/riscv/virt-acpi-build.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
index 1ad68005085..2b374ebacbf 100644
--- a/hw/riscv/virt-acpi-build.c
+++ b/hw/riscv/virt-acpi-build.c
@@ -262,6 +262,7 @@ static void build_rhct(GArray *table_data,
RISCVCPU *cpu = &s->soc[0].harts[0];
uint32_t mmu_offset = 0;
uint8_t satp_mode_max;
+ bool rv32 = riscv_cpu_is_32bit(cpu);
g_autofree char *isa = NULL;
AcpiTable table = { .sig = "RHCT", .rev = 1, .oem_id = s->oem_id,
@@ -281,7 +282,8 @@ static void build_rhct(GArray *table_data,
num_rhct_nodes++;
}
- if (cpu->cfg.satp_mode.supported != 0) {
+ if (!rv32 && cpu->cfg.satp_mode.supported != 0 &&
+ (cpu->cfg.satp_mode.map & ~(1 << VM_1_10_MBARE))) {
num_rhct_nodes++;
}
@@ -341,7 +343,8 @@ static void build_rhct(GArray *table_data,
}
/* MMU node structure */
- if (cpu->cfg.satp_mode.supported != 0) {
+ if (!rv32 && cpu->cfg.satp_mode.supported != 0 &&
+ (cpu->cfg.satp_mode.map & ~(1 << VM_1_10_MBARE))) {
satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
mmu_offset = table_data->len - table.table_offset;
build_append_int_noprefix(table_data, 2, 2); /* Type */
@@ -356,7 +359,7 @@ static void build_rhct(GArray *table_data,
} else if (satp_mode_max == VM_1_10_SV39) {
build_append_int_noprefix(table_data, 0, 1); /* Sv39 */
} else {
- assert(1);
+ g_assert_not_reached();
}
}
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/7] target/riscv: env->misa_mxl is a constant
2025-02-18 16:57 [PATCH 0/7] target/riscv: store max SATP mode as a single integer in RISCVCPUConfig Paolo Bonzini
2025-02-18 16:57 ` [PATCH 1/7] hw/riscv: acpi: only create RHCT MMU entry for supported types Paolo Bonzini
@ 2025-02-18 16:57 ` Paolo Bonzini
2025-03-06 1:16 ` Alistair Francis
2025-02-18 16:57 ` [PATCH 3/7] target/riscv: assert argument to set_satp_mode_max_supported is valid Paolo Bonzini
` (4 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2025-02-18 16:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Alistair Francis
There is nothing that overwrites env->misa_mxl, so it is a constant. Do
not let a corrupted migration stream change the value; changing misa_mxl
would have a snowball effect on, for example, the valid VM modes.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/riscv/machine.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index d8445244ab2..c3d8e7c4005 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -375,6 +375,18 @@ static const VMStateDescription vmstate_ssp = {
}
};
+static bool riscv_validate_misa_mxl(void *opaque, int version_id)
+{
+ RISCVCPU *cpu = RISCV_CPU(opaque);
+ CPURISCVState *env = &cpu->env;
+ RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
+ uint32_t misa_mxl_saved = env->misa_mxl;
+
+ /* Preserve misa_mxl even if the migration stream corrupted it */
+ env->misa_mxl = mcc->misa_mxl_max;
+ return misa_mxl_saved == mcc->misa_mxl_max;
+}
+
const VMStateDescription vmstate_riscv_cpu = {
.name = "cpu",
.version_id = 10,
@@ -394,6 +406,7 @@ const VMStateDescription vmstate_riscv_cpu = {
VMSTATE_UINTTL(env.priv_ver, RISCVCPU),
VMSTATE_UINTTL(env.vext_ver, RISCVCPU),
VMSTATE_UINT32(env.misa_mxl, RISCVCPU),
+ VMSTATE_VALIDATE("MXL must match", riscv_validate_misa_mxl),
VMSTATE_UINT32(env.misa_ext, RISCVCPU),
VMSTATE_UNUSED(4),
VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU),
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/7] target/riscv: assert argument to set_satp_mode_max_supported is valid
2025-02-18 16:57 [PATCH 0/7] target/riscv: store max SATP mode as a single integer in RISCVCPUConfig Paolo Bonzini
2025-02-18 16:57 ` [PATCH 1/7] hw/riscv: acpi: only create RHCT MMU entry for supported types Paolo Bonzini
2025-02-18 16:57 ` [PATCH 2/7] target/riscv: env->misa_mxl is a constant Paolo Bonzini
@ 2025-02-18 16:57 ` Paolo Bonzini
2025-03-06 1:23 ` Alistair Francis
2025-02-18 16:57 ` [PATCH 4/7] target/riscv: cpu: store max SATP mode as a single integer Paolo Bonzini
` (3 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2025-02-18 16:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Alistair Francis
Check that the argument to set_satp_mode_max_supported is valid for
the MXL value of the CPU. It would be a bug in the CPU definition
if it weren't.
In fact, there is such a bug in riscv_bare_cpu_init(): not just
SV32 is not a valid VM mode for 64-bit CPUs, SV64 is not a
valid VM mode at all, not yet at least.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/riscv/cpu.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index cca24b9f1fc..7950b6447f8 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -442,6 +442,8 @@ static void set_satp_mode_max_supported(RISCVCPU *cpu,
cpu->cfg.satp_mode.supported |= (1 << i);
}
}
+
+ assert(cpu->cfg.satp_mode.supported & (1 << satp_mode));
}
/* Set the satp mode to the max supported */
@@ -1502,7 +1504,9 @@ static void riscv_bare_cpu_init(Object *obj)
* satp_mode manually (see set_satp_mode_default()).
*/
#ifndef CONFIG_USER_ONLY
- set_satp_mode_max_supported(cpu, VM_1_10_SV64);
+ set_satp_mode_max_supported(RISCV_CPU(obj),
+ riscv_cpu_mxl(&RISCV_CPU(obj)->env) == MXL_RV32 ?
+ VM_1_10_SV32 : VM_1_10_SV57);
#endif
}
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 4/7] target/riscv: cpu: store max SATP mode as a single integer
2025-02-18 16:57 [PATCH 0/7] target/riscv: store max SATP mode as a single integer in RISCVCPUConfig Paolo Bonzini
` (2 preceding siblings ...)
2025-02-18 16:57 ` [PATCH 3/7] target/riscv: assert argument to set_satp_mode_max_supported is valid Paolo Bonzini
@ 2025-02-18 16:57 ` Paolo Bonzini
2025-03-06 1:30 ` Alistair Francis
2025-03-06 2:57 ` Alistair Francis
2025-02-18 16:57 ` [PATCH 5/7] target/riscv: update max_satp_mode based on QOM properties Paolo Bonzini
` (2 subsequent siblings)
6 siblings, 2 replies; 23+ messages in thread
From: Paolo Bonzini @ 2025-02-18 16:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Alistair Francis
The maximum available SATP mode implies all the shorter virtual address sizes.
Store it in RISCVCPUConfig and avoid recomputing it via satp_mode_max_from_map.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/riscv/cpu_cfg.h | 1 +
target/riscv/cpu.c | 11 +++++------
target/riscv/tcg/tcg-cpu.c | 3 ++-
3 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index b410b1e6038..28d8de978fa 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -192,6 +192,7 @@ struct RISCVCPUConfig {
bool short_isa_string;
#ifndef CONFIG_USER_ONLY
+ int8_t max_satp_mode;
RISCVSATPMap satp_mode;
#endif
};
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7950b6447f8..2d06543217a 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -444,6 +444,7 @@ static void set_satp_mode_max_supported(RISCVCPU *cpu,
}
assert(cpu->cfg.satp_mode.supported & (1 << satp_mode));
+ cpu->cfg.max_satp_mode = satp_mode;
}
/* Set the satp mode to the max supported */
@@ -1177,16 +1178,13 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
{
bool rv32 = riscv_cpu_is_32bit(cpu);
- uint8_t satp_mode_map_max, satp_mode_supported_max;
+ uint8_t satp_mode_map_max;
/* The CPU wants the OS to decide which satp mode to use */
if (cpu->cfg.satp_mode.supported == 0) {
return;
}
- satp_mode_supported_max =
- satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
-
if (cpu->cfg.satp_mode.map == 0) {
if (cpu->cfg.satp_mode.init == 0) {
/* If unset by the user, we fallback to the default satp mode. */
@@ -1215,10 +1213,10 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
/* Make sure the user asked for a supported configuration (HW and qemu) */
- if (satp_mode_map_max > satp_mode_supported_max) {
+ if (satp_mode_map_max > cpu->cfg.max_satp_mode) {
error_setg(errp, "satp_mode %s is higher than hw max capability %s",
satp_mode_str(satp_mode_map_max, rv32),
- satp_mode_str(satp_mode_supported_max, rv32));
+ satp_mode_str(cpu->cfg.max_satp_mode, rv32));
return;
}
@@ -1477,6 +1475,7 @@ static void riscv_cpu_init(Object *obj)
cpu->cfg.cbom_blocksize = 64;
cpu->cfg.cbop_blocksize = 64;
cpu->cfg.cboz_blocksize = 64;
+ cpu->cfg.max_satp_mode = -1;
cpu->env.vext_ver = VEXT_VERSION_1_00_0;
}
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 0a137281de1..a9f59a67e00 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -693,8 +693,9 @@ static bool riscv_cpu_validate_profile_satp(RISCVCPU *cpu,
RISCVCPUProfile *profile,
bool send_warn)
{
- int satp_max = satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
+ int satp_max = cpu->cfg.max_satp_mode;
+ assert(satp_max >= 0);
if (profile->satp_mode > satp_max) {
if (send_warn) {
bool is_32bit = riscv_cpu_is_32bit(cpu);
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 5/7] target/riscv: update max_satp_mode based on QOM properties
2025-02-18 16:57 [PATCH 0/7] target/riscv: store max SATP mode as a single integer in RISCVCPUConfig Paolo Bonzini
` (3 preceding siblings ...)
2025-02-18 16:57 ` [PATCH 4/7] target/riscv: cpu: store max SATP mode as a single integer Paolo Bonzini
@ 2025-02-18 16:57 ` Paolo Bonzini
2025-03-06 1:41 ` Alistair Francis
2025-02-18 16:57 ` [PATCH 6/7] target/riscv: remove supported from RISCVSATPMap Paolo Bonzini
2025-02-18 16:57 ` [PATCH 7/7] target/riscv: move satp_mode.{map,init} out of CPUConfig Paolo Bonzini
6 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2025-02-18 16:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Alistair Francis
Almost all users of cpu->cfg.satp_mode care only about the "max" value
satp_mode_max_from_map(cpu->cfg.satp_mode.map); convert the QOM
properties back into it. For TCG, consult valid_vm[] instead of
the bitmap of accepted modes.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/riscv/cpu.h | 1 -
hw/riscv/virt-acpi-build.c | 14 +++++---------
hw/riscv/virt.c | 5 ++---
target/riscv/cpu.c | 27 ++++++++++-----------------
target/riscv/csr.c | 9 +++++++--
5 files changed, 24 insertions(+), 32 deletions(-)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 97713681cbe..f9b223bf8a7 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -911,7 +911,6 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
target_ulong riscv_new_csr_seed(target_ulong new_value,
target_ulong write_mask);
-uint8_t satp_mode_max_from_map(uint32_t map);
const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit);
/* Implemented in th_csr.c */
diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
index 2b374ebacbf..1a92a84207d 100644
--- a/hw/riscv/virt-acpi-build.c
+++ b/hw/riscv/virt-acpi-build.c
@@ -261,7 +261,6 @@ static void build_rhct(GArray *table_data,
uint32_t isa_offset, num_rhct_nodes, cmo_offset = 0;
RISCVCPU *cpu = &s->soc[0].harts[0];
uint32_t mmu_offset = 0;
- uint8_t satp_mode_max;
bool rv32 = riscv_cpu_is_32bit(cpu);
g_autofree char *isa = NULL;
@@ -282,8 +281,7 @@ static void build_rhct(GArray *table_data,
num_rhct_nodes++;
}
- if (!rv32 && cpu->cfg.satp_mode.supported != 0 &&
- (cpu->cfg.satp_mode.map & ~(1 << VM_1_10_MBARE))) {
+ if (!rv32 && cpu->cfg.max_satp_mode >= VM_1_10_SV39) {
num_rhct_nodes++;
}
@@ -343,20 +341,18 @@ static void build_rhct(GArray *table_data,
}
/* MMU node structure */
- if (!rv32 && cpu->cfg.satp_mode.supported != 0 &&
- (cpu->cfg.satp_mode.map & ~(1 << VM_1_10_MBARE))) {
- satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
+ if (!rv32 && cpu->cfg.max_satp_mode >= VM_1_10_SV39) {
mmu_offset = table_data->len - table.table_offset;
build_append_int_noprefix(table_data, 2, 2); /* Type */
build_append_int_noprefix(table_data, 8, 2); /* Length */
build_append_int_noprefix(table_data, 0x1, 2); /* Revision */
build_append_int_noprefix(table_data, 0, 1); /* Reserved */
/* MMU Type */
- if (satp_mode_max == VM_1_10_SV57) {
+ if (cpu->cfg.max_satp_mode == VM_1_10_SV57) {
build_append_int_noprefix(table_data, 2, 1); /* Sv57 */
- } else if (satp_mode_max == VM_1_10_SV48) {
+ } else if (cpu->cfg.max_satp_mode == VM_1_10_SV48) {
build_append_int_noprefix(table_data, 1, 1); /* Sv48 */
- } else if (satp_mode_max == VM_1_10_SV39) {
+ } else if (cpu->cfg.max_satp_mode == VM_1_10_SV39) {
build_append_int_noprefix(table_data, 0, 1); /* Sv39 */
} else {
g_assert_not_reached();
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 241389d72f8..2394fc71df4 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -236,10 +236,10 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
uint32_t cpu_phandle;
MachineState *ms = MACHINE(s);
bool is_32_bit = riscv_is_32bit(&s->soc[0]);
- uint8_t satp_mode_max;
for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) {
RISCVCPU *cpu_ptr = &s->soc[socket].harts[cpu];
+ int8_t satp_mode_max = cpu_ptr->cfg.max_satp_mode;
g_autofree char *cpu_name = NULL;
g_autofree char *core_name = NULL;
g_autofree char *intc_name = NULL;
@@ -251,8 +251,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
s->soc[socket].hartid_base + cpu);
qemu_fdt_add_subnode(ms->fdt, cpu_name);
- if (cpu_ptr->cfg.satp_mode.supported != 0) {
- satp_mode_max = satp_mode_max_from_map(cpu_ptr->cfg.satp_mode.map);
+ if (satp_mode_max != -1) {
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);
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 2d06543217a..ce71ee95a52 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -387,7 +387,7 @@ static uint8_t satp_mode_from_str(const char *satp_mode_str)
g_assert_not_reached();
}
-uint8_t satp_mode_max_from_map(uint32_t map)
+static uint8_t satp_mode_max_from_map(uint32_t map)
{
/*
* 'map = 0' will make us return (31 - 32), which C will
@@ -453,15 +453,13 @@ static void set_satp_mode_default_map(RISCVCPU *cpu)
/*
* Bare CPUs do not default to the max available.
* Users must set a valid satp_mode in the command
- * line.
+ * line. Otherwise, leave the existing max_satp_mode
+ * in place.
*/
if (object_dynamic_cast(OBJECT(cpu), TYPE_RISCV_BARE_CPU) != NULL) {
warn_report("No satp mode set. Defaulting to 'bare'");
- cpu->cfg.satp_mode.map = (1 << VM_1_10_MBARE);
- return;
+ cpu->cfg.max_satp_mode = VM_1_10_MBARE;
}
-
- cpu->cfg.satp_mode.map = cpu->cfg.satp_mode.supported;
}
#endif
@@ -1180,8 +1178,8 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
bool rv32 = riscv_cpu_is_32bit(cpu);
uint8_t satp_mode_map_max;
- /* The CPU wants the OS to decide which satp mode to use */
- if (cpu->cfg.satp_mode.supported == 0) {
+ if (cpu->cfg.max_satp_mode == -1) {
+ /* The CPU wants the hypervisor to decide which satp mode to allow */
return;
}
@@ -1200,14 +1198,14 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
(cpu->cfg.satp_mode.supported & (1 << i))) {
for (int j = i - 1; j >= 0; --j) {
if (cpu->cfg.satp_mode.supported & (1 << j)) {
- cpu->cfg.satp_mode.map |= (1 << j);
- break;
+ cpu->cfg.max_satp_mode = j;
+ return;
}
}
- break;
}
}
}
+ return;
}
satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
@@ -1237,12 +1235,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
}
}
- /* Finally expand the map so that all valid modes are set */
- for (int i = satp_mode_map_max - 1; i >= 0; --i) {
- if (cpu->cfg.satp_mode.supported & (1 << i)) {
- cpu->cfg.satp_mode.map |= (1 << i);
- }
- }
+ cpu->cfg.max_satp_mode = satp_mode_map_max;
}
#endif
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index afb7544f078..78db9aeda57 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1862,8 +1862,13 @@ static RISCVException read_mstatus(CPURISCVState *env, int csrno,
static bool validate_vm(CPURISCVState *env, target_ulong vm)
{
- uint64_t mode_supported = riscv_cpu_cfg(env)->satp_mode.map;
- return get_field(mode_supported, (1 << vm));
+ bool rv32 = riscv_cpu_mxl(env) == MXL_RV32;
+ RISCVCPU *cpu = env_archcpu(env);
+ int satp_mode_supported_max = cpu->cfg.max_satp_mode;
+ const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
+
+ assert(satp_mode_supported_max >= 0);
+ return vm <= satp_mode_supported_max && valid_vm[vm];
}
static target_ulong legalize_xatp(CPURISCVState *env, target_ulong old_xatp,
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 6/7] target/riscv: remove supported from RISCVSATPMap
2025-02-18 16:57 [PATCH 0/7] target/riscv: store max SATP mode as a single integer in RISCVCPUConfig Paolo Bonzini
` (4 preceding siblings ...)
2025-02-18 16:57 ` [PATCH 5/7] target/riscv: update max_satp_mode based on QOM properties Paolo Bonzini
@ 2025-02-18 16:57 ` Paolo Bonzini
2025-03-06 2:39 ` Alistair Francis
2025-02-18 16:57 ` [PATCH 7/7] target/riscv: move satp_mode.{map,init} out of CPUConfig Paolo Bonzini
6 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2025-02-18 16:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Alistair Francis
"supported" can be computed on the fly based on the max_satp_mode.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/riscv/cpu_cfg.h | 4 +---
target/riscv/cpu.c | 34 ++++++++++++++++++++++++----------
2 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 28d8de978fa..1d7fff8decd 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -29,11 +29,9 @@
*
* init is a 16-bit bitmap used to make sure the user selected a correct
* configuration as per the specification.
- *
- * supported is a 16-bit bitmap used to reflect the hw capabilities.
*/
typedef struct {
- uint16_t map, init, supported;
+ uint16_t map, init;
} RISCVSATPMap;
struct RISCVCPUConfig {
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index ce71ee95a52..86a048b62c5 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -437,14 +437,27 @@ static void set_satp_mode_max_supported(RISCVCPU *cpu,
bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
- for (int i = 0; i <= satp_mode; ++i) {
- if (valid_vm[i]) {
- cpu->cfg.satp_mode.supported |= (1 << i);
- }
+ assert(valid_vm[satp_mode]);
+ cpu->cfg.max_satp_mode = satp_mode;
+}
+
+static bool get_satp_mode_supported(RISCVCPU *cpu, uint16_t *supported)
+{
+ bool rv32 = riscv_cpu_is_32bit(cpu);
+ const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
+ int satp_mode = cpu->cfg.max_satp_mode;
+
+ if (satp_mode == -1) {
+ return false;
}
- assert(cpu->cfg.satp_mode.supported & (1 << satp_mode));
- cpu->cfg.max_satp_mode = satp_mode;
+ *supported = 0;
+ for (int i = 0; i <= satp_mode; ++i) {
+ if (valid_vm[i]) {
+ *supported |= (1 << i);
+ }
+ }
+ return true;
}
/* Set the satp mode to the max supported */
@@ -1176,9 +1189,10 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
{
bool rv32 = riscv_cpu_is_32bit(cpu);
+ uint16_t supported;
uint8_t satp_mode_map_max;
- if (cpu->cfg.max_satp_mode == -1) {
+ if (!get_satp_mode_supported(cpu, &supported)) {
/* The CPU wants the hypervisor to decide which satp mode to allow */
return;
}
@@ -1195,9 +1209,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
*/
for (int i = 1; i < 16; ++i) {
if ((cpu->cfg.satp_mode.init & (1 << i)) &&
- (cpu->cfg.satp_mode.supported & (1 << i))) {
+ supported & (1 << i)) {
for (int j = i - 1; j >= 0; --j) {
- if (cpu->cfg.satp_mode.supported & (1 << j)) {
+ if (supported & (1 << j)) {
cpu->cfg.max_satp_mode = j;
return;
}
@@ -1226,7 +1240,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
for (int i = satp_mode_map_max - 1; i >= 0; --i) {
if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
(cpu->cfg.satp_mode.init & (1 << i)) &&
- (cpu->cfg.satp_mode.supported & (1 << i))) {
+ (supported & (1 << i))) {
error_setg(errp, "cannot disable %s satp mode if %s "
"is enabled", satp_mode_str(i, false),
satp_mode_str(satp_mode_map_max, false));
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 7/7] target/riscv: move satp_mode.{map,init} out of CPUConfig
2025-02-18 16:57 [PATCH 0/7] target/riscv: store max SATP mode as a single integer in RISCVCPUConfig Paolo Bonzini
` (5 preceding siblings ...)
2025-02-18 16:57 ` [PATCH 6/7] target/riscv: remove supported from RISCVSATPMap Paolo Bonzini
@ 2025-02-18 16:57 ` Paolo Bonzini
2025-03-06 2:42 ` [PATCH 7/7] target/riscv: move satp_mode.{map, init} " Alistair Francis
6 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2025-02-18 16:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Alistair Francis
They are used to provide the nice QOM properties for svNN,
but the canonical source of the CPU configuration is now
cpu->cfg.max_satp_mode. Store them in the ArchCPU struct.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/riscv/cpu.h | 14 ++++++++++++++
target/riscv/cpu_cfg.h | 14 --------------
target/riscv/cpu.c | 32 ++++++++++++++++----------------
3 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index f9b223bf8a7..df7a05e7d15 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -492,6 +492,19 @@ struct CPUArchState {
uint64_t rnmi_excpvec;
};
+/*
+ * map is a 16-bit bitmap: the most significant set bit in map is the maximum
+ * satp mode that is supported. It may be chosen by the user and must respect
+ * what qemu implements (valid_1_10_32/64) and what the hw is capable of
+ * (supported bitmap below).
+ *
+ * init is a 16-bit bitmap used to make sure the user selected a correct
+ * configuration as per the specification.
+ */
+typedef struct {
+ uint16_t map, init;
+} RISCVSATPModes;
+
/*
* RISCVCPU:
* @env: #CPURISCVState
@@ -508,6 +521,7 @@ struct ArchCPU {
/* Configuration Settings */
RISCVCPUConfig cfg;
+ RISCVSATPModes satp_modes;
QEMUTimer *pmu_timer;
/* A bitmask of Available programmable counters */
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 1d7fff8decd..7b7067d5bee 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -21,19 +21,6 @@
#ifndef RISCV_CPU_CFG_H
#define RISCV_CPU_CFG_H
-/*
- * map is a 16-bit bitmap: the most significant set bit in map is the maximum
- * satp mode that is supported. It may be chosen by the user and must respect
- * what qemu implements (valid_1_10_32/64) and what the hw is capable of
- * (supported bitmap below).
- *
- * init is a 16-bit bitmap used to make sure the user selected a correct
- * configuration as per the specification.
- */
-typedef struct {
- uint16_t map, init;
-} RISCVSATPMap;
-
struct RISCVCPUConfig {
bool ext_zba;
bool ext_zbb;
@@ -191,7 +178,6 @@ struct RISCVCPUConfig {
#ifndef CONFIG_USER_ONLY
int8_t max_satp_mode;
- RISCVSATPMap satp_mode;
#endif
};
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 86a048b62c5..8ab7fe2e286 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1197,8 +1197,8 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
return;
}
- if (cpu->cfg.satp_mode.map == 0) {
- if (cpu->cfg.satp_mode.init == 0) {
+ if (cpu->satp_modes.map == 0) {
+ if (cpu->satp_modes.init == 0) {
/* If unset by the user, we fallback to the default satp mode. */
set_satp_mode_default_map(cpu);
} else {
@@ -1208,7 +1208,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
* valid_vm_1_10_32/64.
*/
for (int i = 1; i < 16; ++i) {
- if ((cpu->cfg.satp_mode.init & (1 << i)) &&
+ if ((cpu->satp_modes.init & (1 << i)) &&
supported & (1 << i)) {
for (int j = i - 1; j >= 0; --j) {
if (supported & (1 << j)) {
@@ -1222,7 +1222,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
return;
}
- satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
+ satp_mode_map_max = satp_mode_max_from_map(cpu->satp_modes.map);
/* Make sure the user asked for a supported configuration (HW and qemu) */
if (satp_mode_map_max > cpu->cfg.max_satp_mode) {
@@ -1238,8 +1238,8 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
*/
if (!rv32) {
for (int i = satp_mode_map_max - 1; i >= 0; --i) {
- if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
- (cpu->cfg.satp_mode.init & (1 << i)) &&
+ if (!(cpu->satp_modes.map & (1 << i)) &&
+ (cpu->satp_modes.init & (1 << i)) &&
(supported & (1 << i))) {
error_setg(errp, "cannot disable %s satp mode if %s "
"is enabled", satp_mode_str(i, false),
@@ -1327,11 +1327,11 @@ bool riscv_cpu_accelerator_compatible(RISCVCPU *cpu)
static void cpu_riscv_get_satp(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
- RISCVSATPMap *satp_map = opaque;
+ RISCVSATPModes *satp_modes = opaque;
uint8_t satp = satp_mode_from_str(name);
bool value;
- value = satp_map->map & (1 << satp);
+ value = satp_modes->map & (1 << satp);
visit_type_bool(v, name, &value, errp);
}
@@ -1339,7 +1339,7 @@ static void cpu_riscv_get_satp(Object *obj, Visitor *v, const char *name,
static void cpu_riscv_set_satp(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
- RISCVSATPMap *satp_map = opaque;
+ RISCVSATPModes *satp_modes = opaque;
uint8_t satp = satp_mode_from_str(name);
bool value;
@@ -1347,8 +1347,8 @@ static void cpu_riscv_set_satp(Object *obj, Visitor *v, const char *name,
return;
}
- satp_map->map = deposit32(satp_map->map, satp, 1, value);
- satp_map->init |= 1 << satp;
+ satp_modes->map = deposit32(satp_modes->map, satp, 1, value);
+ satp_modes->init |= 1 << satp;
}
void riscv_add_satp_mode_properties(Object *obj)
@@ -1357,16 +1357,16 @@ void riscv_add_satp_mode_properties(Object *obj)
if (cpu->env.misa_mxl == MXL_RV32) {
object_property_add(obj, "sv32", "bool", cpu_riscv_get_satp,
- cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
+ cpu_riscv_set_satp, NULL, &cpu->satp_modes);
} else {
object_property_add(obj, "sv39", "bool", cpu_riscv_get_satp,
- cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
+ cpu_riscv_set_satp, NULL, &cpu->satp_modes);
object_property_add(obj, "sv48", "bool", cpu_riscv_get_satp,
- cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
+ cpu_riscv_set_satp, NULL, &cpu->satp_modes);
object_property_add(obj, "sv57", "bool", cpu_riscv_get_satp,
- cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
+ cpu_riscv_set_satp, NULL, &cpu->satp_modes);
object_property_add(obj, "sv64", "bool", cpu_riscv_get_satp,
- cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
+ cpu_riscv_set_satp, NULL, &cpu->satp_modes);
}
}
--
2.48.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/7] hw/riscv: acpi: only create RHCT MMU entry for supported types
2025-02-18 16:57 ` [PATCH 1/7] hw/riscv: acpi: only create RHCT MMU entry for supported types Paolo Bonzini
@ 2025-03-06 1:13 ` Alistair Francis
2025-03-06 12:11 ` Paolo Bonzini
0 siblings, 1 reply; 23+ messages in thread
From: Alistair Francis @ 2025-03-06 1:13 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Alistair Francis
On Wed, Feb 19, 2025 at 2:58 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Do not create the RHCT MMU type entry for RV32 CPUs, since it
> only has definitions for SV39/SV48/SV57. Likewise, check that
I don't have access to the spec, so I'm going to take your word on this
> satp_mode_max_from_map() will actually return a valid value, skipping
> the MMU type entry if all MMU types were disabled on the command line.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> hw/riscv/virt-acpi-build.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> index 1ad68005085..2b374ebacbf 100644
> --- a/hw/riscv/virt-acpi-build.c
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -262,6 +262,7 @@ static void build_rhct(GArray *table_data,
> RISCVCPU *cpu = &s->soc[0].harts[0];
> uint32_t mmu_offset = 0;
> uint8_t satp_mode_max;
> + bool rv32 = riscv_cpu_is_32bit(cpu);
> g_autofree char *isa = NULL;
>
> AcpiTable table = { .sig = "RHCT", .rev = 1, .oem_id = s->oem_id,
> @@ -281,7 +282,8 @@ static void build_rhct(GArray *table_data,
> num_rhct_nodes++;
> }
>
> - if (cpu->cfg.satp_mode.supported != 0) {
> + if (!rv32 && cpu->cfg.satp_mode.supported != 0 &&
> + (cpu->cfg.satp_mode.map & ~(1 << VM_1_10_MBARE))) {
> num_rhct_nodes++;
> }
>
> @@ -341,7 +343,8 @@ static void build_rhct(GArray *table_data,
> }
>
> /* MMU node structure */
> - if (cpu->cfg.satp_mode.supported != 0) {
> + if (!rv32 && cpu->cfg.satp_mode.supported != 0 &&
> + (cpu->cfg.satp_mode.map & ~(1 << VM_1_10_MBARE))) {
> satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> mmu_offset = table_data->len - table.table_offset;
> build_append_int_noprefix(table_data, 2, 2); /* Type */
> @@ -356,7 +359,7 @@ static void build_rhct(GArray *table_data,
> } else if (satp_mode_max == VM_1_10_SV39) {
> build_append_int_noprefix(table_data, 0, 1); /* Sv39 */
> } else {
> - assert(1);
> + g_assert_not_reached();
> }
> }
>
> --
> 2.48.1
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/7] target/riscv: env->misa_mxl is a constant
2025-02-18 16:57 ` [PATCH 2/7] target/riscv: env->misa_mxl is a constant Paolo Bonzini
@ 2025-03-06 1:16 ` Alistair Francis
2025-03-06 13:00 ` Paolo Bonzini
0 siblings, 1 reply; 23+ messages in thread
From: Alistair Francis @ 2025-03-06 1:16 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Alistair Francis
On Wed, Feb 19, 2025 at 3:01 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> There is nothing that overwrites env->misa_mxl, so it is a constant. Do
The idea is that misa_mxl can change, although that's not supported now.
> not let a corrupted migration stream change the value; changing misa_mxl
Does this actually happen? If the migration data is corrupted won't we
have all sorts of strange issues?
Alistair
> would have a snowball effect on, for example, the valid VM modes.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> target/riscv/machine.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index d8445244ab2..c3d8e7c4005 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -375,6 +375,18 @@ static const VMStateDescription vmstate_ssp = {
> }
> };
>
> +static bool riscv_validate_misa_mxl(void *opaque, int version_id)
> +{
> + RISCVCPU *cpu = RISCV_CPU(opaque);
> + CPURISCVState *env = &cpu->env;
> + RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> + uint32_t misa_mxl_saved = env->misa_mxl;
> +
> + /* Preserve misa_mxl even if the migration stream corrupted it */
> + env->misa_mxl = mcc->misa_mxl_max;
> + return misa_mxl_saved == mcc->misa_mxl_max;
> +}
> +
> const VMStateDescription vmstate_riscv_cpu = {
> .name = "cpu",
> .version_id = 10,
> @@ -394,6 +406,7 @@ const VMStateDescription vmstate_riscv_cpu = {
> VMSTATE_UINTTL(env.priv_ver, RISCVCPU),
> VMSTATE_UINTTL(env.vext_ver, RISCVCPU),
> VMSTATE_UINT32(env.misa_mxl, RISCVCPU),
> + VMSTATE_VALIDATE("MXL must match", riscv_validate_misa_mxl),
> VMSTATE_UINT32(env.misa_ext, RISCVCPU),
> VMSTATE_UNUSED(4),
> VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU),
> --
> 2.48.1
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/7] target/riscv: assert argument to set_satp_mode_max_supported is valid
2025-02-18 16:57 ` [PATCH 3/7] target/riscv: assert argument to set_satp_mode_max_supported is valid Paolo Bonzini
@ 2025-03-06 1:23 ` Alistair Francis
0 siblings, 0 replies; 23+ messages in thread
From: Alistair Francis @ 2025-03-06 1:23 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Alistair Francis
On Wed, Feb 19, 2025 at 2:58 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Check that the argument to set_satp_mode_max_supported is valid for
> the MXL value of the CPU. It would be a bug in the CPU definition
> if it weren't.
>
> In fact, there is such a bug in riscv_bare_cpu_init(): not just
> SV32 is not a valid VM mode for 64-bit CPUs, SV64 is not a
> valid VM mode at all, not yet at least.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/cpu.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index cca24b9f1fc..7950b6447f8 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -442,6 +442,8 @@ static void set_satp_mode_max_supported(RISCVCPU *cpu,
> cpu->cfg.satp_mode.supported |= (1 << i);
> }
> }
> +
> + assert(cpu->cfg.satp_mode.supported & (1 << satp_mode));
> }
>
> /* Set the satp mode to the max supported */
> @@ -1502,7 +1504,9 @@ static void riscv_bare_cpu_init(Object *obj)
> * satp_mode manually (see set_satp_mode_default()).
> */
> #ifndef CONFIG_USER_ONLY
> - set_satp_mode_max_supported(cpu, VM_1_10_SV64);
> + set_satp_mode_max_supported(RISCV_CPU(obj),
> + riscv_cpu_mxl(&RISCV_CPU(obj)->env) == MXL_RV32 ?
> + VM_1_10_SV32 : VM_1_10_SV57);
> #endif
> }
>
> --
> 2.48.1
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/7] target/riscv: cpu: store max SATP mode as a single integer
2025-02-18 16:57 ` [PATCH 4/7] target/riscv: cpu: store max SATP mode as a single integer Paolo Bonzini
@ 2025-03-06 1:30 ` Alistair Francis
2025-03-06 2:57 ` Alistair Francis
1 sibling, 0 replies; 23+ messages in thread
From: Alistair Francis @ 2025-03-06 1:30 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Alistair Francis
On Wed, Feb 19, 2025 at 3:00 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> The maximum available SATP mode implies all the shorter virtual address sizes.
> Store it in RISCVCPUConfig and avoid recomputing it via satp_mode_max_from_map.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/cpu_cfg.h | 1 +
> target/riscv/cpu.c | 11 +++++------
> target/riscv/tcg/tcg-cpu.c | 3 ++-
> 3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index b410b1e6038..28d8de978fa 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -192,6 +192,7 @@ struct RISCVCPUConfig {
> bool short_isa_string;
>
> #ifndef CONFIG_USER_ONLY
> + int8_t max_satp_mode;
> RISCVSATPMap satp_mode;
> #endif
> };
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7950b6447f8..2d06543217a 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -444,6 +444,7 @@ static void set_satp_mode_max_supported(RISCVCPU *cpu,
> }
>
> assert(cpu->cfg.satp_mode.supported & (1 << satp_mode));
> + cpu->cfg.max_satp_mode = satp_mode;
> }
>
> /* Set the satp mode to the max supported */
> @@ -1177,16 +1178,13 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
> static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> {
> bool rv32 = riscv_cpu_is_32bit(cpu);
> - uint8_t satp_mode_map_max, satp_mode_supported_max;
> + uint8_t satp_mode_map_max;
>
> /* The CPU wants the OS to decide which satp mode to use */
> if (cpu->cfg.satp_mode.supported == 0) {
> return;
> }
>
> - satp_mode_supported_max =
> - satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
> -
> if (cpu->cfg.satp_mode.map == 0) {
> if (cpu->cfg.satp_mode.init == 0) {
> /* If unset by the user, we fallback to the default satp mode. */
> @@ -1215,10 +1213,10 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
>
> /* Make sure the user asked for a supported configuration (HW and qemu) */
> - if (satp_mode_map_max > satp_mode_supported_max) {
> + if (satp_mode_map_max > cpu->cfg.max_satp_mode) {
> error_setg(errp, "satp_mode %s is higher than hw max capability %s",
> satp_mode_str(satp_mode_map_max, rv32),
> - satp_mode_str(satp_mode_supported_max, rv32));
> + satp_mode_str(cpu->cfg.max_satp_mode, rv32));
> return;
> }
>
> @@ -1477,6 +1475,7 @@ static void riscv_cpu_init(Object *obj)
> cpu->cfg.cbom_blocksize = 64;
> cpu->cfg.cbop_blocksize = 64;
> cpu->cfg.cboz_blocksize = 64;
> + cpu->cfg.max_satp_mode = -1;
> cpu->env.vext_ver = VEXT_VERSION_1_00_0;
> }
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 0a137281de1..a9f59a67e00 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -693,8 +693,9 @@ static bool riscv_cpu_validate_profile_satp(RISCVCPU *cpu,
> RISCVCPUProfile *profile,
> bool send_warn)
> {
> - int satp_max = satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
> + int satp_max = cpu->cfg.max_satp_mode;
>
> + assert(satp_max >= 0);
> if (profile->satp_mode > satp_max) {
> if (send_warn) {
> bool is_32bit = riscv_cpu_is_32bit(cpu);
> --
> 2.48.1
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/7] target/riscv: update max_satp_mode based on QOM properties
2025-02-18 16:57 ` [PATCH 5/7] target/riscv: update max_satp_mode based on QOM properties Paolo Bonzini
@ 2025-03-06 1:41 ` Alistair Francis
0 siblings, 0 replies; 23+ messages in thread
From: Alistair Francis @ 2025-03-06 1:41 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Alistair Francis
On Wed, Feb 19, 2025 at 2:59 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Almost all users of cpu->cfg.satp_mode care only about the "max" value
> satp_mode_max_from_map(cpu->cfg.satp_mode.map); convert the QOM
> properties back into it. For TCG, consult valid_vm[] instead of
> the bitmap of accepted modes.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/cpu.h | 1 -
> hw/riscv/virt-acpi-build.c | 14 +++++---------
> hw/riscv/virt.c | 5 ++---
> target/riscv/cpu.c | 27 ++++++++++-----------------
> target/riscv/csr.c | 9 +++++++--
> 5 files changed, 24 insertions(+), 32 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 97713681cbe..f9b223bf8a7 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -911,7 +911,6 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
> target_ulong riscv_new_csr_seed(target_ulong new_value,
> target_ulong write_mask);
>
> -uint8_t satp_mode_max_from_map(uint32_t map);
> const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit);
>
> /* Implemented in th_csr.c */
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> index 2b374ebacbf..1a92a84207d 100644
> --- a/hw/riscv/virt-acpi-build.c
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -261,7 +261,6 @@ static void build_rhct(GArray *table_data,
> uint32_t isa_offset, num_rhct_nodes, cmo_offset = 0;
> RISCVCPU *cpu = &s->soc[0].harts[0];
> uint32_t mmu_offset = 0;
> - uint8_t satp_mode_max;
> bool rv32 = riscv_cpu_is_32bit(cpu);
> g_autofree char *isa = NULL;
>
> @@ -282,8 +281,7 @@ static void build_rhct(GArray *table_data,
> num_rhct_nodes++;
> }
>
> - if (!rv32 && cpu->cfg.satp_mode.supported != 0 &&
> - (cpu->cfg.satp_mode.map & ~(1 << VM_1_10_MBARE))) {
> + if (!rv32 && cpu->cfg.max_satp_mode >= VM_1_10_SV39) {
> num_rhct_nodes++;
> }
>
> @@ -343,20 +341,18 @@ static void build_rhct(GArray *table_data,
> }
>
> /* MMU node structure */
> - if (!rv32 && cpu->cfg.satp_mode.supported != 0 &&
> - (cpu->cfg.satp_mode.map & ~(1 << VM_1_10_MBARE))) {
> - satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> + if (!rv32 && cpu->cfg.max_satp_mode >= VM_1_10_SV39) {
> mmu_offset = table_data->len - table.table_offset;
> build_append_int_noprefix(table_data, 2, 2); /* Type */
> build_append_int_noprefix(table_data, 8, 2); /* Length */
> build_append_int_noprefix(table_data, 0x1, 2); /* Revision */
> build_append_int_noprefix(table_data, 0, 1); /* Reserved */
> /* MMU Type */
> - if (satp_mode_max == VM_1_10_SV57) {
> + if (cpu->cfg.max_satp_mode == VM_1_10_SV57) {
> build_append_int_noprefix(table_data, 2, 1); /* Sv57 */
> - } else if (satp_mode_max == VM_1_10_SV48) {
> + } else if (cpu->cfg.max_satp_mode == VM_1_10_SV48) {
> build_append_int_noprefix(table_data, 1, 1); /* Sv48 */
> - } else if (satp_mode_max == VM_1_10_SV39) {
> + } else if (cpu->cfg.max_satp_mode == VM_1_10_SV39) {
> build_append_int_noprefix(table_data, 0, 1); /* Sv39 */
> } else {
> g_assert_not_reached();
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 241389d72f8..2394fc71df4 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -236,10 +236,10 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
> uint32_t cpu_phandle;
> MachineState *ms = MACHINE(s);
> bool is_32_bit = riscv_is_32bit(&s->soc[0]);
> - uint8_t satp_mode_max;
>
> for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) {
> RISCVCPU *cpu_ptr = &s->soc[socket].harts[cpu];
> + int8_t satp_mode_max = cpu_ptr->cfg.max_satp_mode;
> g_autofree char *cpu_name = NULL;
> g_autofree char *core_name = NULL;
> g_autofree char *intc_name = NULL;
> @@ -251,8 +251,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
> s->soc[socket].hartid_base + cpu);
> qemu_fdt_add_subnode(ms->fdt, cpu_name);
>
> - if (cpu_ptr->cfg.satp_mode.supported != 0) {
> - satp_mode_max = satp_mode_max_from_map(cpu_ptr->cfg.satp_mode.map);
> + if (satp_mode_max != -1) {
> 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);
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 2d06543217a..ce71ee95a52 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -387,7 +387,7 @@ static uint8_t satp_mode_from_str(const char *satp_mode_str)
> g_assert_not_reached();
> }
>
> -uint8_t satp_mode_max_from_map(uint32_t map)
> +static uint8_t satp_mode_max_from_map(uint32_t map)
> {
> /*
> * 'map = 0' will make us return (31 - 32), which C will
> @@ -453,15 +453,13 @@ static void set_satp_mode_default_map(RISCVCPU *cpu)
> /*
> * Bare CPUs do not default to the max available.
> * Users must set a valid satp_mode in the command
> - * line.
> + * line. Otherwise, leave the existing max_satp_mode
> + * in place.
> */
> if (object_dynamic_cast(OBJECT(cpu), TYPE_RISCV_BARE_CPU) != NULL) {
> warn_report("No satp mode set. Defaulting to 'bare'");
> - cpu->cfg.satp_mode.map = (1 << VM_1_10_MBARE);
> - return;
> + cpu->cfg.max_satp_mode = VM_1_10_MBARE;
> }
> -
> - cpu->cfg.satp_mode.map = cpu->cfg.satp_mode.supported;
> }
> #endif
>
> @@ -1180,8 +1178,8 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> bool rv32 = riscv_cpu_is_32bit(cpu);
> uint8_t satp_mode_map_max;
>
> - /* The CPU wants the OS to decide which satp mode to use */
> - if (cpu->cfg.satp_mode.supported == 0) {
> + if (cpu->cfg.max_satp_mode == -1) {
> + /* The CPU wants the hypervisor to decide which satp mode to allow */
> return;
> }
>
> @@ -1200,14 +1198,14 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> (cpu->cfg.satp_mode.supported & (1 << i))) {
> for (int j = i - 1; j >= 0; --j) {
> if (cpu->cfg.satp_mode.supported & (1 << j)) {
> - cpu->cfg.satp_mode.map |= (1 << j);
> - break;
> + cpu->cfg.max_satp_mode = j;
> + return;
> }
> }
> - break;
> }
> }
> }
> + return;
> }
>
> satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> @@ -1237,12 +1235,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> }
> }
>
> - /* Finally expand the map so that all valid modes are set */
> - for (int i = satp_mode_map_max - 1; i >= 0; --i) {
> - if (cpu->cfg.satp_mode.supported & (1 << i)) {
> - cpu->cfg.satp_mode.map |= (1 << i);
> - }
> - }
> + cpu->cfg.max_satp_mode = satp_mode_map_max;
> }
> #endif
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index afb7544f078..78db9aeda57 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1862,8 +1862,13 @@ static RISCVException read_mstatus(CPURISCVState *env, int csrno,
>
> static bool validate_vm(CPURISCVState *env, target_ulong vm)
> {
> - uint64_t mode_supported = riscv_cpu_cfg(env)->satp_mode.map;
> - return get_field(mode_supported, (1 << vm));
> + bool rv32 = riscv_cpu_mxl(env) == MXL_RV32;
> + RISCVCPU *cpu = env_archcpu(env);
> + int satp_mode_supported_max = cpu->cfg.max_satp_mode;
> + const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
> +
> + assert(satp_mode_supported_max >= 0);
> + return vm <= satp_mode_supported_max && valid_vm[vm];
> }
>
> static target_ulong legalize_xatp(CPURISCVState *env, target_ulong old_xatp,
> --
> 2.48.1
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 6/7] target/riscv: remove supported from RISCVSATPMap
2025-02-18 16:57 ` [PATCH 6/7] target/riscv: remove supported from RISCVSATPMap Paolo Bonzini
@ 2025-03-06 2:39 ` Alistair Francis
0 siblings, 0 replies; 23+ messages in thread
From: Alistair Francis @ 2025-03-06 2:39 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Alistair Francis
On Wed, Feb 19, 2025 at 2:59 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> "supported" can be computed on the fly based on the max_satp_mode.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/cpu_cfg.h | 4 +---
> target/riscv/cpu.c | 34 ++++++++++++++++++++++++----------
> 2 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 28d8de978fa..1d7fff8decd 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -29,11 +29,9 @@
> *
> * init is a 16-bit bitmap used to make sure the user selected a correct
> * configuration as per the specification.
> - *
> - * supported is a 16-bit bitmap used to reflect the hw capabilities.
> */
> typedef struct {
> - uint16_t map, init, supported;
> + uint16_t map, init;
> } RISCVSATPMap;
>
> struct RISCVCPUConfig {
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index ce71ee95a52..86a048b62c5 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -437,14 +437,27 @@ static void set_satp_mode_max_supported(RISCVCPU *cpu,
> bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
>
> - for (int i = 0; i <= satp_mode; ++i) {
> - if (valid_vm[i]) {
> - cpu->cfg.satp_mode.supported |= (1 << i);
> - }
> + assert(valid_vm[satp_mode]);
> + cpu->cfg.max_satp_mode = satp_mode;
> +}
> +
> +static bool get_satp_mode_supported(RISCVCPU *cpu, uint16_t *supported)
> +{
> + bool rv32 = riscv_cpu_is_32bit(cpu);
> + const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
> + int satp_mode = cpu->cfg.max_satp_mode;
> +
> + if (satp_mode == -1) {
> + return false;
> }
>
> - assert(cpu->cfg.satp_mode.supported & (1 << satp_mode));
> - cpu->cfg.max_satp_mode = satp_mode;
> + *supported = 0;
> + for (int i = 0; i <= satp_mode; ++i) {
> + if (valid_vm[i]) {
> + *supported |= (1 << i);
> + }
> + }
> + return true;
> }
>
> /* Set the satp mode to the max supported */
> @@ -1176,9 +1189,10 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
> static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> {
> bool rv32 = riscv_cpu_is_32bit(cpu);
> + uint16_t supported;
> uint8_t satp_mode_map_max;
>
> - if (cpu->cfg.max_satp_mode == -1) {
> + if (!get_satp_mode_supported(cpu, &supported)) {
> /* The CPU wants the hypervisor to decide which satp mode to allow */
> return;
> }
> @@ -1195,9 +1209,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> */
> for (int i = 1; i < 16; ++i) {
> if ((cpu->cfg.satp_mode.init & (1 << i)) &&
> - (cpu->cfg.satp_mode.supported & (1 << i))) {
> + supported & (1 << i)) {
> for (int j = i - 1; j >= 0; --j) {
> - if (cpu->cfg.satp_mode.supported & (1 << j)) {
> + if (supported & (1 << j)) {
> cpu->cfg.max_satp_mode = j;
> return;
> }
> @@ -1226,7 +1240,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> for (int i = satp_mode_map_max - 1; i >= 0; --i) {
> if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
> (cpu->cfg.satp_mode.init & (1 << i)) &&
> - (cpu->cfg.satp_mode.supported & (1 << i))) {
> + (supported & (1 << i))) {
> error_setg(errp, "cannot disable %s satp mode if %s "
> "is enabled", satp_mode_str(i, false),
> satp_mode_str(satp_mode_map_max, false));
> --
> 2.48.1
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 7/7] target/riscv: move satp_mode.{map, init} out of CPUConfig
2025-02-18 16:57 ` [PATCH 7/7] target/riscv: move satp_mode.{map,init} out of CPUConfig Paolo Bonzini
@ 2025-03-06 2:42 ` Alistair Francis
0 siblings, 0 replies; 23+ messages in thread
From: Alistair Francis @ 2025-03-06 2:42 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Alistair Francis
On Wed, Feb 19, 2025 at 3:00 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> They are used to provide the nice QOM properties for svNN,
> but the canonical source of the CPU configuration is now
> cpu->cfg.max_satp_mode. Store them in the ArchCPU struct.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/cpu.h | 14 ++++++++++++++
> target/riscv/cpu_cfg.h | 14 --------------
> target/riscv/cpu.c | 32 ++++++++++++++++----------------
> 3 files changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index f9b223bf8a7..df7a05e7d15 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -492,6 +492,19 @@ struct CPUArchState {
> uint64_t rnmi_excpvec;
> };
>
> +/*
> + * map is a 16-bit bitmap: the most significant set bit in map is the maximum
> + * satp mode that is supported. It may be chosen by the user and must respect
> + * what qemu implements (valid_1_10_32/64) and what the hw is capable of
> + * (supported bitmap below).
> + *
> + * init is a 16-bit bitmap used to make sure the user selected a correct
> + * configuration as per the specification.
> + */
> +typedef struct {
> + uint16_t map, init;
> +} RISCVSATPModes;
> +
> /*
> * RISCVCPU:
> * @env: #CPURISCVState
> @@ -508,6 +521,7 @@ struct ArchCPU {
>
> /* Configuration Settings */
> RISCVCPUConfig cfg;
> + RISCVSATPModes satp_modes;
>
> QEMUTimer *pmu_timer;
> /* A bitmask of Available programmable counters */
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 1d7fff8decd..7b7067d5bee 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -21,19 +21,6 @@
> #ifndef RISCV_CPU_CFG_H
> #define RISCV_CPU_CFG_H
>
> -/*
> - * map is a 16-bit bitmap: the most significant set bit in map is the maximum
> - * satp mode that is supported. It may be chosen by the user and must respect
> - * what qemu implements (valid_1_10_32/64) and what the hw is capable of
> - * (supported bitmap below).
> - *
> - * init is a 16-bit bitmap used to make sure the user selected a correct
> - * configuration as per the specification.
> - */
> -typedef struct {
> - uint16_t map, init;
> -} RISCVSATPMap;
> -
> struct RISCVCPUConfig {
> bool ext_zba;
> bool ext_zbb;
> @@ -191,7 +178,6 @@ struct RISCVCPUConfig {
>
> #ifndef CONFIG_USER_ONLY
> int8_t max_satp_mode;
> - RISCVSATPMap satp_mode;
> #endif
> };
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 86a048b62c5..8ab7fe2e286 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1197,8 +1197,8 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> return;
> }
>
> - if (cpu->cfg.satp_mode.map == 0) {
> - if (cpu->cfg.satp_mode.init == 0) {
> + if (cpu->satp_modes.map == 0) {
> + if (cpu->satp_modes.init == 0) {
> /* If unset by the user, we fallback to the default satp mode. */
> set_satp_mode_default_map(cpu);
> } else {
> @@ -1208,7 +1208,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> * valid_vm_1_10_32/64.
> */
> for (int i = 1; i < 16; ++i) {
> - if ((cpu->cfg.satp_mode.init & (1 << i)) &&
> + if ((cpu->satp_modes.init & (1 << i)) &&
> supported & (1 << i)) {
> for (int j = i - 1; j >= 0; --j) {
> if (supported & (1 << j)) {
> @@ -1222,7 +1222,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> return;
> }
>
> - satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> + satp_mode_map_max = satp_mode_max_from_map(cpu->satp_modes.map);
>
> /* Make sure the user asked for a supported configuration (HW and qemu) */
> if (satp_mode_map_max > cpu->cfg.max_satp_mode) {
> @@ -1238,8 +1238,8 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> */
> if (!rv32) {
> for (int i = satp_mode_map_max - 1; i >= 0; --i) {
> - if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
> - (cpu->cfg.satp_mode.init & (1 << i)) &&
> + if (!(cpu->satp_modes.map & (1 << i)) &&
> + (cpu->satp_modes.init & (1 << i)) &&
> (supported & (1 << i))) {
> error_setg(errp, "cannot disable %s satp mode if %s "
> "is enabled", satp_mode_str(i, false),
> @@ -1327,11 +1327,11 @@ bool riscv_cpu_accelerator_compatible(RISCVCPU *cpu)
> static void cpu_riscv_get_satp(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> - RISCVSATPMap *satp_map = opaque;
> + RISCVSATPModes *satp_modes = opaque;
> uint8_t satp = satp_mode_from_str(name);
> bool value;
>
> - value = satp_map->map & (1 << satp);
> + value = satp_modes->map & (1 << satp);
>
> visit_type_bool(v, name, &value, errp);
> }
> @@ -1339,7 +1339,7 @@ static void cpu_riscv_get_satp(Object *obj, Visitor *v, const char *name,
> static void cpu_riscv_set_satp(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> - RISCVSATPMap *satp_map = opaque;
> + RISCVSATPModes *satp_modes = opaque;
> uint8_t satp = satp_mode_from_str(name);
> bool value;
>
> @@ -1347,8 +1347,8 @@ static void cpu_riscv_set_satp(Object *obj, Visitor *v, const char *name,
> return;
> }
>
> - satp_map->map = deposit32(satp_map->map, satp, 1, value);
> - satp_map->init |= 1 << satp;
> + satp_modes->map = deposit32(satp_modes->map, satp, 1, value);
> + satp_modes->init |= 1 << satp;
> }
>
> void riscv_add_satp_mode_properties(Object *obj)
> @@ -1357,16 +1357,16 @@ void riscv_add_satp_mode_properties(Object *obj)
>
> if (cpu->env.misa_mxl == MXL_RV32) {
> object_property_add(obj, "sv32", "bool", cpu_riscv_get_satp,
> - cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
> + cpu_riscv_set_satp, NULL, &cpu->satp_modes);
> } else {
> object_property_add(obj, "sv39", "bool", cpu_riscv_get_satp,
> - cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
> + cpu_riscv_set_satp, NULL, &cpu->satp_modes);
> object_property_add(obj, "sv48", "bool", cpu_riscv_get_satp,
> - cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
> + cpu_riscv_set_satp, NULL, &cpu->satp_modes);
> object_property_add(obj, "sv57", "bool", cpu_riscv_get_satp,
> - cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
> + cpu_riscv_set_satp, NULL, &cpu->satp_modes);
> object_property_add(obj, "sv64", "bool", cpu_riscv_get_satp,
> - cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
> + cpu_riscv_set_satp, NULL, &cpu->satp_modes);
> }
> }
>
> --
> 2.48.1
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/7] target/riscv: cpu: store max SATP mode as a single integer
2025-02-18 16:57 ` [PATCH 4/7] target/riscv: cpu: store max SATP mode as a single integer Paolo Bonzini
2025-03-06 1:30 ` Alistair Francis
@ 2025-03-06 2:57 ` Alistair Francis
1 sibling, 0 replies; 23+ messages in thread
From: Alistair Francis @ 2025-03-06 2:57 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Alistair Francis
On Wed, Feb 19, 2025 at 3:00 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> The maximum available SATP mode implies all the shorter virtual address sizes.
> Store it in RISCVCPUConfig and avoid recomputing it via satp_mode_max_from_map.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
This fails to build on the latest riscv-to-apply.next
(https://github.com/alistair23/qemu/tree/riscv-to-apply.next)
../target/riscv/cpu.c: In function ‘riscv_cpu_init’:
../target/riscv/cpu.c:1481:13: error: ‘RISCVCPUConfig’ has no member
named ‘max_satp_mode’
1481 | cpu->cfg.max_satp_mode = -1;
| ^
Do you mind rebasing?
Alistair
> ---
> target/riscv/cpu_cfg.h | 1 +
> target/riscv/cpu.c | 11 +++++------
> target/riscv/tcg/tcg-cpu.c | 3 ++-
> 3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index b410b1e6038..28d8de978fa 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -192,6 +192,7 @@ struct RISCVCPUConfig {
> bool short_isa_string;
>
> #ifndef CONFIG_USER_ONLY
> + int8_t max_satp_mode;
> RISCVSATPMap satp_mode;
> #endif
> };
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7950b6447f8..2d06543217a 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -444,6 +444,7 @@ static void set_satp_mode_max_supported(RISCVCPU *cpu,
> }
>
> assert(cpu->cfg.satp_mode.supported & (1 << satp_mode));
> + cpu->cfg.max_satp_mode = satp_mode;
> }
>
> /* Set the satp mode to the max supported */
> @@ -1177,16 +1178,13 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
> static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> {
> bool rv32 = riscv_cpu_is_32bit(cpu);
> - uint8_t satp_mode_map_max, satp_mode_supported_max;
> + uint8_t satp_mode_map_max;
>
> /* The CPU wants the OS to decide which satp mode to use */
> if (cpu->cfg.satp_mode.supported == 0) {
> return;
> }
>
> - satp_mode_supported_max =
> - satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
> -
> if (cpu->cfg.satp_mode.map == 0) {
> if (cpu->cfg.satp_mode.init == 0) {
> /* If unset by the user, we fallback to the default satp mode. */
> @@ -1215,10 +1213,10 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
>
> /* Make sure the user asked for a supported configuration (HW and qemu) */
> - if (satp_mode_map_max > satp_mode_supported_max) {
> + if (satp_mode_map_max > cpu->cfg.max_satp_mode) {
> error_setg(errp, "satp_mode %s is higher than hw max capability %s",
> satp_mode_str(satp_mode_map_max, rv32),
> - satp_mode_str(satp_mode_supported_max, rv32));
> + satp_mode_str(cpu->cfg.max_satp_mode, rv32));
> return;
> }
>
> @@ -1477,6 +1475,7 @@ static void riscv_cpu_init(Object *obj)
> cpu->cfg.cbom_blocksize = 64;
> cpu->cfg.cbop_blocksize = 64;
> cpu->cfg.cboz_blocksize = 64;
> + cpu->cfg.max_satp_mode = -1;
> cpu->env.vext_ver = VEXT_VERSION_1_00_0;
> }
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 0a137281de1..a9f59a67e00 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -693,8 +693,9 @@ static bool riscv_cpu_validate_profile_satp(RISCVCPU *cpu,
> RISCVCPUProfile *profile,
> bool send_warn)
> {
> - int satp_max = satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
> + int satp_max = cpu->cfg.max_satp_mode;
>
> + assert(satp_max >= 0);
> if (profile->satp_mode > satp_max) {
> if (send_warn) {
> bool is_32bit = riscv_cpu_is_32bit(cpu);
> --
> 2.48.1
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/7] hw/riscv: acpi: only create RHCT MMU entry for supported types
2025-03-06 1:13 ` Alistair Francis
@ 2025-03-06 12:11 ` Paolo Bonzini
0 siblings, 0 replies; 23+ messages in thread
From: Paolo Bonzini @ 2025-03-06 12:11 UTC (permalink / raw)
To: Alistair Francis; +Cc: qemu-devel, Alistair Francis
On Thu, Mar 6, 2025 at 2:13 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Feb 19, 2025 at 2:58 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > Do not create the RHCT MMU type entry for RV32 CPUs, since it
> > only has definitions for SV39/SV48/SV57. Likewise, check that
>
> I don't have access to the spec, so I'm going to take your word on this
Thanks for reviewing - the closest thing I found to a spec are two
Google documents linked from
https://github.com/riscv-non-isa/riscv-acpi/issues/16 and
https://github.com/riscv-non-isa/riscv-acpi/issues/18.
In particular, the MMU type documentation can be found at
https://drive.google.com/file/d/1sKbOa8m1UZw1JkquZYe3F1zQBN1xXsaf/view:
MMU Type (byte length=1, byte offset=7)
0: Sv39
1: Sv48
2: Sv57
All other values are reserved.
Paolo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/7] target/riscv: env->misa_mxl is a constant
2025-03-06 1:16 ` Alistair Francis
@ 2025-03-06 13:00 ` Paolo Bonzini
2025-03-07 0:44 ` Alistair Francis
0 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2025-03-06 13:00 UTC (permalink / raw)
To: Alistair Francis; +Cc: qemu-devel, Alistair Francis
On 3/6/25 02:16, Alistair Francis wrote:
> On Wed, Feb 19, 2025 at 3:01 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> There is nothing that overwrites env->misa_mxl, so it is a constant. Do
>
> The idea is that misa_mxl can change, although that's not supported now.
At run-time, or only at configuration time (before realize)?
>> not let a corrupted migration stream change the value; changing misa_mxl
>
> Does this actually happen? If the migration data is corrupted won't we
> have all sorts of strange issues?
Generally migration data (just like disk image formats) is treated as
security-sensitive, overriding any other considerations. So you have to
assume that the corruption is intentional, and sneaky enough to cause
trouble.
Paolo
> Alistair
>
>> would have a snowball effect on, for example, the valid VM modes.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> target/riscv/machine.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
>> index d8445244ab2..c3d8e7c4005 100644
>> --- a/target/riscv/machine.c
>> +++ b/target/riscv/machine.c
>> @@ -375,6 +375,18 @@ static const VMStateDescription vmstate_ssp = {
>> }
>> };
>>
>> +static bool riscv_validate_misa_mxl(void *opaque, int version_id)
>> +{
>> + RISCVCPU *cpu = RISCV_CPU(opaque);
>> + CPURISCVState *env = &cpu->env;
>> + RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
>> + uint32_t misa_mxl_saved = env->misa_mxl;
>> +
>> + /* Preserve misa_mxl even if the migration stream corrupted it */
>> + env->misa_mxl = mcc->misa_mxl_max;
>> + return misa_mxl_saved == mcc->misa_mxl_max;
>> +}
>> +
>> const VMStateDescription vmstate_riscv_cpu = {
>> .name = "cpu",
>> .version_id = 10,
>> @@ -394,6 +406,7 @@ const VMStateDescription vmstate_riscv_cpu = {
>> VMSTATE_UINTTL(env.priv_ver, RISCVCPU),
>> VMSTATE_UINTTL(env.vext_ver, RISCVCPU),
>> VMSTATE_UINT32(env.misa_mxl, RISCVCPU),
>> + VMSTATE_VALIDATE("MXL must match", riscv_validate_misa_mxl),
>> VMSTATE_UINT32(env.misa_ext, RISCVCPU),
>> VMSTATE_UNUSED(4),
>> VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU),
>> --
>> 2.48.1
>>
>>
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/7] target/riscv: env->misa_mxl is a constant
2025-03-06 13:00 ` Paolo Bonzini
@ 2025-03-07 0:44 ` Alistair Francis
2025-03-10 17:34 ` Paolo Bonzini
0 siblings, 1 reply; 23+ messages in thread
From: Alistair Francis @ 2025-03-07 0:44 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Alistair Francis
On Thu, Mar 6, 2025 at 11:00 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 3/6/25 02:16, Alistair Francis wrote:
> > On Wed, Feb 19, 2025 at 3:01 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >> There is nothing that overwrites env->misa_mxl, so it is a constant. Do
> >
> > The idea is that misa_mxl can change, although that's not supported now.
>
> At run-time, or only at configuration time (before realize)?
Runtime, at least kind of
The RISC-V spec 1.12 and earlier allows MXL to change at runtime by
writing to misa.MXL. QEMU doesn't support this and AFAIK no hardware
does either, but it was something that we might support in the future
(hence the split).
The latest RISC-V priv spec has changed misa.MXL to be read only though.
So I guess although in theory it can be changed at runtime, we are
probably never going to support that now that it's deprecated.
Now that the latest priv spec has dropped the ability to write to
misa.MXL we will probably work towards just consolidating misa_mxl_max
and misa_mxl into a single value that is constant after realise.
>
> >> not let a corrupted migration stream change the value; changing misa_mxl
> >
> > Does this actually happen? If the migration data is corrupted won't we
> > have all sorts of strange issues?
>
> Generally migration data (just like disk image formats) is treated as
> security-sensitive, overriding any other considerations. So you have to
> assume that the corruption is intentional, and sneaky enough to cause
> trouble.
I'm not convinced that this is the thing that we should be checking
for. If someone can corrupt the migration data for an attack there are
better things to change then the MXL
Alistair
>
> Paolo
>
> > Alistair
> >
> >> would have a snowball effect on, for example, the valid VM modes.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >> target/riscv/machine.c | 13 +++++++++++++
> >> 1 file changed, 13 insertions(+)
> >>
> >> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> >> index d8445244ab2..c3d8e7c4005 100644
> >> --- a/target/riscv/machine.c
> >> +++ b/target/riscv/machine.c
> >> @@ -375,6 +375,18 @@ static const VMStateDescription vmstate_ssp = {
> >> }
> >> };
> >>
> >> +static bool riscv_validate_misa_mxl(void *opaque, int version_id)
> >> +{
> >> + RISCVCPU *cpu = RISCV_CPU(opaque);
> >> + CPURISCVState *env = &cpu->env;
> >> + RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> >> + uint32_t misa_mxl_saved = env->misa_mxl;
> >> +
> >> + /* Preserve misa_mxl even if the migration stream corrupted it */
> >> + env->misa_mxl = mcc->misa_mxl_max;
> >> + return misa_mxl_saved == mcc->misa_mxl_max;
> >> +}
> >> +
> >> const VMStateDescription vmstate_riscv_cpu = {
> >> .name = "cpu",
> >> .version_id = 10,
> >> @@ -394,6 +406,7 @@ const VMStateDescription vmstate_riscv_cpu = {
> >> VMSTATE_UINTTL(env.priv_ver, RISCVCPU),
> >> VMSTATE_UINTTL(env.vext_ver, RISCVCPU),
> >> VMSTATE_UINT32(env.misa_mxl, RISCVCPU),
> >> + VMSTATE_VALIDATE("MXL must match", riscv_validate_misa_mxl),
> >> VMSTATE_UINT32(env.misa_ext, RISCVCPU),
> >> VMSTATE_UNUSED(4),
> >> VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU),
> >> --
> >> 2.48.1
> >>
> >>
> >
> >
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/7] target/riscv: env->misa_mxl is a constant
2025-03-07 0:44 ` Alistair Francis
@ 2025-03-10 17:34 ` Paolo Bonzini
2025-03-10 22:18 ` Alistair Francis
0 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2025-03-10 17:34 UTC (permalink / raw)
To: Alistair Francis; +Cc: qemu-devel, Alistair Francis
On Fri, Mar 7, 2025 at 1:45 AM Alistair Francis <alistair23@gmail.com> wrote:
> I'm not convinced that this is the thing that we should be checking
> for. If someone can corrupt the migration data for an attack there are
> better things to change then the MXL
In principle you could have code that uses 2 << MXL to compute the
size of a memcpy, or something like that... never say never. :)
Do you prefer turning all the priv_ver, vext_ver, misa_mxl,
misa_ext_mask fields into VMSTATE_UNUSED? That would also be okay.
I would also add a check for misa_ext against misa_ext_mask and
riscv_cpu_validate_set_extensions().
Paolo
> Alistair
>
> >
> > Paolo
> >
> > > Alistair
> > >
> > >> would have a snowball effect on, for example, the valid VM modes.
> > >>
> > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > >> ---
> > >> target/riscv/machine.c | 13 +++++++++++++
> > >> 1 file changed, 13 insertions(+)
> > >>
> > >> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> > >> index d8445244ab2..c3d8e7c4005 100644
> > >> --- a/target/riscv/machine.c
> > >> +++ b/target/riscv/machine.c
> > >> @@ -375,6 +375,18 @@ static const VMStateDescription vmstate_ssp = {
> > >> }
> > >> };
> > >>
> > >> +static bool riscv_validate_misa_mxl(void *opaque, int version_id)
> > >> +{
> > >> + RISCVCPU *cpu = RISCV_CPU(opaque);
> > >> + CPURISCVState *env = &cpu->env;
> > >> + RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> > >> + uint32_t misa_mxl_saved = env->misa_mxl;
> > >> +
> > >> + /* Preserve misa_mxl even if the migration stream corrupted it */
> > >> + env->misa_mxl = mcc->misa_mxl_max;
> > >> + return misa_mxl_saved == mcc->misa_mxl_max;
> > >> +}
> > >> +
> > >> const VMStateDescription vmstate_riscv_cpu = {
> > >> .name = "cpu",
> > >> .version_id = 10,
> > >> @@ -394,6 +406,7 @@ const VMStateDescription vmstate_riscv_cpu = {
> > >> VMSTATE_UINTTL(env.priv_ver, RISCVCPU),
> > >> VMSTATE_UINTTL(env.vext_ver, RISCVCPU),
> > >> VMSTATE_UINT32(env.misa_mxl, RISCVCPU),
> > >> + VMSTATE_VALIDATE("MXL must match", riscv_validate_misa_mxl),
> > >> VMSTATE_UINT32(env.misa_ext, RISCVCPU),
> > >> VMSTATE_UNUSED(4),
> > >> VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU),
> > >> --
> > >> 2.48.1
> > >>
> > >>
> > >
> > >
> >
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/7] target/riscv: env->misa_mxl is a constant
2025-03-10 17:34 ` Paolo Bonzini
@ 2025-03-10 22:18 ` Alistair Francis
2025-03-11 6:17 ` Paolo Bonzini
0 siblings, 1 reply; 23+ messages in thread
From: Alistair Francis @ 2025-03-10 22:18 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Alistair Francis
On Tue, Mar 11, 2025 at 3:34 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On Fri, Mar 7, 2025 at 1:45 AM Alistair Francis <alistair23@gmail.com> wrote:
> > I'm not convinced that this is the thing that we should be checking
> > for. If someone can corrupt the migration data for an attack there are
> > better things to change then the MXL
>
> In principle you could have code that uses 2 << MXL to compute the
> size of a memcpy, or something like that... never say never. :)
But couldn't they also change the PC at that point and execute
malicious code? Or MTVEC or anything else?
It just seems like this check doesn't actually provide any security.
In the future we will want to merge misa_mxl and misa_mxl_max and this
patch just makes that a little bit harder, for no real gains that I
can see.
>
> Do you prefer turning all the priv_ver, vext_ver, misa_mxl,
> misa_ext_mask fields into VMSTATE_UNUSED? That would also be okay.
I think we do want to migrate them, they contain important information.
Alistair
>
> I would also add a check for misa_ext against misa_ext_mask and
> riscv_cpu_validate_set_extensions().
>
> Paolo
>
> > Alistair
> >
> > >
> > > Paolo
> > >
> > > > Alistair
> > > >
> > > >> would have a snowball effect on, for example, the valid VM modes.
> > > >>
> > > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > >> ---
> > > >> target/riscv/machine.c | 13 +++++++++++++
> > > >> 1 file changed, 13 insertions(+)
> > > >>
> > > >> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> > > >> index d8445244ab2..c3d8e7c4005 100644
> > > >> --- a/target/riscv/machine.c
> > > >> +++ b/target/riscv/machine.c
> > > >> @@ -375,6 +375,18 @@ static const VMStateDescription vmstate_ssp = {
> > > >> }
> > > >> };
> > > >>
> > > >> +static bool riscv_validate_misa_mxl(void *opaque, int version_id)
> > > >> +{
> > > >> + RISCVCPU *cpu = RISCV_CPU(opaque);
> > > >> + CPURISCVState *env = &cpu->env;
> > > >> + RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> > > >> + uint32_t misa_mxl_saved = env->misa_mxl;
> > > >> +
> > > >> + /* Preserve misa_mxl even if the migration stream corrupted it */
> > > >> + env->misa_mxl = mcc->misa_mxl_max;
> > > >> + return misa_mxl_saved == mcc->misa_mxl_max;
> > > >> +}
> > > >> +
> > > >> const VMStateDescription vmstate_riscv_cpu = {
> > > >> .name = "cpu",
> > > >> .version_id = 10,
> > > >> @@ -394,6 +406,7 @@ const VMStateDescription vmstate_riscv_cpu = {
> > > >> VMSTATE_UINTTL(env.priv_ver, RISCVCPU),
> > > >> VMSTATE_UINTTL(env.vext_ver, RISCVCPU),
> > > >> VMSTATE_UINT32(env.misa_mxl, RISCVCPU),
> > > >> + VMSTATE_VALIDATE("MXL must match", riscv_validate_misa_mxl),
> > > >> VMSTATE_UINT32(env.misa_ext, RISCVCPU),
> > > >> VMSTATE_UNUSED(4),
> > > >> VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU),
> > > >> --
> > > >> 2.48.1
> > > >>
> > > >>
> > > >
> > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/7] target/riscv: env->misa_mxl is a constant
2025-03-10 22:18 ` Alistair Francis
@ 2025-03-11 6:17 ` Paolo Bonzini
2025-03-19 1:35 ` Alistair Francis
0 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2025-03-11 6:17 UTC (permalink / raw)
To: Alistair Francis; +Cc: qemu-devel, Alistair Francis
[-- Attachment #1: Type: text/plain, Size: 3634 bytes --]
Il lun 10 mar 2025, 23:18 Alistair Francis <alistair23@gmail.com> ha
scritto:
> On Tue, Mar 11, 2025 at 3:34 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On Fri, Mar 7, 2025 at 1:45 AM Alistair Francis <alistair23@gmail.com>
> wrote:
> > > I'm not convinced that this is the thing that we should be checking
> > > for. If someone can corrupt the migration data for an attack there are
> > > better things to change then the MXL
> >
> > In principle you could have code that uses 2 << MXL to compute the
> > size of a memcpy, or something like that... never say never. :)
>
> But couldn't they also change the PC at that point and execute
> malicious code? Or MTVEC or anything else?
>
The point is exploiting the host, not the guest.
It just seems like this check doesn't actually provide any security.
In the future we will want to merge misa_mxl and misa_mxl_max and this
> patch just makes that a little bit harder, for no real gains that I
> can see.
>
> > Do you prefer turning all the priv_ver, vext_ver, misa_mxl,
> > misa_ext_mask fields into VMSTATE_UNUSED? That would also be okay.
>
> I think we do want to migrate them, they contain important information.
>
They are constant though, aren't they? Properties aren't migrated, they are
set on the command line of the destination.
I will drop the patch, but I think there's some work to do on the migration
of RISC-V CPU state.
Paolo
> Alistair
>
> >
> > I would also add a check for misa_ext against misa_ext_mask and
> > riscv_cpu_validate_set_extensions().
> >
> > Paolo
> >
> > > Alistair
> > >
> > > >
> > > > Paolo
> > > >
> > > > > Alistair
> > > > >
> > > > >> would have a snowball effect on, for example, the valid VM modes.
> > > > >>
> > > > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > > >> ---
> > > > >> target/riscv/machine.c | 13 +++++++++++++
> > > > >> 1 file changed, 13 insertions(+)
> > > > >>
> > > > >> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> > > > >> index d8445244ab2..c3d8e7c4005 100644
> > > > >> --- a/target/riscv/machine.c
> > > > >> +++ b/target/riscv/machine.c
> > > > >> @@ -375,6 +375,18 @@ static const VMStateDescription vmstate_ssp
> = {
> > > > >> }
> > > > >> };
> > > > >>
> > > > >> +static bool riscv_validate_misa_mxl(void *opaque, int version_id)
> > > > >> +{
> > > > >> + RISCVCPU *cpu = RISCV_CPU(opaque);
> > > > >> + CPURISCVState *env = &cpu->env;
> > > > >> + RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> > > > >> + uint32_t misa_mxl_saved = env->misa_mxl;
> > > > >> +
> > > > >> + /* Preserve misa_mxl even if the migration stream corrupted
> it */
> > > > >> + env->misa_mxl = mcc->misa_mxl_max;
> > > > >> + return misa_mxl_saved == mcc->misa_mxl_max;
> > > > >> +}
> > > > >> +
> > > > >> const VMStateDescription vmstate_riscv_cpu = {
> > > > >> .name = "cpu",
> > > > >> .version_id = 10,
> > > > >> @@ -394,6 +406,7 @@ const VMStateDescription vmstate_riscv_cpu = {
> > > > >> VMSTATE_UINTTL(env.priv_ver, RISCVCPU),
> > > > >> VMSTATE_UINTTL(env.vext_ver, RISCVCPU),
> > > > >> VMSTATE_UINT32(env.misa_mxl, RISCVCPU),
> > > > >> + VMSTATE_VALIDATE("MXL must match",
> riscv_validate_misa_mxl),
> > > > >> VMSTATE_UINT32(env.misa_ext, RISCVCPU),
> > > > >> VMSTATE_UNUSED(4),
> > > > >> VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU),
> > > > >> --
> > > > >> 2.48.1
> > > > >>
> > > > >>
> > > > >
> > > > >
> > > >
> > >
> >
>
>
[-- Attachment #2: Type: text/html, Size: 6131 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/7] target/riscv: env->misa_mxl is a constant
2025-03-11 6:17 ` Paolo Bonzini
@ 2025-03-19 1:35 ` Alistair Francis
0 siblings, 0 replies; 23+ messages in thread
From: Alistair Francis @ 2025-03-19 1:35 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Alistair Francis
On Tue, Mar 11, 2025 at 4:18 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
>
> Il lun 10 mar 2025, 23:18 Alistair Francis <alistair23@gmail.com> ha scritto:
>>
>> On Tue, Mar 11, 2025 at 3:34 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>> >
>> > On Fri, Mar 7, 2025 at 1:45 AM Alistair Francis <alistair23@gmail.com> wrote:
>> > > I'm not convinced that this is the thing that we should be checking
>> > > for. If someone can corrupt the migration data for an attack there are
>> > > better things to change then the MXL
>> >
>> > In principle you could have code that uses 2 << MXL to compute the
>> > size of a memcpy, or something like that... never say never. :)
>>
>> But couldn't they also change the PC at that point and execute
>> malicious code? Or MTVEC or anything else?
>
>
> The point is exploiting the host, not the guest.
Ah! Ok, I misunderstood.
>
>> It just seems like this check doesn't actually provide any security.
>>
>> In the future we will want to merge misa_mxl and misa_mxl_max and this
>> patch just makes that a little bit harder, for no real gains that I
>> can see.
>>
>> > Do you prefer turning all the priv_ver, vext_ver, misa_mxl,
>> > misa_ext_mask fields into VMSTATE_UNUSED? That would also be okay.
>>
>> I think we do want to migrate them, they contain important information.
>
>
> They are constant though, aren't they? Properties aren't migrated, they are set on the command line of the destination.
I didn't think about properties not being migrated.
At which point I think it does then make sense to make them VMSTATE_UNUSED
Alistair
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2025-03-19 1:37 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-18 16:57 [PATCH 0/7] target/riscv: store max SATP mode as a single integer in RISCVCPUConfig Paolo Bonzini
2025-02-18 16:57 ` [PATCH 1/7] hw/riscv: acpi: only create RHCT MMU entry for supported types Paolo Bonzini
2025-03-06 1:13 ` Alistair Francis
2025-03-06 12:11 ` Paolo Bonzini
2025-02-18 16:57 ` [PATCH 2/7] target/riscv: env->misa_mxl is a constant Paolo Bonzini
2025-03-06 1:16 ` Alistair Francis
2025-03-06 13:00 ` Paolo Bonzini
2025-03-07 0:44 ` Alistair Francis
2025-03-10 17:34 ` Paolo Bonzini
2025-03-10 22:18 ` Alistair Francis
2025-03-11 6:17 ` Paolo Bonzini
2025-03-19 1:35 ` Alistair Francis
2025-02-18 16:57 ` [PATCH 3/7] target/riscv: assert argument to set_satp_mode_max_supported is valid Paolo Bonzini
2025-03-06 1:23 ` Alistair Francis
2025-02-18 16:57 ` [PATCH 4/7] target/riscv: cpu: store max SATP mode as a single integer Paolo Bonzini
2025-03-06 1:30 ` Alistair Francis
2025-03-06 2:57 ` Alistair Francis
2025-02-18 16:57 ` [PATCH 5/7] target/riscv: update max_satp_mode based on QOM properties Paolo Bonzini
2025-03-06 1:41 ` Alistair Francis
2025-02-18 16:57 ` [PATCH 6/7] target/riscv: remove supported from RISCVSATPMap Paolo Bonzini
2025-03-06 2:39 ` Alistair Francis
2025-02-18 16:57 ` [PATCH 7/7] target/riscv: move satp_mode.{map,init} out of CPUConfig Paolo Bonzini
2025-03-06 2:42 ` [PATCH 7/7] target/riscv: move satp_mode.{map, init} " Alistair Francis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).