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