diff mbox series

[PATCHv7,3/3] arm-virt: add secure pl061 for reset/power down

Message ID 20210115101126.4259-4-maxim.uvarov@linaro.org
State New
Headers show
Series arm-virt: add secure pl061 for reset/power down | expand

Commit Message

Maxim Uvarov Jan. 15, 2021, 10:11 a.m. UTC 
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(+)

-- 
2.17.1

Comments

Peter Maydell Jan. 19, 2021, 1:07 p.m. UTC | #1 
On Fri, 15 Jan 2021 at 10:11, 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>

> ---

>  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


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 ?

> +

> +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));


You've connected the POWEROFF gpio line to 'reset' and the
REBOOT line to 'shutdown'. This looks like it's backwards.

> +    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);


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/gpio-restart.txt
https://www.kernel.org/doc/Documentation/devicetree/bindings/power/reset/gpio-poweroff.txt

thanks
-- PMM
Maxim Uvarov Jan. 19, 2021, 1:47 p.m. UTC | #2 
On Tue, 19 Jan 2021 at 16:07, Peter Maydell <peter.maydell@linaro.org> wrote:
>

> On Fri, 15 Jan 2021 at 10:11, 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>

> > ---

> >  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

>

> These aren't ATF specific, so you could name them SECURE_GPIO_POWEROFF

> and SECURE_GPIO_REBOOT.

>

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.


> > +

> > +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));

>

> You've connected the POWEROFF gpio line to 'reset' and the

> REBOOT line to 'shutdown'. This looks like it's backwards.

>


Oh, yes. Thanks for finding that.

> > +    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);

>

> There doesn't seem to be any documented 'gpio-pwr' devicetree

> binding. Where does this come from ?

>

gpio-pwr created from the first patch.  There are no bindings yet.

> I think the bindings you want to be using are

> https://www.kernel.org/doc/Documentation/devicetree/bindings/power/reset/gpio-restart.txt

> https://www.kernel.org/doc/Documentation/devicetree/bindings/power/reset/gpio-poweroff.txt

>

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
Peter Maydell Jan. 19, 2021, 2:12 p.m. UTC | #3 
On Tue, 19 Jan 2021 at 13:47, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>

> On Tue, 19 Jan 2021 at 16:07, Peter Maydell <peter.maydell@linaro.org> wrote:

> >

> > On Fri, 15 Jan 2021 at 10:11, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

> > 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.


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.

> > > +    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);

> >

> > There doesn't seem to be any documented 'gpio-pwr' devicetree

> > binding. Where does this come from ?

> >

> gpio-pwr created from the first patch.  There are no bindings yet.


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.

> > I think the bindings you want to be using are

> > https://www.kernel.org/doc/Documentation/devicetree/bindings/power/reset/gpio-restart.txt

> > https://www.kernel.org/doc/Documentation/devicetree/bindings/power/reset/gpio-poweroff.txt

> >

> 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".


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
diff mbox series

Patch

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 {