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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>]

On Thu, 4 Jun 2020, Julien Grall wrote:
> 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.

OK, thank you, that it also what I think.


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

We are writing a wikipage to explain how to boot Xen on RPi4, because of
the unique firmware/bootloader and the non-upstream patches. We can add
a recommandation to use TF for production to the wikipage.