|
| From: | Liran Alon |
| Subject: | Re: [PATCH] acpi: Add Windows ACPI Emulated Device Table (WAET) |
| Date: | Thu, 12 Mar 2020 13:30:01 +0200 |
| User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
On 12/03/2020 8:12, Michael S. Tsirkin wrote:
On Thu, Mar 12, 2020 at 01:20:02AM +0200, Liran Alon wrote:On 11/03/2020 22:36, Michael S. Tsirkin wrote:Thanks for the patch! Some questions/comments: On Wed, Mar 11, 2020 at 07:08:26PM +0200, Liran Alon 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.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 changes behavior as documented in the WAET specification linked below (and the comments above the flags definitions). Specifically for ACPI_WAET_PM_TIMER_GOOD (Which is the only bit we set), the guest performs only one read of ACPI PM Timer instead of multiple to obtain it's value. Which improves performance as it removes unnecessary VMExits.Sounds excellent. Pls include this info in the commit log.
Ok. Will do in v2.
This is a quite an old patch of ours that I upstream now to contribute to community. I will need to re-setup such environment for gathering exact performance numbers.As with any performance optimization, pls add a bit of info about how you tested and what kind of speedup was seen.
Having said that, note that there isn't really a trade-off here between better performance or something else. We just expose a bit to guest that says to it: "You don't need to do this useless thing that cause unnecessary VMExits. You can just do this simple operation which is always better because we support it". Therefore, as long as other guests just ignore this ACPI table (Which they do as far as I've seen from the vast variety of instances we have run on production for over 5 years), exposing this just have positive effect.
Also note that besides VMware which expose it by default, you can also see this exposed by default by some cloud hypervisors, such as GCP: [ 0.000000] ACPI: WAET 0x00000000BFFF5CE0 000028 (v01 Google GOOGWAET 00000001 GOOG 00000001)
Windows measures at boot-time if the hardware have "changed too much" since activation. The way it does so, is calculating a "weighted diff score" based on a number of hardware properties.It also makes sure to introduce the new ACPI table only for new machine-types.OK and why is that?As ACPI tables are guest-visible, we should make sure to not change it between machine-types. For example, a change in ACPI tables may invalidate a Windows guest license activation (As platform have changed).I don't think there's something like this taken into account, no.
It is at least documented internally in Ravello that some guests have been witnessed to broke their license activation because of ACPI/SMBIOS changes.
Right. That's why I think it's a good practice to have this flag and tie it to machine-type.But this is just a good practice in general and in the past it was said by maintainers that this is one of the main reasons that ACPI and SMBIOS generation have moved from SeaBIOS to QEMU.I think a flag to disable this might make sense though. For example, some guests might behave differently and get broken.
Guest-visible changes shouldn't be exposed to old machine-types.
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)Add documentation that it's a Windows Emulated Device Flags table, helpful to speed up windows guests, and ignored by others.
Ok. Will do in v2.
+{ + AcpiTableWaet *waet; + + waet = acpi_data_push(table_data, sizeof(*waet));Can combine with the previous line.Ok. Will do in v2.+ 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 https://urldefense.com/v3/__http://support.amd.com/TechDocs/48882_IOMMU.pdf__;!!GqivPVa7Brio!OAQpLo9QhdHiNDa_aRLR_ma1nWLZU1aQhDozYgUlrqBZiz1vKdZgg-lTDMIj_5g$ @@ -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: + * https://urldefense.com/v3/__http://download.microsoft.com/download/7/E/7/7E7662CF-CBEA-470B-A97E-CE7CE0D98DC2/WAET.docx____;!!GqivPVa7Brio!MX1Hzr4X9NtS4pcT1Kb4VlDoV-pobn4n6YYQCkU3U-7imIaxXmu_ZsQzPB0e2Tc$Please include - name of the spec
The name of the spec is "Windows ACPI Emulated Devices Table". You can see this by entering above link...
The above link is to version 1.0 of the document (Which as far as I know, is the only version ever released). So the bits exists in all revisions. Which documentation do you want me to add then?- earliest revision that includes the relevant bits
Also, according to your previous comment, I'm removing these bits definitions from here and just putting (1 << 1) directly in build_waet() code with a comment of what is the bit I'm signaling there (i.e. PM_TIMER_GOOD).
+ */ + +/* + * 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)Include the name of the field exactly as it appears in the spec pls. "RTC good"?
Yes, it's named "RTC good" in spec.Anyway, I removed this bit and it's documentation from v2 as you asked in previous reply.
So if you feel you need to document that this bit is clear, you can do it like this: /* Bit 0 - PV RTC which doesn't need an acknowledgment after an interrupt assert. Clear since our RTC behaves like the hardware one. */+/* + * 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)Go easy on what we do, and harder on why please: /* ACPI PM timer good - tells windows guests our PM timer is reliable - * guests then avoid re-reading it. */ should be enough.
Ok... Will change to your phrasing in v2. -Liran
| [Prev in Thread] | Current Thread | [Next in Thread] |