v7: - same as v6, but resplit patches: patch 2 no function changes and refactor gpio setup for virt platfrom and patch 3 adds secure gpio. v6: - 64k align gpio memory region (Andrew Jones) - adjusted memory region to map this address in the corresponding atf patch v5: - removed vms flag, added fdt (Andrew Jones) - added patch3 to combine secure and non secure pl061. It has to be more easy to review if this changes are in the separate patch. 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/7556d07e87f755c602cd9...
Previus discussion for reboot issue was here: https://www.mail-archive.com/qemu-devel@nongnu.org/msg757705.html Maxim Uvarov (3): hw: gpio: implement gpio-pwr driver for qemu reset/poweroff arm-virt: refactor gpios creation arm-virt: add secure pl061 for reset/power down
hw/arm/Kconfig | 1 + hw/arm/virt.c | 117 ++++++++++++++++++++++++++++++++++-------- hw/gpio/Kconfig | 3 ++ hw/gpio/gpio_pwr.c | 70 +++++++++++++++++++++++++ hw/gpio/meson.build | 1 + include/hw/arm/virt.h | 2 + 6 files changed, 174 insertions(+), 20 deletions(-) create mode 100644 hw/gpio/gpio_pwr.c
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'))
On Fri, 15 Jan 2021 at 10:11, Maxim Uvarov maxim.uvarov@linaro.org wrote:
The comment says we perform the actions when the lines are asserted...
...but the code performs the actions when the lines are de-asserted, ie when they go to 0. I think the code should be "if (level)".
thanks -- PMM
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 | 67 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 20 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 96985917d3..26bb66e8e1 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,22 @@ 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); + if (gpio == VIRT_GPIO) { + qemu_fdt_setprop_string(vms->fdt, "/chosen", "stdout-path", nodename); + } else { + /* 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");
- 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); + qemu_fdt_setprop_string(vms->fdt, "/secure-chosen", "stdout-path", + nodename); + } 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 +2017,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 */
On Fri, 15 Jan 2021 at 10:11, Maxim Uvarov maxim.uvarov@linaro.org wrote:
You don't want to set /chosen/stdout-path (that is specific to the uart, it's telling the kernel where it should send its bootup output by default).
Similarly here you don't want to set /secure-chosen/stdout-path.
Patch looks OK otherwise.
thanks -- PMM
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 | 50 +++++++++++++++++++++++++++++++++++++++++++ include/hw/arm/virt.h | 2 ++ 3 files changed, 53 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 26bb66e8e1..436ae894c9 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,46 @@ static void create_gpio_keys(const VirtMachineState *vms, "gpios", phandle, 3, 0); }
+#define ATF_GPIO_POWEROFF 3 +#define ATF_GPIO_REBOOT 4 + +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, 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)); + + qemu_fdt_add_subnode(vms->fdt, "/gpio-pwr"); + qemu_fdt_setprop_string(vms->fdt, "/gpio-pwr", "compatible", "gpio-pwr"); + qemu_fdt_setprop_cell(vms->fdt, "/gpio-pwr", "#size-cells", 0); + qemu_fdt_setprop_cell(vms->fdt, "/gpio-pwr", "#address-cells", 1); + + qemu_fdt_add_subnode(vms->fdt, "/gpio-pwr/poweroff"); + qemu_fdt_setprop_string(vms->fdt, "/gpio-pwr/poweroff", + "label", "GPIO PWR Poweroff"); + qemu_fdt_setprop_cell(vms->fdt, "/gpio-pwr/poweroff", "code", + ATF_GPIO_POWEROFF); + qemu_fdt_setprop_cells(vms->fdt, "/gpio-pwr/poweroff", + "gpios", phandle, 3, 0); + + qemu_fdt_add_subnode(vms->fdt, "/gpio-pwr/reboot"); + qemu_fdt_setprop_string(vms->fdt, "/gpio-pwr/reboot", + "label", "GPIO PWR Reboot"); + qemu_fdt_setprop_cell(vms->fdt, "/gpio-pwr/reboot", "code", + ATF_GPIO_REBOOT); + qemu_fdt_setprop_cells(vms->fdt, "/gpio-pwr/reboot", + "gpios", phandle, 3, 0); +} + static void create_gpio_devices(const VirtMachineState *vms, int gpio, MemoryRegion *mem) { @@ -888,6 +929,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); } }
@@ -2020,6 +2063,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); @@ -2635,8 +2682,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 {
On Fri, 15 Jan 2021 at 10:11, Maxim Uvarov maxim.uvarov@linaro.org wrote:
These aren't ATF specific, so you could name them SECURE_GPIO_POWEROFF and SECURE_GPIO_REBOOT.
Remind me why we start with GPIO line number 3 and not 0 ?
You've connected the POWEROFF gpio line to 'reset' and the REBOOT line to 'shutdown'. This looks like it's backwards.
There doesn't seem to be any documented 'gpio-pwr' devicetree binding. Where does this come from ?
I think the bindings you want to be using are https://www.kernel.org/doc/Documentation/devicetree/bindings/power/reset/gpi... https://www.kernel.org/doc/Documentation/devicetree/bindings/power/reset/gpi...
thanks -- PMM
On Tue, 19 Jan 2021 at 16:07, Peter Maydell peter.maydell@linaro.org wrote:
OK.
Remind me why we start with GPIO line number 3 and not 0 ?
Original gpio power key use 3 and 4 (non-secure). I just selected the same to be consistent.
Oh, yes. Thanks for finding that.
gpio-pwr created from the first patch. There are no bindings yet.
These handles are from 'secure memory' where linux does not have access. But I think we can use that binding with other compatible. Like compatible = "gpio-poweroff,secure".
Maxim.
thanks -- PMM
On Tue, 19 Jan 2021 at 13:47, Maxim Uvarov maxim.uvarov@linaro.org wrote:
Those are different GPIO lines on a different PL061 doing a different job. I don't think they need to be the same number. The power keys are on 3 and 4 because pins 0, 1 and 2 were reserved for PCI hotplug, CPU hotplug and memory hotplug. Unless you have some similar reason why you need to reserve pins on the secure PL061, I would just start from 0.
You can't use bindings you've just made up -- you have to get them accepted into the kernel's official devicetree documentation if the ones already there aren't sufficient, before you can add code to QEMU that generates them.
That's not how you specify that a node is only relevant to the secure world: you set the 'status' property to 'disabled' and the 'secure-status' property to 'okay': https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/secure.txt
thanks -- PMM
tf-a@lists.trustedfirmware.org