All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCHv3] arm-virt: add secure pl061 for reset/power down
@ 2021-01-06 16:34 Maxim Uvarov
  2021-01-06 16:36 ` Maxim Uvarov
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Maxim Uvarov @ 2021-01-06 16:34 UTC (permalink / raw)
  To: qemu-arm, qemu-devel
  Cc: peter.maydell, Maxim Uvarov, f4bug, Jose.Marinho, tf-a

Add secure pl061 for reset/power down machine from
the secure world (Arm Trusted Firmware).
Use the same gpio 3 and gpio 4 which were used by
non acpi variant of linux power control gpios.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 v3: added missed include qemu/log.h for qemu_log(.. 
 v2: replace printf with qemu_log (Philippe Mathieu-Daudé)

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

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..eff0345303 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 },
@@ -189,6 +190,7 @@ static const int a15irqmap[] = {
     [VIRT_GPIO] = 7,
     [VIRT_SECURE_UART] = 8,
     [VIRT_ACPI_GED] = 9,
+    [VIRT_SECURE_GPIO] = 10,
     [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
     [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
     [VIRT_SMMU] = 74,    /* ...to 74 + NUM_SMMU_IRQS - 1 */
@@ -864,6 +866,24 @@ static void create_gpio(const VirtMachineState *vms)
     g_free(nodename);
 }
 
+static void create_gpio_secure(const VirtMachineState *vms)
+{
+    DeviceState *pl061_dev;
+    static DeviceState *gpio_pwr_dev;
+
+    hwaddr base = vms->memmap[VIRT_SECURE_GPIO].base;
+    int irq = vms->irqmap[VIRT_SECURE_GPIO];
+
+    pl061_dev = sysbus_create_simple("pl061", base,
+                                     qdev_get_gpio_in(vms->gic, irq));
+
+    gpio_pwr_dev = sysbus_create_simple("gpio-pwr", -1,
+                                        qdev_get_gpio_in(pl061_dev, 3));
+
+    qdev_connect_gpio_out(pl061_dev, 3, qdev_get_gpio_in(gpio_pwr_dev, 3));
+    qdev_connect_gpio_out(pl061_dev, 4, qdev_get_gpio_in(gpio_pwr_dev, 4));
+}
+
 static void create_virtio_devices(const VirtMachineState *vms)
 {
     int i;
@@ -1993,6 +2013,10 @@ static void machvirt_init(MachineState *machine)
         create_gpio(vms);
     }
 
+    if (vms->secure) {
+        create_gpio_secure(vms);
+    }
+
      /* connect powerdown request */
      vms->powerdown_notifier.notify = virt_powerdown_req;
      qemu_register_powerdown_notifier(&vms->powerdown_notifier);
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..0d0680c9f7
--- /dev/null
+++ b/hw/gpio/gpio_pwr.c
@@ -0,0 +1,85 @@
+/*
+ * 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
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/irq.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;
+    qemu_irq irq;
+};
+
+static void gpio_pwr_set_irq(void *opaque, int irq, int level)
+{
+    GPIO_PWR_State *s = (GPIO_PWR_State *)opaque;
+
+    qemu_set_irq(s->irq, 1);
+
+    if (level) {
+        return;
+    }
+
+    switch (irq) {
+    case 3:
+        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+        break;
+    case 4:
+        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "qemu; gpio_pwr: unknown interrupt %d lvl %d\n",
+                      irq, level);
+    }
+}
+
+
+static void gpio_pwr_realize(DeviceState *dev, Error **errp)
+{
+    GPIO_PWR_State *s = GPIOPWR(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    sysbus_init_irq(sbd, &s->irq);
+    qdev_init_gpio_in(dev, gpio_pwr_set_irq, 8);
+}
+
+static void gpio_pwr_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = gpio_pwr_realize;
+}
+
+static const TypeInfo gpio_pwr_info = {
+    .name          = TYPE_GPIOPWR,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(GPIO_PWR_State),
+    .class_init    = gpio_pwr_class_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'))
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index abf54fab49..77a4523cc7 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,
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCHv3] arm-virt: add secure pl061 for reset/power down
  2021-01-06 16:34 [PATCHv3] arm-virt: add secure pl061 for reset/power down Maxim Uvarov
@ 2021-01-06 16:36 ` Maxim Uvarov
  2021-01-06 16:47 ` Philippe Mathieu-Daudé
  2021-01-08 14:31 ` Peter Maydell
  2 siblings, 0 replies; 4+ messages in thread
From: Maxim Uvarov @ 2021-01-06 16:36 UTC (permalink / raw)
  To: qemu-arm, QEMU Developers; +Cc: Peter Maydell, f4bug, Jose Marinho, tf-a

Please skip v2 and use v3. I had to check that one line change code
compiles. qemu_log() requires include header for that function.

Best regards,
Maxim.

On Wed, 6 Jan 2021 at 19:34, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
> Add secure pl061 for reset/power down machine from
> the secure world (Arm Trusted Firmware).
> Use the same gpio 3 and gpio 4 which were used by
> non acpi variant of linux power control gpios.
>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  v3: added missed include qemu/log.h for qemu_log(..
>  v2: replace printf with qemu_log (Philippe Mathieu-Daudé)
>
>  hw/arm/Kconfig        |  1 +
>  hw/arm/virt.c         | 24 ++++++++++++
>  hw/gpio/Kconfig       |  3 ++
>  hw/gpio/gpio_pwr.c    | 85 +++++++++++++++++++++++++++++++++++++++++++
>  hw/gpio/meson.build   |  1 +
>  include/hw/arm/virt.h |  1 +
>  6 files changed, 115 insertions(+)
>  create mode 100644 hw/gpio/gpio_pwr.c
>
> 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..eff0345303 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 },
> @@ -189,6 +190,7 @@ static const int a15irqmap[] = {
>      [VIRT_GPIO] = 7,
>      [VIRT_SECURE_UART] = 8,
>      [VIRT_ACPI_GED] = 9,
> +    [VIRT_SECURE_GPIO] = 10,
>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>      [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
>      [VIRT_SMMU] = 74,    /* ...to 74 + NUM_SMMU_IRQS - 1 */
> @@ -864,6 +866,24 @@ static void create_gpio(const VirtMachineState *vms)
>      g_free(nodename);
>  }
>
> +static void create_gpio_secure(const VirtMachineState *vms)
> +{
> +    DeviceState *pl061_dev;
> +    static DeviceState *gpio_pwr_dev;
> +
> +    hwaddr base = vms->memmap[VIRT_SECURE_GPIO].base;
> +    int irq = vms->irqmap[VIRT_SECURE_GPIO];
> +
> +    pl061_dev = sysbus_create_simple("pl061", base,
> +                                     qdev_get_gpio_in(vms->gic, irq));
> +
> +    gpio_pwr_dev = sysbus_create_simple("gpio-pwr", -1,
> +                                        qdev_get_gpio_in(pl061_dev, 3));
> +
> +    qdev_connect_gpio_out(pl061_dev, 3, qdev_get_gpio_in(gpio_pwr_dev, 3));
> +    qdev_connect_gpio_out(pl061_dev, 4, qdev_get_gpio_in(gpio_pwr_dev, 4));
> +}
> +
>  static void create_virtio_devices(const VirtMachineState *vms)
>  {
>      int i;
> @@ -1993,6 +2013,10 @@ static void machvirt_init(MachineState *machine)
>          create_gpio(vms);
>      }
>
> +    if (vms->secure) {
> +        create_gpio_secure(vms);
> +    }
> +
>       /* connect powerdown request */
>       vms->powerdown_notifier.notify = virt_powerdown_req;
>       qemu_register_powerdown_notifier(&vms->powerdown_notifier);
> 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..0d0680c9f7
> --- /dev/null
> +++ b/hw/gpio/gpio_pwr.c
> @@ -0,0 +1,85 @@
> +/*
> + * 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
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/irq.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;
> +    qemu_irq irq;
> +};
> +
> +static void gpio_pwr_set_irq(void *opaque, int irq, int level)
> +{
> +    GPIO_PWR_State *s = (GPIO_PWR_State *)opaque;
> +
> +    qemu_set_irq(s->irq, 1);
> +
> +    if (level) {
> +        return;
> +    }
> +
> +    switch (irq) {
> +    case 3:
> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +        break;
> +    case 4:
> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "qemu; gpio_pwr: unknown interrupt %d lvl %d\n",
> +                      irq, level);
> +    }
> +}
> +
> +
> +static void gpio_pwr_realize(DeviceState *dev, Error **errp)
> +{
> +    GPIO_PWR_State *s = GPIOPWR(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +
> +    sysbus_init_irq(sbd, &s->irq);
> +    qdev_init_gpio_in(dev, gpio_pwr_set_irq, 8);
> +}
> +
> +static void gpio_pwr_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = gpio_pwr_realize;
> +}
> +
> +static const TypeInfo gpio_pwr_info = {
> +    .name          = TYPE_GPIOPWR,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(GPIO_PWR_State),
> +    .class_init    = gpio_pwr_class_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'))
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index abf54fab49..77a4523cc7 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,
> --
> 2.17.1
>


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCHv3] arm-virt: add secure pl061 for reset/power down
  2021-01-06 16:34 [PATCHv3] arm-virt: add secure pl061 for reset/power down Maxim Uvarov
  2021-01-06 16:36 ` Maxim Uvarov
@ 2021-01-06 16:47 ` Philippe Mathieu-Daudé
  2021-01-08 14:31 ` Peter Maydell
  2 siblings, 0 replies; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-01-06 16:47 UTC (permalink / raw)
  To: Maxim Uvarov, qemu-arm, qemu-devel; +Cc: peter.maydell, Jose.Marinho, tf-a

On 1/6/21 5:34 PM, Maxim Uvarov wrote:
> Add secure pl061 for reset/power down machine from
> the secure world (Arm Trusted Firmware).
> Use the same gpio 3 and gpio 4 which were used by
> non acpi variant of linux power control gpios.
> 
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  v3: added missed include qemu/log.h for qemu_log(.. 
>  v2: replace printf with qemu_log (Philippe Mathieu-Daudé)

Thanks!

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
>  hw/arm/Kconfig        |  1 +
>  hw/arm/virt.c         | 24 ++++++++++++
>  hw/gpio/Kconfig       |  3 ++
>  hw/gpio/gpio_pwr.c    | 85 +++++++++++++++++++++++++++++++++++++++++++
>  hw/gpio/meson.build   |  1 +
>  include/hw/arm/virt.h |  1 +
>  6 files changed, 115 insertions(+)
>  create mode 100644 hw/gpio/gpio_pwr.


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCHv3] arm-virt: add secure pl061 for reset/power down
  2021-01-06 16:34 [PATCHv3] arm-virt: add secure pl061 for reset/power down Maxim Uvarov
  2021-01-06 16:36 ` Maxim Uvarov
  2021-01-06 16:47 ` Philippe Mathieu-Daudé
@ 2021-01-08 14:31 ` Peter Maydell
  2 siblings, 0 replies; 4+ messages in thread
From: Peter Maydell @ 2021-01-08 14:31 UTC (permalink / raw)
  To: Maxim Uvarov
  Cc: tf-a, qemu-arm, QEMU Developers, Jose.Marinho,
	Philippe Mathieu-Daudé

On Wed, 6 Jan 2021 at 16:34, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
> Add secure pl061 for reset/power down machine from
> the secure world (Arm Trusted Firmware).
> Use the same gpio 3 and gpio 4 which were used by
> non acpi variant of linux power control gpios.
>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  v3: added missed include qemu/log.h for qemu_log(..
>  v2: replace printf with qemu_log (Philippe Mathieu-Daudé)
>
>  hw/arm/Kconfig        |  1 +
>  hw/arm/virt.c         | 24 ++++++++++++
>  hw/gpio/Kconfig       |  3 ++
>  hw/gpio/gpio_pwr.c    | 85 +++++++++++++++++++++++++++++++++++++++++++
>  hw/gpio/meson.build   |  1 +
>  include/hw/arm/virt.h |  1 +
>  6 files changed, 115 insertions(+)
>  create mode 100644 hw/gpio/gpio_pwr.c

Could you put "Implement new gpio_pwr device" and "wire up
gpio_pwr device in virt board" in separate patches please?
(That is, turn this into a two-patch patchset.)

> +static void create_gpio_secure(const VirtMachineState *vms)
> +{
> +    DeviceState *pl061_dev;
> +    static DeviceState *gpio_pwr_dev;
> +
> +    hwaddr base = vms->memmap[VIRT_SECURE_GPIO].base;
> +    int irq = vms->irqmap[VIRT_SECURE_GPIO];
> +
> +    pl061_dev = sysbus_create_simple("pl061", base,
> +                                     qdev_get_gpio_in(vms->gic, irq));

sysbus_create_simple() will map the device into the default
(NonSecure) address space. You need to pass secure_sysmem into
the create_ function and use that (compare create_uart()).

> +
> +    gpio_pwr_dev = sysbus_create_simple("gpio-pwr", -1,
> +                                        qdev_get_gpio_in(pl061_dev, 3));
> +
> +    qdev_connect_gpio_out(pl061_dev, 3, qdev_get_gpio_in(gpio_pwr_dev, 3));
> +    qdev_connect_gpio_out(pl061_dev, 4, qdev_get_gpio_in(gpio_pwr_dev, 4));
> +}
> +
>  static void create_virtio_devices(const VirtMachineState *vms)
>  {
>      int i;
> @@ -1993,6 +2013,10 @@ static void machvirt_init(MachineState *machine)
>          create_gpio(vms);
>      }
>
> +    if (vms->secure) {
> +        create_gpio_secure(vms);
> +    }

The 'virt' board has strict versioning -- this means that, for
instance, the 'virt-5.2' machine must always look exactly like
the version of 'virt' that shipped in QEMU's 5.2 release, with
no extra guest visible devices. So you need to add the flags
and code so that this new secure PL061 is only present from
virt-6.0 and onwards. This means a flag no_secure_gpio in the
VirtMachineClass and a flag secure_gpio in the VirtMachineState,
a line setting vmc->no_secure_gpio to true in virt_machine_5_2_options(),
code in virt_instance_init() to set vms->secure_gpio from
vmc->no_secure_gpio, and then only create the secure GPIO if
vms->secure_gpio is true. The 'no_its'/'its' flags are the
right pattern to use. (Yes, having no_foo in the class struct
and foo in the state struct is deliberate.)

> diff --git a/hw/gpio/gpio_pwr.c b/hw/gpio/gpio_pwr.c
> new file mode 100644
> index 0000000000..0d0680c9f7
> --- /dev/null
> +++ b/hw/gpio/gpio_pwr.c
> @@ -0,0 +1,85 @@
> +/*
> + * 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
> + */

What is the interface of this device intended to be?
As written it has:
 * 8 input GPIO lines, of which all except 3 and 4 are
   ignored; asserting input line 3 triggers a shutdown,
   and asserting input line 4 triggers a reset
 * one output IRQ line, which is asserted whenever any
   input GPIO line is set to any level, and never cleared

This seems a very weird choice of interface. I was expecting
something much more simple:
 * no output IRQ or GPIO lines
 * two named input GPIO lines:
    'reset' : when asserted, trigger system reset
    'shutdown' : when asserted, trigger system shutdown

thanks
-- PMM


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-01-08 14:33 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-06 16:34 [PATCHv3] arm-virt: add secure pl061 for reset/power down Maxim Uvarov
2021-01-06 16:36 ` Maxim Uvarov
2021-01-06 16:47 ` Philippe Mathieu-Daudé
2021-01-08 14:31 ` Peter Maydell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.