| Message ID | 20210120092748.14789-4-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm-virt: add secure pl061 for reset/power down | expand |
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
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
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>
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
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 {
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(+)