qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Liran Alon
Subject: Re: [PATCH] acpi: Add Windows ACPI Emulated Device Table (WAET)
Date: Thu, 12 Mar 2020 14:55:50 +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 14:19, Michael S. Tsirkin wrote:

On Thu, Mar 12, 2020 at 01:30:01PM +0200, Liran Alon wrote:

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.

As with any
performance optimization, pls add a bit of info about how you tested
and what kind of speedup was seen.

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.

Having said that, note that there isn't really a trade-off here between
better performance or something else.

Well some guests are known to make crazy assumptions. E.g. they would
see this bit and say "oh so I know this hyperv" or something to
that end.

I agree some guests make crazy assumptions like this.
For this specific case, we haven't witnessed this in a very wide variety of guests. Another evidence that this is pretty much safe is that this is exposed by default in VMware, Xen HVM, GCP and AWS. 
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)


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.

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 is at least documented internally in Ravello that some guests have been
witnessed to broke their license activation because of ACPI/SMBIOS changes.

Any data on which changes exactly?
All I know about is this list, though it's pretty old.
https://urldefense.com/v3/__https://docs.microsoft.com/en-us/previous-versions/windows/it-pro/windows-xp/bb457054(v=technet.10)?redirectedfrom=MSDN__;!!GqivPVa7Brio!Ib_edgcD6o9nJ4KPgv-iV660VzKRAUJUIuQvlr_RT0JRSZxehpt37AmxFt84MtI$
This list is indeed very incomplete. For example, a simple change in BIOS-UUID exposed via SMBIOS also breaks activation. 

Any chance it was actually to do with the EOM table that bypasses
activation?


No, we never expose that table to guest in Ravello.

But anyway, this is kinda of a side-discussion...
My general argument is that we prefer that a guest running with machine-type X won't be exposed with new hardware/bios properties. 


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.

Right. That's why I think it's a good practice to have this flag and tie it
to machine-type.

Tying things to the machine type is not what I had in mind.
A separate flag would also be helpful so users can tweak this
for new machine types, too.
I think it's unnecessary, given how common WAET ACPI table is exposed by default by other hypervisors. 

But if you insist, I can add such flag on a separate commit in v2...
Where do you want to have such flag? It cannot be a property of some qdev object. 
So you want to add a new QEMU_OPTION_no_weat in vl.c?


Guest-visible changes shouldn't be exposed to old machine-types.

Well almost any change in qemu is guest visible to some level.
Even optimizations are guest visible.
We made changes in ACPI without versioning in the past but I'm not
opposed to versioning here. However in that case pls do add a bit
of documentation about why this is done here.
I remember that maintainers have explicitly specified that ACPI/SMBIOS should not be changed between machine-types. This have been one of the reasons to move ACPI/SMBIOS generation from SeaBIOS to QEMU control. 

What can of documentation you want me to add and where?
The only thing I can say is that I tie it to machine-type because I do not think a given machine-type should suddenly change BIOS exposed info to guest. But that's kinda generic. I haven't found similar documentation in other ACPI-disable flags to copy from (E.g. do_not_add_smb_acpi). 


What I am asking about is whether we need a flag to disable
this as part of the stable interface.
I personally think not. But if you think otherwise, can you provide guidance of where you suggest to add this flag? 
As the only place I see fit is adding a new QEMU_OPTION_no_weat.





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...

Links go stale. Then someone will have to dig to find the new location.
Name of the document will be helpful for that.
I don't get what you wish to add. The name of the document is "Windows ACPI Emulated Device Table"... It's reasonable to expect someone which encounters this link to be broken to search Google for "Windows ACPI Emulated Device Table specification". 
- earliest revision that includes the relevant bits

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?

1. name of the document
2. revision of the document that includes the bit (if multiple,
include the earliest revision)


I have added on top of build_waet() the following comment in v2:

+/*
+ * Windows ACPI Emulated Devices Table
+ * (Version 1.0 - April 6, 2009)
+ *
+ * Helpful to speedup Windows guests and ignored by others.
+ */

I hope this match to what you are looking for...

-Liran
reply via email to
[Prev in Thread] Current Thread [Next in Thread]