Re: [PATCH 08/11] arm: virt: create GWDT watchdog paired with WDAT ACPI table

[Markus Armbruster <armbru@redhat.com>]

Igor Mammedov <imammedo@redhat.com> writes:

> On Wed, 25 Feb 2026 15:19:56 +0000
> Daniel P. Berrangé <berrange@redhat.com> wrote:
>
>> On Thu, Feb 19, 2026 at 01:17:51PM +0100, Igor Mammedov wrote:
>> > On Wed, 18 Feb 2026 19:08:36 +0000
>> > Peter Maydell <peter.maydell@linaro.org> wrote:
>> >   
>> > > On Fri, 6 Feb 2026 at 13:15, Igor Mammedov <imammedo@redhat.com> wrote:  
>> > > >
>> > > > Add SBSA generic watchdog to virt machine type with
>> > > > all necessary wiring for ACPI watchdog. Which includes
>> > > > setting its frequency to 1KHz (max that WDAT is able to handle).
>> > > >
>> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> > > > ---    
>> > >   
>> > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> > > > index 390845c503..caf5700ed2 100644
>> > > > --- a/hw/arm/virt.c
>> > > > +++ b/hw/arm/virt.c
>> > > > @@ -93,6 +93,7 @@
>> > > >  #include "hw/cxl/cxl.h"
>> > > >  #include "hw/cxl/cxl_host.h"
>> > > >  #include "qemu/guest-random.h"
>> > > > +#include "hw/watchdog/sbsa_gwdt.h"
>> > > >
>> > > >  static GlobalProperty arm_virt_compat[] = {
>> > > >      { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "48" },
>> > > > @@ -194,6 +195,8 @@ static const MemMapEntry base_memmap[] = {
>> > > >      [VIRT_PVTIME] =             { 0x090a0000, 0x00010000 },
>> > > >      [VIRT_SECURE_GPIO] =        { 0x090b0000, 0x00001000 },
>> > > >      [VIRT_ACPI_PCIHP] =         { 0x090c0000, ACPI_PCIHP_SIZE },
>> > > > +    [VIRT_GWDT_REFRESH] =       { 0x090d0000, 0x00001000 },
>> > > > +    [VIRT_GWDT_CONTROL] =       { 0x090d1000, 0x00001000 },
>> > > >      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>> > > >      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>> > > >      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
>> > > > @@ -245,12 +248,32 @@ static const int a15irqmap[] = {
>> > > >      [VIRT_GPIO] = 7,
>> > > >      [VIRT_UART1] = 8,
>> > > >      [VIRT_ACPI_GED] = 9,
>> > > > +    [VIRT_GWDT_WS0] = 10,
>> > > >      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>> > > >      [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
>> > > >      [VIRT_SMMU] = 74,    /* ...to 74 + NUM_SMMU_IRQS - 1 */
>> > > >      [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
>> > > >  };
>> > > >
>> > > > +static void create_wdt(const VirtMachineState *vms)
>> > > > +{
>> > > > +    hwaddr rbase = vms->memmap[VIRT_GWDT_REFRESH].base;
>> > > > +    hwaddr cbase = vms->memmap[VIRT_GWDT_CONTROL].base;
>> > > > +    int irq = vms->irqmap[VIRT_GWDT_WS0];
>> > > > +    DeviceState *dev = qdev_new(TYPE_WDT_SBSA);
>> > > > +    SysBusDevice *s = SYS_BUS_DEVICE(dev);
>> > > > +
>> > > > +    /*
>> > > > +     * Set watchdog tick freq to 1Kz as it's the max WDAT driver
>> > > > +     * is able to handle.
>> > > > +     */
>> > > > +    qdev_prop_set_uint64(dev, "clock-frequency", 1000 /* 1KHz */);
>> > > > +    sysbus_realize_and_unref(s, &error_fatal);
>> > > > +    sysbus_mmio_map(s, 0, rbase);
>> > > > +    sysbus_mmio_map(s, 1, cbase);
>> > > > +    sysbus_connect_irq(s, 0, qdev_get_gpio_in(vms->gic, irq));
>> > > > +}    
>> > > 
>> > > Please can you also add support for exposing this device
>> > > in the device tree ?  
>> > 
>> > It's possible,
>> > but we probably should not enable it if acpi variant was requested,
>> > to avoid confusion on guest side.
>> > 
>> > For Windows it doesn't really mater, for linux it does.
>> > on x86 linux guest uses a quirk to disable native iTCO watchdog
>> > in favor of WDAT one if later is present.
>> > I assume quirk is not desirable so we should expose only a preferred
>> > variant.
>> >   
>> > >   
>> > > > +
>> > > >  static void create_randomness(MachineState *ms, const char *node)
>> > > >  {
>> > > >      struct {
>> > > > @@ -2515,6 +2538,9 @@ static void machvirt_init(MachineState *machine)
>> > > >      vms->highmem_ecam &= (!firmware_loaded || aarch64);
>> > > >
>> > > >      create_rtc(vms);
>> > > > +    if (machine->acpi_watchdog) {
>> > > > +        create_wdt(vms);
>> > > > +    }    
>> > > 
>> > > Can we have a command line option name that isn't ACPI
>> > > specific, please? There's nothing inherent to ACPI about
>> > > "I would like a watchdog device".  
>> > 
>> > that is specifically asking for ACPI flavor being used/configured.
>> > acpi specific option conflates 2 things:
>> >    1. watchdog device creation (of the board choice) with properties tuned for WDAT usage
>> >    2. how to expose it on firmware level (in this case ACPI WDAT table)
>> > 
>> > other option, I've considered was
>> >  -device some_wd[,fw=dt|acpi|none]
>> > but then 'fw' is not exactly device property but rather a machine one
>> > (we can 'abuse' it/use as a proxy of cause).
>> > However it doesn't work for boards that have builtin watchdog.  
>> 
>> Right, if there was a case where we were already using -device to
>> create the watchdog, then I would suggest having a simple
>> 'wadt=on|off' property as standard for any watchdog wanting that
>> ACPI abstraction wrapper.
>> 
>> That could (hypothetically) be the case if we had chosen to add
>> WADT suport backed by i6300esb, eg -device i6300esb,wadt=on|off
>> 
>> For cases built-in to the machine type, I'd suggest <type>-wadt=on|off
>> 
>> eg for Q35,  "-machine q35,itco-wadt=on|off"
>> 
>> for virt, '-machine virt,gwdt-wadt=on|off'
>
> CCin Markus since it touches QAPI stuff.
>
> For respin, I've went ahead with a bit more generic approach
> (as a user, I wouldn't really care what kind of built-in watchdog board ships,
> and having 1 property instead total instead of machine specific ones makes it
> easier for mgmt layer to deal with).
>
> here is current idea:
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 6411e68856..a0c6719b58 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1257,6 +1271,14 @@ static void machine_class_init(ObjectClass *oc, const void *data)
>          NULL, NULL);
>      object_class_property_set_description(oc, "memory",
>          "Memory size configuration");
> +
> +    object_class_property_add_enum(oc, "watchdog", "WatchdogType",
> +        &WatchdogType_lookup,
> +        machine_get_watchdog, machine_set_watchdog);
> +     object_class_property_set_description(oc, "watchdog",
> +        "Use: auto (watchdog is configured according board defaults),"
> +        " off (disabled), native (built-in watchdog), wdat (ACPI based watchdog)]. "
> +        " Default: auto");
>  }

Adds the property to abstract base type "machine", i.e. every machine
has it.

Fundamentally assumes there is at most one onboard watchdog of interest.

I'm lacking context, please bear with me: who or what is going to set
this property, and for what purpose?

> diff --git a/qapi/machine-common.json b/qapi/machine-common.json
> index 92e84dfb14..7f5e10340f 100644
> --- a/qapi/machine-common.json
> +++ b/qapi/machine-common.json
> @@ -9,6 +9,23 @@
>  # Common machine types
>  # ********************
>  ##
> +##
> +# @WatchdogType:
> +#
> +# On board watchdog configuration.
> +#
> +# @auto: Watchdog is configured according to board defaults.

As far as I can tell, we don't document "boards" (machine types)
anywhere, let alone "board defaults".  Not this patch's fault, of
course.  The meaning of @auto remains unclear.

> +#
> +# @off: Watchdog disabled (ARM).
> +#
> +# @native: Native watchdog (arm/virt: sbsa-gwdt, x86/q53: TCO)
> +#
> +# @wdat: ACPI WDAT watchdog.
> +#
> +# Since: 10.2

11.0

> +##
> +{ 'enum': 'WatchdogType',
> +  'data': [ 'auto', 'off', 'native', 'wdat' ] }

What do managament applications need to know about onboard watchdog
configuration?

Do they need to know what @auto means for the machine type at hand?

Do they ever need to pick a value?  Do they need to know which values
work with the machine type then?

>> Q35 is easier as we rely on the guest quirks to disable direct
>> access via both paths.
>> 
>> With GWDT we need to decide if gwdt-wadt=on should imply DTB
>> disabled, or if we just expose both & rely on a guest quirk
>> or worst case, need a separate property to toggle DTB too.
>> 
>> > I've used device property for x86 Q35 TCO watchdog and
>> > then using -global to enable it as workaround in previous version.
>> > Ugly but doable workaround. It puts a burden on users to learn how
>> > to configure it for each support watchdog/board combo.
>> > It's nightmare to discover/maintain though.

Our unsolved "how to configure onboard devices" problem bites again.

-global can be pressed into service when there's just once instance of
the device type.  The drawbacks you describe are real.  I want to see
less of it, not more.

>> > Hence in this version, I've used a generic machine property approach
>> > that exposes feature in uniform way across boards.

We've done that before.  It works, but I don't think it scale up to a
complete solution (I understand you are not after a complete solution
here, and that's okay).

For instance, machine pc-q35-FOO has two onboard cfi.pflash01 devices.
To enable configuration of their block backends, we alias their "drive"
properties as machine properties "pflash0" and "pflash1".  Their other
properties remain inaccessible.

Adding a few alias properties to the machine type is workable.  A bit ad
hoc, perhaps.  Bothersome to document, so we largely don't.

Adding alias properties for everything anyone could possibly want to
configure feels unworkable.

This is *not* an objection to solving the problem at hand with a machine
property.

>> > enum might work, but ...
>> >   machine.watchdog = [acpi | what else could we put here???]
>> > 
>> > I'm not married to a way how we expose/configure it and will rewrite
>> > it to something that works for majority.
>> >   
>> > > thanks
>> > > -- PMM
>> > >   
>> >   
>> 
>> With regards,
>> Daniel