[PATCH] hw/riscv: boot: Don't use CSRs if they are disabled

[PATCH] hw/riscv: boot: Don't use CSRs if they are disabled

[Alistair Francis]

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

Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled

[Daniel Henrique Barboza]

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]);

Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled

[Alistair Francis]

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]);

Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled

[Daniel Henrique Barboza]

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]);

Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled

[Jesse Taube]

 > 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

Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled

[Alistair Francis]

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

Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled

[Alistair Francis]

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
>

Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled

[Bin Meng]

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

Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled

[Alistair Francis]

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

Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled

[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

Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled

[Alistair Francis]

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

Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled

[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

Re: [PATCH] hw/riscv: boot: Don't use CSRs if they are disabled

[Alistair Francis]

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