Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 20 Mar 2024, Julien Grall wrote:
> Hi John,
> 
> On 20/03/2024 16:24, John Ernberg wrote:
> > Hi Bertrand,
> > 
> > On 3/13/24 11:07, Bertrand Marquis wrote:
> > > Hi,
> > > 
> > > > On 8 Mar 2024, at 15:04, Julien Grall <julien@xxxxxxx> wrote:
> > > > 
> > > > Hi John,
> > > > 
> > > > Thank you for the reply.
> > > > 
> > > > On 08/03/2024 13:40, John Ernberg wrote:
> > > > > On 3/7/24 00:07, Julien Grall wrote:
> > > > > >    > Ping on the watchdog discussion bits.
> > > > > > 
> > > > > > Sorry for the late reply.
> > > > > > 
> > > > > > On 06/03/2024 13:13, John Ernberg wrote:
> > > > > > > On 2/9/24 14:14, John Ernberg wrote:
> > > > > > > > 
> > > > > > > > >       * IMX_SIP_TIMER_*:  This seems to be related to the
> > > > > > > > > watchdog.
> > > > > > > > > Shouldn't dom0 rely on the watchdog provided by Xen instead?
> > > > > > > > > So those
> > > > > > > > > call will be used by Xen.
> > > > > > > > 
> > > > > > > > That is indeed a watchdog SIP, and also for setting the SoC
> > > > > > > > internal RTC
> > > > > > > > if it is being used.
> > > > > > > > 
> > > > > > > > I looked around if there was previous discussion and only really
> > > > > > > > found [3].
> > > > > > > > Is the xen/xen/include/watchdog.h header meant to be for this
> > > > > > > > kind of
> > > > > > > > watchdog support or is that more for the VM watchdog? Looking at
> > > > > > > > the x86
> > > > > > > > ACPI NMI watchdog it seems like the former, but I have never
> > > > > > > > worked with
> > > > > > > > x86 nor ACPI.
> > > > > > 
> > > > > > include/watchdog.h contains helper to configure the watchdog for
> > > > > > Xen. We
> > > > > > also have per-VM watchdog and this is configured by the hypercall
> > > > > > SCHEDOP_watchdog.
> > > > > > 
> > > > > > > > 
> > > > > > > > Currently forwarding it to Dom0 has not caused any watchdog
> > > > > > > > resets with
> > > > > > > > our watchdog timeout settings, our specific Dom0 setup and VM
> > > > > > > > count.
> > > > > > 
> > > > > > IIUC, the SMC API for the watchdog would be similar to the ACPI NMI
> > > > > > watchdog. So I think it would make more sense if this is not exposed
> > > > > > to
> > > > > > dom0 (even if Xen is doing nothing with it).
> > > > > > 
> > > > > > Can you try to hide the SMCs and check if dom0 still behave
> > > > > > properly?
> > > > > > 
> > > > > > Cheers,
> > > > > > 
> > > > > This SMC manages a hardware watchdog, if it's not pinged within a
> > > > > specific interval the entire board resets.
> > > > 
> > > > Do you know what's the default interval? Is it large enough so Xen +
> > > > dom0 can boot (at least up to when the watchdog driver is initialized)?
> > > > 
> > > > > If I block the SMCs the watchdog driver in Dom0 will fail to ping the
> > > > > watchdog, triggering a board reset because the system looks to have
> > > > > become unresponsive. The reason this patch set started is because we
> > > > > couldn't ping the watchdog when running with Xen.
> > > > > In our specific system the bootloader enables the watchdog as early as
> > > > > possible so that we can get watchdog protection for as much of the
> > > > > boot
> > > > > as we possibly can.
> > > > > So, if we are to block the SMC from Dom0, then Xen needs to take over
> > > > > the pinging. It could be implemented similarly to the NMI watchdog,
> > > > > except that the system will reset if the ping is missed rather than
> > > > > backtrace.
> > > > > It would also mean that Xen will get a whole watchdog driver-category
> > > > > due to the watchdog being vendor and sometimes even SoC specific when
> > > > > it
> > > > > comes to Arm.
> > > > > My understanding of the domain watchdog code is that today the domain
> > > > > needs to call SCHEDOP_watchdog at least once to start the watchdog
> > > > > timer. Since watchdog protection through the whole boot process is
> > > > > desirable we'd need some core changes, such as an option to start the
> > > > > domain watchdog on init. >
> > > > > It's quite a big change to make
> > > > 
> > > > For clarification, above you seem to mention two changes:
> > > > 
> > > > 1) Allow Xen to use the HW watchdog
> > > > 2) Allow the domain to use the watchdog early
> > > > 
> > > > I am assuming by big change, you are referring to 2?
> > > > 
> > > > , while I am not against doing it if it
> > > > > makes sense, I now wonder if Xen should manage hardware watchdogs.
> > > > > Looking at the domain watchdog code it looks like if a domain does not
> > > > > get enough execution time, the watchdog will not be pinged enough and
> > > > > the guest will be reset. So either watchdog approach requires Dom0 to
> > > > > get execution time. Dom0 also needs to service all the PV backends
> > > > > it's
> > > > > responsible for. I'm not sure it's valuable to add another layer of
> > > > > watchdog for this scenario as the end result (checking that the entire
> > > > > system works) is achieved without it as well.
> > > > > So, before I try to find the time to make a proposal for moving the
> > > > > hardware watchdog bit to Xen, do we really want it?
> > > > 
> > > > Thanks for the details. Given that the watchdog is enabled by the
> > > > bootloader, I think we want Xen to drive the watchdog for two reasons:
> > > > 1) In true dom0less environment, dom0 would not exist
> > > > 2) You are relying on Xen + Dom0 to boot (or at least enough to get the
> > > > watchdog working) within the watchdog interval.
> > > 
> > > Definitely we need to consider the case where there is no Dom0.
> > > 
> > > I think there are in fact 3 different use cases here:
> > > - watchdog fully driven in a domain (dom0 or another): would work if it is
> > > expected
> > >     that Xen + Domain boot time is under the watchdog initial refresh
> > > rate. I think this
> > >     could make sense on some applications where your system depends on a
> > > specific
> > >     domain to be properly booted to work. This would require an initial
> > > refresh time
> > >     configurable in the boot loader probably.
> > 
> > This is our use-case. ^
> > 
> > Our dom0 is monitoring and managing the other domains in our system.
> > Without dom0 working the system isn't really working as a whole.
> > 
> > @Julien: Would you be ok with the patch set continuing in the direction
> > of the
> > original proposal, letting another party (or me at a later time) implement
> > the fully driven by Xen option?
> I am concerned about this particular point from Bertrand:
> 
> "would work if it is expected that Xen + Domain boot time is under the
> watchdog initial refresh rate."
> 
> How will a user be able to figure out how to initially configure the watchdog?
> Is this something you can easily configure in the bootloader at runtime?
> 
> 
> Overall, I am not for this approach at least in the current status. I would be
> more inclined if there are some documentations explaining how this is supposed
> to be configured on NXP, so others can use the code.
>
> Anyway, this is why we have multiple Arm maintainers for this kind of
> situation. If they other agrees with you, then they can ack the patch and this
> can be merged.


The approach here would not be my choice either. However, I think it
would be nice to have better support for NXP imx8 boards in upstream
Xen. To that end, I would ack these patches but I would ask to add a
document under xen.git/docs/ explaining the approach, limitations, and
requirements, so that someone else can use the code effectively.