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

[Andrew Jones <ajones@ventanamicro.com>]

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