* [PATCHv8 1/3] hw: gpio: implement gpio-pwr driver for qemu reset/poweroff
2021-01-20 9:27 [PATCHv8 0/3] arm-virt: add secure pl061 for reset/power down Maxim Uvarov
@ 2021-01-20 9:27 ` Maxim Uvarov
2021-01-22 15:48 ` Peter Maydell
2021-01-20 9:27 ` [PATCHv8 2/3] arm-virt: refactor gpios creation Maxim Uvarov
2021-01-20 9:27 ` [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down Maxim Uvarov
2 siblings, 1 reply; 10+ messages in thread
From: Maxim Uvarov @ 2021-01-20 9:27 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: peter.maydell, drjones, Maxim Uvarov, Jose.Marinho, f4bug, tf-a
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..7714fa0dc4
--- /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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCHv8 1/3] hw: gpio: implement gpio-pwr driver for qemu reset/poweroff
2021-01-20 9:27 ` [PATCHv8 1/3] hw: gpio: implement gpio-pwr driver for qemu reset/poweroff Maxim Uvarov
@ 2021-01-22 15:48 ` Peter Maydell
0 siblings, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2021-01-22 15:48 UTC (permalink / raw)
To: Maxim Uvarov
Cc: Andrew Jones, Jose Marinho, QEMU Developers,
Philippe Mathieu-Daudé,
tf-a, qemu-arm
On Wed, 20 Jan 2021 at 09:27, 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
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCHv8 2/3] arm-virt: refactor gpios creation
2021-01-20 9:27 [PATCHv8 0/3] arm-virt: add secure pl061 for reset/power down Maxim Uvarov
2021-01-20 9:27 ` [PATCHv8 1/3] hw: gpio: implement gpio-pwr driver for qemu reset/poweroff Maxim Uvarov
@ 2021-01-20 9:27 ` Maxim Uvarov
2021-01-22 8:07 ` Andrew Jones
2021-01-20 9:27 ` [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down Maxim Uvarov
2 siblings, 1 reply; 10+ messages in thread
From: Maxim Uvarov @ 2021-01-20 9:27 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: peter.maydell, drjones, Maxim Uvarov, Jose.Marinho, f4bug, tf-a
No functional change. Just refactor code to better
support secure and normal world gpios.
Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
hw/arm/virt.c | 64 ++++++++++++++++++++++++++++++++++-----------------
1 file changed, 43 insertions(+), 21 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 96985917d3..c427ce5f81 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -820,17 +820,43 @@ static void virt_powerdown_req(Notifier *n, void *opaque)
}
}
-static void create_gpio(const VirtMachineState *vms)
+static void create_gpio_keys(const VirtMachineState *vms,
+ DeviceState *pl061_dev,
+ uint32_t phandle)
+{
+ gpio_key_dev = sysbus_create_simple("gpio-key", -1,
+ qdev_get_gpio_in(pl061_dev, 3));
+
+ qemu_fdt_add_subnode(vms->fdt, "/gpio-keys");
+ qemu_fdt_setprop_string(vms->fdt, "/gpio-keys", "compatible", "gpio-keys");
+ qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#size-cells", 0);
+ qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#address-cells", 1);
+
+ qemu_fdt_add_subnode(vms->fdt, "/gpio-keys/poweroff");
+ qemu_fdt_setprop_string(vms->fdt, "/gpio-keys/poweroff",
+ "label", "GPIO Key Poweroff");
+ qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys/poweroff", "linux,code",
+ KEY_POWER);
+ qemu_fdt_setprop_cells(vms->fdt, "/gpio-keys/poweroff",
+ "gpios", phandle, 3, 0);
+}
+
+static void create_gpio_devices(const VirtMachineState *vms, int gpio,
+ MemoryRegion *mem)
{
char *nodename;
DeviceState *pl061_dev;
- hwaddr base = vms->memmap[VIRT_GPIO].base;
- hwaddr size = vms->memmap[VIRT_GPIO].size;
- int irq = vms->irqmap[VIRT_GPIO];
+ hwaddr base = vms->memmap[gpio].base;
+ hwaddr size = vms->memmap[gpio].size;
+ int irq = vms->irqmap[gpio];
const char compat[] = "arm,pl061\0arm,primecell";
+ SysBusDevice *s;
- pl061_dev = sysbus_create_simple("pl061", base,
- qdev_get_gpio_in(vms->gic, irq));
+ 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));
+ sysbus_connect_irq(s, 0, qdev_get_gpio_in(vms->gic, irq));
uint32_t phandle = qemu_fdt_alloc_phandle(vms->fdt);
nodename = g_strdup_printf("/pl061@%" PRIx64, base);
@@ -847,21 +873,17 @@ static void create_gpio(const VirtMachineState *vms)
qemu_fdt_setprop_string(vms->fdt, nodename, "clock-names", "apb_pclk");
qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle", phandle);
- gpio_key_dev = sysbus_create_simple("gpio-key", -1,
- qdev_get_gpio_in(pl061_dev, 3));
- qemu_fdt_add_subnode(vms->fdt, "/gpio-keys");
- qemu_fdt_setprop_string(vms->fdt, "/gpio-keys", "compatible", "gpio-keys");
- qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#size-cells", 0);
- qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#address-cells", 1);
-
- qemu_fdt_add_subnode(vms->fdt, "/gpio-keys/poweroff");
- qemu_fdt_setprop_string(vms->fdt, "/gpio-keys/poweroff",
- "label", "GPIO Key Poweroff");
- qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys/poweroff", "linux,code",
- KEY_POWER);
- qemu_fdt_setprop_cells(vms->fdt, "/gpio-keys/poweroff",
- "gpios", phandle, 3, 0);
+ if (gpio != VIRT_GPIO) {
+ /* Mark as not usable by the normal world */
+ qemu_fdt_setprop_string(vms->fdt, nodename, "status", "disabled");
+ qemu_fdt_setprop_string(vms->fdt, nodename, "secure-status", "okay");
+ }
g_free(nodename);
+
+ /* Child gpio devices */
+ if (gpio == VIRT_GPIO) {
+ create_gpio_keys(vms, pl061_dev, phandle);
+ }
}
static void create_virtio_devices(const VirtMachineState *vms)
@@ -1990,7 +2012,7 @@ static void machvirt_init(MachineState *machine)
if (has_ged && aarch64 && firmware_loaded && virt_is_acpi_enabled(vms)) {
vms->acpi_dev = create_acpi_ged(vms);
} else {
- create_gpio(vms);
+ create_gpio_devices(vms, VIRT_GPIO, sysmem);
}
/* connect powerdown request */
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCHv8 2/3] arm-virt: refactor gpios creation
2021-01-20 9:27 ` [PATCHv8 2/3] arm-virt: refactor gpios creation Maxim Uvarov
@ 2021-01-22 8:07 ` Andrew Jones
0 siblings, 0 replies; 10+ messages in thread
From: Andrew Jones @ 2021-01-22 8:07 UTC (permalink / raw)
To: Maxim Uvarov
Cc: peter.maydell, Jose.Marinho, qemu-devel, f4bug, tf-a, qemu-arm
On Wed, Jan 20, 2021 at 12:27:47PM +0300, Maxim Uvarov wrote:
> No functional change. Just refactor code to better
> support secure and normal world gpios.
>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
> hw/arm/virt.c | 64 ++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 43 insertions(+), 21 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 96985917d3..c427ce5f81 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -820,17 +820,43 @@ static void virt_powerdown_req(Notifier *n, void *opaque)
> }
> }
>
> -static void create_gpio(const VirtMachineState *vms)
> +static void create_gpio_keys(const VirtMachineState *vms,
> + DeviceState *pl061_dev,
> + uint32_t phandle)
> +{
> + gpio_key_dev = sysbus_create_simple("gpio-key", -1,
> + qdev_get_gpio_in(pl061_dev, 3));
> +
> + qemu_fdt_add_subnode(vms->fdt, "/gpio-keys");
> + qemu_fdt_setprop_string(vms->fdt, "/gpio-keys", "compatible", "gpio-keys");
> + qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#size-cells", 0);
> + qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#address-cells", 1);
> +
> + qemu_fdt_add_subnode(vms->fdt, "/gpio-keys/poweroff");
> + qemu_fdt_setprop_string(vms->fdt, "/gpio-keys/poweroff",
> + "label", "GPIO Key Poweroff");
> + qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys/poweroff", "linux,code",
> + KEY_POWER);
> + qemu_fdt_setprop_cells(vms->fdt, "/gpio-keys/poweroff",
> + "gpios", phandle, 3, 0);
> +}
> +
> +static void create_gpio_devices(const VirtMachineState *vms, int gpio,
> + MemoryRegion *mem)
> {
> char *nodename;
> DeviceState *pl061_dev;
> - hwaddr base = vms->memmap[VIRT_GPIO].base;
> - hwaddr size = vms->memmap[VIRT_GPIO].size;
> - int irq = vms->irqmap[VIRT_GPIO];
> + hwaddr base = vms->memmap[gpio].base;
> + hwaddr size = vms->memmap[gpio].size;
> + int irq = vms->irqmap[gpio];
> const char compat[] = "arm,pl061\0arm,primecell";
> + SysBusDevice *s;
>
> - pl061_dev = sysbus_create_simple("pl061", base,
> - qdev_get_gpio_in(vms->gic, irq));
> + 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));
> + sysbus_connect_irq(s, 0, qdev_get_gpio_in(vms->gic, irq));
>
> uint32_t phandle = qemu_fdt_alloc_phandle(vms->fdt);
> nodename = g_strdup_printf("/pl061@%" PRIx64, base);
> @@ -847,21 +873,17 @@ static void create_gpio(const VirtMachineState *vms)
> qemu_fdt_setprop_string(vms->fdt, nodename, "clock-names", "apb_pclk");
> qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle", phandle);
>
> - gpio_key_dev = sysbus_create_simple("gpio-key", -1,
> - qdev_get_gpio_in(pl061_dev, 3));
> - qemu_fdt_add_subnode(vms->fdt, "/gpio-keys");
> - qemu_fdt_setprop_string(vms->fdt, "/gpio-keys", "compatible", "gpio-keys");
> - qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#size-cells", 0);
> - qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#address-cells", 1);
> -
> - qemu_fdt_add_subnode(vms->fdt, "/gpio-keys/poweroff");
> - qemu_fdt_setprop_string(vms->fdt, "/gpio-keys/poweroff",
> - "label", "GPIO Key Poweroff");
> - qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys/poweroff", "linux,code",
> - KEY_POWER);
> - qemu_fdt_setprop_cells(vms->fdt, "/gpio-keys/poweroff",
> - "gpios", phandle, 3, 0);
> + if (gpio != VIRT_GPIO) {
> + /* Mark as not usable by the normal world */
> + qemu_fdt_setprop_string(vms->fdt, nodename, "status", "disabled");
> + qemu_fdt_setprop_string(vms->fdt, nodename, "secure-status", "okay");
> + }
nit: The above if-block could/should have waited until the next patch to
be added.
> g_free(nodename);
> +
> + /* Child gpio devices */
> + if (gpio == VIRT_GPIO) {
Same nit as above, the next patch is where we should start conditionally
doing stuff based on the gpio type.
> + create_gpio_keys(vms, pl061_dev, phandle);
> + }
> }
>
> static void create_virtio_devices(const VirtMachineState *vms)
> @@ -1990,7 +2012,7 @@ static void machvirt_init(MachineState *machine)
> if (has_ged && aarch64 && firmware_loaded && virt_is_acpi_enabled(vms)) {
> vms->acpi_dev = create_acpi_ged(vms);
> } else {
> - create_gpio(vms);
> + create_gpio_devices(vms, VIRT_GPIO, sysmem);
> }
>
> /* connect powerdown request */
> --
> 2.17.1
>
>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Thanks,
drew
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down
2021-01-20 9:27 [PATCHv8 0/3] arm-virt: add secure pl061 for reset/power down Maxim Uvarov
2021-01-20 9:27 ` [PATCHv8 1/3] hw: gpio: implement gpio-pwr driver for qemu reset/poweroff Maxim Uvarov
2021-01-20 9:27 ` [PATCHv8 2/3] arm-virt: refactor gpios creation Maxim Uvarov
@ 2021-01-20 9:27 ` Maxim Uvarov
2021-01-22 8:29 ` Andrew Jones
2021-01-22 15:47 ` Peter Maydell
2 siblings, 2 replies; 10+ messages in thread
From: Maxim Uvarov @ 2021-01-20 9:27 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: peter.maydell, drjones, Maxim Uvarov, Jose.Marinho, f4bug, tf-a
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,
+ DeviceState *pl061_dev,
+ uint32_t phandle)
+{
+ DeviceState *gpio_pwr_dev;
+
+ /* gpio-pwr */
+ gpio_pwr_dev = sysbus_create_simple("gpio-pwr", -1, NULL);
+
+ /* 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down
2021-01-20 9:27 ` [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down Maxim Uvarov
@ 2021-01-22 8:29 ` Andrew Jones
2021-01-22 10:09 ` Peter Maydell
2021-01-22 15:47 ` Peter Maydell
1 sibling, 1 reply; 10+ messages in thread
From: Andrew Jones @ 2021-01-22 8:29 UTC (permalink / raw)
To: Maxim Uvarov
Cc: peter.maydell, Jose.Marinho, qemu-devel, f4bug, tf-a, qemu-arm
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down
2021-01-22 8:29 ` Andrew Jones
@ 2021-01-22 10:09 ` Peter Maydell
2021-01-22 10:17 ` Andrew Jones
0 siblings, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2021-01-22 10:09 UTC (permalink / raw)
To: Andrew Jones
Cc: Maxim Uvarov, Jose Marinho, QEMU Developers,
Philippe Mathieu-Daudé,
tf-a, qemu-arm
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
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down
2021-01-22 10:09 ` Peter Maydell
@ 2021-01-22 10:17 ` Andrew Jones
0 siblings, 0 replies; 10+ messages in thread
From: Andrew Jones @ 2021-01-22 10:17 UTC (permalink / raw)
To: Peter Maydell
Cc: Maxim Uvarov, Jose Marinho, QEMU Developers,
Philippe Mathieu-Daudé,
tf-a, qemu-arm
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down
2021-01-20 9:27 ` [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down Maxim Uvarov
2021-01-22 8:29 ` Andrew Jones
@ 2021-01-22 15:47 ` Peter Maydell
1 sibling, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2021-01-22 15:47 UTC (permalink / raw)
To: Maxim Uvarov
Cc: Andrew Jones, Jose Marinho, QEMU Developers,
Philippe Mathieu-Daudé,
tf-a, qemu-arm
On Wed, 20 Jan 2021 at 09:27, Maxim Uvarov <maxim.uvarov@linaro.org> 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>
A nit, which I raise only because you'll need a respin anyway:
> + /* 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));
> + qemu_fdt_add_subnode(vms->fdt, "/gpio-restart");
We have three different names for the same thing here: 'reboot',
'reset' and 'restart'. If we name the GPIO line SECURE_GPIO_RESET
we can at least get that down to two.
thanks
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread