This adds a mechanism to detect stalls on the guest vCPUS by creating a
per CPU hrtimer which periodically 'pets' the host backend driver.

This device driver acts as a soft lockup detector by relying on the host
backend driver to measure the elapesed time between subsequent 'pet' events.
If the elapsed time doesn't match an expected value, the backend driver
decides that the guest vCPU is locked and resets the guest. The host
backend driver takes into account the time that the guest is not
running. The communication with the backend driver is done through MMIO
and the register layout of the virtual watchdog is described as part of
the backend driver changes.

The host backend driver is implemented as part of:
https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3548817

Changelog v2:
 - move the driver to misc as this does not cope with watchdog core
   subsystem
 - fix the dt-bindings warnings

Sebastian Ene (2):
  dt-bindings: vm-wdt: Add qemu,vm-watchdog compatible
  misc: Add a mechanism to detect stalls on guest vCPUs

 .../devicetree/bindings/misc/vm-wdt.yaml      |  44 ++++
 drivers/misc/Kconfig                          |   8 +
 drivers/misc/Makefile                         |   1 +
 drivers/misc/vm-wdt.c                         | 215 ++++++++++++++++++
 4 files changed, 268 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/vm-wdt.yaml
 create mode 100644 drivers/misc/vm-wdt.c

-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

On Fri, Apr 22, 2022 at 02:19:48PM +0000, Sebastian Ene wrote:
> This adds a mechanism to detect stalls on the guest vCPUS by creating a
> per CPU hrtimer which periodically 'pets' the host backend driver.
> 
> This device driver acts as a soft lockup detector by relying on the host
> backend driver to measure the elapesed time between subsequent 'pet' events.
> If the elapsed time doesn't match an expected value, the backend driver
> decides that the guest vCPU is locked and resets the guest. The host
> backend driver takes into account the time that the guest is not
> running. The communication with the backend driver is done through MMIO
> and the register layout of the virtual watchdog is described as part of
> the backend driver changes.
> 
> The host backend driver is implemented as part of:
> https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3548817
> 
> Changelog v2:
>  - move the driver to misc as this does not cope with watchdog core
>    subsystem

Wait, why does it not cope with it?  That's not documented anywhere in
your patch that adds the driver.  In fact, most of the text here needs
to be in the changelog for the driver submission, not thrown away in the
00/XX email that will never end up in the kernel tree.

thanks,

greg k-h

On Sat, Apr 23, 2022 at 08:51:16AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Apr 22, 2022 at 02:19:48PM +0000, Sebastian Ene wrote:
> > This adds a mechanism to detect stalls on the guest vCPUS by creating a
> > per CPU hrtimer which periodically 'pets' the host backend driver.
> > 
> > This device driver acts as a soft lockup detector by relying on the host
> > backend driver to measure the elapesed time between subsequent 'pet' events.
> > If the elapsed time doesn't match an expected value, the backend driver
> > decides that the guest vCPU is locked and resets the guest. The host
> > backend driver takes into account the time that the guest is not
> > running. The communication with the backend driver is done through MMIO
> > and the register layout of the virtual watchdog is described as part of
> > the backend driver changes.
> > 
> > The host backend driver is implemented as part of:
> > https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3548817
> > 
> > Changelog v2:
> >  - move the driver to misc as this does not cope with watchdog core
> >    subsystem

Hello Greg,

> 
> Wait, why does it not cope with it?  That's not documented anywhere in
> your patch that adds the driver.  In fact, most of the text here needs
> to be in the changelog for the driver submission, not thrown away in the
> 00/XX email that will never end up in the kernel tree.
> 
> thanks,
> 
> greg k-h

From the previous feedback that I received on this patch it seems that
watchdog core is not intended to be used for this type of driver. This
watchdog device tracks the elapsed time on a per-cpu basis,
since KVM schedules vCPUs independently. Watchdog core is not intended
to detect CPU stalls and the drivers don't have a notion of CPU.

Thanks,
Sebastian

On Sat, 23 Apr 2022 10:02:24 +0100,
Sebastian Ene <sebastianene@google.com> wrote:
> 
> On Sat, Apr 23, 2022 at 08:51:16AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Apr 22, 2022 at 02:19:48PM +0000, Sebastian Ene wrote:
> > > This adds a mechanism to detect stalls on the guest vCPUS by creating a
> > > per CPU hrtimer which periodically 'pets' the host backend driver.
> > > 
> > > This device driver acts as a soft lockup detector by relying on the host
> > > backend driver to measure the elapesed time between subsequent 'pet' events.
> > > If the elapsed time doesn't match an expected value, the backend driver
> > > decides that the guest vCPU is locked and resets the guest. The host
> > > backend driver takes into account the time that the guest is not
> > > running. The communication with the backend driver is done through MMIO
> > > and the register layout of the virtual watchdog is described as part of
> > > the backend driver changes.
> > > 
> > > The host backend driver is implemented as part of:
> > > https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3548817
> > > 
> > > Changelog v2:
> > >  - move the driver to misc as this does not cope with watchdog core
> > >    subsystem
> 
> Hello Greg,
> 
> > 
> > Wait, why does it not cope with it?  That's not documented anywhere in
> > your patch that adds the driver.  In fact, most of the text here needs
> > to be in the changelog for the driver submission, not thrown away in the
> > 00/XX email that will never end up in the kernel tree.
> > 
> > thanks,
> > 
> > greg k-h
> 
> From the previous feedback that I received on this patch it seems that
> watchdog core is not intended to be used for this type of driver. This
> watchdog device tracks the elapsed time on a per-cpu basis,
> since KVM schedules vCPUs independently. Watchdog core is not intended
> to detect CPU stalls and the drivers don't have a notion of CPU.

I must say that I don't really get the objection against the watchdog
approach. OK, there is no userspace aspect to this.  But we already
use watchdogs for more than just userspace (reboot is one of the major
use cases).

There already are per-CPU watchdog in the tree: see how the
fsl-ls208xa platform has one SP805 per CPU (8 of them in total). As
far as I can tell, there was no objection to this. So what is special
about this one?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

On Sat, Apr 23, 2022 at 10:36:56AM +0100, Marc Zyngier wrote:
> On Sat, 23 Apr 2022 10:02:24 +0100,
> Sebastian Ene <sebastianene@google.com> wrote:
> > 
> > On Sat, Apr 23, 2022 at 08:51:16AM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Apr 22, 2022 at 02:19:48PM +0000, Sebastian Ene wrote:
> > > > This adds a mechanism to detect stalls on the guest vCPUS by creating a
> > > > per CPU hrtimer which periodically 'pets' the host backend driver.
> > > > 
> > > > This device driver acts as a soft lockup detector by relying on the host
> > > > backend driver to measure the elapesed time between subsequent 'pet' events.
> > > > If the elapsed time doesn't match an expected value, the backend driver
> > > > decides that the guest vCPU is locked and resets the guest. The host
> > > > backend driver takes into account the time that the guest is not
> > > > running. The communication with the backend driver is done through MMIO
> > > > and the register layout of the virtual watchdog is described as part of
> > > > the backend driver changes.
> > > > 
> > > > The host backend driver is implemented as part of:
> > > > https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3548817
> > > > 
> > > > Changelog v2:
> > > >  - move the driver to misc as this does not cope with watchdog core
> > > >    subsystem
> > 
> > Hello Greg,
> > 
> > > 
> > > Wait, why does it not cope with it?  That's not documented anywhere in
> > > your patch that adds the driver.  In fact, most of the text here needs
> > > to be in the changelog for the driver submission, not thrown away in the
> > > 00/XX email that will never end up in the kernel tree.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > From the previous feedback that I received on this patch it seems that
> > watchdog core is not intended to be used for this type of driver. This
> > watchdog device tracks the elapsed time on a per-cpu basis,
> > since KVM schedules vCPUs independently. Watchdog core is not intended
> > to detect CPU stalls and the drivers don't have a notion of CPU.

Hello Marc,

> 
> I must say that I don't really get the objection against the watchdog
> approach. OK, there is no userspace aspect to this.  But we already
> use watchdogs for more than just userspace (reboot is one of the major
> use cases).
> 
> There already are per-CPU watchdog in the tree: see how the
> fsl-ls208xa platform has one SP805 per CPU (8 of them in total). As
> far as I can tell, there was no objection to this. So what is special
> about this one?

I think the difference is in the fact that this driver expects hrtimers
which are CPU binded to execute the periodic watchdog 'pet'. We would
require a strong thread affinity setting if we rely on userspace to
do this 'pet' operation.

Thanks,
Sebastian

> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.