[PATCH RFC] target: riscv: Fix satp mode initialization based on profile

[PATCH RFC] target: riscv: Fix satp mode initialization based on profile

[Alexandre Ghiti]

The satp mode is set using the svXX properties, but that actually
restricts the satp mode to the minimum required by the profile and
prevents the use of higher satp modes.

Fix this by not setting any svXX property and allow all satp mode to be
supported.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
---
 target/riscv/tcg/tcg-cpu.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 5aef9eef36..ca2d2950eb 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -1232,9 +1232,6 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
 #ifndef CONFIG_USER_ONLY
     if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) {
         object_property_set_bool(obj, "mmu", true, NULL);
-        const char *satp_prop = satp_mode_str(profile->satp_mode,
-                                              riscv_cpu_is_32bit(cpu));
-        object_property_set_bool(obj, satp_prop, profile->enabled, NULL);
     }
 #endif
 
-- 
2.34.1

Re: [PATCH RFC] target: riscv: Fix satp mode initialization based on profile

[Björn Töpel]

Alexandre Ghiti <alexghiti@rivosinc.com> writes:

> The satp mode is set using the svXX properties, but that actually
> restricts the satp mode to the minimum required by the profile and
> prevents the use of higher satp modes.
>
> Fix this by not setting any svXX property and allow all satp mode to be
> supported.

I figured I'll add some more findings.

The RISC-V profile initialization seems a bit brittle; Without Alex' fix
we're getting different supported satp on the first hart (0) than the
other harts; Hart 0 has sv57, whereas the other harts has enforced sv39.
This caused the smp bringup on Linux to fail.

Some observations on, e.g., a
"rv64,b=on,zbc=off,v=true,vlen=256,elen=64,sscofpmf=on,svade=on,svinval=on,svnapot=on,svpbmt=on,zcb=on,zcmop=on,zfhmin=on,zicond=on,zimop=on,zkt=on,zvbb=on,zvfhmin=on,zvkt=on,zkr=on"
8 hart machine:

When realizing the cpus, the first cpu calls riscv_cpu_add_profiles()
all profiles are disabled, whereas for the other cpu calls to
riscv_cpu_add_profiles() have some profiles enabled. Having some
profiles enabled, will issue a call to cpu_set_profile() that will
enforce the satp code that Alex removes in this patch.

The riscv_cpu_validate_profile() function is called after
riscv_cpu_add_profiles(), which explains why the harts subsequent the
first one will get the cpu_set_profile() call. The first hart will not
have cpu_set_profile() called.

IOW, there are more issues hiding here, in addition to Alex' fix.


Björn

Re: [PATCH RFC] target: riscv: Fix satp mode initialization based on profile

[Daniel Henrique Barboza]

On 5/19/25 9:07 AM, Björn Töpel wrote:
> Alexandre Ghiti <alexghiti@rivosinc.com> writes:
> 
>> The satp mode is set using the svXX properties, but that actually
>> restricts the satp mode to the minimum required by the profile and
>> prevents the use of higher satp modes.
>>
>> Fix this by not setting any svXX property and allow all satp mode to be
>> supported.
> 
> I figured I'll add some more findings.
> 
> The RISC-V profile initialization seems a bit brittle; Without Alex' fix
> we're getting different supported satp on the first hart (0) than the
> other harts; Hart 0 has sv57, whereas the other harts has enforced sv39.
> This caused the smp bringup on Linux to fail.
> 
> Some observations on, e.g., a
> "rv64,b=on,zbc=off,v=true,vlen=256,elen=64,sscofpmf=on,svade=on,svinval=on,svnapot=on,svpbmt=on,zcb=on,zcmop=on,zfhmin=on,zicond=on,zimop=on,zkt=on,zvbb=on,zvfhmin=on,zvkt=on,zkr=on"
> 8 hart machine:
> 
> When realizing the cpus, the first cpu calls riscv_cpu_add_profiles()
> all profiles are disabled, whereas for the other cpu calls to
> riscv_cpu_add_profiles() have some profiles enabled. Having some
> profiles enabled, will issue a call to cpu_set_profile() that will
> enforce the satp code that Alex removes in this patch.
> 
> The riscv_cpu_validate_profile() function is called after
> riscv_cpu_add_profiles(), which explains why the harts subsequent the
> first one will get the cpu_set_profile() call. The first hart will not
> have cpu_set_profile() called.
> 
> IOW, there are more issues hiding here, in addition to Alex' fix.

There is. I'll send a fix shortly. Thanks for letting us know!


Daniel

> 
> 
> Björn

Re: [PATCH RFC] target: riscv: Fix satp mode initialization based on profile

[Paolo Bonzini]

On 5/19/25 14:07, Björn Töpel wrote:
> When realizing the cpus, the first cpu calls riscv_cpu_add_profiles()
> all profiles are disabled, whereas for the other cpu calls to
> riscv_cpu_add_profiles() have some profiles enabled. Having some
> profiles enabled, will issue a call to cpu_set_profile() that will
> enforce the satp code that Alex removes in this patch.
Ah so the problem is that *parent* profiles are not enabled until
riscv_cpu_add_profiles().

With my patches to introduce RISCVCPUDef, it's a pretty easy fix:

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 629ac37501e..04b929af41c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1083,6 +1083,19 @@ static bool riscv_cpu_is_dynamic(Object *cpu_obj)
      return object_dynamic_cast(cpu_obj, TYPE_RISCV_DYNAMIC_CPU) != NULL;
  }
  
+static void riscv_cpu_enable_profile(RISCVCPU *cpu,
+                                     RISCVCPUProfile *profile)
+{
+    profile->enabled = true;
+
+    if (profile->u_parent) {
+        riscv_cpu_enable_profile(cpu, profile->u_parent);
+    }
+    if (profile->s_parent) {
+        riscv_cpu_enable_profile(cpu, profile->s_parent);
+    }
+}
+
  static void riscv_cpu_init(Object *obj)
  {
      RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(obj);
@@ -1121,7 +1134,7 @@ static void riscv_cpu_init(Object *obj)
      cpu->cfg.max_satp_mode = -1;
  
      if (mcc->def->profile) {
-        mcc->def->profile->enabled = true;
+        riscv_cpu_enable_profile(cpu, mcc->def->profile);
      }
  
      env->misa_ext_mask = env->misa_ext = mcc->def->misa_ext;

Since they're all reviewed and Alistair has flushed his queue, I'll
send them in my next pull request.

On top of them, probably profiles should also be converted to use
RISCVCPUCfg and riscv_cpu_enable_profile() can then enable all the
flags with riscv_cpu_cfg_merge().

In general a lot (if not all) of the profile code should be moved out
of tcg-cpu.c and into riscv_cpu_class_base_init().  I didn't do that
because I didn't want to balloon an already-large series, but it's a
pretty obvious extension of the RISCVCPUDef concept to include all
profile features.

Paolo

Re: [PATCH RFC] target: riscv: Fix satp mode initialization based on profile

[Daniel Henrique Barboza]

On 5/20/25 7:53 AM, Paolo Bonzini wrote:
> On 5/19/25 14:07, Björn Töpel wrote:
>> When realizing the cpus, the first cpu calls riscv_cpu_add_profiles()
>> all profiles are disabled, whereas for the other cpu calls to
>> riscv_cpu_add_profiles() have some profiles enabled. Having some
>> profiles enabled, will issue a call to cpu_set_profile() that will
>> enforce the satp code that Alex removes in this patch.
> Ah so the problem is that *parent* profiles are not enabled until
> riscv_cpu_add_profiles().

The problem in Björn's case is that we shouldn't take the profile code path
for any CPUs since he's not enabling any profiles. There's a bug in how we're
detecting a profile presence for QMP for CPU0 and how this detection is changing
the cpu_init of CPU1 and above. I'll send a fix for it today.

> 
> With my patches to introduce RISCVCPUDef, it's a pretty easy fix:
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 629ac37501e..04b929af41c 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1083,6 +1083,19 @@ static bool riscv_cpu_is_dynamic(Object *cpu_obj)
>       return object_dynamic_cast(cpu_obj, TYPE_RISCV_DYNAMIC_CPU) != NULL;
>   }
> 
> +static void riscv_cpu_enable_profile(RISCVCPU *cpu,
> +                                     RISCVCPUProfile *profile)
> +{
> +    profile->enabled = true;
> +
> +    if (profile->u_parent) {
> +        riscv_cpu_enable_profile(cpu, profile->u_parent);
> +    }
> +    if (profile->s_parent) {
> +        riscv_cpu_enable_profile(cpu, profile->s_parent);
> +    }
> +}
> +
>   static void riscv_cpu_init(Object *obj)
>   {
>       RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(obj);
> @@ -1121,7 +1134,7 @@ static void riscv_cpu_init(Object *obj)
>       cpu->cfg.max_satp_mode = -1;
> 
>       if (mcc->def->profile) {
> -        mcc->def->profile->enabled = true;
> +        riscv_cpu_enable_profile(cpu, mcc->def->profile);
>       }
> 
>       env->misa_ext_mask = env->misa_ext = mcc->def->misa_ext;
> 
> Since they're all reviewed and Alistair has flushed his queue, I'll
> send them in my next pull request.
> 
> On top of them, probably profiles should also be converted to use
> RISCVCPUCfg and riscv_cpu_enable_profile() can then enable all the
> flags with riscv_cpu_cfg_merge().
> 

If we can do the same thing with less abstractions than what we're using
ATM I'll all for it.

> In general a lot (if not all) of the profile code should be moved out
> of tcg-cpu.c and into riscv_cpu_class_base_init().  I didn't do that
> because I didn't want to balloon an already-large series, but it's a
> pretty obvious extension of the RISCVCPUDef concept to include all
> profile features.

Note that KVM RISC-V does not have the same profile support as TCG - I'm not
sure if KVM RISC-V has RVA22 support, let alone RVA23. If we move the profile
logic to cpu.c we need to be careful with TCG assumptions affecting KVM CPUs.


Thanks,


Daniel

> 
> Paolo
> 

Re: [PATCH RFC] target: riscv: Fix satp mode initialization based on profile

[Paolo Bonzini]

On Tue, May 20, 2025 at 1:33 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
> > In general a lot (if not all) of the profile code should be moved out
> > of tcg-cpu.c and into riscv_cpu_class_base_init().  I didn't do that
> > because I didn't want to balloon an already-large series, but it's a
> > pretty obvious extension of the RISCVCPUDef concept to include all
> > profile features.
>
> Note that KVM RISC-V does not have the same profile support as TCG - I'm not
> sure if KVM RISC-V has RVA22 support, let alone RVA23. If we move the profile
> logic to cpu.c we need to be careful with TCG assumptions affecting KVM CPUs.

Yes, but then that's even more of a reason to move code to cpu.c. It
means that "-cpu rva22s64 --enable-kvm" will print the appropriate
warning or errors.

Paolo

Re: [PATCH RFC] target: riscv: Fix satp mode initialization based on profile

[Daniel Henrique Barboza]

On 5/16/25 9:23 AM, Alexandre Ghiti wrote:
> The satp mode is set using the svXX properties, but that actually
> restricts the satp mode to the minimum required by the profile and
> prevents the use of higher satp modes.

For rva23s64, in "Optional Extensions" we'll find:

https://github.com/riscv/riscv-profiles/blob/main/src/rva23-profile.adoc

- Sv48 Page-based 48-bit virtual-memory system.
- Sv57 Page-based 57-bit virtual-memory system.

So in theory we could go up to sv57 for rva23s64 (and rva22s64, just checked).
Changing satp_mode to the maximum allowed seems sensible.

But allowing all satp modes to go in a profile defeats the purpose, doesn't it?
None of the existing profiles in QEMU claims supports sv64. Granted, I'm not a
satp expert, but removing the satp restriction in profiles doesn't seem right.


Thanks,

Daniel


> 
> Fix this by not setting any svXX property and allow all satp mode to be
> supported.
> 
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>   target/riscv/tcg/tcg-cpu.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 5aef9eef36..ca2d2950eb 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -1232,9 +1232,6 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
>   #ifndef CONFIG_USER_ONLY
>       if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) {
>           object_property_set_bool(obj, "mmu", true, NULL);
> -        const char *satp_prop = satp_mode_str(profile->satp_mode,
> -                                              riscv_cpu_is_32bit(cpu));
> -        object_property_set_bool(obj, satp_prop, profile->enabled, NULL);
>       }
>   #endif
>   

Re: [PATCH RFC] target: riscv: Fix satp mode initialization based on profile

[Andrew Jones]

On Mon, May 19, 2025 at 09:48:14AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 5/16/25 9:23 AM, Alexandre Ghiti wrote:
> > The satp mode is set using the svXX properties, but that actually
> > restricts the satp mode to the minimum required by the profile and
> > prevents the use of higher satp modes.
> 
> For rva23s64, in "Optional Extensions" we'll find:

The keyword is "Optional". The profile should only set sv39 since that's
what rva23 (and rv22) have for the mandatory support. sv48 and sv57 are
both optional so, while the user should be allowed to turn them on, just
like other optional extensions, they shouldn't be on by default since we
don't set any optional extensions on by default.

So we don't want this change, but fixing any bugs with the first hart vs.
the other harts is of course necessary.

Thanks,
drew

> 
> https://github.com/riscv/riscv-profiles/blob/main/src/rva23-profile.adoc
> 
> - Sv48 Page-based 48-bit virtual-memory system.
> - Sv57 Page-based 57-bit virtual-memory system.
> 
> So in theory we could go up to sv57 for rva23s64 (and rva22s64, just checked).
> Changing satp_mode to the maximum allowed seems sensible.
> 
> But allowing all satp modes to go in a profile defeats the purpose, doesn't it?
> None of the existing profiles in QEMU claims supports sv64. Granted, I'm not a
> satp expert, but removing the satp restriction in profiles doesn't seem right.
> 
> 
> Thanks,
> 
> Daniel
> 
> 
> > 
> > Fix this by not setting any svXX property and allow all satp mode to be
> > supported.
> > 
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
> >   target/riscv/tcg/tcg-cpu.c | 3 ---
> >   1 file changed, 3 deletions(-)
> > 
> > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> > index 5aef9eef36..ca2d2950eb 100644
> > --- a/target/riscv/tcg/tcg-cpu.c
> > +++ b/target/riscv/tcg/tcg-cpu.c
> > @@ -1232,9 +1232,6 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
> >   #ifndef CONFIG_USER_ONLY
> >       if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) {
> >           object_property_set_bool(obj, "mmu", true, NULL);
> > -        const char *satp_prop = satp_mode_str(profile->satp_mode,
> > -                                              riscv_cpu_is_32bit(cpu));
> > -        object_property_set_bool(obj, satp_prop, profile->enabled, NULL);
> >       }
> >   #endif
> 
> 

Re: [PATCH RFC] target: riscv: Fix satp mode initialization based on profile

[Daniel Henrique Barboza]

On 5/19/25 1:35 PM, Andrew Jones wrote:
> On Mon, May 19, 2025 at 09:48:14AM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 5/16/25 9:23 AM, Alexandre Ghiti wrote:
>>> The satp mode is set using the svXX properties, but that actually
>>> restricts the satp mode to the minimum required by the profile and
>>> prevents the use of higher satp modes.
>>
>> For rva23s64, in "Optional Extensions" we'll find:
> 
> The keyword is "Optional". The profile should only set sv39 since that's
> what rva23 (and rv22) have for the mandatory support. sv48 and sv57 are
> both optional so, while the user should be allowed to turn them on, just
> like other optional extensions, they shouldn't be on by default since we
> don't set any optional extensions on by default.

What about satp validation for a profile? For both rva22 and rva23 the mandatory
satp is sv39, but up to sv57 is also ok. Do we care if a sv64 CPU claims rva23
support?

I am aware that sv64 also means sv57 support but I'm worried about migration
compatibility. Let's say we migrate between two hosts A and B that claim
to be rva23 compliant. A is running sv64, B is running sv57. If the software
running in A is actually using satp sv64 we can't migrate A to B.

> 
> So we don't want this change, but fixing any bugs with the first hart vs.
> the other harts is of course necessary.

I'm working on it. I'll decouple the QMP bits (all the profile validation business
is a QMP problem in the end) from the core CPU finalize logic. I'll send patches
soon.


Thanks,

Daniel

> 
> Thanks,
> drew
> 
>>
>> https://github.com/riscv/riscv-profiles/blob/main/src/rva23-profile.adoc
>>
>> - Sv48 Page-based 48-bit virtual-memory system.
>> - Sv57 Page-based 57-bit virtual-memory system.
>>
>> So in theory we could go up to sv57 for rva23s64 (and rva22s64, just checked).
>> Changing satp_mode to the maximum allowed seems sensible.
>>
>> But allowing all satp modes to go in a profile defeats the purpose, doesn't it?
>> None of the existing profiles in QEMU claims supports sv64. Granted, I'm not a
>> satp expert, but removing the satp restriction in profiles doesn't seem right.
>>
>>
>> Thanks,
>>
>> Daniel
>>
>>
>>>
>>> Fix this by not setting any svXX property and allow all satp mode to be
>>> supported.
>>>
>>> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
>>> ---
>>>    target/riscv/tcg/tcg-cpu.c | 3 ---
>>>    1 file changed, 3 deletions(-)
>>>
>>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
>>> index 5aef9eef36..ca2d2950eb 100644
>>> --- a/target/riscv/tcg/tcg-cpu.c
>>> +++ b/target/riscv/tcg/tcg-cpu.c
>>> @@ -1232,9 +1232,6 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
>>>    #ifndef CONFIG_USER_ONLY
>>>        if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) {
>>>            object_property_set_bool(obj, "mmu", true, NULL);
>>> -        const char *satp_prop = satp_mode_str(profile->satp_mode,
>>> -                                              riscv_cpu_is_32bit(cpu));
>>> -        object_property_set_bool(obj, satp_prop, profile->enabled, NULL);
>>>        }
>>>    #endif
>>
>>

Re: [PATCH RFC] target: riscv: Fix satp mode initialization based on profile

[Andrew Jones]

On Mon, May 19, 2025 at 02:15:05PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 5/19/25 1:35 PM, Andrew Jones wrote:
> > On Mon, May 19, 2025 at 09:48:14AM -0300, Daniel Henrique Barboza wrote:
> > > 
> > > 
> > > On 5/16/25 9:23 AM, Alexandre Ghiti wrote:
> > > > The satp mode is set using the svXX properties, but that actually
> > > > restricts the satp mode to the minimum required by the profile and
> > > > prevents the use of higher satp modes.
> > > 
> > > For rva23s64, in "Optional Extensions" we'll find:
> > 
> > The keyword is "Optional". The profile should only set sv39 since that's
> > what rva23 (and rv22) have for the mandatory support. sv48 and sv57 are
> > both optional so, while the user should be allowed to turn them on, just
> > like other optional extensions, they shouldn't be on by default since we
> > don't set any optional extensions on by default.
> 
> What about satp validation for a profile? For both rva22 and rva23 the mandatory
> satp is sv39, but up to sv57 is also ok. Do we care if a sv64 CPU claims rva23
> support?

Is sv64 defined yet? I thought it's just a placeholder. Anyway, I'd expect
it to be like sv57 and sv48 in that each narrower width must be supported,
which means sv39 would also still be supported, and that means the cpu
could be rva23. If, OTOH, an sv64 cpu cannot support sv39, then the cpu
cannot have both, so it cannot be rva23. IOW, as long as sv39 is _also_
supported, then sv64 is OK to select and still be rva23.

> 
> I am aware that sv64 also means sv57 support but I'm worried about migration
> compatibility. Let's say we migrate between two hosts A and B that claim
> to be rva23 compliant. A is running sv64, B is running sv57. If the software
> running in A is actually using satp sv64 we can't migrate A to B.

A and B are incompatible already if A is '-cpu rva23,sv64=on' and B is
'-cpu rva23,sv57=on', so the migration manager should already disallow
that.

> 
> > 
> > So we don't want this change, but fixing any bugs with the first hart vs.
> > the other harts is of course necessary.
> 
> I'm working on it. I'll decouple the QMP bits (all the profile validation business
> is a QMP problem in the end) from the core CPU finalize logic. I'll send patches
> soon.

Great, thanks!

Another comment I thought of later that I should have put in my original
reply is that we of course want 'max' to default to the widest QEMU
supports. Then, users that want to ensure they get sv57 or sv64 without
having to explicitly add it to their command lines can use 'max'.
Specifying '-cpu rva23' means you just want the mandatory base of the
given profile and if you want more than that then you need to append each
optional extension explicitly. If we don't have that documented somewhere,
then maybe we should, in order to help clarify the intent.

Thanks,
drew

> 
> 
> Thanks,
> 
> Daniel
> 
> > 
> > Thanks,
> > drew
> > 
> > > 
> > > https://github.com/riscv/riscv-profiles/blob/main/src/rva23-profile.adoc
> > > 
> > > - Sv48 Page-based 48-bit virtual-memory system.
> > > - Sv57 Page-based 57-bit virtual-memory system.
> > > 
> > > So in theory we could go up to sv57 for rva23s64 (and rva22s64, just checked).
> > > Changing satp_mode to the maximum allowed seems sensible.
> > > 
> > > But allowing all satp modes to go in a profile defeats the purpose, doesn't it?
> > > None of the existing profiles in QEMU claims supports sv64. Granted, I'm not a
> > > satp expert, but removing the satp restriction in profiles doesn't seem right.
> > > 
> > > 
> > > Thanks,
> > > 
> > > Daniel
> > > 
> > > 
> > > > 
> > > > Fix this by not setting any svXX property and allow all satp mode to be
> > > > supported.
> > > > 
> > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > > ---
> > > >    target/riscv/tcg/tcg-cpu.c | 3 ---
> > > >    1 file changed, 3 deletions(-)
> > > > 
> > > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> > > > index 5aef9eef36..ca2d2950eb 100644
> > > > --- a/target/riscv/tcg/tcg-cpu.c
> > > > +++ b/target/riscv/tcg/tcg-cpu.c
> > > > @@ -1232,9 +1232,6 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
> > > >    #ifndef CONFIG_USER_ONLY
> > > >        if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) {
> > > >            object_property_set_bool(obj, "mmu", true, NULL);
> > > > -        const char *satp_prop = satp_mode_str(profile->satp_mode,
> > > > -                                              riscv_cpu_is_32bit(cpu));
> > > > -        object_property_set_bool(obj, satp_prop, profile->enabled, NULL);
> > > >        }
> > > >    #endif
> > > 
> > > 
> 

Re: [PATCH RFC] target: riscv: Fix satp mode initialization based on profile

[Daniel Henrique Barboza]

On 5/20/25 4:50 AM, Andrew Jones wrote:
> On Mon, May 19, 2025 at 02:15:05PM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 5/19/25 1:35 PM, Andrew Jones wrote:
>>> On Mon, May 19, 2025 at 09:48:14AM -0300, Daniel Henrique Barboza wrote:
>>>>
>>>>
>>>> On 5/16/25 9:23 AM, Alexandre Ghiti wrote:
>>>>> The satp mode is set using the svXX properties, but that actually
>>>>> restricts the satp mode to the minimum required by the profile and
>>>>> prevents the use of higher satp modes.
>>>>
>>>> For rva23s64, in "Optional Extensions" we'll find:
>>>
>>> The keyword is "Optional". The profile should only set sv39 since that's
>>> what rva23 (and rv22) have for the mandatory support. sv48 and sv57 are
>>> both optional so, while the user should be allowed to turn them on, just
>>> like other optional extensions, they shouldn't be on by default since we
>>> don't set any optional extensions on by default.
>>
>> What about satp validation for a profile? For both rva22 and rva23 the mandatory
>> satp is sv39, but up to sv57 is also ok. Do we care if a sv64 CPU claims rva23
>> support?
> 
> Is sv64 defined yet? I thought it's just a placeholder. Anyway, I'd expect
> it to be like sv57 and sv48 in that each narrower width must be supported,
> which means sv39 would also still be supported, and that means the cpu
> could be rva23. If, OTOH, an sv64 cpu cannot support sv39, then the cpu
> cannot have both, so it cannot be rva23. IOW, as long as sv39 is _also_
> supported, then sv64 is OK to select and still be rva23.

We have sv64 defined in QEMU. Not sure if it's already a thing or not ....
it seems to me that ppl cares to sv57 mostly, so perhaps my sv64 worry
unjustified.

Just took a look in how we're validating satp for profiles and we're
allowing a higher satp mode than the profile mandates, issuing warnings
if the satp mode is set to a lower mode than the profile value.


So I guess the way we would like people to use rva23s64 with sv57 would be:

-cpu rva23s64,sv57=on


> 
>>
>> I am aware that sv64 also means sv57 support but I'm worried about migration
>> compatibility. Let's say we migrate between two hosts A and B that claim
>> to be rva23 compliant. A is running sv64, B is running sv57. If the software
>> running in A is actually using satp sv64 we can't migrate A to B.
> 
> A and B are incompatible already if A is '-cpu rva23,sv64=on' and B is
> '-cpu rva23,sv57=on', so the migration manager should already disallow
> that.
> 
>>
>>>
>>> So we don't want this change, but fixing any bugs with the first hart vs.
>>> the other harts is of course necessary.
>>
>> I'm working on it. I'll decouple the QMP bits (all the profile validation business
>> is a QMP problem in the end) from the core CPU finalize logic. I'll send patches
>> soon.
> 
> Great, thanks!
> 
> Another comment I thought of later that I should have put in my original
> reply is that we of course want 'max' to default to the widest QEMU
> supports. Then, users that want to ensure they get sv57 or sv64 without
> having to explicitly add it to their command lines can use 'max'.
> Specifying '-cpu rva23' means you just want the mandatory base of the
> given profile and if you want more than that then you need to append each
> optional extension explicitly. If we don't have that documented somewhere,
> then maybe we should, in order to help clarify the intent.
> 

max CPU is using satp_mode = sv57. Since sv64 is mostly a placeholder then I
believe it's all good. Perhaps we could add a satp_mode_latest value for these
situations.


Thanks,

Daniel

> Thanks,
> drew
> 
>>
>>
>> Thanks,
>>
>> Daniel
>>
>>>
>>> Thanks,
>>> drew
>>>
>>>>
>>>> https://github.com/riscv/riscv-profiles/blob/main/src/rva23-profile.adoc
>>>>
>>>> - Sv48 Page-based 48-bit virtual-memory system.
>>>> - Sv57 Page-based 57-bit virtual-memory system.
>>>>
>>>> So in theory we could go up to sv57 for rva23s64 (and rva22s64, just checked).
>>>> Changing satp_mode to the maximum allowed seems sensible.
>>>>
>>>> But allowing all satp modes to go in a profile defeats the purpose, doesn't it?
>>>> None of the existing profiles in QEMU claims supports sv64. Granted, I'm not a
>>>> satp expert, but removing the satp restriction in profiles doesn't seem right.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Daniel
>>>>
>>>>
>>>>>
>>>>> Fix this by not setting any svXX property and allow all satp mode to be
>>>>> supported.
>>>>>
>>>>> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
>>>>> ---
>>>>>     target/riscv/tcg/tcg-cpu.c | 3 ---
>>>>>     1 file changed, 3 deletions(-)
>>>>>
>>>>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
>>>>> index 5aef9eef36..ca2d2950eb 100644
>>>>> --- a/target/riscv/tcg/tcg-cpu.c
>>>>> +++ b/target/riscv/tcg/tcg-cpu.c
>>>>> @@ -1232,9 +1232,6 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
>>>>>     #ifndef CONFIG_USER_ONLY
>>>>>         if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) {
>>>>>             object_property_set_bool(obj, "mmu", true, NULL);
>>>>> -        const char *satp_prop = satp_mode_str(profile->satp_mode,
>>>>> -                                              riscv_cpu_is_32bit(cpu));
>>>>> -        object_property_set_bool(obj, satp_prop, profile->enabled, NULL);
>>>>>         }
>>>>>     #endif
>>>>
>>>>
>>

Re: [PATCH RFC] target: riscv: Fix satp mode initialization based on profile

[Andrew Jones]

On Tue, May 20, 2025 at 07:50:15AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 5/20/25 4:50 AM, Andrew Jones wrote:
> > On Mon, May 19, 2025 at 02:15:05PM -0300, Daniel Henrique Barboza wrote:
> > > 
> > > 
> > > On 5/19/25 1:35 PM, Andrew Jones wrote:
> > > > On Mon, May 19, 2025 at 09:48:14AM -0300, Daniel Henrique Barboza wrote:
> > > > > 
> > > > > 
> > > > > On 5/16/25 9:23 AM, Alexandre Ghiti wrote:
> > > > > > The satp mode is set using the svXX properties, but that actually
> > > > > > restricts the satp mode to the minimum required by the profile and
> > > > > > prevents the use of higher satp modes.
> > > > > 
> > > > > For rva23s64, in "Optional Extensions" we'll find:
> > > > 
> > > > The keyword is "Optional". The profile should only set sv39 since that's
> > > > what rva23 (and rv22) have for the mandatory support. sv48 and sv57 are
> > > > both optional so, while the user should be allowed to turn them on, just
> > > > like other optional extensions, they shouldn't be on by default since we
> > > > don't set any optional extensions on by default.
> > > 
> > > What about satp validation for a profile? For both rva22 and rva23 the mandatory
> > > satp is sv39, but up to sv57 is also ok. Do we care if a sv64 CPU claims rva23
> > > support?
> > 
> > Is sv64 defined yet? I thought it's just a placeholder. Anyway, I'd expect
> > it to be like sv57 and sv48 in that each narrower width must be supported,
> > which means sv39 would also still be supported, and that means the cpu
> > could be rva23. If, OTOH, an sv64 cpu cannot support sv39, then the cpu
> > cannot have both, so it cannot be rva23. IOW, as long as sv39 is _also_
> > supported, then sv64 is OK to select and still be rva23.
> 
> We have sv64 defined in QEMU. Not sure if it's already a thing or not ....
> it seems to me that ppl cares to sv57 mostly, so perhaps my sv64 worry
> unjustified.
> 
> Just took a look in how we're validating satp for profiles and we're
> allowing a higher satp mode than the profile mandates, issuing warnings
> if the satp mode is set to a lower mode than the profile value.
> 
> 
> So I guess the way we would like people to use rva23s64 with sv57 would be:
> 
> -cpu rva23s64,sv57=on

yup

> 
> 
> > 
> > > 
> > > I am aware that sv64 also means sv57 support but I'm worried about migration
> > > compatibility. Let's say we migrate between two hosts A and B that claim
> > > to be rva23 compliant. A is running sv64, B is running sv57. If the software
> > > running in A is actually using satp sv64 we can't migrate A to B.
> > 
> > A and B are incompatible already if A is '-cpu rva23,sv64=on' and B is
> > '-cpu rva23,sv57=on', so the migration manager should already disallow
> > that.
> > 
> > > 
> > > > 
> > > > So we don't want this change, but fixing any bugs with the first hart vs.
> > > > the other harts is of course necessary.
> > > 
> > > I'm working on it. I'll decouple the QMP bits (all the profile validation business
> > > is a QMP problem in the end) from the core CPU finalize logic. I'll send patches
> > > soon.
> > 
> > Great, thanks!
> > 
> > Another comment I thought of later that I should have put in my original
> > reply is that we of course want 'max' to default to the widest QEMU
> > supports. Then, users that want to ensure they get sv57 or sv64 without
> > having to explicitly add it to their command lines can use 'max'.
> > Specifying '-cpu rva23' means you just want the mandatory base of the
> > given profile and if you want more than that then you need to append each
> > optional extension explicitly. If we don't have that documented somewhere,
> > then maybe we should, in order to help clarify the intent.
> > 
> 
> max CPU is using satp_mode = sv57. Since sv64 is mostly a placeholder then I
> believe it's all good. Perhaps we could add a satp_mode_latest value for these
> situations.

Sounds good.

Thanks,
drew

> 
> 
> Thanks,
> 
> Daniel
> 
> > Thanks,
> > drew
> > 
> > > 
> > > 
> > > Thanks,
> > > 
> > > Daniel
> > > 
> > > > 
> > > > Thanks,
> > > > drew
> > > > 
> > > > > 
> > > > > https://github.com/riscv/riscv-profiles/blob/main/src/rva23-profile.adoc
> > > > > 
> > > > > - Sv48 Page-based 48-bit virtual-memory system.
> > > > > - Sv57 Page-based 57-bit virtual-memory system.
> > > > > 
> > > > > So in theory we could go up to sv57 for rva23s64 (and rva22s64, just checked).
> > > > > Changing satp_mode to the maximum allowed seems sensible.
> > > > > 
> > > > > But allowing all satp modes to go in a profile defeats the purpose, doesn't it?
> > > > > None of the existing profiles in QEMU claims supports sv64. Granted, I'm not a
> > > > > satp expert, but removing the satp restriction in profiles doesn't seem right.
> > > > > 
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > Daniel
> > > > > 
> > > > > 
> > > > > > 
> > > > > > Fix this by not setting any svXX property and allow all satp mode to be
> > > > > > supported.
> > > > > > 
> > > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > > > > ---
> > > > > >     target/riscv/tcg/tcg-cpu.c | 3 ---
> > > > > >     1 file changed, 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> > > > > > index 5aef9eef36..ca2d2950eb 100644
> > > > > > --- a/target/riscv/tcg/tcg-cpu.c
> > > > > > +++ b/target/riscv/tcg/tcg-cpu.c
> > > > > > @@ -1232,9 +1232,6 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
> > > > > >     #ifndef CONFIG_USER_ONLY
> > > > > >         if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) {
> > > > > >             object_property_set_bool(obj, "mmu", true, NULL);
> > > > > > -        const char *satp_prop = satp_mode_str(profile->satp_mode,
> > > > > > -                                              riscv_cpu_is_32bit(cpu));
> > > > > > -        object_property_set_bool(obj, satp_prop, profile->enabled, NULL);
> > > > > >         }
> > > > > >     #endif
> > > > > 
> > > > > 
> > > 
>