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

[Andrew Jones]

On Wed, Jan 13, 2021 at 10:30:47AM +0300, Maxim Uvarov wrote:
> - the same size for secure and non secure gpio. Arm doc says that
> secure memory is also split on 4k pages. So one page here has to be
> ok.

To be clear, does that means 4k pages must be used? I'm not concerned
with the size, but the alignment. If it's possible to use larger page
sizes with secure memory, then we need to align to the maximum page
size that may be used.

Thanks,
drew


> - will add dtb.
> - I think then less options is better. So I will remove
> vmc->secure_gpio flag and keep only vmc flag.
> 
> Regards,
> Maxim.
> 
> On Tue, 12 Jan 2021 at 19:28, Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Tue, Jan 12, 2021 at 11:25:30AM -0500, Andrew Jones wrote:
> > > 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
> >
> > I meant to write 'vmc->no_secure_gpio and vms->secure' here.
> >
> > Thanks,
> > drew
> >
> > > always need both vmc and vms state, see 'no_ged'.
> > >
> > > Thanks,
> > > drew
> >
>