On Thu, Mar 12, 2020 at 05:27:45PM +0100, Igor Mammedov wrote:
> On Wed, 11 Mar 2020 19:08:26 +0200
> Liran Alon <liran.a...@oracle.com> 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. 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 <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)
> 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;
