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

[Andrew Jones]

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?

> +
> +    /* connect secure pl061 to gpio-pwr */
> +    qdev_connect_gpio_out(pl061_dev, SECURE_GPIO_REBOOT,
> +                          qdev_get_gpio_in_named(gpio_pwr_dev, "reset", 0));
> +    qdev_connect_gpio_out(pl061_dev, SECURE_GPIO_POWEROFF,
> +                          qdev_get_gpio_in_named(gpio_pwr_dev, "shutdown", 
> 0));
> +
> +    qemu_fdt_add_subnode(vms->fdt, "/gpio-poweroff");
> +    qemu_fdt_setprop_string(vms->fdt, "/gpio-poweroff", "compatible",
> +                            "gpio-poweroff");
> +    qemu_fdt_setprop_cells(vms->fdt, "/gpio-poweroff",
> +                           "gpios", phandle, SECURE_GPIO_POWEROFF, 0);
> +    qemu_fdt_setprop_string(vms->fdt, "/gpio-poweroff", "status", 
> "disabled");
> +    qemu_fdt_setprop_string(vms->fdt, "/gpio-poweroff", "secure-status",
> +                            "okay");
> +
> +    qemu_fdt_add_subnode(vms->fdt, "/gpio-restart");
> +    qemu_fdt_setprop_string(vms->fdt, "/gpio-restart", "compatible",
> +                            "gpio-restart");
> +    qemu_fdt_setprop_cells(vms->fdt, "/gpio-restart",
> +                           "gpios", phandle, SECURE_GPIO_REBOOT, 0);
> +    qemu_fdt_setprop_string(vms->fdt, "/gpio-restart", "status", "disabled");
> +    qemu_fdt_setprop_string(vms->fdt, "/gpio-restart", "secure-status",
> +                            "okay");
> +}
> +
>  static void create_gpio_devices(const VirtMachineState *vms, int gpio,
>                                  MemoryRegion *mem)
>  {
> @@ -883,6 +921,8 @@ static void create_gpio_devices(const VirtMachineState 
> *vms, int gpio,
>      /* Child gpio devices */
>      if (gpio == VIRT_GPIO) {
>          create_gpio_keys(vms, pl061_dev, phandle);
> +    } else {
> +        create_gpio_pwr(vms, pl061_dev, phandle);
>      }
>  }
>  
> @@ -2015,6 +2055,10 @@ static void machvirt_init(MachineState *machine)
>          create_gpio_devices(vms, VIRT_GPIO, sysmem);
>      }
>  
> +    if (vms->secure && !vmc->no_secure_gpio) {
> +        create_gpio_devices(vms, VIRT_SECURE_GPIO, secure_sysmem);
> +    }
> +
>       /* connect powerdown request */
>       vms->powerdown_notifier.notify = virt_powerdown_req;
>       qemu_register_powerdown_notifier(&vms->powerdown_notifier);
> @@ -2630,8 +2674,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(6, 0)
>  
>  static void virt_machine_5_2_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>      virt_machine_6_0_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_5_2, hw_compat_5_2_len);
> +    vmc->no_secure_gpio = true;
>  }
>  DEFINE_VIRT_MACHINE(5, 2)
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index abf54fab49..6f6c85ffcf 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -81,6 +81,7 @@ enum {
>      VIRT_GPIO,
>      VIRT_SECURE_UART,
>      VIRT_SECURE_MEM,
> +    VIRT_SECURE_GPIO,
>      VIRT_PCDIMM_ACPI,
>      VIRT_ACPI_GED,
>      VIRT_NVDIMM_ACPI,
> @@ -127,6 +128,7 @@ struct VirtMachineClass {
>      bool kvm_no_adjvtime;
>      bool no_kvm_steal_time;
>      bool acpi_expose_flash;
> +    bool no_secure_gpio;
>  };
>  
>  struct VirtMachineState {
> -- 
> 2.17.1
> 
>

Thanks,
drew