| Message ID | 20240828005019.57705-1-sebastian.huber@embedded-brains.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/xilinx_zynq: Enable Security Extensions | expand |
On Wed, 28 Aug 2024 at 01:51, Sebastian Huber <sebastian.huber@embedded-brains.de> wrote: > > The system supports the Security Extensions (core and GIC). This change is > necessary to run tests which pass on the real hardware. > > Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de> (Added the maintainers to cc.) Does the system have any secure-only devices, RAM, etc? How much testing have you done with this change? (The main reason we disabled has-el3 on this board back in 2014 was as a backwards-compatibility thing when we added EL3 support to the CPU model -- we didn't have a ton of images for the board so we erred on the safe side of not changing the behaviour to avoid potentially breaking existing guest code.) > --- > hw/arm/xilinx_zynq.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c > index 3c56b9abe1..37c234f5ab 100644 > --- a/hw/arm/xilinx_zynq.c > +++ b/hw/arm/xilinx_zynq.c > @@ -219,14 +219,6 @@ static void zynq_init(MachineState *machine) > for (n = 0; n < smp_cpus; n++) { > Object *cpuobj = object_new(machine->cpu_type); > > - /* > - * By default A9 CPUs have EL3 enabled. This board does not currently > - * support EL3 so the CPU EL3 property is disabled before realization. > - */ > - if (object_property_find(cpuobj, "has_el3")) { > - object_property_set_bool(cpuobj, "has_el3", false, &error_fatal); > - } > - > object_property_set_int(cpuobj, "midr", ZYNQ_BOARD_MIDR, > &error_fatal); > object_property_set_int(cpuobj, "reset-cbar", MPCORE_PERIPHBASE, thanks -- PMM
Hello Peter, ----- Am 29. Aug 2024 um 14:50 schrieb Peter Maydell peter.maydell@linaro.org: > On Wed, 28 Aug 2024 at 01:51, Sebastian Huber > <sebastian.huber@embedded-brains.de> wrote: >> >> The system supports the Security Extensions (core and GIC). This change is >> necessary to run tests which pass on the real hardware. >> >> Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de> > > (Added the maintainers to cc.) > > Does the system have any secure-only devices, RAM, etc? > > How much testing have you done with this change? (The main > reason we disabled has-el3 on this board back in 2014 was > as a backwards-compatibility thing when we added EL3 support > to the CPU model -- we didn't have a ton of images for the > board so we erred on the safe side of not changing the > behaviour to avoid potentially breaking existing guest code.) I only tested this change with the RTEMS test suite. I added new tests for the exception handling and this change was necessary to trigger FIQs. I haven't used this machine to work with Linux so far.
On Thu, Aug 29, 2024 at 01:50:02PM +0100, Peter Maydell wrote: > On Wed, 28 Aug 2024 at 01:51, Sebastian Huber > <sebastian.huber@embedded-brains.de> wrote: > > > > The system supports the Security Extensions (core and GIC). This change is > > necessary to run tests which pass on the real hardware. > > > > Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de> > > (Added the maintainers to cc.) > > Does the system have any secure-only devices, RAM, etc? Yes, on real HW there are but I don't think we've modelled any of it yet. There's TZ both on the SoC and also ability to create FPGA logic that can issue secure/non-secure transactions. Here's an overview: https://docs.amd.com/v/u/en-US/ug1019-zynq-trustzone The primary use-case for the upstream Zynq-7000 QEMU models has historically been to run the Open Source SW stack from Linux (some times from u-boot) and up. It's important that we don't break that. So as long as we add additional support without breaking direct Linux boots, I think it's OK to incrementally enable missing pieces even if there's not yet coherent support for firmware boot. In this case, IIUC, when doing direct Linux boot, TYPE_ARM_LINUX_BOOT_IF will take care of the GIC setup for us. > > How much testing have you done with this change? (The main > reason we disabled has-el3 on this board back in 2014 was > as a backwards-compatibility thing when we added EL3 support > to the CPU model -- we didn't have a ton of images for the > board so we erred on the safe side of not changing the > behaviour to avoid potentially breaking existing guest code.) I tried this patch on a couple of my images and it works fine for me! Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com> Tested-by: Edgar E. Iglesias <edgar.iglesias@amd.com> > > > --- > > hw/arm/xilinx_zynq.c | 8 -------- > > 1 file changed, 8 deletions(-) > > > > diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c > > index 3c56b9abe1..37c234f5ab 100644 > > --- a/hw/arm/xilinx_zynq.c > > +++ b/hw/arm/xilinx_zynq.c > > @@ -219,14 +219,6 @@ static void zynq_init(MachineState *machine) > > for (n = 0; n < smp_cpus; n++) { > > Object *cpuobj = object_new(machine->cpu_type); > > > > - /* > > - * By default A9 CPUs have EL3 enabled. This board does not currently > > - * support EL3 so the CPU EL3 property is disabled before realization. > > - */ > > - if (object_property_find(cpuobj, "has_el3")) { > > - object_property_set_bool(cpuobj, "has_el3", false, &error_fatal); > > - } > > - > > object_property_set_int(cpuobj, "midr", ZYNQ_BOARD_MIDR, > > &error_fatal); > > object_property_set_int(cpuobj, "reset-cbar", MPCORE_PERIPHBASE, > > thanks > -- PMM
On Fri, 30 Aug 2024 at 09:05, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > On Thu, Aug 29, 2024 at 01:50:02PM +0100, Peter Maydell wrote: > > On Wed, 28 Aug 2024 at 01:51, Sebastian Huber > > <sebastian.huber@embedded-brains.de> wrote: > > > > > > The system supports the Security Extensions (core and GIC). This change is > > > necessary to run tests which pass on the real hardware. > > > > > > Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de> > > > > (Added the maintainers to cc.) > > > > Does the system have any secure-only devices, RAM, etc? > > Yes, on real HW there are but I don't think we've modelled any of it yet. > There's TZ both on the SoC and also ability to create FPGA logic that > can issue secure/non-secure transactions. Here's an overview: > https://docs.amd.com/v/u/en-US/ug1019-zynq-trustzone > > The primary use-case for the upstream Zynq-7000 QEMU models has historically > been to run the Open Source SW stack from Linux (some times from u-boot) > and up. It's important that we don't break that. > > So as long as we add additional support without breaking direct Linux > boots, I think it's OK to incrementally enable missing pieces even > if there's not yet coherent support for firmware boot. > > In this case, IIUC, when doing direct Linux boot, TYPE_ARM_LINUX_BOOT_IF > will take care of the GIC setup for us. Yep, that's the way it's supposed to work. OK, let's enable this; it's the beginning of the release cycle for 9.2 so there's plenty of time to fix any problem that might get reported to us before release. Applied to target-arm.next, thanks. -- PMM
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c index 3c56b9abe1..37c234f5ab 100644 --- a/hw/arm/xilinx_zynq.c +++ b/hw/arm/xilinx_zynq.c @@ -219,14 +219,6 @@ static void zynq_init(MachineState *machine) for (n = 0; n < smp_cpus; n++) { Object *cpuobj = object_new(machine->cpu_type); - /* - * By default A9 CPUs have EL3 enabled. This board does not currently - * support EL3 so the CPU EL3 property is disabled before realization. - */ - if (object_property_find(cpuobj, "has_el3")) { - object_property_set_bool(cpuobj, "has_el3", false, &error_fatal); - } - object_property_set_int(cpuobj, "midr", ZYNQ_BOARD_MIDR, &error_fatal); object_property_set_int(cpuobj, "reset-cbar", MPCORE_PERIPHBASE,
The system supports the Security Extensions (core and GIC). This change is necessary to run tests which pass on the real hardware. Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de> --- hw/arm/xilinx_zynq.c | 8 -------- 1 file changed, 8 deletions(-)