* [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled
@ 2023-01-23 3:57 Alistair Francis
2023-01-23 10:24 ` Daniel Henrique Barboza
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Alistair Francis @ 2023-01-23 3:57 UTC (permalink / raw)
To: qemu-devel, qemu-riscv
Cc: palmer, alistair.francis, Bin Meng, alistair23, bmeng.cn
From: Alistair Francis <alistair.francis@wdc.com>
If the CSRs and CSR instructions are disabled because the Zicsr
extension isn't enabled then we want to make sure we don't run any CSR
instructions in the boot ROM.
This patches removes the CSR instructions from the reset-vec if the
extension isn't enabled. We replace the instruction with a NOP instead.
Note that we don't do this for the SiFive U machine, as we are modelling
the hardware in that case.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1447
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
hw/riscv/boot.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 2594276223..cb27798a25 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -356,6 +356,15 @@ void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts
reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */
}
+ if (!harts->harts[0].cfg.ext_icsr) {
+ /*
+ * The Zicsr extension has been disabled, so let's ensure we don't
+ * run the CSR instruction. Let's fill the address with a non
+ * compressed nop.
+ */
+ reset_vec[2] = 0x00000013; /* addi x0, x0, 0 */
+ }
+
/* copy in the reset vector in little_endian byte order */
for (i = 0; i < ARRAY_SIZE(reset_vec); i++) {
reset_vec[i] = cpu_to_le32(reset_vec[i]);
--
2.39.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled
2023-01-23 3:57 [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled Alistair Francis
@ 2023-01-23 10:24 ` Daniel Henrique Barboza
2023-01-23 11:51 ` Alistair Francis
2023-01-23 12:02 ` Daniel Henrique Barboza
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Daniel Henrique Barboza @ 2023-01-23 10:24 UTC (permalink / raw)
To: Alistair Francis, qemu-devel, qemu-riscv
Cc: palmer, alistair.francis, Bin Meng, alistair23, bmeng.cn
On 1/23/23 00:57, Alistair Francis wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
>
> If the CSRs and CSR instructions are disabled because the Zicsr
> extension isn't enabled then we want to make sure we don't run any CSR
> instructions in the boot ROM.
>
> This patches removes the CSR instructions from the reset-vec if the
> extension isn't enabled. We replace the instruction with a NOP instead.
>
> Note that we don't do this for the SiFive U machine, as we are modelling
> the hardware in that case.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1447
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
Shouldn't we also handle the sifive_u/sifive_e boards? Their reset vectors
aren't being covered by riscv_set_rom_reset_vec() (it's on my TODO, didn't
send it yet because sifive uses an extra MSEL pin at the start of the vector).
Daniel
> hw/riscv/boot.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 2594276223..cb27798a25 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -356,6 +356,15 @@ void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts
> reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */
> }
>
> + if (!harts->harts[0].cfg.ext_icsr) {
> + /*
> + * The Zicsr extension has been disabled, so let's ensure we don't
> + * run the CSR instruction. Let's fill the address with a non
> + * compressed nop.
> + */
> + reset_vec[2] = 0x00000013; /* addi x0, x0, 0 */
> + }
> +
> /* copy in the reset vector in little_endian byte order */
> for (i = 0; i < ARRAY_SIZE(reset_vec); i++) {
> reset_vec[i] = cpu_to_le32(reset_vec[i]);
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled
2023-01-23 10:24 ` Daniel Henrique Barboza
@ 2023-01-23 11:51 ` Alistair Francis
0 siblings, 0 replies; 13+ messages in thread
From: Alistair Francis @ 2023-01-23 11:51 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: Alistair Francis, qemu-devel, qemu-riscv, palmer,
alistair.francis, Bin Meng, bmeng.cn
On Mon, Jan 23, 2023 at 8:25 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 1/23/23 00:57, Alistair Francis wrote:
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > If the CSRs and CSR instructions are disabled because the Zicsr
> > extension isn't enabled then we want to make sure we don't run any CSR
> > instructions in the boot ROM.
> >
> > This patches removes the CSR instructions from the reset-vec if the
> > extension isn't enabled. We replace the instruction with a NOP instead.
> >
> > Note that we don't do this for the SiFive U machine, as we are modelling
> > the hardware in that case.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1447
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
>
> Shouldn't we also handle the sifive_u/sifive_e boards? Their reset vectors
> aren't being covered by riscv_set_rom_reset_vec() (it's on my TODO, didn't
> send it yet because sifive uses an extra MSEL pin at the start of the vector).
I feel that those boards are modelling hardware, so in this case we
should model what the hardware does. I don't even think that a user
could disable the CSR extension on those boards.
Alistair
>
>
>
> Daniel
>
>
>
> > hw/riscv/boot.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 2594276223..cb27798a25 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -356,6 +356,15 @@ void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts
> > reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */
> > }
> >
> > + if (!harts->harts[0].cfg.ext_icsr) {
> > + /*
> > + * The Zicsr extension has been disabled, so let's ensure we don't
> > + * run the CSR instruction. Let's fill the address with a non
> > + * compressed nop.
> > + */
> > + reset_vec[2] = 0x00000013; /* addi x0, x0, 0 */
> > + }
> > +
> > /* copy in the reset vector in little_endian byte order */
> > for (i = 0; i < ARRAY_SIZE(reset_vec); i++) {
> > reset_vec[i] = cpu_to_le32(reset_vec[i]);
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled
2023-01-23 3:57 [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled Alistair Francis
2023-01-23 10:24 ` Daniel Henrique Barboza
@ 2023-01-23 12:02 ` Daniel Henrique Barboza
2023-01-23 23:59 ` Alistair Francis
2023-01-24 1:24 ` Bin Meng
3 siblings, 0 replies; 13+ messages in thread
From: Daniel Henrique Barboza @ 2023-01-23 12:02 UTC (permalink / raw)
To: Alistair Francis, qemu-devel, qemu-riscv
Cc: palmer, alistair.francis, Bin Meng, alistair23, bmeng.cn
On 1/23/23 00:57, Alistair Francis wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
>
> If the CSRs and CSR instructions are disabled because the Zicsr
> extension isn't enabled then we want to make sure we don't run any CSR
> instructions in the boot ROM.
>
> This patches removes the CSR instructions from the reset-vec if the
> extension isn't enabled. We replace the instruction with a NOP instead.
>
> Note that we don't do this for the SiFive U machine, as we are modelling
> the hardware in that case.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1447
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> hw/riscv/boot.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 2594276223..cb27798a25 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -356,6 +356,15 @@ void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts
> reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */
> }
>
> + if (!harts->harts[0].cfg.ext_icsr) {
> + /*
> + * The Zicsr extension has been disabled, so let's ensure we don't
> + * run the CSR instruction. Let's fill the address with a non
> + * compressed nop.
> + */
> + reset_vec[2] = 0x00000013; /* addi x0, x0, 0 */
> + }
> +
> /* copy in the reset vector in little_endian byte order */
> for (i = 0; i < ARRAY_SIZE(reset_vec); i++) {
> reset_vec[i] = cpu_to_le32(reset_vec[i]);
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled
2023-01-23 3:57 [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled Alistair Francis
2023-01-23 10:24 ` Daniel Henrique Barboza
2023-01-23 12:02 ` Daniel Henrique Barboza
@ 2023-01-23 23:59 ` Alistair Francis
2023-01-24 1:24 ` Bin Meng
3 siblings, 0 replies; 13+ messages in thread
From: Alistair Francis @ 2023-01-23 23:59 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel, qemu-riscv, palmer, alistair.francis, Bin Meng,
bmeng.cn
On Mon, Jan 23, 2023 at 1:58 PM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> If the CSRs and CSR instructions are disabled because the Zicsr
> extension isn't enabled then we want to make sure we don't run any CSR
> instructions in the boot ROM.
>
> This patches removes the CSR instructions from the reset-vec if the
> extension isn't enabled. We replace the instruction with a NOP instead.
>
> Note that we don't do this for the SiFive U machine, as we are modelling
> the hardware in that case.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1447
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Thanks!
Applied to riscv-to-apply.next
Alistair
> ---
> hw/riscv/boot.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 2594276223..cb27798a25 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -356,6 +356,15 @@ void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts
> reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */
> }
>
> + if (!harts->harts[0].cfg.ext_icsr) {
> + /*
> + * The Zicsr extension has been disabled, so let's ensure we don't
> + * run the CSR instruction. Let's fill the address with a non
> + * compressed nop.
> + */
> + reset_vec[2] = 0x00000013; /* addi x0, x0, 0 */
> + }
> +
> /* copy in the reset vector in little_endian byte order */
> for (i = 0; i < ARRAY_SIZE(reset_vec); i++) {
> reset_vec[i] = cpu_to_le32(reset_vec[i]);
> --
> 2.39.0
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled
2023-01-23 3:57 [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled Alistair Francis
` (2 preceding siblings ...)
2023-01-23 23:59 ` Alistair Francis
@ 2023-01-24 1:24 ` Bin Meng
2023-01-24 1:42 ` Alistair Francis
3 siblings, 1 reply; 13+ messages in thread
From: Bin Meng @ 2023-01-24 1:24 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel, qemu-riscv, palmer, alistair.francis, Bin Meng,
alistair23
On Mon, Jan 23, 2023 at 11:58 AM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> If the CSRs and CSR instructions are disabled because the Zicsr
> extension isn't enabled then we want to make sure we don't run any CSR
> instructions in the boot ROM.
>
> This patches removes the CSR instructions from the reset-vec if the
> extension isn't enabled. We replace the instruction with a NOP instead.
>
> Note that we don't do this for the SiFive U machine, as we are modelling
> the hardware in that case.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1447
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> hw/riscv/boot.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 2594276223..cb27798a25 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -356,6 +356,15 @@ void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts
> reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */
> }
>
> + if (!harts->harts[0].cfg.ext_icsr) {
> + /*
> + * The Zicsr extension has been disabled, so let's ensure we don't
> + * run the CSR instruction. Let's fill the address with a non
> + * compressed nop.
> + */
> + reset_vec[2] = 0x00000013; /* addi x0, x0, 0 */
> + }
This is fine for a UP system. I am not sure how SMP can be supported
without Zicsr as we need to assign hartid in a0.
> +
> /* copy in the reset vector in little_endian byte order */
> for (i = 0; i < ARRAY_SIZE(reset_vec); i++) {
> reset_vec[i] = cpu_to_le32(reset_vec[i]);
> --
Regards,
Bin
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled
2023-01-24 1:24 ` Bin Meng
@ 2023-01-24 1:42 ` Alistair Francis
2023-01-26 12:03 ` Bin Meng
0 siblings, 1 reply; 13+ messages in thread
From: Alistair Francis @ 2023-01-24 1:42 UTC (permalink / raw)
To: Bin Meng
Cc: Alistair Francis, qemu-devel, qemu-riscv, palmer,
alistair.francis, Bin Meng
On Tue, Jan 24, 2023 at 11:24 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Mon, Jan 23, 2023 at 11:58 AM Alistair Francis
> <alistair.francis@opensource.wdc.com> wrote:
> >
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > If the CSRs and CSR instructions are disabled because the Zicsr
> > extension isn't enabled then we want to make sure we don't run any CSR
> > instructions in the boot ROM.
> >
> > This patches removes the CSR instructions from the reset-vec if the
> > extension isn't enabled. We replace the instruction with a NOP instead.
> >
> > Note that we don't do this for the SiFive U machine, as we are modelling
> > the hardware in that case.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1447
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > hw/riscv/boot.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 2594276223..cb27798a25 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -356,6 +356,15 @@ void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts
> > reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */
> > }
> >
> > + if (!harts->harts[0].cfg.ext_icsr) {
> > + /*
> > + * The Zicsr extension has been disabled, so let's ensure we don't
> > + * run the CSR instruction. Let's fill the address with a non
> > + * compressed nop.
> > + */
> > + reset_vec[2] = 0x00000013; /* addi x0, x0, 0 */
> > + }
>
> This is fine for a UP system. I am not sure how SMP can be supported
> without Zicsr as we need to assign hartid in a0.
Yeah. My thinking was that no one would be using a multicore system
without Zicsr as it's such a core extension. If they are running
without Zicsr they have probably hard coded a lot of things anyway and
don't expect this to work.
In general I think it's pretty rare to even run a RISC-V core without
Zicsr at all.
Alistair
>
> > +
> > /* copy in the reset vector in little_endian byte order */
> > for (i = 0; i < ARRAY_SIZE(reset_vec); i++) {
> > reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > --
>
> Regards,
> Bin
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled
2023-01-24 1:42 ` Alistair Francis
@ 2023-01-26 12:03 ` Bin Meng
2023-01-29 23:18 ` Alistair Francis
0 siblings, 1 reply; 13+ messages in thread
From: Bin Meng @ 2023-01-26 12:03 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, qemu-devel, qemu-riscv, palmer,
alistair.francis, Bin Meng
On Tue, Jan 24, 2023 at 9:42 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, Jan 24, 2023 at 11:24 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Mon, Jan 23, 2023 at 11:58 AM Alistair Francis
> > <alistair.francis@opensource.wdc.com> wrote:
> > >
> > > From: Alistair Francis <alistair.francis@wdc.com>
> > >
> > > If the CSRs and CSR instructions are disabled because the Zicsr
> > > extension isn't enabled then we want to make sure we don't run any CSR
> > > instructions in the boot ROM.
> > >
> > > This patches removes the CSR instructions from the reset-vec if the
> > > extension isn't enabled. We replace the instruction with a NOP instead.
> > >
> > > Note that we don't do this for the SiFive U machine, as we are modelling
> > > the hardware in that case.
> > >
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1447
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > > hw/riscv/boot.c | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > > index 2594276223..cb27798a25 100644
> > > --- a/hw/riscv/boot.c
> > > +++ b/hw/riscv/boot.c
> > > @@ -356,6 +356,15 @@ void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts
> > > reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */
> > > }
> > >
> > > + if (!harts->harts[0].cfg.ext_icsr) {
> > > + /*
> > > + * The Zicsr extension has been disabled, so let's ensure we don't
> > > + * run the CSR instruction. Let's fill the address with a non
> > > + * compressed nop.
> > > + */
> > > + reset_vec[2] = 0x00000013; /* addi x0, x0, 0 */
> > > + }
> >
> > This is fine for a UP system. I am not sure how SMP can be supported
> > without Zicsr as we need to assign hartid in a0.
>
> Yeah. My thinking was that no one would be using a multicore system
> without Zicsr as it's such a core extension. If they are running
> without Zicsr they have probably hard coded a lot of things anyway and
> don't expect this to work.
>
> In general I think it's pretty rare to even run a RISC-V core without
> Zicsr at all.
>
As QEMU implements Zicsr anyway, and there is no way to support SMP
without Zicsr, should we disallow user to disable Zicsr in QEMU?
Regards,
Bin
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled
2023-01-26 12:03 ` Bin Meng
@ 2023-01-29 23:18 ` Alistair Francis
2023-01-31 12:31 ` Bin Meng
0 siblings, 1 reply; 13+ messages in thread
From: Alistair Francis @ 2023-01-29 23:18 UTC (permalink / raw)
To: Bin Meng
Cc: Alistair Francis, qemu-devel, qemu-riscv, palmer,
alistair.francis, Bin Meng
On Thu, Jan 26, 2023 at 10:03 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Tue, Jan 24, 2023 at 9:42 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Tue, Jan 24, 2023 at 11:24 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > On Mon, Jan 23, 2023 at 11:58 AM Alistair Francis
> > > <alistair.francis@opensource.wdc.com> wrote:
> > > >
> > > > From: Alistair Francis <alistair.francis@wdc.com>
> > > >
> > > > If the CSRs and CSR instructions are disabled because the Zicsr
> > > > extension isn't enabled then we want to make sure we don't run any CSR
> > > > instructions in the boot ROM.
> > > >
> > > > This patches removes the CSR instructions from the reset-vec if the
> > > > extension isn't enabled. We replace the instruction with a NOP instead.
> > > >
> > > > Note that we don't do this for the SiFive U machine, as we are modelling
> > > > the hardware in that case.
> > > >
> > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1447
> > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > ---
> > > > hw/riscv/boot.c | 9 +++++++++
> > > > 1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > > > index 2594276223..cb27798a25 100644
> > > > --- a/hw/riscv/boot.c
> > > > +++ b/hw/riscv/boot.c
> > > > @@ -356,6 +356,15 @@ void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts
> > > > reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */
> > > > }
> > > >
> > > > + if (!harts->harts[0].cfg.ext_icsr) {
> > > > + /*
> > > > + * The Zicsr extension has been disabled, so let's ensure we don't
> > > > + * run the CSR instruction. Let's fill the address with a non
> > > > + * compressed nop.
> > > > + */
> > > > + reset_vec[2] = 0x00000013; /* addi x0, x0, 0 */
> > > > + }
> > >
> > > This is fine for a UP system. I am not sure how SMP can be supported
> > > without Zicsr as we need to assign hartid in a0.
> >
> > Yeah. My thinking was that no one would be using a multicore system
> > without Zicsr as it's such a core extension. If they are running
> > without Zicsr they have probably hard coded a lot of things anyway and
> > don't expect this to work.
> >
> > In general I think it's pretty rare to even run a RISC-V core without
> > Zicsr at all.
> >
>
> As QEMU implements Zicsr anyway, and there is no way to support SMP
> without Zicsr, should we disallow user to disable Zicsr in QEMU?
I feel like we don't need to do that. Here's my thinking:
Zicsr is a RISC-V extension, the RISC-V spec splits it out so that it
can be disabled. In theory someone could build a multi-hart CPU
without Zicsr in hardware, so QEMU should be able to model it.
As well as that Zicsr is enabled by default, so a user has to know
enough to disable it manually. At which point they probably know what
they are doing, especially as no standard software will run without
Zicsr. If that's what someone wants to do then we should allow them
to, even if it's a bit strange.
Alistair
>
> Regards,
> Bin
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled
2023-01-29 23:18 ` Alistair Francis
@ 2023-01-31 12:31 ` Bin Meng
2023-02-02 0:26 ` Alistair Francis
0 siblings, 1 reply; 13+ messages in thread
From: Bin Meng @ 2023-01-31 12:31 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, qemu-devel, qemu-riscv, palmer,
alistair.francis, Bin Meng
On Mon, Jan 30, 2023 at 7:19 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Jan 26, 2023 at 10:03 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Tue, Jan 24, 2023 at 9:42 AM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Tue, Jan 24, 2023 at 11:24 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > On Mon, Jan 23, 2023 at 11:58 AM Alistair Francis
> > > > <alistair.francis@opensource.wdc.com> wrote:
> > > > >
> > > > > From: Alistair Francis <alistair.francis@wdc.com>
> > > > >
> > > > > If the CSRs and CSR instructions are disabled because the Zicsr
> > > > > extension isn't enabled then we want to make sure we don't run any CSR
> > > > > instructions in the boot ROM.
> > > > >
> > > > > This patches removes the CSR instructions from the reset-vec if the
> > > > > extension isn't enabled. We replace the instruction with a NOP instead.
> > > > >
> > > > > Note that we don't do this for the SiFive U machine, as we are modelling
> > > > > the hardware in that case.
> > > > >
> > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1447
> > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > ---
> > > > > hw/riscv/boot.c | 9 +++++++++
> > > > > 1 file changed, 9 insertions(+)
> > > > >
> > > > > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > > > > index 2594276223..cb27798a25 100644
> > > > > --- a/hw/riscv/boot.c
> > > > > +++ b/hw/riscv/boot.c
> > > > > @@ -356,6 +356,15 @@ void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts
> > > > > reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */
> > > > > }
> > > > >
> > > > > + if (!harts->harts[0].cfg.ext_icsr) {
> > > > > + /*
> > > > > + * The Zicsr extension has been disabled, so let's ensure we don't
> > > > > + * run the CSR instruction. Let's fill the address with a non
> > > > > + * compressed nop.
> > > > > + */
> > > > > + reset_vec[2] = 0x00000013; /* addi x0, x0, 0 */
> > > > > + }
> > > >
> > > > This is fine for a UP system. I am not sure how SMP can be supported
> > > > without Zicsr as we need to assign hartid in a0.
> > >
> > > Yeah. My thinking was that no one would be using a multicore system
> > > without Zicsr as it's such a core extension. If they are running
> > > without Zicsr they have probably hard coded a lot of things anyway and
> > > don't expect this to work.
> > >
> > > In general I think it's pretty rare to even run a RISC-V core without
> > > Zicsr at all.
> > >
> >
> > As QEMU implements Zicsr anyway, and there is no way to support SMP
> > without Zicsr, should we disallow user to disable Zicsr in QEMU?
>
> I feel like we don't need to do that. Here's my thinking:
>
> Zicsr is a RISC-V extension, the RISC-V spec splits it out so that it
> can be disabled. In theory someone could build a multi-hart CPU
> without Zicsr in hardware, so QEMU should be able to model it.
Correct. But if Zicsr is not present, then the standard privileged
architecture which qemu-system-riscv currently supports, is inherently
not present, either.
If a user chooses to disable Zicsr, current QEMU system emulation is
useless then.
That's why I believe we shouldn't allow users to disable Zicsr for
qemu-system-riscv.
> As well as that Zicsr is enabled by default, so a user has to know
> enough to disable it manually. At which point they probably know what
> they are doing, especially as no standard software will run without
> Zicsr. If that's what someone wants to do then we should allow them
> to, even if it's a bit strange.
>
For qemu-riscv, disabling Zicsr is feasible as long as the codes does
not touch any CSR, e.g.: timer, counters, fcsr, etc.
Regards,
Bin
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled
2023-01-31 12:31 ` Bin Meng
@ 2023-02-02 0:26 ` Alistair Francis
0 siblings, 0 replies; 13+ messages in thread
From: Alistair Francis @ 2023-02-02 0:26 UTC (permalink / raw)
To: Bin Meng
Cc: Alistair Francis, qemu-devel, qemu-riscv, palmer,
alistair.francis, Bin Meng
On Tue, Jan 31, 2023 at 10:31 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Mon, Jan 30, 2023 at 7:19 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Thu, Jan 26, 2023 at 10:03 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > On Tue, Jan 24, 2023 at 9:42 AM Alistair Francis <alistair23@gmail.com> wrote:
> > > >
> > > > On Tue, Jan 24, 2023 at 11:24 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > On Mon, Jan 23, 2023 at 11:58 AM Alistair Francis
> > > > > <alistair.francis@opensource.wdc.com> wrote:
> > > > > >
> > > > > > From: Alistair Francis <alistair.francis@wdc.com>
> > > > > >
> > > > > > If the CSRs and CSR instructions are disabled because the Zicsr
> > > > > > extension isn't enabled then we want to make sure we don't run any CSR
> > > > > > instructions in the boot ROM.
> > > > > >
> > > > > > This patches removes the CSR instructions from the reset-vec if the
> > > > > > extension isn't enabled. We replace the instruction with a NOP instead.
> > > > > >
> > > > > > Note that we don't do this for the SiFive U machine, as we are modelling
> > > > > > the hardware in that case.
> > > > > >
> > > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1447
> > > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > > ---
> > > > > > hw/riscv/boot.c | 9 +++++++++
> > > > > > 1 file changed, 9 insertions(+)
> > > > > >
> > > > > > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > > > > > index 2594276223..cb27798a25 100644
> > > > > > --- a/hw/riscv/boot.c
> > > > > > +++ b/hw/riscv/boot.c
> > > > > > @@ -356,6 +356,15 @@ void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts
> > > > > > reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */
> > > > > > }
> > > > > >
> > > > > > + if (!harts->harts[0].cfg.ext_icsr) {
> > > > > > + /*
> > > > > > + * The Zicsr extension has been disabled, so let's ensure we don't
> > > > > > + * run the CSR instruction. Let's fill the address with a non
> > > > > > + * compressed nop.
> > > > > > + */
> > > > > > + reset_vec[2] = 0x00000013; /* addi x0, x0, 0 */
> > > > > > + }
> > > > >
> > > > > This is fine for a UP system. I am not sure how SMP can be supported
> > > > > without Zicsr as we need to assign hartid in a0.
> > > >
> > > > Yeah. My thinking was that no one would be using a multicore system
> > > > without Zicsr as it's such a core extension. If they are running
> > > > without Zicsr they have probably hard coded a lot of things anyway and
> > > > don't expect this to work.
> > > >
> > > > In general I think it's pretty rare to even run a RISC-V core without
> > > > Zicsr at all.
> > > >
> > >
> > > As QEMU implements Zicsr anyway, and there is no way to support SMP
> > > without Zicsr, should we disallow user to disable Zicsr in QEMU?
> >
> > I feel like we don't need to do that. Here's my thinking:
> >
> > Zicsr is a RISC-V extension, the RISC-V spec splits it out so that it
> > can be disabled. In theory someone could build a multi-hart CPU
> > without Zicsr in hardware, so QEMU should be able to model it.
>
> Correct. But if Zicsr is not present, then the standard privileged
> architecture which qemu-system-riscv currently supports, is inherently
> not present, either.
>
> If a user chooses to disable Zicsr, current QEMU system emulation is
> useless then.
I wouldn't say useless. If a user does disable Zicsr then effectively
they have disabled the priv spec.
>
> That's why I believe we shouldn't allow users to disable Zicsr for
> qemu-system-riscv.
We currently support disabling it and it appears that people are using
it, so I don't think it makes sense to remove.
I agree for a standard use case Zicsr will always be enabled, but I
can picture users running experiments/tests/benchmarks and wanting to
disable the extension.
Alistair
>
> > As well as that Zicsr is enabled by default, so a user has to know
> > enough to disable it manually. At which point they probably know what
> > they are doing, especially as no standard software will run without
> > Zicsr. If that's what someone wants to do then we should allow them
> > to, even if it's a bit strange.
> >
>
> For qemu-riscv, disabling Zicsr is feasible as long as the codes does
> not touch any CSR, e.g.: timer, counters, fcsr, etc.
>
> Regards,
> Bin
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled
@ 2023-01-23 22:14 Jesse Taube
2023-01-23 23:56 ` Alistair Francis
0 siblings, 1 reply; 13+ messages in thread
From: Jesse Taube @ 2023-01-23 22:14 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel, Daniel Henrique Barboza, Daniel Henrique Barboza
> From: Alistair Francis <alistair.francis@wdc.com>
>
> If the CSRs and CSR instructions are disabled because the Zicsr
> extension isn't enabled then we want to make sure we don't run any CSR
> instructions in the boot ROM.
>
> This patches removes the CSR instructions from the reset-vec if the
> extension isn't enabled. We replace the instruction with a NOP instead.
>
> Note that we don't do this for the SiFive U machine, as we are modelling
> the hardware in that case.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1447
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> hw/riscv/boot.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 2594276223..cb27798a25 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -356,6 +356,15 @@ void riscv_setup_rom_reset_vec(MachineState
*machine,
> RISCVHartArrayState *harts
> reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */
> }
>
> + if (!harts->harts[0].cfg.ext_icsr) {
> + /*
> + * The Zicsr extension has been disabled, so let's ensure we
don't
> + * run the CSR instruction. Let's fill the address with a non
> + * compressed nop.
> + */
> + reset_vec[2] = 0x00000013; /* addi x0, x0, 0 */
reset_vec[2] = 0x00000513; /* li a0, 0 */
Shouldn't it be li as a0 should contain the cpu number. The regs are
initialized with 0 so its not necessary but nice to be explicit.
Thanks,
Jesse T
> + }
> +
> /* copy in the reset vector in little_endian byte order */
> for (i = 0; i < ARRAY_SIZE(reset_vec); i++) {
> reset_vec[i] = cpu_to_le32(reset_vec[i]);
> --
> 2.39.0
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled
2023-01-23 22:14 Jesse Taube
@ 2023-01-23 23:56 ` Alistair Francis
0 siblings, 0 replies; 13+ messages in thread
From: Alistair Francis @ 2023-01-23 23:56 UTC (permalink / raw)
To: 20230123035754.75553-1-alistair.francis@opensource.wdc.com,
mr.bossman075@gmail.com
Cc: qemu-devel@nongnu.org, dbarboza@ventanamicro.com
On Mon, 2023-01-23 at 17:14 -0500, Jesse Taube wrote:
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > If the CSRs and CSR instructions are disabled because the Zicsr
> > extension isn't enabled then we want to make sure we don't run any
> CSR
> > instructions in the boot ROM.
> >
> > This patches removes the CSR instructions from the reset-vec if
> the
> > extension isn't enabled. We replace the instruction with a NOP
> instead.
> >
> > Note that we don't do this for the SiFive U machine, as we are
> modelling
> > the hardware in that case.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1447
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > hw/riscv/boot.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 2594276223..cb27798a25 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -356,6 +356,15 @@ void riscv_setup_rom_reset_vec(MachineState
> *machine,
> > RISCVHartArrayState *harts
> > reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */
> > }
> >
> > + if (!harts->harts[0].cfg.ext_icsr) {
> > + /*
> > + * The Zicsr extension has been disabled, so let's ensure
> we
> don't
> > + * run the CSR instruction. Let's fill the address with a
> non
> > + * compressed nop.
> > + */
> > + reset_vec[2] = 0x00000013; /* addi x0, x0, 0 */
> reset_vec[2] = 0x00000513; /* li a0, 0 */
> Shouldn't it be li as a0 should contain the cpu number. The regs are
> initialized with 0 so its not necessary but nice to be explicit.
So we can't just run:
li a0, x
where x is the hart ID as each CPU runs the same reset code.
So we could run:
li a0, 0
To zero out a0, but as a0 is 0 on reset I don't think we need to.
This current instruction should be decoded as a NOP, which I think is
simpler for users to understand. Otherwise we will have code on CPU1
that sets a0 to 0, which to me seems more confusing.
Alistair
>
> Thanks,
> Jesse T
> > + }
> > +
> > /* copy in the reset vector in little_endian byte order */
> > for (i = 0; i < ARRAY_SIZE(reset_vec); i++) {
> > reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > --
> > 2.39.0
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-02-02 0:27 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-23 3:57 [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled Alistair Francis
2023-01-23 10:24 ` Daniel Henrique Barboza
2023-01-23 11:51 ` Alistair Francis
2023-01-23 12:02 ` Daniel Henrique Barboza
2023-01-23 23:59 ` Alistair Francis
2023-01-24 1:24 ` Bin Meng
2023-01-24 1:42 ` Alistair Francis
2023-01-26 12:03 ` Bin Meng
2023-01-29 23:18 ` Alistair Francis
2023-01-31 12:31 ` Bin Meng
2023-02-02 0:26 ` Alistair Francis
-- strict thread matches above, loose matches on Subject: below --
2023-01-23 22:14 Jesse Taube
2023-01-23 23:56 ` Alistair Francis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).