Re: [PATCH v3 04/17] arm: virt: create sbsa-gwdt watchdog

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 29 Jun 2026 10:37:59 +0200
Eric Auger <eauger@redhat.com> wrote:

> Hi Igor,
> 
> On 6/24/26 12:28 PM, Igor Mammedov wrote:
> > Allow to use SBSA generic watchdog with virt machine type.
> > (includes conditional generation of corresponding FDT and
> > ACPI GTDT descriptors)
> > 
> > Use '-device sbsa-gwdt' to command line to enable it.
> > 
> > Instead of using dynamic sysbus infra to wire up MMIO/IRQ/FDT,
> > statically assign resources in machine's mem/irq maps and wire
> > them up at device (pre_)plug handlers. It's similar to dynamic
> > sysbus wiring, modulo resources are nailed down statically,
> > and wiring is limited to virt machine only.
> > (Benefit is that tests don't break anymore on rebase due to
> > address being stable)
> > 
> > Tested with Fedora 43:
> >   FDT: -M virt,acpi=off -device sbsa-gwdt
> >   ACPI: -M virt -device sbsa-gwdt
> > 
> > Note:
> > Windows sees GTDT, initializes watchdog but instead pinging WRR
> > it sets/advances WOR to way too large value, so it's never going
> > to trigger watchdog reboot (it's Windows driver issue though).
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v3:
> >   - (Eric) assign MMIO/IRQ statically and abandon most of
> >      dynamic sysbus machinery.
> >   - (Peter) set watchdog freq to system clock explicitly,
> >     machine version compat won't work in case host is not runing
> >     1GHz clock. (Tested on Jetson machine)
> > ---
> >  include/hw/arm/virt.h    |  3 +++
> >  hw/arm/Kconfig           |  1 +
> >  hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++++--
> >  hw/arm/virt.c            | 37 +++++++++++++++++++++++++++++++++++++
> >  hw/core/sysbus-fdt.c     |  2 ++
> >  hw/watchdog/sbsa_gwdt.c  |  1 +
> >  6 files changed, 71 insertions(+), 2 deletions(-)
> > 
[...]
> > @@ -3820,6 +3824,10 @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> >          qlist_append_str(reserved_regions, resv_prop_str);
> >          qdev_prop_set_array(dev, "reserved-regions", reserved_regions);
> >          g_free(resv_prop_str);
> > +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_WDT_SBSA)) {
> > +        uint64_t cntfrq = object_property_get_int(OBJECT(qemu_get_cpu(0)),
> > +                                                  "cntfrq", &error_abort);  
> missing extra line.
> > +        qdev_prop_set_uint64(dev, "clock-frequency", cntfrq);  
> 
> Can you explain why you cannot do that in virt_machine_device_plug_cb()?

sure,
all prep work, typically is done at pre_plug time. (it's stage we are allowed to fail)

While at plug time we typically do final (post realize) wiring/exposing model to guest
(no allowed to fail (I need to do cleanup and remove errp from all plug handlers
but that is orthogonal to series))

> >      } else if (object_dynamic_cast(OBJECT(dev), TYPE_ARM_SMMUV3)) {
> >          if (vms->legacy_smmuv3_present || vms->iommu == VIRT_IOMMU_VIRTIO) {
> >              error_setg(errp, "virt machine already has %s set. "
[...]

> > diff --git a/hw/watchdog/sbsa_gwdt.c b/hw/watchdog/sbsa_gwdt.c
> > index acb970e8b3..c4dd8005b7 100644
> > --- a/hw/watchdog/sbsa_gwdt.c
> > +++ b/hw/watchdog/sbsa_gwdt.c
> > @@ -285,6 +285,7 @@ static void wdt_sbsa_gwdt_class_init(ObjectClass *klass, const void *data)
> >      dc->realize = wdt_sbsa_gwdt_realize;
> >      device_class_set_legacy_reset(dc, wdt_sbsa_gwdt_reset);
> >      dc->hotpluggable = false;
> > +    dc->user_creatable = true;
> >      set_bit(DEVICE_CATEGORY_WATCHDOG, dc->categories);
> >      dc->vmsd = &vmstate_sbsa_gwdt;
> >      dc->desc = "SBSA-compliant generic watchdog device";  
> You may document somewhere that the device can be dynamically
> instantiated, in machvirt alone and this required machine specific
> hardwiring.

Any suggestion wrt where to put it? 

> 
> Thanks
> 
> Eric
>