Re: [PATCH] xen/rpi4: implement watchdog-based reset

[Julien Grall <julien@xxxxxxx>]

On 04/06/2020 17:46, Stefano Stabellini wrote:
> On Thu, 4 Jun 2020, André Przywara wrote:
> > On 04/06/2020 17:24, Stefano Stabellini wrote:
> > > On Thu, 4 Jun 2020, André Przywara wrote:
> > > > On 04/06/2020 09:48, Julien Grall wrote:
> > > >
> > > > Hi,
> > > >
> > > > > On 03/06/2020 23:31, Stefano Stabellini wrote:
> > > > > > Touching the watchdog is required to be able to reboot the board.
> > > > > In general the preferred method is PSCI. Does it mean RPI 4 doesn't
> > > > > support PSCI at all?
> > > > There is mainline Trusted Firmware (TF-A) support for the RPi4 for a few
> > > > months now, which includes proper PSCI support (both for SMP bringup and
> > > > system reset/shutdown). At least that should work, if not, it's a bug.
> > > > An EDK-2 build for RPi4 bundles TF-A automatically, but you can use TF-A
> > > > without it, with or without U-Boot: It works as a drop-in replacement
> > > > for armstub.bin. Instruction for building it (one line!) are here:
> > > > https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/docs/plat/rpi4.rst
> > > >
> > > > > > The implementation is based on
> > > > > > drivers/watchdog/bcm2835_wdt.c:__bcm2835_restart in Linux.
> > > > > Can you give the baseline? This would allow us to track an issue and
> > > > > port them.
> > > > Given the above I don't think it's a good idea to add extra platform
> > > > specific code to Xen.
> > > The RPi4, at least the one I have, doesn't come with any TF, and it
> > My RPi4 didn't come with anything, actually ;-) It's just a matter of
> > what you put in the uSD card slot.
> >
> > > doesn't come with PSCI in device tree.
> > TF-A patches the PSCI nodes in:
> > https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/plat/rpi/rpi4?id=f67fa69cb6937a7fc559bbec4a7acce5edefa888
> >
> > > As a user, I would rather have
> > > this patch (even downstream) than having to introduce TF in my build and
> > > deployment just to be able to reboot.
> > I get your point, but would rather put more pressure on people using
> > TF-A. After all you run without CPU hotplug, A72 errata workarounds and
> > without Spectre/Meltdown fixes. What was the IP address of your board
> > again? ;-)
> Please send a pull request to remove __bcm2835_restart from the Linux
> kernel, once it is removed from there I'd be happy to take it away from
> Xen too ;-)
Xen is not a slave of Linux. We make our own informed decision ;).

> I know I am being cheeky but we have enough battles to fight and enough
> problems with Xen -- I don't think we should use the hypervisor as a
> leverage to get people to use or upgrade TF. We just need to get good
> functionalities to our users with the less amount of friction possible.
Well it is nice to have functionality but you also need to have Xen 
running reliably and safely. No-one wants to drive in car with no brake 
on a windy road. Or maybe I am wrong? ;)
> Everything you mentioned are good reason to use TF, and this patch does
> not take anything away from it. My suggestion would be to work with
> raspberrypi.org to have TF installed by default by the  .
We actually did use the hypervisor as a leverage in the past. A pretty 
good example is RPI 3.
Anyway, the patch is pretty simple and limited to the platform. So I 
would be inclined to accept it.
Although this is just sweeping stability concern under the carpet and 
hoping for the best. What are the odds this is going to be used in 
production like that?
Cheers,

--
Julien Grall