Re: [PATCH v2 09/21] arm: virt: create sbsa_gwdt watchdog

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 4 Jun 2026 18:48:28 +0200
Eric Auger <eauger@redhat.com> 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 <imammedo@redhat.com>
> > ---
> >  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) */  
> 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 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";  
> Thanks
> 
> Eric
> 
>