Re: [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down
On Wed, 20 Jan 2021 at 09:27, 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 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
Re: [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down
On Fri, Jan 22, 2021 at 10:09:35AM +, Peter Maydell wrote:
> On Fri, 22 Jan 2021 at 08:29, Andrew Jones 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
> > > ---
> > > 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] = { 0x0908, ACPI_GED_EVT_SEL_LEN },
> > > [VIRT_NVDIMM_ACPI] ={ 0x0909, NVDIMM_ACPI_IO_LEN},
> > > [VIRT_PVTIME] = { 0x090a, 0x0001 },
> > > +[VIRT_SECURE_GPIO] ={ 0x090b, 0x1000 },
> > > [VIRT_MMIO] = { 0x0a00, 0x0200 },
> > > /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that
> > > size */
> > > [VIRT_PLATFORM_BUS] = { 0x0c00, 0x0200 },
> > > @@ -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), _fatal);
> )
>
Thanks, I should have looked more closely at that.
With the function name change to include "secure".
Reviewed-by: Andrew Jones
Re: [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down
On Fri, 22 Jan 2021 at 08:29, Andrew Jones 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
> > ---
> > 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] = { 0x0908, ACPI_GED_EVT_SEL_LEN },
> > [VIRT_NVDIMM_ACPI] ={ 0x0909, NVDIMM_ACPI_IO_LEN},
> > [VIRT_PVTIME] = { 0x090a, 0x0001 },
> > +[VIRT_SECURE_GPIO] ={ 0x090b, 0x1000 },
> > [VIRT_MMIO] = { 0x0a00, 0x0200 },
> > /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that
> > size */
> > [VIRT_PLATFORM_BUS] = { 0x0c00, 0x0200 },
> > @@ -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), _fatal);
)
thanks
-- PMM
Re: [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down
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
> ---
> 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] = { 0x0908, ACPI_GED_EVT_SEL_LEN },
> [VIRT_NVDIMM_ACPI] ={ 0x0909, NVDIMM_ACPI_IO_LEN},
> [VIRT_PVTIME] = { 0x090a, 0x0001 },
> +[VIRT_SECURE_GPIO] ={ 0x090b, 0x1000 },
> [VIRT_MMIO] = { 0x0a00, 0x0200 },
> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size
> */
> [VIRT_PLATFORM_BUS] = { 0x0c00, 0x0200 },
> @@ -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(>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,
>
[PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down
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
---
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] = { 0x0908, ACPI_GED_EVT_SEL_LEN },
[VIRT_NVDIMM_ACPI] ={ 0x0909, NVDIMM_ACPI_IO_LEN},
[VIRT_PVTIME] = { 0x090a, 0x0001 },
+[VIRT_SECURE_GPIO] ={ 0x090b, 0x1000 },
[VIRT_MMIO] = { 0x0a00, 0x0200 },
/* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
[VIRT_PLATFORM_BUS] = { 0x0c00, 0x0200 },
@@ -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(>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
