On 2024/01/24 17:16, Andrew Jones wrote:
On Wed, Jan 24, 2024 at 12:08:33PM +0900, Akihiko Odaki wrote:
On 2024/01/23 17:20, Andrew Jones wrote:
On Mon, Jan 22, 2024 at 02:55:50PM +0000, Alex Bennée wrote:
From: Akihiko Odaki <akihiko.odaki@xxxxxxxxxx>
The effective MXL value matters when booting.
I'd prefer this commit message get some elaboration. riscv_is_32bit()
is used in a variety of contexts, some where it should be reporting
the max misa.mxl. However, when used for booting an S-mode kernel it
should indeed report the effective mxl. I think we're fine with the
change, though, because at init and on reset the effective mxl is set
to the max mxl, so, in those contexts, where riscv_is_32bit() should
be reporting the max, it does.
Signed-off-by: Akihiko Odaki <akihiko.odaki@xxxxxxxxxx>
Message-Id: <20240103173349.398526-23-alex.bennee@xxxxxxxxxx>
Message-Id: <20231213-riscv-v7-1-a760156a337f@xxxxxxxxxx>
Signed-off-by: Alex Bennée <alex.bennee@xxxxxxxxxx>
---
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 0ffca05189f..bc67c0bd189 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;
}
Assuming everyone agrees with what I've written above, then maybe we
should write something similar in a comment above this function.
Thanks,
drew
The corresponding commit in my series has a more elaborated message:
https://patchew.org/QEMU/20240115-riscv-v9-0-ff171e1aedc8@xxxxxxxxxx/20240115-riscv-v9-1-ff171e1aedc8@xxxxxxxxxx/
I've pulled the message from that link and quoted it below
A later commit requires one extra step to retrieve misa_mxl_max. As
misa_mxl is semantically more correct and does not need such a extra
step, refer to misa_mxl instead. Below is the explanation why misa_mxl
is more semantically correct to refer to than misa_mxl_max in this case.
Currently misa_mxl always equals to misa_mxl_max so it does not matter
That's true, but I think that's due to a bug in write_misa(), which
shouldn't be masking val with the extension mask until mxl has been
extracted.
misa.MXL writes are not supported since the MISA write code was
introduced with commit f18637cd611c ("RISC-V: Add misa runtime write
support"). It doesn't matter if we allow the guest to write MXL; the
firmware code is emulated by QEMU when QEMU loads a kernel.
which of misa_mxl or misa_mxl_max to refer to. However, it is possible
to have different values for misa_mxl and misa_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 misa_mxl_max when
misa_mxl != misa_mxl_max.
Right, but that doesn't say anything more than the original one line,
"The effective MXL value matters when booting."
What do you think is missing?
What I'm looking for is a code comment explaining how riscv_is_32bit()
is always safe to use. Something like
/*
* Checking the effective mxl is always correct, because the effective
* mxl will be equal to the max mxl at initialization and also on reset,
* which are the times when it should check the maximum mxl. Later, if
* firmware writes misa with a smaller mxl, then that mxl should be
* used in checks.
It is misleading to say the maximum MXL should be checked only at
initialization and on reset. We can refer to the maximum MXL anytime; we
just don't need it to load a kernel.
Regards,
Akihiko Odaki
