The effective MXL value matters when booting.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/riscv/boot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 52bf8e67de..dad3f6e7b1 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -36,7 +36,7 @@
 
 bool riscv_is_32bit(RISCVHartArrayState *harts)
 {
-    return harts->harts[0].env.misa_mxl_max == MXL_RV32;
+    return harts->harts[0].env.misa_mxl == MXL_RV32;
 }
 
 /*
-- 
2.42.0

On Mon, Oct 30, 2023 at 3:50 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> The effective MXL value matters when booting.

This doesn't sound right. Surely the max is what matters here

Also, this was specifically changed to misa_mxl_max in db23e5d981a
"target/riscv: Replace riscv_cpu_is_32bit with riscv_cpu_mxl".

This needs a much better description of why this change should be made

Alistair

>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/riscv/boot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 52bf8e67de..dad3f6e7b1 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -36,7 +36,7 @@
>
>  bool riscv_is_32bit(RISCVHartArrayState *harts)
>  {
> -    return harts->harts[0].env.misa_mxl_max == MXL_RV32;
> +    return harts->harts[0].env.misa_mxl == MXL_RV32;
>  }
>
>  /*
> --
> 2.42.0
>
>

On 2023/11/23 12:04, Alistair Francis wrote:
> On Mon, Oct 30, 2023 at 3:50 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> The effective MXL value matters when booting.
> 
> This doesn't sound right. Surely the max is what matters here
> 
> Also, this was specifically changed to misa_mxl_max in db23e5d981a
> "target/riscv: Replace riscv_cpu_is_32bit with riscv_cpu_mxl".
> 
> This needs a much better description of why this change should be made
 >
 > Alistair

The kernel will be executed with the current MXL rather than the initial 
MXL value so the current MXL should be used here.

For example, if you are going to emulate a system that has a RV64 CPU 
and a firmware that sets the MXL to RV32, then mxl_max should be 
MXL_RV64 and mxl should be MXL_RV32, and the kernel should be assumed as 
a RV32 binary. Loading a 64-bit kernel will not work in such a case.

You can find a similar example in x86_64: x86_64 systems typically 
starts in 16-bit mode, and the firmware switches to 64-bit mode. When 
emulating those systems, QEMU switches to 64-bit mode and loads a 64-bit 
kernel.

Regards,
Akihiko Odaki

On Thu, Nov 23, 2023 at 5:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2023/11/23 12:04, Alistair Francis wrote:
> > On Mon, Oct 30, 2023 at 3:50 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> The effective MXL value matters when booting.
> >
> > This doesn't sound right. Surely the max is what matters here
> >
> > Also, this was specifically changed to misa_mxl_max in db23e5d981a
> > "target/riscv: Replace riscv_cpu_is_32bit with riscv_cpu_mxl".
> >
> > This needs a much better description of why this change should be made
>  >
>  > Alistair
>
> The kernel will be executed with the current MXL rather than the initial
> MXL value so the current MXL should be used here.
>
> For example, if you are going to emulate a system that has a RV64 CPU
> and a firmware that sets the MXL to RV32, then mxl_max should be
> MXL_RV64 and mxl should be MXL_RV32, and the kernel should be assumed as
> a RV32 binary. Loading a 64-bit kernel will not work in such a case.

But this is called before the firmware runs, so it won't be changed by firmware.

Maybe it's worth putting what this fixes in the commit message?

Alistair

>
> You can find a similar example in x86_64: x86_64 systems typically
> starts in 16-bit mode, and the firmware switches to 64-bit mode. When
> emulating those systems, QEMU switches to 64-bit mode and loads a 64-bit
> kernel.
>
> Regards,
> Akihiko Odaki

On 2023/12/15 14:34, Alistair Francis wrote:
> On Thu, Nov 23, 2023 at 5:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2023/11/23 12:04, Alistair Francis wrote:
>>> On Mon, Oct 30, 2023 at 3:50 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> The effective MXL value matters when booting.
>>>
>>> This doesn't sound right. Surely the max is what matters here
>>>
>>> Also, this was specifically changed to misa_mxl_max in db23e5d981a
>>> "target/riscv: Replace riscv_cpu_is_32bit with riscv_cpu_mxl".
>>>
>>> This needs a much better description of why this change should be made
>>   >
>>   > Alistair
>>
>> The kernel will be executed with the current MXL rather than the initial
>> MXL value so the current MXL should be used here.
>>
>> For example, if you are going to emulate a system that has a RV64 CPU
>> and a firmware that sets the MXL to RV32, then mxl_max should be
>> MXL_RV64 and mxl should be MXL_RV32, and the kernel should be assumed as
>> a RV32 binary. Loading a 64-bit kernel will not work in such a case.
> 
> But this is called before the firmware runs, so it won't be changed by firmware.

It's more like QEMU emulates the firmware. It's the responsibility of 
the firmware to load kernels for the real hardware, but QEMU does it 
instead.

The firmware can change the MXL to load a 32-bit kernel on a 64-bit 
system so if QEMU happens to emulate such a behavior, mxl should be used 
when loading the kernel instead of mxl_max. QEMU currently does not 
implement such a feature, but in such a case mxl == mxl_max so it does 
not hurt to use mxl.

> 
> Maybe it's worth putting what this fixes in the commit message?

What about:

A later commit requires one extra step to retrieve mxl_max. As mxl is 
semantically more correct and does not need such a extra step, refer to 
mxl instead.

Currently mxl always equals to mxl_max so it does not matter which of 
mxl or mxl_max to refer to. However, it is possible to have different 
values for mxl and mxl_max if QEMU gains a new feature to load a RV32 
kernel on a RV64 system, for example. For such a behavior, the real 
system will need the firmware to switch MXL to RV32, and if QEMU 
implements the same behavior, mxl will represent the MXL that 
corresponds to the kernel being loaded. Therefore, it is more 
appropriate to refer to mxl instead of mxl_max when mxl != mxl_max.

Regards,
Akihiko Odaki

On Fri, Dec 15, 2023 at 4:34 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2023/12/15 14:34, Alistair Francis wrote:
> > On Thu, Nov 23, 2023 at 5:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> On 2023/11/23 12:04, Alistair Francis wrote:
> >>> On Mon, Oct 30, 2023 at 3:50 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>>>
> >>>> The effective MXL value matters when booting.
> >>>
> >>> This doesn't sound right. Surely the max is what matters here
> >>>
> >>> Also, this was specifically changed to misa_mxl_max in db23e5d981a
> >>> "target/riscv: Replace riscv_cpu_is_32bit with riscv_cpu_mxl".
> >>>
> >>> This needs a much better description of why this change should be made
> >>   >
> >>   > Alistair
> >>
> >> The kernel will be executed with the current MXL rather than the initial
> >> MXL value so the current MXL should be used here.
> >>
> >> For example, if you are going to emulate a system that has a RV64 CPU
> >> and a firmware that sets the MXL to RV32, then mxl_max should be
> >> MXL_RV64 and mxl should be MXL_RV32, and the kernel should be assumed as
> >> a RV32 binary. Loading a 64-bit kernel will not work in such a case.
> >
> > But this is called before the firmware runs, so it won't be changed by firmware.
>
> It's more like QEMU emulates the firmware. It's the responsibility of
> the firmware to load kernels for the real hardware, but QEMU does it
> instead.
>
> The firmware can change the MXL to load a 32-bit kernel on a 64-bit
> system so if QEMU happens to emulate such a behavior, mxl should be used
> when loading the kernel instead of mxl_max. QEMU currently does not
> implement such a feature, but in such a case mxl == mxl_max so it does
> not hurt to use mxl.
>
> >
> > Maybe it's worth putting what this fixes in the commit message?
>
> What about:
>
> A later commit requires one extra step to retrieve mxl_max. As mxl is
> semantically more correct and does not need such a extra step, refer to
> mxl instead.
>
> Currently mxl always equals to mxl_max so it does not matter which of
> mxl or mxl_max to refer to. However, it is possible to have different
> values for mxl and mxl_max if QEMU gains a new feature to load a RV32
> kernel on a RV64 system, for example. For such a behavior, the real
> system will need the firmware to switch MXL to RV32, and if QEMU
> implements the same behavior, mxl will represent the MXL that
> corresponds to the kernel being loaded. Therefore, it is more
> appropriate to refer to mxl instead of mxl_max when mxl != mxl_max.

Great! That explains it really well. Can you include that in the
commit message in the next revision

Alistair

>
> Regards,
> Akihiko Odaki