Re: Re: [PATCH v3 01/21] hw/riscv: Use misa_mxl instead of misa_mxl_max

[Andrew Jones]

On Thu, Jan 25, 2024 at 05:23:20PM +0900, Akihiko Odaki wrote:
> 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,
> > > > drew
> > > 
> > > The 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 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.

Of course it matters. mxl will only change if we allow the guest to write
it. Being aware of when/why mxl changes, i.e. is no longer equal to the
max mxl, is the whole point of this patch.

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

Um, what I wrote below...

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

Of course mxl_max can be checked whenever one wants, but you're changing
a function named riscv_is_32bit(), not
riscv_is_32bit_when_loading_a_kernel(). riscv_is_32bit() is used in
contexts where one should be concerned as to whether mxl == mxl_max
or not, because what they really want is the max. The comment I suggest
will allow them to rest easy.

It's concerning that it doesn't appear you were aware that
riscv_is_32bit() is used outside kernel loading contexts, which means it's
unlikely you knew it was safe to make this change at all... I'm actually
starting to think that maybe we should not make this change.
riscv_is_32bit() should always check mxl_max and something like
riscv_effective_mxl_is_32bit() should check mxl.

drew