[PATCH] hw/arm/xilinx_zynq: Enable Security Extensions

[PATCH] hw/arm/xilinx_zynq: Enable Security Extensions

[Sebastian Huber]

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(-)

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,
-- 
2.35.3

Re: [PATCH] hw/arm/xilinx_zynq: Enable Security Extensions

[Peter Maydell]

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

Re: [PATCH] hw/arm/xilinx_zynq: Enable Security Extensions

[Sebastian Huber]

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.

-- 
embedded brains GmbH & Co. KG
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/

Re: [PATCH] hw/arm/xilinx_zynq: Enable Security Extensions

[Edgar E. Iglesias]

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

Re: [PATCH] hw/arm/xilinx_zynq: Enable Security Extensions

[Peter Maydell]

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