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/dd4401d8eb8e0f3018b33...
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
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'))
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
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;
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
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
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
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
- 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
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
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
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.
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.
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
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.
tf-a@lists.trustedfirmware.org