Re: [PATCH v2 03/21] x86: q35: generate WDAT ACPI table

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 13 May 2026 11:28:01 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, May 13, 2026 at 11:47:19PM +0800, Zhao Liu wrote:
> > On Tue, Mar 03, 2026 at 10:25:14AM +0100, Igor Mammedov wrote:  
> > > Date: Tue,  3 Mar 2026 10:25:14 +0100
> > > From: Igor Mammedov <imammedo@redhat.com>
> > > Subject: [PATCH v2 03/21] x86: q35: generate WDAT ACPI table
> > > 
> > > It will generate WDAT table [1] customized for TCO watchdog.
> > > This allows Windows guests (Windows Server 2008/Vista+) to
> > > use TCO watchdog using built-in generic driver, which
> > > alleviates need to install vendor specific drivers.
> > > 
> > > Given that enabling it might change guest behaviour
> > > (both Windows/Linux) the feature is disabled by default.
> > > 
> > > Users that need it can enable the feature with
> > > following CLI option.
> > >     -machine wdat=on
> > > 
> > > 1)
> > >    "Hardware Watchdog Timers Design Specification"
> > >     https://uefi.org/acpi 'Watchdog Action Table (WDAT)'
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Reviewed-by: Ani Sinha <anisinha@redhat.com>
> > > ---
> > >  include/hw/acpi/wdat-ich9.h | 15 +++++++
> > >  hw/acpi/meson.build         |  3 +-
> > >  hw/acpi/wdat-ich9-stub.c    | 15 +++++++
> > >  hw/acpi/wdat-ich9.c         | 90 +++++++++++++++++++++++++++++++++++++
> > >  hw/i386/acpi-build.c        | 12 +++++
> > >  5 files changed, 134 insertions(+), 1 deletion(-)
> > >  create mode 100644 include/hw/acpi/wdat-ich9.h
> > >  create mode 100644 hw/acpi/wdat-ich9-stub.c
> > >  create mode 100644 hw/acpi/wdat-ich9.c  
> > 
> > ...
> >   
> > > +void build_ich9_wdat(GArray *table_data, BIOSLinker *linker, const char *oem_id,
> > > +                     const char *oem_table_id, uint64_t tco_base)
> > > +{
> > > +    AcpiTable table = { .sig = "WDAT", .rev = 1, .oem_id = oem_id,
> > > +                        .oem_table_id = oem_table_id };
> > > +    struct AcpiGenericAddress tco_rld =  TCO_REG(tco_base, 0x0, 16);
> > > +    struct AcpiGenericAddress tco2_sts = TCO_REG(tco_base, 0x6, 16);
> > > +    struct AcpiGenericAddress tco1_cnt = TCO_REG(tco_base, 0x8, 16);
> > > +    struct AcpiGenericAddress tco_tmr =  TCO_REG(tco_base, 0x12, 16);
> > > +
> > > +    acpi_table_begin(&table, table_data);
> > > +    build_append_int_noprefix(table_data, 0x20, 4); /* Watchdog Header Length */
> > > +    build_append_int_noprefix(table_data, 0xff, 2); /* PCI Segment */
> > > +    build_append_int_noprefix(table_data, 0xff, 1); /* PCI Bus Number */
> > > +    build_append_int_noprefix(table_data, 0xff, 1); /* PCI Device Number */
> > > +    build_append_int_noprefix(table_data, 0xff, 1); /* PCI Function Number */
> > > +    build_append_int_noprefix(table_data, 0, 3);    /* Reserved */
> > > +    /*
> > > +     * limits/resolution are defined by ICH9 TCO spec
> > > +     */
> > > +    build_append_int_noprefix(table_data, 0x258, 4);/* Timer Period, ms */
> > > +    build_append_int_noprefix(table_data, 0x3ff, 4);/* Maximum Count */
> > > +    build_append_int_noprefix(table_data, 0x4, 4);  /* Minimum Count */  
> > 
> > Spec said: The time-out period before the WDT fires is recommended to be at
> > least 5 minutes and is required to be less than 4,294,967,296 count
> > intervals.
> > 
> > Here we encode 0x4 * 0x258 = 2400ms, less than 5 minutes. But I
> > understand because "5 minutes" is just "recommanded" by spec, so 2400ms is
> > okay.
> > 
> > LGTM,
> > 
> > Reviewed-by: Zhao Liu <zhao1.liu@intel.com>  
> 
> 
> maybe a comment why it's shorter?

it's not shorter nor arbitrary value, comments already say it all.
and provide enough context for spec lookup:
 WDAT:
   /Timer Period/	Contains the period of one timer count (in milliseconds).

 it's timer resolution.
   iTCO spec says: "The timer is clocked at approximately 0.6 seconds"

   and then MIN/MAX aren't default value either, they are limits per iTCO spec,
   as comment above says. The guest OS sets actual value within declared here constrains.

So no extra comments were added (just enough for spec lookup).
But my view is obviously biased,
just say what more you'd like to see in comments and I'll post fixup.