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