Message ID | 20230123035754.75553-1-alistair.francis@opensource.wdc.com |
---|---|
State | New |
Headers | show |
Series | hw/riscv: boot: Don't use CSRs if they are disabled | expand |
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]);
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]);
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]);
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 >
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
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
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
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
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
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
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]);