On Fri, 27 Feb 2026 08:24:42 +0100 Markus Armbruster <[email protected]> wrote:
> Igor Mammedov <[email protected]> writes: > > > On Wed, 25 Feb 2026 15:19:56 +0000 > > Daniel P. Berrangé <[email protected]> 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 <[email protected]> wrote: > >> > > >> > > On Fri, 6 Feb 2026 at 13:15, Igor Mammedov <[email protected]> > >> > > 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 <[email protected]> > >> > > > --- > >> > > > >> > > > 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. in past (which might be different from now 'policies'), we used to add a property to abstract machine when there is more than 1 board/target that consumes it. This follows that trend. If that is not desirable now, I can move it specific boards at the cost of some boiler-plate code duplication. Boards of interest in this series are arm/virt and q35 > Fundamentally assumes there is at most one onboard watchdog of interest. there might be, hence default 'auto' to describe various boards default/ implicit watchdog selection. User might set it but is doesn't make much sense to configure already default state. > I'm lacking context, please bear with me: who or what is going to set > this property, and for what purpose? it's for users/mgmt apps that wish to explicitly optin to use watchdog. majority users don't need it (and can leave it at default 'auto' or 'off' state) but depending on usecase, watchdog presence can be a mandatory requirement. and here comes a choice for a layer that knows what guest OS supports to pick a suitable watchdog. - native: requires from user upfront knowledge what built-in watchdog is and if it's supported by guest - wdat: abstracts user from need to know what built-in watchdog is as still requires user to know is guest supports it As of today guest drivers support is in following state: TCO watchdog on Q35 (linux: supported, Windows: not supported) sbsa-gwdt watchdog in arm world - (linux: supported, Windows: broken) wdat abstract watchdog (Q35 and arm) - (linux: supported, Windows: supported) peddling points in favor of wdat (MS and my to some extent) From above selection, wdat is widely supported already for 10 years, and while it's not ideal/limited than purpose built drivers and relies on less that ideal MS spec. It lets use out of box drivers (both Linux and Windows) without need to install ones on guest side. From QEMU maintenance pov, wdat abstraction uses under the hood native watchdog, but guest OS doesn't care as all it does in wdat driver is executing a series of mmio instructions as specified in WDAT ACPI table. And QEMU can transparently change hidden native watchdog to another one without need to reconfigure guest OS. With deprecation and old machine types being removed we can gradually phase out undesirable watchdog impl and replace it with what we would fancy in that moment. > > 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. would mentioning q35 and arm/virt here be of any help? Suggestions how to deal with it? > > > +# > > +# @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? given it emulates firmware knob (with qemu acting as fw), if watchdog is needed, mgmt would need to pick either native or wdat one based on what works for guest OS. > Do they need to know what @auto means for the machine type at hand? I don't think they would need to know what 'auto' means. > Do they ever need to pick a value? Do they need to know which values > work with the machine type then? tentatively answer to both questions is yes. not impl. yet though) valid states per board would be: q35 { auto -> native, wdat, off - not supported } arm/virt {auto -> off, native, wdat, off } other boards might have different mapping. Easiest way to check for this is duplicated per board 'watchdog' property with per board setter that verifies setting. Or in case a of genric machine property, a callback to let boards add their own logic in generic property setter. > >> 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. using aliases might work for q35 since it doesn't have 'off' option, i.e. we can add 'wdat' property to TCO watchdog and then let alias it to q35 as watchdog property: { native -> tco.wdat = off, wdat -> tco.wdat = on } for arm/virt it would look like, adding wdat property to sbsa-gwdt watchdog and then aliasing it machine's 'watchdog' property with following mapping { off - don't create watchdog at all, native -> sbsa-gwdt.wdat=off, wdat -> sbsa-gwdt.wdat=on } it doesn't seem like a workable approach > >> > 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 >
