Thanks for the patch! Some questions/comments:
On Wed, Mar 11, 2020 at 07:08:26PM +0200, Liran Alon wrote:
> From: Elad Gabay <elad.ga...@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.ga...@oracle.com>
> Co-developed-by: Liran Alon <liran.a...@oracle.com>
> Signed-off-by: Liran Alon <liran.a...@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
