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

[Igor Mammedov]

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.




> +
> +    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;