[PATCH 0/7] target/riscv: store max SATP mode as a single integer in RISCVCPUConfig

[PATCH 0/7] target/riscv: store max SATP mode as a single integer in RISCVCPUConfig

[Paolo Bonzini]

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

[PATCH 1/7] hw/riscv: acpi: only create RHCT MMU entry for supported types

[Paolo Bonzini]

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

Re: [PATCH 1/7] hw/riscv: acpi: only create RHCT MMU entry for supported types

[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
>
>

Re: [PATCH 1/7] hw/riscv: acpi: only create RHCT MMU entry for supported types

[Paolo Bonzini]

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

[PATCH 2/7] target/riscv: env->misa_mxl is a constant

[Paolo Bonzini]

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

Re: [PATCH 2/7] target/riscv: env->misa_mxl is a constant

[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
>
>

Re: [PATCH 2/7] target/riscv: env->misa_mxl is a constant

[Paolo Bonzini]

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

Re: [PATCH 2/7] target/riscv: env->misa_mxl is a constant

[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
> >>
> >>
> >
> >
>

Re: [PATCH 2/7] target/riscv: env->misa_mxl is a constant

[Paolo Bonzini]

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

Re: [PATCH 2/7] target/riscv: env->misa_mxl is a constant

[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
> > > >>
> > > >>
> > > >
> > > >
> > >
> >
>

Re: [PATCH 2/7] target/riscv: env->misa_mxl is a constant

[Paolo Bonzini]

[-- 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 --]

Re: [PATCH 2/7] target/riscv: env->misa_mxl is a constant

[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

[PATCH 3/7] target/riscv: assert argument to set_satp_mode_max_supported is valid

[Paolo Bonzini]

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

Re: [PATCH 3/7] target/riscv: assert argument to set_satp_mode_max_supported is valid

[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
>
>

[PATCH 4/7] target/riscv: cpu: store max SATP mode as a single integer

[Paolo Bonzini]

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

Re: [PATCH 4/7] target/riscv: cpu: store max SATP mode as a single integer

[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
>
>

Re: [PATCH 4/7] target/riscv: cpu: store max SATP mode as a single integer

[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
>
>

[PATCH 5/7] target/riscv: update max_satp_mode based on QOM properties

[Paolo Bonzini]

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

Re: [PATCH 5/7] target/riscv: update max_satp_mode based on QOM properties

[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
>
>

[PATCH 6/7] target/riscv: remove supported from RISCVSATPMap

[Paolo Bonzini]

"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

Re: [PATCH 6/7] target/riscv: remove supported from RISCVSATPMap

[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
>
>

[PATCH 7/7] target/riscv: move satp_mode.{map,init} out of CPUConfig

[Paolo Bonzini]

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

Re: [PATCH 7/7] target/riscv: move satp_mode.{map, init} out of CPUConfig

[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
>
>