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

[Andrew Jones <drjones@redhat.com>]

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