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

[Alistair Francis <Alistair.Francis@wdc.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