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

["Michael S. Tsirkin" <mst@redhat.com>]

Thanks for the patch! Some questions/comments:

On Wed, Mar 11, 2020 at 07:08:26PM +0200, Liran Alon wrote:
> From: Elad Gabay <elad.gabay@oracle.com>
> 
> 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.

Could you add a bit more info? Why is this so useful we are adding this
by default? How does it change windows behaviour when present?

> It also makes sure to introduce
> the new ACPI table only for new machine-types.

OK and why is that?

> 
> Signed-off-by: Elad Gabay <elad.gabay@oracle.com>
> Co-developed-by: Liran Alon <liran.alon@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  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)
> +{
> +    AcpiTableWaet *waet;
> +
> +    waet = acpi_data_push(table_data, sizeof(*waet));

Can combine with the previous line.

> +    waet->emulated_device_flags = cpu_to_le32(ACPI_WAET_PM_TIMER_GOOD);
> +
> +    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);
> +    }
> +
>      /* 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)
> +

ACPI spec is so huge we really can't add enums for all values,
it just does not scale.


So we switched to a different way to do this: you add e.g. 1 << 1
in the code directly, and put the comments there.


Igor this is becoming a FAQ. Could you write up the way ACPI
generation code should look?


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