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

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

[Maxim Uvarov]

 v4: rework patches accodring to Peter Maydells comments:
	- split patches on gpio-pwr driver and arm-virt integration.
	- start secure gpio only from virt-6.0.
	- rework qemu interface for gpio-pwr to use 2 named gpio.
	- put secure gpio to secure name space.
 v3: added missed include qemu/log.h for qemu_log(.. 
 v2: replace printf with qemu_log (Philippe Mathieu-Daudé)

This patch works together with ATF patch:
     https://github.com/muvarov/arm-trusted-firmware/commit/dd4401d8eb8e0f3018b335b81ce7a96d6cb16d0f 

Previus discussion for reboot issue was here:
     https://www.mail-archive.com/qemu-devel@nongnu.org/msg757705.html

Maxim Uvarov (2):
  hw: gpio: implement gpio-pwr driver for qemu reset/poweroff
  arm-virt: add secure pl061 for reset/power down

 hw/arm/Kconfig        |  1 +
 hw/arm/virt.c         | 40 +++++++++++++++++++++++++
 hw/gpio/Kconfig       |  3 ++
 hw/gpio/gpio_pwr.c    | 70 +++++++++++++++++++++++++++++++++++++++++++
 hw/gpio/meson.build   |  1 +
 include/hw/arm/virt.h |  3 ++
 6 files changed, 118 insertions(+)
 create mode 100644 hw/gpio/gpio_pwr.c

-- 
2.17.1

[PATCHv4 1/2] hw: gpio: implement gpio-pwr driver for qemu reset/poweroff

[Maxim Uvarov]

Implement gpio-pwr driver to allow reboot and poweroff machine.
This is simple driver with just 2 gpios lines. Current use case
is to reboot and poweroff virt machine in secure mode. Secure
pl066 gpio chip is needed for that.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 hw/gpio/Kconfig     |  3 ++
 hw/gpio/gpio_pwr.c  | 70 +++++++++++++++++++++++++++++++++++++++++++++
 hw/gpio/meson.build |  1 +
 3 files changed, 74 insertions(+)
 create mode 100644 hw/gpio/gpio_pwr.c

diff --git a/hw/gpio/Kconfig b/hw/gpio/Kconfig
index b6fdaa2586..f0e7405f6e 100644
--- a/hw/gpio/Kconfig
+++ b/hw/gpio/Kconfig
@@ -8,5 +8,8 @@ config PL061
 config GPIO_KEY
     bool
 
+config GPIO_PWR
+    bool
+
 config SIFIVE_GPIO
     bool
diff --git a/hw/gpio/gpio_pwr.c b/hw/gpio/gpio_pwr.c
new file mode 100644
index 0000000000..8ed8d5d24f
--- /dev/null
+++ b/hw/gpio/gpio_pwr.c
@@ -0,0 +1,70 @@
+/*
+ * GPIO qemu power controller
+ *
+ * Copyright (c) 2020 Linaro Limited
+ *
+ * Author: Maxim Uvarov <maxim.uvarov@linaro.org>
+ *
+ * Virtual gpio driver which can be used on top of pl061
+ * to reboot and shutdown qemu virtual machine. One of use
+ * case is gpio driver for secure world application (ARM
+ * Trusted Firmware.).
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+/*
+ * QEMU interface:
+ * two named input GPIO lines:
+ *   'reset' : when asserted, trigger system reset
+ *   'shutdown' : when asserted, trigger system shutdown
+ */
+
+#include "qemu/osdep.h"
+#include "hw/sysbus.h"
+#include "sysemu/runstate.h"
+
+#define TYPE_GPIOPWR "gpio-pwr"
+OBJECT_DECLARE_SIMPLE_TYPE(GPIO_PWR_State, GPIOPWR)
+
+struct GPIO_PWR_State {
+    SysBusDevice parent_obj;
+};
+
+static void gpio_pwr_reset(void *opaque, int n, int level)
+{
+    if (!level) {
+        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+    }
+}
+
+static void gpio_pwr_shutdown(void *opaque, int n, int level)
+{
+    if (!level) {
+        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+    }
+}
+
+static void gpio_pwr_init(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+
+    qdev_init_gpio_in_named(dev, gpio_pwr_reset, "reset", 1);
+    qdev_init_gpio_in_named(dev, gpio_pwr_shutdown, "shutdown", 1);
+}
+
+static const TypeInfo gpio_pwr_info = {
+    .name          = TYPE_GPIOPWR,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(GPIO_PWR_State),
+    .instance_init = gpio_pwr_init,
+};
+
+static void gpio_pwr_register_types(void)
+{
+    type_register_static(&gpio_pwr_info);
+}
+
+type_init(gpio_pwr_register_types)
diff --git a/hw/gpio/meson.build b/hw/gpio/meson.build
index 5c0a7d7b95..79568f00ce 100644
--- a/hw/gpio/meson.build
+++ b/hw/gpio/meson.build
@@ -1,5 +1,6 @@
 softmmu_ss.add(when: 'CONFIG_E500', if_true: files('mpc8xxx.c'))
 softmmu_ss.add(when: 'CONFIG_GPIO_KEY', if_true: files('gpio_key.c'))
+softmmu_ss.add(when: 'CONFIG_GPIO_PWR', if_true: files('gpio_pwr.c'))
 softmmu_ss.add(when: 'CONFIG_MAX7310', if_true: files('max7310.c'))
 softmmu_ss.add(when: 'CONFIG_PL061', if_true: files('pl061.c'))
 softmmu_ss.add(when: 'CONFIG_PUV3', if_true: files('puv3_gpio.c'))
-- 
2.17.1

Re: [PATCHv4 1/2] hw: gpio: implement gpio-pwr driver for qemu reset/poweroff

[Hao Wu via]

[-- Attachment #1: Type: text/plain, Size: 3636 bytes --]

On Tue, Jan 12, 2021 at 6:36 AM Maxim Uvarov <maxim.uvarov@linaro.org>
wrote:

> Implement gpio-pwr driver to allow reboot and poweroff machine.
> This is simple driver with just 2 gpios lines. Current use case
> is to reboot and poweroff virt machine in secure mode. Secure
> pl066 gpio chip is needed for that.
>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>
Reviewed-by: Hao Wu <wuhaotsh@google.com>

> ---
>  hw/gpio/Kconfig     |  3 ++
>  hw/gpio/gpio_pwr.c  | 70 +++++++++++++++++++++++++++++++++++++++++++++
>  hw/gpio/meson.build |  1 +
>  3 files changed, 74 insertions(+)
>  create mode 100644 hw/gpio/gpio_pwr.c
>
> diff --git a/hw/gpio/Kconfig b/hw/gpio/Kconfig
> index b6fdaa2586..f0e7405f6e 100644
> --- a/hw/gpio/Kconfig
> +++ b/hw/gpio/Kconfig
> @@ -8,5 +8,8 @@ config PL061
>  config GPIO_KEY
>      bool
>
> +config GPIO_PWR
> +    bool
> +
>  config SIFIVE_GPIO
>      bool
> diff --git a/hw/gpio/gpio_pwr.c b/hw/gpio/gpio_pwr.c
> new file mode 100644
> index 0000000000..8ed8d5d24f
> --- /dev/null
> +++ b/hw/gpio/gpio_pwr.c
> @@ -0,0 +1,70 @@
> +/*
> + * GPIO qemu power controller
> + *
> + * Copyright (c) 2020 Linaro Limited
> + *
> + * Author: Maxim Uvarov <maxim.uvarov@linaro.org>
> + *
> + * Virtual gpio driver which can be used on top of pl061
> + * to reboot and shutdown qemu virtual machine. One of use
> + * case is gpio driver for secure world application (ARM
> + * Trusted Firmware.).
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> + * See the COPYING file in the top-level directory.
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +/*
> + * QEMU interface:
> + * two named input GPIO lines:
> + *   'reset' : when asserted, trigger system reset
> + *   'shutdown' : when asserted, trigger system shutdown
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/sysbus.h"
> +#include "sysemu/runstate.h"
> +
> +#define TYPE_GPIOPWR "gpio-pwr"
> +OBJECT_DECLARE_SIMPLE_TYPE(GPIO_PWR_State, GPIOPWR)
> +
> +struct GPIO_PWR_State {
> +    SysBusDevice parent_obj;
> +};
> +
> +static void gpio_pwr_reset(void *opaque, int n, int level)
> +{
> +    if (!level) {
> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +    }
> +}
> +
> +static void gpio_pwr_shutdown(void *opaque, int n, int level)
> +{
> +    if (!level) {
> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +    }
> +}
> +
> +static void gpio_pwr_init(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +
> +    qdev_init_gpio_in_named(dev, gpio_pwr_reset, "reset", 1);
> +    qdev_init_gpio_in_named(dev, gpio_pwr_shutdown, "shutdown", 1);
> +}
> +
> +static const TypeInfo gpio_pwr_info = {
> +    .name          = TYPE_GPIOPWR,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(GPIO_PWR_State),
> +    .instance_init = gpio_pwr_init,
> +};
> +
> +static void gpio_pwr_register_types(void)
> +{
> +    type_register_static(&gpio_pwr_info);
> +}
> +
> +type_init(gpio_pwr_register_types)
> diff --git a/hw/gpio/meson.build b/hw/gpio/meson.build
> index 5c0a7d7b95..79568f00ce 100644
> --- a/hw/gpio/meson.build
> +++ b/hw/gpio/meson.build
> @@ -1,5 +1,6 @@
>  softmmu_ss.add(when: 'CONFIG_E500', if_true: files('mpc8xxx.c'))
>  softmmu_ss.add(when: 'CONFIG_GPIO_KEY', if_true: files('gpio_key.c'))
> +softmmu_ss.add(when: 'CONFIG_GPIO_PWR', if_true: files('gpio_pwr.c'))
>  softmmu_ss.add(when: 'CONFIG_MAX7310', if_true: files('max7310.c'))
>  softmmu_ss.add(when: 'CONFIG_PL061', if_true: files('pl061.c'))
>  softmmu_ss.add(when: 'CONFIG_PUV3', if_true: files('puv3_gpio.c'))
> --
> 2.17.1
>
>
>

[-- Attachment #2: Type: text/html, Size: 4872 bytes --]

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

[Maxim Uvarov]

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 },
     [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));
+}
+
 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;
+    }
+
     /* 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

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

[Andrew Jones]

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 

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

[Peter Maydell]

On Tue, 12 Jan 2021 at 15:35, Andrew Jones <drjones@redhat.com> wrote:
>
> 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.

> > +    /* 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.

We should be adding something to the DTB, yes. Look at
how create_uart() does this -- you set the 'status' and
'secure-status' properties to indicate that the device is
secure-world only.



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

We're just following the same pattern as vmc->no_its/vms->its,
aren't we ?

thanks
-- PMM

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

[Andrew Jones]

On Tue, Jan 12, 2021 at 04:00:23PM +0000, Peter Maydell wrote:
> On Tue, 12 Jan 2021 at 15:35, Andrew Jones <drjones@redhat.com> wrote:
> >
> > 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.
> 
> > > +    /* 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.
> 
> We should be adding something to the DTB, yes. Look at
> how create_uart() does this -- you set the 'status' and
> 'secure-status' properties to indicate that the device is
> secure-world only.
> 
> 
> 
> > > +    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() ?
> 
> We're just following the same pattern as vmc->no_its/vms->its,
> aren't we ?
>

'its' is a property that can be changed on the command line. Unless
we want to be able to manage 'secure-gpio' separately from 'secure',
then I think vmc->its plus 'secure' should be sufficient. We don't
always need both vmc and vms state, see 'no_ged'.

Thanks,
drew

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

[Andrew Jones]

On Tue, Jan 12, 2021 at 11:25:30AM -0500, Andrew Jones wrote:
> On Tue, Jan 12, 2021 at 04:00:23PM +0000, Peter Maydell wrote:
> > On Tue, 12 Jan 2021 at 15:35, Andrew Jones <drjones@redhat.com> wrote:
> > >
> > > 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.
> > 
> > > > +    /* 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.
> > 
> > We should be adding something to the DTB, yes. Look at
> > how create_uart() does this -- you set the 'status' and
> > 'secure-status' properties to indicate that the device is
> > secure-world only.
> > 
> > 
> > 
> > > > +    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() ?
> > 
> > We're just following the same pattern as vmc->no_its/vms->its,
> > aren't we ?
> >
> 
> 'its' is a property that can be changed on the command line. Unless
> we want to be able to manage 'secure-gpio' separately from 'secure',
> then I think vmc->its plus 'secure' should be sufficient. We don't

I meant to write 'vmc->no_secure_gpio and vms->secure' here.

Thanks,
drew

> always need both vmc and vms state, see 'no_ged'.
> 
> Thanks,
> drew

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

[Maxim Uvarov]

- the same size for secure and non secure gpio. Arm doc says that
secure memory is also split on 4k pages. So one page here has to be
ok.
- will add dtb.
- I think then less options is better. So I will remove
vmc->secure_gpio flag and keep only vmc flag.

Regards,
Maxim.

On Tue, 12 Jan 2021 at 19:28, Andrew Jones <drjones@redhat.com> wrote:
>
> On Tue, Jan 12, 2021 at 11:25:30AM -0500, Andrew Jones wrote:
> > On Tue, Jan 12, 2021 at 04:00:23PM +0000, Peter Maydell wrote:
> > > On Tue, 12 Jan 2021 at 15:35, Andrew Jones <drjones@redhat.com> wrote:
> > > >
> > > > 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.
> > >
> > > > > +    /* 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.
> > >
> > > We should be adding something to the DTB, yes. Look at
> > > how create_uart() does this -- you set the 'status' and
> > > 'secure-status' properties to indicate that the device is
> > > secure-world only.
> > >
> > >
> > >
> > > > > +    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() ?
> > >
> > > We're just following the same pattern as vmc->no_its/vms->its,
> > > aren't we ?
> > >
> >
> > 'its' is a property that can be changed on the command line. Unless
> > we want to be able to manage 'secure-gpio' separately from 'secure',
> > then I think vmc->its plus 'secure' should be sufficient. We don't
>
> I meant to write 'vmc->no_secure_gpio and vms->secure' here.
>
> Thanks,
> drew
>
> > always need both vmc and vms state, see 'no_ged'.
> >
> > Thanks,
> > drew
>

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

[Andrew Jones]

On Wed, Jan 13, 2021 at 10:30:47AM +0300, Maxim Uvarov wrote:
> - the same size for secure and non secure gpio. Arm doc says that
> secure memory is also split on 4k pages. So one page here has to be
> ok.

To be clear, does that means 4k pages must be used? I'm not concerned
with the size, but the alignment. If it's possible to use larger page
sizes with secure memory, then we need to align to the maximum page
size that may be used.

Thanks,
drew


> - will add dtb.
> - I think then less options is better. So I will remove
> vmc->secure_gpio flag and keep only vmc flag.
> 
> Regards,
> Maxim.
> 
> On Tue, 12 Jan 2021 at 19:28, Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Tue, Jan 12, 2021 at 11:25:30AM -0500, Andrew Jones wrote:
> > > On Tue, Jan 12, 2021 at 04:00:23PM +0000, Peter Maydell wrote:
> > > > On Tue, 12 Jan 2021 at 15:35, Andrew Jones <drjones@redhat.com> wrote:
> > > > >
> > > > > 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.
> > > >
> > > > > > +    /* 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.
> > > >
> > > > We should be adding something to the DTB, yes. Look at
> > > > how create_uart() does this -- you set the 'status' and
> > > > 'secure-status' properties to indicate that the device is
> > > > secure-world only.
> > > >
> > > >
> > > >
> > > > > > +    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() ?
> > > >
> > > > We're just following the same pattern as vmc->no_its/vms->its,
> > > > aren't we ?
> > > >
> > >
> > > 'its' is a property that can be changed on the command line. Unless
> > > we want to be able to manage 'secure-gpio' separately from 'secure',
> > > then I think vmc->its plus 'secure' should be sufficient. We don't
> >
> > I meant to write 'vmc->no_secure_gpio and vms->secure' here.
> >
> > Thanks,
> > drew
> >
> > > always need both vmc and vms state, see 'no_ged'.
> > >
> > > Thanks,
> > > drew
> >
> 

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

[Peter Maydell]

On Thu, 14 Jan 2021 at 00:04, Andrew Jones <drjones@redhat.com> wrote:
>
> On Wed, Jan 13, 2021 at 10:30:47AM +0300, Maxim Uvarov wrote:
> > - the same size for secure and non secure gpio. Arm doc says that
> > secure memory is also split on 4k pages. So one page here has to be
> > ok.
>
> To be clear, does that means 4k pages must be used? I'm not concerned
> with the size, but the alignment. If it's possible to use larger page
> sizes with secure memory, then we need to align to the maximum page
> size that may be used.

I think we should just align on 64K, to be more future-proof.
Even if secure software today uses 4K pages, it doesn't hurt
to align the device such that some hypothetical future 64K
page using secure software can use it.

thanks
-- PMM

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

[Maxim Uvarov]

On Thu, 14 Jan 2021 at 12:50, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 14 Jan 2021 at 00:04, Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Wed, Jan 13, 2021 at 10:30:47AM +0300, Maxim Uvarov wrote:
> > > - the same size for secure and non secure gpio. Arm doc says that
> > > secure memory is also split on 4k pages. So one page here has to be
> > > ok.
> >
> > To be clear, does that means 4k pages must be used? I'm not concerned
> > with the size, but the alignment. If it's possible to use larger page
> > sizes with secure memory, then we need to align to the maximum page
> > size that may be used.
>
> I think we should just align on 64K, to be more future-proof.
> Even if secure software today uses 4K pages, it doesn't hurt
> to align the device such that some hypothetical future 64K
> page using secure software can use it.
>
> thanks
> -- PMM

Does that mean that in that case you need all regions to be 64k
aligned? I mean secure and non-secure.
Has anybody tested 64k pages under qemu?
    [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 }
    [VIRT_UART] =               { 0x09000000, 0x00001000 },
    [VIRT_RTC] =                { 0x09010000, 0x00001000 },
    [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
    [VIRT_SECURE_GPIO] =        { 0x09031000, 0x00001000 },
    [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
   [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },

Maxim.

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

[Maxim Uvarov]

On Thu, 14 Jan 2021 at 14:22, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
> On Thu, 14 Jan 2021 at 12:50, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Thu, 14 Jan 2021 at 00:04, Andrew Jones <drjones@redhat.com> wrote:
> > >
> > > On Wed, Jan 13, 2021 at 10:30:47AM +0300, Maxim Uvarov wrote:
> > > > - the same size for secure and non secure gpio. Arm doc says that
> > > > secure memory is also split on 4k pages. So one page here has to be
> > > > ok.
> > >
> > > To be clear, does that means 4k pages must be used? I'm not concerned
> > > with the size, but the alignment. If it's possible to use larger page
> > > sizes with secure memory, then we need to align to the maximum page
> > > size that may be used.
> >
> > I think we should just align on 64K, to be more future-proof.
> > Even if secure software today uses 4K pages, it doesn't hurt
> > to align the device such that some hypothetical future 64K
> > page using secure software can use it.
> >
> > thanks
> > -- PMM
>
> Does that mean that in that case you need all regions to be 64k
> aligned? I mean secure and non-secure.
> Has anybody tested 64k pages under qemu?
>     [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 }
>     [VIRT_UART] =               { 0x09000000, 0x00001000 },
>     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
>     [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
>     [VIRT_SECURE_GPIO] =        { 0x09031000, 0x00001000 },
>     [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
>    [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>
> Maxim.

I.e. I see comment:
 * Note that devices should generally be placed at multiples of 0x10000,
 * to accommodate guests using 64K pages.
 */

but it's not clear why UART, RTC and GPIO is not aligned to 64k.

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

[Peter Maydell]

On Thu, 14 Jan 2021 at 11:24, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
> On Thu, 14 Jan 2021 at 14:22, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> > Does that mean that in that case you need all regions to be 64k
> > aligned? I mean secure and non-secure.
> > Has anybody tested 64k pages under qemu?
> >     [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 }
> >     [VIRT_UART] =               { 0x09000000, 0x00001000 },
> >     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
> >     [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
> >     [VIRT_SECURE_GPIO] =        { 0x09031000, 0x00001000 },
> >     [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
> >    [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
> >
> > Maxim.
>
> I.e. I see comment:
>  * Note that devices should generally be placed at multiples of 0x10000,
>  * to accommodate guests using 64K pages.
>  */
>
> but it's not clear why UART, RTC and GPIO is not aligned to 64k.

Er, 0x09000000, 0x09010000 and 0x09030000 are all 64K aligned addresses.

thanks
-- PMM

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

[Maxim Uvarov]

On Thu, 14 Jan 2021 at 14:48, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 14 Jan 2021 at 11:24, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> >
> > On Thu, 14 Jan 2021 at 14:22, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> > > Does that mean that in that case you need all regions to be 64k
> > > aligned? I mean secure and non-secure.
> > > Has anybody tested 64k pages under qemu?
> > >     [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 }
> > >     [VIRT_UART] =               { 0x09000000, 0x00001000 },
> > >     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
> > >     [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
> > >     [VIRT_SECURE_GPIO] =        { 0x09031000, 0x00001000 },
> > >     [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
> > >    [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
> > >
> > > Maxim.
> >
> > I.e. I see comment:
> >  * Note that devices should generally be placed at multiples of 0x10000,
> >  * to accommodate guests using 64K pages.
> >  */
> >
> > but it's not clear why UART, RTC and GPIO is not aligned to 64k.
>
> Er, 0x09000000, 0x09010000 and 0x09030000 are all 64K aligned addresses.
>
> thanks
> -- PMM

thanks, will send an updated version.