Hi Igor, On 5/28/26 4:37 PM, Igor Mammedov wrote: > On Thu, 28 May 2026 11:59:06 +0200 > Eric Auger <[email protected]> wrote: > >> Hi, >> >> On 5/28/26 11:04 AM, Eric Auger wrote: >>> Hi Igor, >>> >>> On 5/27/26 2:49 PM, Igor Mammedov wrote: >>>> On Tue, 26 May 2026 15:58:08 +0200 >>>> Eric Auger <[email protected]> wrote: >>>> >>>>> Hi Igor, >>>>> >>>>> On 3/3/26 10:25 AM, 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. >>>>> I thank that in general Peter is not a big fan of dynamic instantiation >>>>> of sysbus devices. Don't you have means to instantiate it statically in >>>>> arm virt memory map? > > On Peter's part, there weren't any objection to the idea using -device. > (neither in v1 nor in this revision). > > >>>> /me neither >>>> >>>> in previous version there was a machine option to enable the feature >>>> to that was rejected in favor of device. >>>> >>>> in this case for platform -device of sysbus based device, I don't think >>>> we can do better that dynamic sysbus. >>> >>> sorry I may have missed some previous discussion details. As far as I >>> can read, I understand you could instantiate the wdt in a free MMIO >>> slot, unconfitionnaly just as it is done in hw/arm/sbsa-ref.c. Maybe add >>> a compat to avoid having it by default on older machine types. Then >>> about the type of the integration gtdt or wat, can't you have a device >>> compat? >> After further reading of the previous version thread, I wonder if you >> shouldn't always instantiate the watchdog in the machine instead. >> >> Add a device property allowing to switch between GWDT or WDAT version. >> >> In arm virt, >> >> 1) if you discover the GWDT version is selected (default) you generate >> both dt and GTDT ACPI table >> 2) if you discover the WDAT version is selected, you only generate WDAT >> ACPI description and you output an error in case of DT boot. >> >> You may add a machine no_wdt flag to handle compats for older machine >> types if needed. > > rereading v1 thread it seems that there weren't much objection to adding > a dedicated virt machine property to enable 'wdat' [1] /like what is done to > q35/. > > problem was in conflating too generic whatchdog=none/auto/native/acpi into > one knob and discussion quickly went down rabbit hole of semantics. > > For Q35, we don't really have a choice as it's already builtin device, > but for arm/virt we can do better than a bunch of compats and proxy > properties. (as Markus noted in v1 review, we've done that/still do > but it doesn't scale well) > > I can go old route and revert to simplified v1 that would: > * make gwdt builtin device on arm/virt board > (i.e. default to SBSA native watchdog with GTDT ACPI table > and relevant FDT entries) would make sense to me. > * add a machine.gwdt-wdat=on|off property > 1. that will make gwdt to use 1KHz frequency > 2. enable WDAT ACPI table > 3. disable GTDT ACPI table and relevant FDT entries > as they are mutually exclusive with #1 point > * add a compat knob to disable gwdt on old machine types. Either that or device prop setting as we have for ACPI PCI HP (technique you advocated at some point). > > Re-spinning that wouldn't take much effort, all the code already exist, > just needs mostly reshuffling. > > 1) I think Daniel was ok having something like gwdt-wdat=on|off > from libvirt pov. > > That said, > Having watchdog as an optional -device is much cleaner compared to > adding knobs to machine (both internal compats and external to enable > specific watchdog). I am OK with that too. However in general I think Peter prefers having them statically instantiated if possible. All the more so I think that having a watchdog, always instantiated, in its GTDT/FDT form (not WDAT ACPI only) in machvirt would make sense in general and does not make much trouble in terms of MMIO layout.> Good (well bad actualy) example of compats/props is set of > ITS ones, one eventually can untangle what depends on what > and how that works together, but I'd rather avoid doing that. > > Perhaps I went overboard with move to -device. > Anyways, all I can do is suggest in my opinion a better way, > but it's upto arm folks to pick a preferred approach. > >> Would that make sense? >> >> Otherwise, can you point me the msgid where the -device was prefered? > Sorry, rereading v1 thread, -device was just an idea to try > (but by no means a preferred/consensus one). This is what I understood too.
Thanks Eric > >> >> Thanks >> >> Eric >>> >>> Thanks >>> >>> Eric >>> >>>> >>>> >>>>> >>>>> Eric >>>>>> >>>>>> Tested with Fedora 43: >>>>>> FDT: -M virt,acpi=off -device sbsa_gwdt >>>>>> ACPI: -M virt -device sbsa_gwdt >>>>>> >>>>>> Signed-off-by: Igor Mammedov <[email protected]> >>>>>> --- >>>>>> hw/arm/Kconfig | 1 + >>>>>> hw/arm/virt-acpi-build.c | 33 +++++++++++++++++++++++++++++++-- >>>>>> hw/arm/virt.c | 2 ++ >>>>>> hw/core/sysbus-fdt.c | 32 ++++++++++++++++++++++++++++++++ >>>>>> hw/watchdog/sbsa_gwdt.c | 1 + >>>>>> 5 files changed, 67 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig >>>>>> index c66c452737..1222efadd1 100644 >>>>>> --- a/hw/arm/Kconfig >>>>>> +++ b/hw/arm/Kconfig >>>>>> @@ -36,6 +36,7 @@ config ARM_VIRT >>>>>> select VIRTIO_MEM_SUPPORTED >>>>>> select ACPI_CXL >>>>>> select ACPI_HMAT >>>>>> + select WDT_SBSA >>>>>> >>>>>> config CUBIEBOARD >>>>>> bool >>>>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >>>>>> index 719d2f994e..9c14b1d4d5 100644 >>>>>> --- a/hw/arm/virt-acpi-build.c >>>>>> +++ b/hw/arm/virt-acpi-build.c >>>>>> @@ -64,6 +64,7 @@ >>>>>> #include "hw/virtio/virtio-acpi.h" >>>>>> #include "target/arm/cpu.h" >>>>>> #include "target/arm/multiprocessing.h" >>>>>> +#include "hw/watchdog/sbsa_gwdt.h" >>>>>> >>>>>> #define ARM_SPI_BASE 32 >>>>>> >>>>>> @@ -863,6 +864,8 @@ build_gtdt(GArray *table_data, BIOSLinker *linker, >>>>>> VirtMachineState *vms) >>>>>> const uint32_t irqflags = 0; /* Interrupt is Level triggered */ >>>>>> AcpiTable table = { .sig = "GTDT", .rev = 3, .oem_id = vms->oem_id, >>>>>> .oem_table_id = vms->oem_table_id }; >>>>>> + uint32_t gtdt_start = table_data->len; >>>>>> + Object *wdt = object_resolve_type_unambiguous(TYPE_WDT_SBSA, NULL); >>>>>> >>>>>> acpi_table_begin(&table, table_data); >>>>>> >>>>>> @@ -893,10 +896,15 @@ build_gtdt(GArray *table_data, BIOSLinker *linker, >>>>>> VirtMachineState *vms) >>>>>> build_append_int_noprefix(table_data, irqflags, 4); >>>>>> /* CntReadBase Physical address */ >>>>>> build_append_int_noprefix(table_data, 0xFFFFFFFFFFFFFFFF, 8); >>>>>> + >>>>>> /* Platform Timer Count */ >>>>>> - build_append_int_noprefix(table_data, 0, 4); >>>>>> + build_append_int_noprefix(table_data, wdt ? 1 : 0, 4); >>>>>> /* Platform Timer Offset */ >>>>>> - build_append_int_noprefix(table_data, 0, 4); >>>>>> + build_append_int_noprefix(table_data, >>>>>> + wdt ? (table_data->len - gtdt_start) + >>>>>> + 4 + 4 + 4 /* len of this & following 2 fields to skip */ >>>>>> + : 0, 4); >>>>>> + >>>>>> if (vms->ns_el2_virt_timer_irq) { >>>>>> /* Virtual EL2 Timer GSIV */ >>>>>> build_append_int_noprefix(table_data, >>>>>> ARCH_TIMER_NS_EL2_VIRT_IRQ, 4); >>>>>> @@ -906,6 +914,27 @@ build_gtdt(GArray *table_data, BIOSLinker *linker, >>>>>> VirtMachineState *vms) >>>>>> build_append_int_noprefix(table_data, 0, 4); >>>>>> build_append_int_noprefix(table_data, 0, 4); >>>>>> } >>>>>> + >>>>>> + /* ACPI 6.5 spec: 5.2.25.2 ARM Generic Watchdog Structure (Table >>>>>> 5-124) */ >>>>>> + if (wdt) { >>>>>> + PlatformBusDevice *pbus = >>>>>> PLATFORM_BUS_DEVICE(vms->platform_bus_dev); >>>>>> + SysBusDevice *sd = SYS_BUS_DEVICE(wdt); >>>>>> + hwaddr mmio_base = vms->memmap[VIRT_PLATFORM_BUS].base; >>>>>> + hwaddr rbase = mmio_base + platform_bus_get_mmio_addr(pbus, sd, >>>>>> 0); >>>>>> + hwaddr cbase = mmio_base + platform_bus_get_mmio_addr(pbus, sd, >>>>>> 1); >>>>>> + int irq = ARM_SPI_BASE + vms->irqmap[VIRT_PLATFORM_BUS] + >>>>>> + platform_bus_get_irqn(pbus, sd, 0); >>>>>> + >>>>>> + build_append_int_noprefix(table_data, 1 /* Type: Watchdog GT >>>>>> */, 1); >>>>>> + build_append_int_noprefix(table_data, 28 /* Length */, 2); >>>>>> + build_append_int_noprefix(table_data, 0, 1); /* Reserved */ >>>>>> + /* RefreshFrame Physical Address */ >>>>>> + build_append_int_noprefix(table_data, rbase, 8); >>>>>> + /* WatchdogControlFrame Physical Address */ >>>>>> + build_append_int_noprefix(table_data, cbase, 8); >>>>>> + build_append_int_noprefix(table_data, irq, 4); /* Watchdog >>>>>> Timer GSIV */ >>>>>> + build_append_int_noprefix(table_data, 0, 4); /* Watchdog Timer >>>>>> Flags */ >>>>>> + } >>>>>> acpi_table_end(linker, &table); >>>>>> } >>>>>> >>>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >>>>>> index cab2e21e8a..a87991b591 100644 >>>>>> --- a/hw/arm/virt.c >>>>>> +++ b/hw/arm/virt.c >>>>>> @@ -95,6 +95,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_defaults[] = { >>>>>> { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "48" }, >>>>>> @@ -3474,6 +3475,7 @@ static void virt_machine_class_init(ObjectClass >>>>>> *oc, const void *data) >>>>>> machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE); >>>>>> machine_class_allow_dynamic_sysbus_dev(mc, TYPE_UEFI_VARS_SYSBUS); >>>>>> machine_class_allow_dynamic_sysbus_dev(mc, TYPE_ARM_SMMUV3); >>>>>> + machine_class_allow_dynamic_sysbus_dev(mc, TYPE_WDT_SBSA); >>>>>> #ifdef CONFIG_TPM >>>>>> machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS); >>>>>> #endif >>>>>> diff --git a/hw/core/sysbus-fdt.c b/hw/core/sysbus-fdt.c >>>>>> index 89d0c46445..76aa1bc579 100644 >>>>>> --- a/hw/core/sysbus-fdt.c >>>>>> +++ b/hw/core/sysbus-fdt.c >>>>>> @@ -36,6 +36,7 @@ >>>>>> #include "hw/display/ramfb.h" >>>>>> #include "hw/uefi/var-service-api.h" >>>>>> #include "hw/arm/fdt.h" >>>>>> +#include "hw/watchdog/sbsa_gwdt.h" >>>>>> >>>>>> /* >>>>>> * internal struct that contains the information to create dynamic >>>>>> @@ -118,6 +119,36 @@ static int add_uefi_vars_node(SysBusDevice *sbdev, >>>>>> void *opaque) >>>>>> return 0; >>>>>> } >>>>>> >>>>>> +static int add_sbsa_gwdt_node(SysBusDevice *sbdev, void *opaque) >>>>>> +{ >>>>>> + PlatformBusFDTData *data = opaque; >>>>>> + PlatformBusDevice *pbus = data->pbus; >>>>>> + const char *parent_node = data->pbus_node_name; >>>>>> + void *fdt = data->fdt; >>>>>> + uint64_t cbase, rbase; >>>>>> + char *nodename; >>>>>> + int irq; >>>>>> + >>>>>> + cbase = platform_bus_get_mmio_addr(pbus, sbdev, 1); /* control >>>>>> frame */ >>>>>> + rbase = platform_bus_get_mmio_addr(pbus, sbdev, 0); /* refresh >>>>>> frame */ >>>>>> + irq = platform_bus_get_irqn(pbus, sbdev, 0); >>>>>> + >>>>>> + nodename = g_strdup_printf("%s/watchdog@%" PRIx64, parent_node, >>>>>> cbase); >>>>>> + qemu_fdt_add_subnode(fdt, nodename); >>>>>> + >>>>>> + qemu_fdt_setprop_string(fdt, nodename, "compatible", >>>>>> "arm,sbsa-gwdt"); >>>>>> + qemu_fdt_setprop_cells(fdt, nodename, "reg", >>>>>> + cbase, SBSA_GWDT_CMMIO_SIZE, >>>>>> + rbase, SBSA_GWDT_RMMIO_SIZE); >>>>>> + qemu_fdt_setprop_cells(fdt, nodename, "interrupts", >>>>>> + GIC_FDT_IRQ_TYPE_SPI, data->irq_start + irq, >>>>>> + GIC_FDT_IRQ_FLAGS_LEVEL_HI); >>>>>> + qemu_fdt_setprop_cell(fdt, nodename, "timeout-sec", 30); >>>>>> + >>>>>> + g_free(nodename); >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> static int no_fdt_node(SysBusDevice *sbdev, void *opaque) >>>>>> { >>>>>> return 0; >>>>>> @@ -140,6 +171,7 @@ static const BindingEntry bindings[] = { >>>>>> TYPE_BINDING(TYPE_ARM_SMMUV3, no_fdt_node), >>>>>> TYPE_BINDING(TYPE_RAMFB_DEVICE, no_fdt_node), >>>>>> TYPE_BINDING(TYPE_UEFI_VARS_SYSBUS, add_uefi_vars_node), >>>>>> + TYPE_BINDING(TYPE_WDT_SBSA, add_sbsa_gwdt_node), >>>>>> TYPE_BINDING("", NULL), /* last element */ >>>>>> }; >>>>>> >>>>>> 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"; >>>>> >>>> >>> >> >
