Re: Re: [PATCH v3 01/21] hw/riscv: Use misa_mxl instead of misa_mxl_max
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 +, Alex Bennée wrote:
> > > > > From: Akihiko Odaki
> > > > >
> > > > > 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
> > > > > Message-Id: <20240103173349.398526-23-alex.ben...@linaro.org>
> > > > > Message-Id: <20231213-riscv-v7-1-a760156a3...@daynix.com>
> > > > > Signed-off-by: Alex Bennée
> > > > > ---
> > > > >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-ff171e1ae...@daynix.com/20240115-riscv-v9-1-ff171e1ae...@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_i
Re: [PATCH v3 01/21] hw/riscv: Use misa_mxl instead of misa_mxl_max
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 +, Alex Bennée wrote:
From: Akihiko Odaki
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
Message-Id: <20240103173349.398526-23-alex.ben...@linaro.org>
Message-Id: <20231213-riscv-v7-1-a760156a3...@daynix.com>
Signed-off-by: Alex Bennée
---
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-ff171e1ae...@daynix.com/20240115-riscv-v9-1-ff171e1ae...@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.
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
Re: Re: [PATCH v3 01/21] hw/riscv: Use misa_mxl instead of misa_mxl_max
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 +, Alex Bennée wrote:
> > > From: Akihiko Odaki
> > >
> > > 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
> > > Message-Id: <20240103173349.398526-23-alex.ben...@linaro.org>
> > > Message-Id: <20231213-riscv-v7-1-a760156a3...@daynix.com>
> > > Signed-off-by: Alex Bennée
> > > ---
> > > 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-ff171e1ae...@daynix.com/20240115-riscv-v9-1-ff171e1ae...@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.
> 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 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.
*/
Thanks,
drew
Re: [PATCH v3 01/21] hw/riscv: Use misa_mxl instead of misa_mxl_max
On 2024/01/23 17:20, Andrew Jones wrote:
On Mon, Jan 22, 2024 at 02:55:50PM +, Alex Bennée wrote:
From: Akihiko Odaki
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
Message-Id: <20240103173349.398526-23-alex.ben...@linaro.org>
Message-Id: <20231213-riscv-v7-1-a760156a3...@daynix.com>
Signed-off-by: Alex Bennée
---
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-ff171e1ae...@daynix.com/20240115-riscv-v9-1-ff171e1ae...@daynix.com/
Alex, can you pull it?
Regards,
Akihiko Odaki
Re: [PATCH v3 01/21] hw/riscv: Use misa_mxl instead of misa_mxl_max
On Mon, Jan 22, 2024 at 02:55:50PM +, Alex Bennée wrote:
> From: Akihiko Odaki
>
> 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
> Message-Id: <20240103173349.398526-23-alex.ben...@linaro.org>
> Message-Id: <20231213-riscv-v7-1-a760156a3...@daynix.com>
> Signed-off-by: Alex Bennée
> ---
> 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
Re: [PATCH v3 01/21] hw/riscv: Use misa_mxl instead of misa_mxl_max
On Tue, Jan 23, 2024 at 12:57 AM Alex Bennée wrote:
>
> From: Akihiko Odaki
>
> The effective MXL value matters when booting.
>
> Signed-off-by: Akihiko Odaki
> Message-Id: <20240103173349.398526-23-alex.ben...@linaro.org>
> Message-Id: <20231213-riscv-v7-1-a760156a3...@daynix.com>
> Signed-off-by: Alex Bennée
Reviewed-by: Alistair Francis
Alistair
> ---
> 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;
> }
>
> /*
> --
> 2.39.2
>
>
[PATCH v3 01/21] hw/riscv: Use misa_mxl instead of misa_mxl_max
From: Akihiko Odaki
The effective MXL value matters when booting.
Signed-off-by: Akihiko Odaki
Message-Id: <20240103173349.398526-23-alex.ben...@linaro.org>
Message-Id: <20231213-riscv-v7-1-a760156a3...@daynix.com>
Signed-off-by: Alex Bennée
---
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;
}
/*
--
2.39.2
