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

[Andrew Jones <ajones@ventanamicro.com>]

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