Re: [PATCHv4 2/2] arm-virt: add secure pl061 for reset/power down

[Andrew Jones <drjones@redhat.com>]

On Tue, Jan 12, 2021 at 05:30:58PM +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         | 40 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/virt.h |  3 +++
>  3 files changed, 44 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 96985917d3..19605390c2 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -147,6 +147,7 @@ static const MemMapEntry base_memmap[] = {
>      [VIRT_RTC] =                { 0x09010000, 0x00001000 },
>      [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
>      [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
> +    [VIRT_SECURE_GPIO] =        { 0x09031000, 0x00001000 },

Does secure world require 4K pages?

>      [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
>      [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
>      [VIRT_PCDIMM_ACPI] =        { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
> @@ -864,6 +865,32 @@ static void create_gpio(const VirtMachineState *vms)
>      g_free(nodename);
>  }
>  
> +#define ATF_GPIO_POWEROFF 3
> +#define ATF_GPIO_REBOOT   4
> +
> +static void create_gpio_secure(const VirtMachineState *vms, MemoryRegion *mem)
> +{
> +    DeviceState *gpio_pwr_dev;
> +    SysBusDevice *s;
> +    hwaddr base = vms->memmap[VIRT_SECURE_GPIO].base;
> +    DeviceState *pl061_dev;
> +
> +    /* Secure pl061 */
> +    pl061_dev = qdev_new("pl061");
> +    s = SYS_BUS_DEVICE(pl061_dev);
> +    sysbus_realize_and_unref(s, &error_fatal);
> +    memory_region_add_subregion(mem, base, sysbus_mmio_get_region(s, 0));
> +
> +    /* gpio-pwr */
> +    gpio_pwr_dev = sysbus_create_simple("gpio-pwr", -1, NULL);
> +
> +    /* connect secure pl061 to gpio-pwr */
> +    qdev_connect_gpio_out(pl061_dev, ATF_GPIO_POWEROFF,
> +                          qdev_get_gpio_in_named(gpio_pwr_dev, "reset", 0));
> +    qdev_connect_gpio_out(pl061_dev, ATF_GPIO_REBOOT,
> +                          qdev_get_gpio_in_named(gpio_pwr_dev, "shutdown", 0));

I don't know anything about secure world, but it seems odd that we don't
need to add anything to the DTB.

> +}
> +
>  static void create_virtio_devices(const VirtMachineState *vms)
>  {
>      int i;
> @@ -1993,6 +2020,10 @@ static void machvirt_init(MachineState *machine)
>          create_gpio(vms);
>      }
>  
> +    if (vms->secure && vms->secure_gpio) {
> +        create_gpio_secure(vms, secure_sysmem);
> +    }
> +
>       /* connect powerdown request */
>       vms->powerdown_notifier.notify = virt_powerdown_req;
>       qemu_register_powerdown_notifier(&vms->powerdown_notifier);
> @@ -2567,6 +2598,12 @@ static void virt_instance_init(Object *obj)
>          vms->its = true;
>      }
>  
> +    if (vmc->no_secure_gpio) {
> +        vms->secure_gpio = false;
> +    }  else {
> +        vms->secure_gpio = true;
> +    }

nit: vms->secure_gpio = !vmc->no_secure_gpio

But do we even need vms->secure_gpio? Why not just do

 if (vms->secure && !vmc->no_secure_gpio) {
     create_gpio_secure(vms, secure_sysmem);
 }

in machvirt_init() ?

> +
>      /* Default disallows iommu instantiation */
>      vms->iommu = VIRT_IOMMU_NONE;
>  
> @@ -2608,8 +2645,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..a140e75444 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 {
> @@ -136,6 +138,7 @@ struct VirtMachineState {
>      FWCfgState *fw_cfg;
>      PFlashCFI01 *flash[2];
>      bool secure;
> +    bool secure_gpio;
>      bool highmem;
>      bool highmem_ecam;
>      bool its;
> -- 
> 2.17.1
> 
>

Thanks,
drew