| Message ID | 20231030054834.39145-2-akihiko.odaki@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | gdbstub and TCG plugin improvements | expand |
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
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; } /*
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(-)