Re: [PATCH v2 09/21] arm: virt: create sbsa_gwdt watchdog
On Thu, 4 Jun 2026 18:48:28 +0200
Eric Auger 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.
> don't we use '-' instead of '_' in general. At least this is the case
> for dynamically instantiable arm-smmuv3 device. Better to align
device names are usually ABI, when the get leaked to users,
and another place it might break would be migration.
I agree with s/_/-/, but let me check/test. if it doesn't break
migration, I think we are good to go with rename.
>
> also this is the terminology used in the fdt compat string
> $id: http://devicetree.org/schemas/watchdog/arm,sbsa-gwdt.yaml#
> const: arm,sbsa-gwdt
>
> >
> > Tested with Fedora 43:
> > FDT: -M virt,acpi=off -device sbsa_gwdt
> > ACPI: -M virt -device sbsa_gwdt
> >
> > Signed-off-by: Igor Mammedov
> > ---
> > 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, 0x, 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) */
> 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 */
> level,
Re: [PATCH v2 09/21] arm: virt: create sbsa_gwdt watchdog
On 6/4/26 6:48 PM, Eric Auger 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.
> don't we use '-' instead of '_' in general. At least this is the case
> for dynamically instantiable arm-smmuv3 device. Better to align
>
> also this is the terminology used in the fdt compat string
> $id: http://devicetree.org/schemas/watchdog/arm,sbsa-gwdt.yaml#
> const: arm,sbsa-gwdt
I realize this is not your choice as the device is already named that
way. #define TYPE_WDT_SBSA "sbsa_gwdt"
So I don't think we can anything about that anymore
although in general other devices use "-" I think
#define TYPE_UEFI_VARS_SYSBUS"uefi-vars-sysbus"
Eric
>
>>
>> Tested with Fedora 43:
>> FDT: -M virt,acpi=off -device sbsa_gwdt
>> ACPI: -M virt -device sbsa_gwdt
>>
>> Signed-off-by: Igor Mammedov
>> ---
>> 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, 0x, 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)
>> */
> 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 */
> level, active high, non secure
>> +}
>> acpi_table_end(linker, &table);
>> }
>>
>>
Re: [PATCH v2 09/21] arm: virt: create sbsa_gwdt watchdog
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.
don't we use '-' instead of '_' in general. At least this is the case
for dynamically instantiable arm-smmuv3 device. Better to align
also this is the terminology used in the fdt compat string
$id: http://devicetree.org/schemas/watchdog/arm,sbsa-gwdt.yaml#
const: arm,sbsa-gwdt
>
> Tested with Fedora 43:
> FDT: -M virt,acpi=off -device sbsa_gwdt
> ACPI: -M virt -device sbsa_gwdt
>
> Signed-off-by: Igor Mammedov
> ---
> 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, 0x, 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)
> */
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
> */
level, active high, non secure
> +}
> 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
Re: [PATCH v2 09/21] arm: virt: create sbsa_gwdt watchdog
On Mon, 1 Jun 2026 18:37:05 +0200 Eric Auger wrote: > Hi Igor, > > On 5/28/26 4:37 PM, Igor Mammedov wrote: > > On Thu, 28 May 2026 11:59:06 +0200 > > Eric Auger 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 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. forgot about why it's not default ATM, the reason being that native/default GWDT causes issues on Windows (only WDAT variant works). the last 2 patches trying to do some damage control, but what we can do on qemu side is limited. and builtin gwdt we would end up with a mess of gwdt=on|of and wdat=o|n, So I'd rather have watchdog as an optional device included to VM when asked by user. With side benefit reducing attack surface when device is not needed compared to built-in by default variant. > > * 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). that was for symmetry with existing acpi pci hp feature on x86 if I recall correctly. There is no much options for builtin devices, we either * use -global to tweak them (historical variant) * more recent trend is proxy property in -machine both not great variants, I've tried in v1 to propose generic -machine watchdog=foo to at least limit a zoo of options, but that was rejected. > > > > 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
Re: [PATCH v2 09/21] arm: virt: create sbsa_gwdt watchdog
Hi Igor, On 5/28/26 4:37 PM, Igor Mammedov wrote: > On Thu, 28 May 2026 11:59:06 +0200 > Eric Auger 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 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 >> --- >> hw/arm/Kconfig | 1 + >>
Re: [PATCH v2 09/21] arm: virt: create sbsa_gwdt watchdog
On Thu, 28 May 2026 11:59:06 +0200 Eric Auger 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 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) * 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. 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). 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). > > 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 > --- > 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 >
Re: [PATCH v2 09/21] arm: virt: create sbsa_gwdt watchdog
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 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?
>>
>> /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.
Would that make sense?
Otherwise, can you point me the msgid where the -device was prefered?
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
---
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, 0x, 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) {
Re: [PATCH v2 09/21] arm: virt: create sbsa_gwdt watchdog
Hi Igor,
On 5/27/26 2:49 PM, Igor Mammedov wrote:
> On Tue, 26 May 2026 15:58:08 +0200
> Eric Auger 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?
>
> /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?
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
>>> ---
>>> 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, 0x, 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 */
>>> +
Re: [PATCH v2 09/21] arm: virt: create sbsa_gwdt watchdog
On Tue, 26 May 2026 15:58:08 +0200
Eric Auger 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?
/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.
>
> Eric
> >
> > Tested with Fedora 43:
> > FDT: -M virt,acpi=off -device sbsa_gwdt
> > ACPI: -M virt -device sbsa_gwdt
> >
> > Signed-off-by: Igor Mammedov
> > ---
> > 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, 0x, 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
Re: [PATCH v2 09/21] arm: virt: create sbsa_gwdt watchdog
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?
Eric
>
> Tested with Fedora 43:
> FDT: -M virt,acpi=off -device sbsa_gwdt
> ACPI: -M virt -device sbsa_gwdt
>
> Signed-off-by: Igor Mammedov
> ---
> 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, 0x, 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);
>
