Re: [PATCHv4 2/2] arm-virt: add secure pl061 for reset/power down

[Peter Maydell]

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