Re: [PATCH] acpi: Add Windows ACPI Emulated Device Table (WAET)

[Michael S. Tsirkin]

On Thu, Mar 12, 2020 at 05:27:45PM +0100, Igor Mammedov wrote:
> On Wed, 11 Mar 2020 19:08:26 +0200
> Liran Alon <address@hidden> wrote:
> 
> > From: Elad Gabay <address@hidden>
> > 
> > Microsoft introduced this ACPI table to avoid Windows guests performing
> > various workarounds for device erratas. As the virtual device emulated
> > by VMM may not have the errata.
> > 
> > Currently, WAET allows hypervisor to inform guest about two
> > specific behaviors: One for RTC and the other for ACPI PM Timer.
> > 
> > Support for WAET have been introduced since Windows Vista. This ACPI
> > table is also exposed by other hypervisors, such as VMware, by default.
> > 
> > This patch adds WAET ACPI Table to QEMU. It also makes sure to introduce
> > the new ACPI table only for new machine-types.
> 
> in addition to comments made by Michael ...
> 
> > 
> > Signed-off-by: Elad Gabay <address@hidden>
> > Co-developed-by: Liran Alon <address@hidden>
> > Signed-off-by: Liran Alon <address@hidden>
> > ---
> >  hw/i386/acpi-build.c        | 18 ++++++++++++++++++
> >  hw/i386/pc_piix.c           |  2 ++
> >  hw/i386/pc_q35.c            |  2 ++
> >  include/hw/acpi/acpi-defs.h | 25 +++++++++++++++++++++++++
> >  include/hw/i386/pc.h        |  1 +
> >  5 files changed, 48 insertions(+)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 9c4e46fa7466..29f70741cd96 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -2512,6 +2512,19 @@ build_dmar_q35(GArray *table_data, BIOSLinker 
> > *linker)
> >      build_header(linker, table_data, (void *)(table_data->data + 
> > dmar_start),
> >                   "DMAR", table_data->len - dmar_start, 1, NULL, NULL);
> >  }
> > +
> > +static void
> > +build_waet(GArray *table_data, BIOSLinker *linker)
> see build_hmat_lb() for example how to doc comment for such function
> should look like. Use earliest spec version where table was introduced.
> 
> > +{
> > +    AcpiTableWaet *waet;
> > +
> > +    waet = acpi_data_push(table_data, sizeof(*waet));
> > +    waet->emulated_device_flags = cpu_to_le32(ACPI_WAET_PM_TIMER_GOOD);
> 
> we don't use packed structures for building ACPI tables anymore (there is
> old code that still does but that's being converted when we touch it)
> 
> pls use build_append_int_noprefix() api instead, see build_amd_iommu() as
> an example how to build binary tables using it and how to use comments
> to document fields.
> Basic idea is that api makes function building a table match table's
> description in spec (each call represents a row in spec) and comment
> belonging to a row should contain verbatim field name as used by spec
> so reader could copy/past and grep it easily.


BTW how about a better name for this function?

> 
> 
> 
> > +
> > +    build_header(linker, table_data,
> > +                 (void *)waet, "WAET", sizeof(*waet), 1, NULL, NULL);
> > +}
> > +
> >  /*
> >   *   IVRS table as specified in AMD IOMMU Specification v2.62, Section 5.2
> >   *   accessible here http://support.amd.com/TechDocs/48882_IOMMU.pdf
> > @@ -2859,6 +2872,11 @@ void acpi_build(AcpiBuildTables *tables, 
> > MachineState *machine)
> >                            machine->nvdimms_state, machine->ram_slots);
> >      }
> >  
> > +    if (!pcmc->do_not_add_waet_acpi) {
> > +        acpi_add_table(table_offsets, tables_blob);
> > +        build_waet(tables_blob, tables->linker);
> > +    }
> 
> we typically do not version ACPI table changes (there might be exceptions
> but it should be a justified one).
> ACPI tables are considered to be a part of firmware (even though they are
> generated by QEMU) so on QEMU upgrade user gets a new firmware along with
> new ACPI tables.
> 
> > +
> >      /* Add tables supplied by user (if any) */
> >      for (u = acpi_table_first(); u; u = acpi_table_next(u)) {
> >          unsigned len = acpi_table_len(u);
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 9088db8fb601..2d11a8b50a9c 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -432,9 +432,11 @@ DEFINE_I440FX_MACHINE(v5_0, "pc-i440fx-5.0", NULL,
> >  
> >  static void pc_i440fx_4_2_machine_options(MachineClass *m)
> >  {
> > +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> >      pc_i440fx_5_0_machine_options(m);
> >      m->alias = NULL;
> >      m->is_default = false;
> > +    pcmc->do_not_add_waet_acpi = true;
> >      compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> >      compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
> >  }
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 84cf925cf43a..1e0a726b27a7 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -361,8 +361,10 @@ DEFINE_Q35_MACHINE(v5_0, "pc-q35-5.0", NULL,
> >  
> >  static void pc_q35_4_2_machine_options(MachineClass *m)
> >  {
> > +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> >      pc_q35_5_0_machine_options(m);
> >      m->alias = NULL;
> > +    pcmc->do_not_add_waet_acpi = true;
> >      compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> >      compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
> >  }
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index 57a3f58b0c9a..803c904471d5 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -634,4 +634,29 @@ struct AcpiIortRC {
> >  } QEMU_PACKED;
> >  typedef struct AcpiIortRC AcpiIortRC;
> >  
> > +/*
> > + * Windows ACPI Emulated Devices Table.
> > + * Specification:
> > + * 
> > http://download.microsoft.com/download/7/E/7/7E7662CF-CBEA-470B-A97E-CE7CE0D98DC2/WAET.docx
> > + */
> > +
> > +/*
> > + * Indicates whether the RTC has been enhanced not to require 
> > acknowledgment
> > + * after it asserts an interrupt. With this bit set, an interrupt handler 
> > can
> > + * bypass reading the RTC register C to unlatch the pending interrupt.
> > + */
> > +#define ACPI_WAET_RTC_GOOD      (1 << 0)
> > +/*
> > + * Indicates whether the ACPI PM timer has been enhanced not to require
> > + * multiple reads. With this bit set, only one read of the ACPI PM timer is
> > + * necessary to obtain a reliable value.
> > + */
> > +#define ACPI_WAET_PM_TIMER_GOOD (1 << 1)
> > +
> > +struct AcpiTableWaet {
> > +    ACPI_TABLE_HEADER_DEF
> > +    uint32_t emulated_device_flags;
> > +} QEMU_PACKED;
> > +typedef struct AcpiTableWaet AcpiTableWaet;
> > +
> >  #endif
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 60c988c4a5aa..f1f64e8f45c8 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -100,6 +100,7 @@ typedef struct PCMachineClass {
> >      int legacy_acpi_table_size;
> >      unsigned acpi_data_size;
> >      bool do_not_add_smb_acpi;
> > +    bool do_not_add_waet_acpi;
> >  
> >      /* SMBIOS compat: */
> >      bool smbios_defaults;