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