Re: [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down

[Andrew Jones]

On Fri, Jan 22, 2021 at 10:09:35AM +0000, Peter Maydell wrote:
> On Fri, 22 Jan 2021 at 08:29, Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Wed, Jan 20, 2021 at 12:27:48PM +0300, Maxim Uvarov wrote:
> > > Add secure pl061 for reset/power down machine from
> > > the secure world (Arm Trusted Firmware). Connect it
> > > with gpio-pwr driver.
> > >
> > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> > > ---
> > >  hw/arm/Kconfig        |  1 +
> > >  hw/arm/virt.c         | 47 +++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/arm/virt.h |  2 ++
> > >  3 files changed, 50 insertions(+)
> > >
> > > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > > index 0a242e4c5d..13cc42dcc8 100644
> > > --- a/hw/arm/Kconfig
> > > +++ b/hw/arm/Kconfig
> > > @@ -17,6 +17,7 @@ config ARM_VIRT
> > >      select PL011 # UART
> > >      select PL031 # RTC
> > >      select PL061 # GPIO
> > > +    select GPIO_PWR
> > >      select PLATFORM_BUS
> > >      select SMBIOS
> > >      select VIRTIO_MMIO
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index c427ce5f81..060a5f492e 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -153,6 +153,7 @@ static const MemMapEntry base_memmap[] = {
> > >      [VIRT_ACPI_GED] =           { 0x09080000, ACPI_GED_EVT_SEL_LEN },
> > >      [VIRT_NVDIMM_ACPI] =        { 0x09090000, NVDIMM_ACPI_IO_LEN},
> > >      [VIRT_PVTIME] =             { 0x090a0000, 0x00010000 },
> > > +    [VIRT_SECURE_GPIO] =        { 0x090b0000, 0x00001000 },
> > >      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
> > >      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that 
> > > size */
> > >      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> > > @@ -841,6 +842,43 @@ static void create_gpio_keys(const VirtMachineState 
> > > *vms,
> > >                             "gpios", phandle, 3, 0);
> > >  }
> > >
> > > +#define SECURE_GPIO_POWEROFF 0
> > > +#define SECURE_GPIO_REBOOT   1
> > > +
> > > +static void create_gpio_pwr(const VirtMachineState *vms,
> >
> > This function is specific to the secure view. I think it should have
> > "secure" in its name.
> >
> > > +                            DeviceState *pl061_dev,
> > > +                            uint32_t phandle)
> > > +{
> > > +    DeviceState *gpio_pwr_dev;
> > > +
> > > +    /* gpio-pwr */
> > > +    gpio_pwr_dev = sysbus_create_simple("gpio-pwr", -1, NULL);
> >
> > Should this device be in secure memory?
> 
> It's not in any memory at all -- -1 as the address argument
> to sysbus_create_simple() means "no MMIO regions to map". The
> only way it's connected to the rest of the system is via  the
> secure-only PL061, so the NS world can't get at it.
> 
> (sysbus_create_simple("device", -1, NULL) is equivalent to:
>  dev = qdev_new("device");
>  sysbus_realize_and_unref(SYSBUS_DEVICE(dev), &error_fatal);
> )
>

Thanks, I should have looked more closely at that.

With the function name change to include "secure".

Reviewed-by: Andrew Jones <drjones@redhat.com>