|
| From: | Akihiko Odaki |
| Subject: | Re: [PATCH v3 01/21] hw/riscv: Use misa_mxl instead of misa_mxl_max |
| Date: | Thu, 25 Jan 2024 17:23:20 +0900 |
| User-agent: | Mozilla Thunderbird |
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@daynix.com> 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@daynix.com> Message-Id: <20240103173349.398526-23-alex.bennee@linaro.org> Message-Id: <20231213-riscv-v7-1-a760156a337f@daynix.com> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- 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, drewThe corresponding commit in my series has a more elaborated message: https://patchew.org/QEMU/20240115-riscv-v9-0-ff171e1aedc8@daynix.com/20240115-riscv-v9-1-ff171e1aedc8@daynix.com/I've pulled the message from that link and quoted it belowA 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 matterThat'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
| [Prev in Thread] | Current Thread | [Next in Thread] |