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(+)
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. It also makes sure to introduce
the new ACPI table only for new machine-types.
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));
+ 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)
+
+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
Patchew URL: https://patchew.org/QEMU/20200311170826.79419-1-liran.alon@oracle.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === Using expected file 'tests/data/acpi/pc/HPET' Looking for expected file 'tests/data/acpi/pc/WAET' ** ERROR:/tmp/qemu-test/src/tests/qtest/bios-tables-test.c:367:load_expected_aml: assertion failed: (exp_sdt.aml_file) ERROR - Bail out! ERROR:/tmp/qemu-test/src/tests/qtest/bios-tables-test.c:367:load_expected_aml: assertion failed: (exp_sdt.aml_file) make: *** [check-qtest-x86_64] Error 1 make: *** Waiting for unfinished jobs.... TEST check-unit: tests/test-bufferiszero qemu-system-aarch64: -accel kvm: invalid accelerator kvm --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=5598a4498742491c9be76e1225f60fe5', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-f5g9sd65/src/docker-src.2020-03-11-14.47.34.6055:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=5598a4498742491c9be76e1225f60fe5 make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-f5g9sd65/src' make: *** [docker-run-test-quick@centos7] Error 2 real 11m45.203s user 0m8.706s The full log is available at http://patchew.org/logs/20200311170826.79419-1-liran.alon@oracle.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On 11/03/2020 20:59, no-reply@patchew.org wrote: > Patchew URL: https://urldefense.com/v3/__https://patchew.org/QEMU/20200311170826.79419-1-liran.alon@oracle.com/__;!!GqivPVa7Brio!L4XXKjkDknE86ihbnytm45vsQI41J-QWVCZRoXEXtPKIAsMmknrGJWVPZpKgLyM$ > > Hi, > > This series failed the docker-quick@centos7 build test. Please find the testing commands and > their output below. If you have Docker installed, you can probably reproduce it > locally. > > === TEST SCRIPT BEGIN === > #!/bin/bash > make docker-image-centos7 V=1 NETWORK=1 > time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 > === TEST SCRIPT END === > > Using expected file 'tests/data/acpi/pc/HPET' > Looking for expected file 'tests/data/acpi/pc/WAET' > ** > ERROR:/tmp/qemu-test/src/tests/qtest/bios-tables-test.c:367:load_expected_aml: assertion failed: (exp_sdt.aml_file) > ERROR - Bail out! ERROR:/tmp/qemu-test/src/tests/qtest/bios-tables-test.c:367:load_expected_aml: assertion failed: (exp_sdt.aml_file) My bad. Didn't notice there are tests which verifies ACPI haven't changed and requires update for such patch. Will submit a patch for this test in v2. -Liran
On Wed, Mar 11, 2020 at 09:08:56PM +0200, Liran Alon wrote: > > On 11/03/2020 20:59, no-reply@patchew.org wrote: > > Patchew URL: https://urldefense.com/v3/__https://patchew.org/QEMU/20200311170826.79419-1-liran.alon@oracle.com/__;!!GqivPVa7Brio!L4XXKjkDknE86ihbnytm45vsQI41J-QWVCZRoXEXtPKIAsMmknrGJWVPZpKgLyM$ > > > > Hi, > > > > This series failed the docker-quick@centos7 build test. Please find the testing commands and > > their output below. If you have Docker installed, you can probably reproduce it > > locally. > > > > === TEST SCRIPT BEGIN === > > #!/bin/bash > > make docker-image-centos7 V=1 NETWORK=1 > > time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 > > === TEST SCRIPT END === > > > > Using expected file 'tests/data/acpi/pc/HPET' > > Looking for expected file 'tests/data/acpi/pc/WAET' > > ** > > ERROR:/tmp/qemu-test/src/tests/qtest/bios-tables-test.c:367:load_expected_aml: assertion failed: (exp_sdt.aml_file) > > ERROR - Bail out! ERROR:/tmp/qemu-test/src/tests/qtest/bios-tables-test.c:367:load_expected_aml: assertion failed: (exp_sdt.aml_file) > > My bad. Didn't notice there are tests which verifies ACPI haven't changed > and requires update for such patch. > Will submit a patch for this test in v2. > > -Liran > Notice the process as documented in ./tests/qtest/bios-tables-test.c -- MST
On 11/03/2020 22:24, Michael S. Tsirkin wrote: > Notice the process as documented in ./tests/qtest/bios-tables-test.c > Thanks for explicitly pointing me to that process. I have followed the process described there (Both steps 1-3 and steps 4-7). On step (6), I have noted that many existing ACPI tables don't have expected binaries for all the execution-matrix. E.g. tests/data/acpi/pc/APIC.{bridge, ipmikcs, memhp, numamem} are all missing. Similar missing files exists for FACP, FACS, HPET and MCFG. I should add for WAET the expected binaries for all the execution-matrix right? Is it just an existing issue that for the existing tables some of the expected binaries are missing? But the tests seems to pass. Can you clarify this for me? Thanks, -Liran
On Thu, Mar 12, 2020 at 03:31:49AM +0200, Liran Alon wrote: > > On 11/03/2020 22:24, Michael S. Tsirkin wrote: > > Notice the process as documented in ./tests/qtest/bios-tables-test.c > > > Thanks for explicitly pointing me to that process. > > I have followed the process described there (Both steps 1-3 and steps 4-7). > On step (6), I have noted that many existing ACPI tables don't have expected > binaries for all the execution-matrix. > E.g. tests/data/acpi/pc/APIC.{bridge, ipmikcs, memhp, numamem} are all > missing. > Similar missing files exists for FACP, FACS, HPET and MCFG. > > I should add for WAET the expected binaries for all the execution-matrix > right? > Is it just an existing issue that for the existing tables some of the > expected binaries are missing? But the tests seems to pass. > Can you clarify this for me? > > Thanks, > -Liran It's because of this (which we should probably rewrite as a loop): try_again: aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine, sdt->aml, ext); if (getenv("V")) { fprintf(stderr, "Looking for expected file '%s'\n", aml_file); } if (g_file_test(aml_file, G_FILE_TEST_EXISTS)) { exp_sdt.aml_file = aml_file; } else if (*ext != '\0') { /* try fallback to generic (extension less) expected file */ ext = ""; g_free(aml_file); goto try_again; } if WAET is always added, then a single WAET will be enough for you. -- MST
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
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 <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 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. > >> 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). 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. > >> 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. 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!OAQpLo9QhdHiNDa_aRLR_ma1nWLZU1aQhDozYgUlrqBZiz1vKdZgg-lTY0xxqc8$ >> + */ >> + >> +/* >> + * 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. Ok. I will change this as you say in v2. BTW it seems other code in acpi-build.c still relies on flags definitions in acpi-defs.h (As I have done in this v1). E.g. ACPI_DMAR_TYPE_*, ACPI_APIC_*, ACPI_FADT_F_*. I assume this is just code that wasn't changed yet to the new convention? > Igor this is becoming a FAQ. Could you write up the way ACPI > generation code should look? +1. Thanks for the review, -Liran
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 <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 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. As with any performance optimization, pls add a bit of info about how you tested and what kind of speedup was seen. > > > > > 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. > 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. > > > > > 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) Add documentation that it's a Windows Emulated Device Flags table, helpful to speed up windows guests, and ignored by others. > > > +{ > > > + 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: > > > + * http://download.microsoft.com/download/7/E/7/7E7662CF-CBEA-470B-A97E-CE7CE0D98DC2/WAET.docx__ Please include - name of the spec - earliest revision that includes the relevant bits > > > + */ > > > + > > > +/* > > > + * 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"? 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. > > > + > > 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. > > Ok. I will change this as you say in v2. > > BTW it seems other code in acpi-build.c still relies on flags definitions in > acpi-defs.h (As I have done in this v1). E.g. ACPI_DMAR_TYPE_*, ACPI_APIC_*, > ACPI_FADT_F_*. > I assume this is just code that wasn't changed yet to the new convention? Yes - we only do it when we are actually changing code. > > Igor this is becoming a FAQ. Could you write up the way ACPI > > generation code should look? > > +1. > > Thanks for the review, > -Liran >
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 <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 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. 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. >> 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. Guest-visible changes shouldn't be exposed to old machine-types. > > >>>> 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) > 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... > - 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? 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
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 <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 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. > 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://docs.microsoft.com/en-us/previous-versions/windows/it-pro/windows-xp/bb457054(v=technet.10)?redirectedfrom=MSDN Any chance it was actually to do with the EOM table that bypasses activation? > > > 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. > 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. What I am asking about is whether we need a flag to disable this as part of the stable interface. > > > > > > > > > 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) > > 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. > > - 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) -- MST
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 <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 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 <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) >>> 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
On Thu, 12 Mar 2020 14:55:50 +0200 Liran Alon <liran.alon@oracle.com> wrote: > 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 <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 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? If it doesn't break any windows guests we probably don't need an option. Can you test if old guests are booting fine with new table, to confirm that it's fine? (starting with XPsp3) > > >> 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 <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) > >>> 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 > >
On 12/03/2020 18:35, Igor Mammedov wrote: > On Thu, 12 Mar 2020 14:55:50 +0200 > Liran Alon <liran.alon@oracle.com> wrote: > >> 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: >>>>>> 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? > If it doesn't break any windows guests we probably don't need an option. > Can you test if old guests are booting fine with new table, to confirm > that it's fine? (starting with XPsp3) Old guests boot fine with the new WAET table. We are running with this table in production for many years with many Windows XP guests (and much more esoteric guests) Just to verify, I've just now run it with a WinXP SP3 VM and it works just fine. So should I remove the flag completely or remain with the current functionality I have that makes sure WAET is only exposed on new machine-types? -Liran >>>> 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.
On Thu, 12 Mar 2020 20:48:48 +0200 Liran Alon <liran.alon@oracle.com> wrote: > On 12/03/2020 18:35, Igor Mammedov wrote: > > On Thu, 12 Mar 2020 14:55:50 +0200 > > Liran Alon <liran.alon@oracle.com> wrote: > > > >> 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: > >>>>>> 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? > > If it doesn't break any windows guests we probably don't need an option. > > Can you test if old guests are booting fine with new table, to confirm > > that it's fine? (starting with XPsp3) > > Old guests boot fine with the new WAET table. > We are running with this table in production for many years with many > Windows XP guests (and much more esoteric guests) > > Just to verify, I've just now run it with a WinXP SP3 VM and it works > just fine. > So should I remove the flag completely or remain with the current > functionality I have that makes sure WAET is only exposed on new > machine-types? In this case I'd drop flag. > > -Liran > > >>>> 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. >
On Wed, 11 Mar 2020 19:08:26 +0200 Liran Alon <liran.alon@oracle.com> 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. 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.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) 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. > + > + 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;
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.alon@oracle.com> 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. 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.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) > 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;
On Thu, 12 Mar 2020 13:09:51 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > 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.alon@oracle.com> 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. 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.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) > > 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? how about [aml|acpi]_int_raw [...]
On Fri, Mar 13, 2020 at 10:36:56AM +0100, Igor Mammedov wrote: > On Thu, 12 Mar 2020 13:09:51 -0400 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > 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.alon@oracle.com> 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. 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.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) > > > 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? > > how about [aml|acpi]_int_raw > [...] I'm not sure how this helps. I think the main problems are 1- very long name 2- only makes sense if you know that ACPI has a special integer prefix 3- easy to confuse which is the value which is the length 4- length is in bytes (typical documentation is in bits) Your suggestion only fixes issue 1. Having listed it all out, I suggest the following for the purpose of building structures: acpi/aml/build_append_u8 acpi/aml/build_append_u16 acpi/aml/build_append_u32 acpi/aml/build_append_u64 and maybe acpi/aml/build_append_pad( length) I'm not sure what the best prefix is. I guess we can have them all with the slightly different arguments. -- MST
On Fri, 13 Mar 2020 11:26:45 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Mar 13, 2020 at 10:36:56AM +0100, Igor Mammedov wrote: > > On Thu, 12 Mar 2020 13:09:51 -0400 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > 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.alon@oracle.com> 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. 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.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) > > > > 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? > > > > how about [aml|acpi]_int_raw > > [...] > > I'm not sure how this helps. I think the main problems are > 1- very long name > 2- only makes sense if you know that ACPI has a special integer prefix > 3- easy to confuse which is the value which is the length > 4- length is in bytes (typical documentation is in bits) in acpi spec, they use bytes mostly (with occasional bits deviation) > > Your suggestion only fixes issue 1. that's what I don't like the most about current name, it's way too long. > Having listed it all out, I suggest the following for the purpose of > building structures: > > acpi/aml/build_append_u8 > acpi/aml/build_append_u16 > acpi/aml/build_append_u32 > acpi/aml/build_append_u64 I prefer having length argument, so when I'm reviewing code, I'm basically comparing it with value in the table. The same applies to function name, having a bunch of different names would be distracting, at least where it comes to composing tables. So I prefer keeping current list of arguments. > and maybe > acpi/aml/build_append_pad( length) > > I'm not sure what the best prefix is. I guess we can have them all > with the slightly different arguments.
On 12/03/2020 18:27, Igor Mammedov wrote: > On Wed, 11 Mar 2020 19:08:26 +0200 > Liran Alon <liran.alon@oracle.com> wrote: >> + >> +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. Note that WAET is a table that is not part of ACPI spec officially. It's specified on it's own document, there is only a single version, and there is only a single table in that document describing that table structure. Therefore, I cannot write a comment such as build_hmat_lb() have: /* * ACPI 6.3: 5.2.27.4 System Locality Latency and Bandwidth Information * Structure: Table 5-146 */ My best attempt to do something similar in v2 is: /* * Windows ACPI Emulated Devices Table * (Version 1.0 - April 6, 2009) * Spec: http://download.microsoft.com/download/7/E/7/7E7662CF-CBEA-470B-A97E-CE7CE0D98DC2/WAET.docx * * Helpful to speedup Windows guests and ignored by others. */ If it's not sufficient. Please suggest alternative phrasing which I would use in v2. > >> +{ >> + 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. Thanks for pointing this out. I will make sure to update my code accordingly in v2. > > > > >> + >> + 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!On_WsDCS8ysOeUG17h1l3dTpWEm79AHwMHLbbUgsvagBSpgZAk5U1cXddn6ZNOU$ >> @@ -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. Hmm... I would have expected as a QEMU user that upgrading QEMU may update my firmware exposed table (Such as ACPI), but only if I don't specify I wish to run on a specific machine-type. In that case, I would've expect to be exposed with exact same firmware information. I understood that this was one of the main reasons why ACPI/SMBIOS generation was moved from SeaBIOS to QEMU. If you think this isn't the case, I can just remove this flag (Makes code simpler). What do you prefer? Thanks for the review, -Liran
On Thu, Mar 12, 2020 at 07:28:31PM +0200, Liran Alon wrote: > > On 12/03/2020 18:27, Igor Mammedov wrote: > > On Wed, 11 Mar 2020 19:08:26 +0200 > > Liran Alon <liran.alon@oracle.com> wrote: > > > + > > > +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. > > Note that WAET is a table that is not part of ACPI spec officially. > It's specified on it's own document, there is only a single version, and > there is only a single table in that document describing that table > structure. > > Therefore, I cannot write a comment such as build_hmat_lb() have: > /* > * ACPI 6.3: 5.2.27.4 System Locality Latency and Bandwidth Information > * Structure: Table 5-146 > */ > > My best attempt to do something similar in v2 is: > /* > * Windows ACPI Emulated Devices Table > * (Version 1.0 - April 6, 2009) > * Spec: http://download.microsoft.com/download/7/E/7/7E7662CF-CBEA-470B-A97E-CE7CE0D98DC2/WAET.docx > * > * Helpful to speedup Windows guests and ignored by others. > */ > > If it's not sufficient. Please suggest alternative phrasing which I would > use in v2. > > > > > > +{ > > > + 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. > Thanks for pointing this out. > I will make sure to update my code accordingly in v2. > > > > > > > > > > > + > > > + 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!On_WsDCS8ysOeUG17h1l3dTpWEm79AHwMHLbbUgsvagBSpgZAk5U1cXddn6ZNOU$ > > > @@ -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. > > Hmm... I would have expected as a QEMU user that upgrading QEMU may update > my firmware exposed table (Such as ACPI), > but only if I don't specify I wish to run on a specific machine-type. In > that case, I would've expect to be exposed with exact same firmware > information. > I understood that this was one of the main reasons why ACPI/SMBIOS > generation was moved from SeaBIOS to QEMU. > > If you think this isn't the case, I can just remove this flag (Makes code > simpler). What do you prefer? > > Thanks for the review, > -Liran > I'm inclined to agree, but no biggie if Igor disagrees let's go along with his opinion. -- MST
On 12/03/2020 21:47, Michael S. Tsirkin wrote: > On Thu, Mar 12, 2020 at 07:28:31PM +0200, Liran Alon wrote: >> On 12/03/2020 18:27, Igor Mammedov wrote: >>> On Wed, 11 Mar 2020 19:08:26 +0200 >>> Liran Alon <liran.alon@oracle.com> wrote: >>> >>> 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. >> Hmm... I would have expected as a QEMU user that upgrading QEMU may update >> my firmware exposed table (Such as ACPI), >> but only if I don't specify I wish to run on a specific machine-type. In >> that case, I would've expect to be exposed with exact same firmware >> information. >> I understood that this was one of the main reasons why ACPI/SMBIOS >> generation was moved from SeaBIOS to QEMU. >> >> If you think this isn't the case, I can just remove this flag (Makes code >> simpler). What do you prefer? >> >> Thanks for the review, >> -Liran >> > I'm inclined to agree, but no biggie if Igor disagrees let's go along > with his opinion. > I will wait for Igor's reply on this before I submit v2 (I have it ready with the flag still existing). To make sure that v2 passes all review comments. ;) -Liran
On Thu, 12 Mar 2020 19:28:31 +0200 Liran Alon <liran.alon@oracle.com> wrote: > On 12/03/2020 18:27, Igor Mammedov wrote: > > On Wed, 11 Mar 2020 19:08:26 +0200 > > Liran Alon <liran.alon@oracle.com> wrote: > >> + [...] > > 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. > > Hmm... I would have expected as a QEMU user that upgrading QEMU may > update my firmware exposed table (Such as ACPI), > but only if I don't specify I wish to run on a specific machine-type. In > that case, I would've expect to be exposed with exact same firmware > information. That would be ideal but it's not the case with current QEMU, even with specific machine type user will get new firmware when it's started with upgraded QEMU which usually ships with new firmware. mgmt layer theoretically can take care of maintaining different firmwares on host and explicitly specify which should be used (though I'm not aware of any doing it) another issue with adding flags consistently for every acpi related change would complicate code quite a bit making it hard to read/maintain, hence flags are used only when we have to introduce them (i.e when it would break guest). > I understood that this was one of the main reasons why ACPI/SMBIOS > generation was moved from SeaBIOS to QEMU. If I recall correctly, Michael moved table to QEMU so we won't have to extend ABI for constantly growing ACPI interface and then maintain it forever, which indeed would require using compat machinery for every knob (which is unsustainable). [...]
On 13/03/2020 12:05, Igor Mammedov wrote: > On Thu, 12 Mar 2020 19:28:31 +0200 > Liran Alon <liran.alon@oracle.com> wrote: > >> On 12/03/2020 18:27, Igor Mammedov wrote: >>> On Wed, 11 Mar 2020 19:08:26 +0200 >>> Liran Alon <liran.alon@oracle.com> wrote: >>>> + > [...] >>> 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. >> Hmm... I would have expected as a QEMU user that upgrading QEMU may >> update my firmware exposed table (Such as ACPI), >> but only if I don't specify I wish to run on a specific machine-type. In >> that case, I would've expect to be exposed with exact same firmware >> information. > That would be ideal but it's not the case with current QEMU, even with > specific machine type user will get new firmware when it's started with > upgraded QEMU which usually ships with new firmware. > > mgmt layer theoretically can take care of maintaining different firmwares > on host and explicitly specify which should be used (though I'm not aware > of any doing it) > > another issue with adding flags consistently for every acpi related > change would complicate code quite a bit making it hard to read/maintain, > hence flags are used only when we have to introduce them (i.e when it > would break guest). > >> I understood that this was one of the main reasons why ACPI/SMBIOS >> generation was moved from SeaBIOS to QEMU. > If I recall correctly, Michael moved table to QEMU so we won't have to > extend ABI for constantly growing ACPI interface and then maintain it > forever, which indeed would require using compat machinery for every > knob (which is unsustainable). > > [...] > Ok. Thanks very much for expressing your opinion. So I would just remove flag and submit v2 without it. -Liran
Patchew URL: https://patchew.org/QEMU/20200311170826.79419-1-liran.alon@oracle.com/ Hi, This series failed the asan build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1 === TEST SCRIPT END === PASS 1 fdc-test /x86_64/fdc/cmos PASS 2 fdc-test /x86_64/fdc/no_media_on_start PASS 3 fdc-test /x86_64/fdc/read_without_media ==6296==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 check-qstring /public/from_str PASS 2 check-qstring /public/get_str PASS 3 check-qstring /public/append_chr --- PASS 32 test-opts-visitor /visitor/opts/range/beyond PASS 33 test-opts-visitor /visitor/opts/dict/unvisited MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-coroutine -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-coroutine" ==6371==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==6371==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffe06631000; bottom 0x7fa3eaad3000; size: 0x005a1bb5e000 (387011960832) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 11 fdc-test /x86_64/fdc/read_no_dma_18 --- PASS 13 fdc-test /x86_64/fdc/fuzz-registers MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img tests/qtest/ide-test -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="ide-test" PASS 14 test-aio /aio/timer/schedule ==6386==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 15 test-aio /aio/coroutine/queue-chaining PASS 16 test-aio /aio-gsource/flush PASS 17 test-aio /aio-gsource/bh/schedule --- PASS 25 test-aio /aio-gsource/event/wait PASS 26 test-aio /aio-gsource/event/flush PASS 27 test-aio /aio-gsource/event/wait/no-flush-cb ==6394==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 ide-test /x86_64/ide/identify ==6400==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 2 ide-test /x86_64/ide/flush PASS 28 test-aio /aio-gsource/timer/schedule MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-aio-multithread -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-aio-multithread" ==6406==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==6409==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-aio-multithread /aio/multi/lifecycle PASS 3 ide-test /x86_64/ide/bmdma/simple_rw ==6426==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 4 ide-test /x86_64/ide/bmdma/trim PASS 2 test-aio-multithread /aio/multi/schedule ==6432==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 3 test-aio-multithread /aio/multi/mutex/contended PASS 4 test-aio-multithread /aio/multi/mutex/handoff PASS 5 test-aio-multithread /aio/multi/mutex/mcs ==6458==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 6 test-aio-multithread /aio/multi/mutex/pthread MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-throttle -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-throttle" PASS 1 test-throttle /throttle/leak_bucket --- PASS 6 test-throttle /throttle/detach_attach PASS 7 test-throttle /throttle/config_functions PASS 8 test-throttle /throttle/accounting ==6465==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 9 test-throttle /throttle/groups PASS 10 test-throttle /throttle/config/enabled PASS 11 test-throttle /throttle/config/conflicting --- MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-thread-pool -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-thread-pool" PASS 1 test-thread-pool /thread-pool/submit PASS 2 test-thread-pool /thread-pool/submit-aio ==6469==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 3 test-thread-pool /thread-pool/submit-co PASS 4 test-thread-pool /thread-pool/submit-many PASS 5 test-thread-pool /thread-pool/cancel --- PASS 3 test-hbitmap /hbitmap/size/unaligned PASS 4 test-hbitmap /hbitmap/iter/empty PASS 5 test-hbitmap /hbitmap/iter/partial ==6540==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 6 test-hbitmap /hbitmap/iter/granularity PASS 7 test-hbitmap /hbitmap/iter/iter_and_reset PASS 8 test-hbitmap /hbitmap/get/all --- PASS 28 test-hbitmap /hbitmap/truncate/shrink/medium PASS 29 test-hbitmap /hbitmap/truncate/shrink/large PASS 30 test-hbitmap /hbitmap/meta/zero ==6546==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 31 test-hbitmap /hbitmap/meta/one PASS 32 test-hbitmap /hbitmap/meta/byte PASS 33 test-hbitmap /hbitmap/meta/word --- PASS 44 test-hbitmap /hbitmap/next_dirty_area/next_dirty_area_4 PASS 45 test-hbitmap /hbitmap/next_dirty_area/next_dirty_area_after_truncate MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-bdrv-drain -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-bdrv-drain" ==6555==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-bdrv-drain /bdrv-drain/nested PASS 2 test-bdrv-drain /bdrv-drain/multiparent PASS 3 test-bdrv-drain /bdrv-drain/set_aio_context --- PASS 28 test-bdrv-drain /bdrv-drain/blockjob/iothread/drain_all PASS 29 test-bdrv-drain /bdrv-drain/blockjob/iothread/drain PASS 30 test-bdrv-drain /bdrv-drain/blockjob/iothread/drain_subtree ==6552==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 31 test-bdrv-drain /bdrv-drain/blockjob/iothread/error/drain_all PASS 32 test-bdrv-drain /bdrv-drain/blockjob/iothread/error/drain PASS 33 test-bdrv-drain /bdrv-drain/blockjob/iothread/error/drain_subtree --- PASS 41 test-bdrv-drain /bdrv-drain/bdrv_drop_intermediate/poll PASS 42 test-bdrv-drain /bdrv-drain/replace_child/mid-drain MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-bdrv-graph-mod -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-bdrv-graph-mod" ==6598==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-bdrv-graph-mod /bdrv-graph-mod/update-perm-tree PASS 2 test-bdrv-graph-mod /bdrv-graph-mod/should-update-child MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-blockjob -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-blockjob" ==6602==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-blockjob /blockjob/ids PASS 2 test-blockjob /blockjob/cancel/created PASS 3 test-blockjob /blockjob/cancel/running --- PASS 7 test-blockjob /blockjob/cancel/pending PASS 8 test-blockjob /blockjob/cancel/concluded MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-blockjob-txn -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-blockjob-txn" ==6606==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-blockjob-txn /single/success PASS 2 test-blockjob-txn /single/failure PASS 3 test-blockjob-txn /single/cancel --- PASS 6 test-blockjob-txn /pair/cancel PASS 7 test-blockjob-txn /pair/fail-cancel-race MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-block-backend -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-block-backend" ==6610==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-block-backend /block-backend/drain_aio_error PASS 2 test-block-backend /block-backend/drain_all_aio_error MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-block-iothread -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-block-iothread" ==6614==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-block-iothread /sync-op/pread PASS 2 test-block-iothread /sync-op/pwrite PASS 3 test-block-iothread /sync-op/load_vmstate --- PASS 15 test-block-iothread /propagate/diamond PASS 16 test-block-iothread /propagate/mirror MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-image-locking -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-image-locking" ==6634==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-image-locking /image-locking/basic PASS 2 test-image-locking /image-locking/set-perm-abort MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-x86-cpuid -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-x86-cpuid" --- PASS 1 rcutorture /rcu/torture/1reader PASS 2 rcutorture /rcu/torture/10readers MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-rcu-list -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-rcu-list" ==6686==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-rcu-list /rcu/qlist/single-threaded PASS 2 test-rcu-list /rcu/qlist/short-few PASS 3 test-rcu-list /rcu/qlist/long-many MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-rcu-simpleq -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-rcu-simpleq" PASS 1 test-rcu-simpleq /rcu/qsimpleq/single-threaded ==6731==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 2 test-rcu-simpleq /rcu/qsimpleq/short-few PASS 3 test-rcu-simpleq /rcu/qsimpleq/long-many MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-rcu-tailq -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-rcu-tailq" PASS 1 test-rcu-tailq /rcu/qtailq/single-threaded PASS 2 test-rcu-tailq /rcu/qtailq/short-few ==6776==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 3 test-rcu-tailq /rcu/qtailq/long-many MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-rcu-slist -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-rcu-slist" PASS 1 test-rcu-slist /rcu/qslist/single-threaded PASS 2 test-rcu-slist /rcu/qslist/short-few ==6836==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 3 test-rcu-slist /rcu/qslist/long-many MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-qdist -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-qdist" PASS 1 test-qdist /qdist/none --- PASS 8 test-qdist /qdist/binning/shrink MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-qht -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-qht" PASS 5 ide-test /x86_64/ide/bmdma/various_prdts ==6849==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==6849==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffc2f4c4000; bottom 0x7f46aef98000; size: 0x00b58052c000 (779541987328) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 6 ide-test /x86_64/ide/bmdma/no_busmaster PASS 7 ide-test /x86_64/ide/flush/nodev ==6860==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 8 ide-test /x86_64/ide/flush/empty_drive ==6865==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 9 ide-test /x86_64/ide/flush/retry_pci ==6871==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 10 ide-test /x86_64/ide/flush/retry_isa ==6877==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 11 ide-test /x86_64/ide/cdrom/pio ==6883==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 12 ide-test /x86_64/ide/cdrom/pio_large ==6889==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 13 ide-test /x86_64/ide/cdrom/dma MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img tests/qtest/ahci-test -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="ahci-test" ==6903==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 ahci-test /x86_64/ahci/sanity ==6909==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 2 ahci-test /x86_64/ahci/pci_spec ==6915==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 3 ahci-test /x86_64/ahci/pci_enable ==6921==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 4 ahci-test /x86_64/ahci/hba_spec PASS 1 test-qht /qht/mode/default ==6927==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 2 test-qht /qht/mode/resize MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-qht-par -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-qht-par" PASS 5 ahci-test /x86_64/ahci/hba_enable ==6942==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-qht-par /qht/parallel/2threads-0%updates-1s PASS 6 ahci-test /x86_64/ahci/identify ==6950==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 7 ahci-test /x86_64/ahci/max PASS 2 test-qht-par /qht/parallel/2threads-20%updates-1s MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-bitops -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-bitops" --- PASS 3 test-bitcnt /bitcnt/ctpop32 PASS 4 test-bitcnt /bitcnt/ctpop64 MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-qdev-global-props -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-qdev-global-props" ==6960==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-qdev-global-props /qdev/properties/static/default PASS 2 test-qdev-global-props /qdev/properties/static/global PASS 3 test-qdev-global-props /qdev/properties/dynamic/global --- PASS 18 test-qemu-opts /qemu-opts/to_qdict/filtered PASS 19 test-qemu-opts /qemu-opts/to_qdict/duplicates MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-keyval -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-keyval" ==6994==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-keyval /keyval/keyval_parse PASS 2 test-keyval /keyval/keyval_parse/list PASS 3 test-keyval /keyval/visit/bool --- PASS 4 test-write-threshold /write-threshold/not-trigger PASS 5 test-write-threshold /write-threshold/trigger MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-crypto-hash -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-crypto-hash" ==6994==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fff39fa4000; bottom 0x7fd57cdfe000; size: 0x0029bd1a6000 (179266281472) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 1 test-crypto-hash /crypto/hash/iov --- PASS 3 test-crypto-hmac /crypto/hmac/prealloc PASS 4 test-crypto-hmac /crypto/hmac/digest MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-crypto-cipher -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-crypto-cipher" ==7017==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-crypto-cipher /crypto/cipher/aes-ecb-128 PASS 2 test-crypto-cipher /crypto/cipher/aes-ecb-192 PASS 3 test-crypto-cipher /crypto/cipher/aes-ecb-256 --- PASS 27 test-crypto-cipher /crypto/cipher/null-iv PASS 28 test-crypto-cipher /crypto/cipher/short-plaintext MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-crypto-secret -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-crypto-secret" ==7017==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffc5fbcd000; bottom 0x7f0a305fe000; size: 0x00f22f5cf000 (1040176705536) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 10 ahci-test /x86_64/ahci/io/pio/lba28/simple/low --- PASS 15 test-crypto-secret /crypto/secret/crypt/missingiv PASS 16 test-crypto-secret /crypto/secret/crypt/badiv MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-crypto-tlscredsx509 -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-crypto-tlscredsx509" ==7034==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/perfectserver PASS 2 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/perfectclient PASS 3 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/goodca1 ==7034==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffca67f4000; bottom 0x7f5b459fe000; size: 0x00a160df6000 (693114986496) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 11 ahci-test /x86_64/ahci/io/pio/lba28/simple/high ==7044==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7044==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fff46f3a000; bottom 0x7f3cf09fe000; size: 0x00c25653c000 (834671984640) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 4 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/goodca2 --- PASS 6 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/badca1 PASS 7 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/badca2 PASS 8 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/badca3 ==7050==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7050==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffd9567d000; bottom 0x7fba61ffe000; size: 0x00433367f000 (288625258496) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 9 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/goodserver1 PASS 10 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/goodserver2 PASS 13 ahci-test /x86_64/ahci/io/pio/lba28/double/low PASS 11 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/goodserver3 ==7056==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 12 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/goodserver4 ==7056==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffede07f000; bottom 0x7fe8fd5fe000; size: 0x0015e0a81000 (93963423744) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 14 ahci-test /x86_64/ahci/io/pio/lba28/double/high ==7062==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7062==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffd195e4000; bottom 0x7fd722b7c000; size: 0x0025f6a68000 (163051896832) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 15 ahci-test /x86_64/ahci/io/pio/lba28/long/zero PASS 13 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/goodserver5 PASS 14 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/goodserver6 ==7068==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7068==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffe329af000; bottom 0x7fd8ae9fe000; size: 0x002583fb1000 (161128058880) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 15 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/goodserver7 --- PASS 33 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/inactive2 PASS 34 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/inactive3 PASS 16 ahci-test /x86_64/ahci/io/pio/lba28/long/low ==7074==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 35 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/chain1 PASS 36 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/chain2 PASS 37 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/missingca PASS 38 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/missingserver PASS 39 test-crypto-tlscredsx509 /qcrypto/tlscredsx509/missingclient MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-crypto-tlssession -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-crypto-tlssession" ==7074==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fff066a5000; bottom 0x7fe24c1fe000; size: 0x001cba4a7000 (123384524800) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 1 test-crypto-tlssession /qcrypto/tlssession/psk PASS 17 ahci-test /x86_64/ahci/io/pio/lba28/long/high PASS 2 test-crypto-tlssession /qcrypto/tlssession/basicca ==7084==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 3 test-crypto-tlssession /qcrypto/tlssession/differentca PASS 18 ahci-test /x86_64/ahci/io/pio/lba28/short/zero PASS 4 test-crypto-tlssession /qcrypto/tlssession/altname1 ==7090==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 19 ahci-test /x86_64/ahci/io/pio/lba28/short/low ==7096==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 20 ahci-test /x86_64/ahci/io/pio/lba28/short/high ==7102==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 5 test-crypto-tlssession /qcrypto/tlssession/altname2 ==7102==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fff5e3de000; bottom 0x7f9b8cdfe000; size: 0x0063d15e0000 (428714360832) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 21 ahci-test /x86_64/ahci/io/pio/lba48/simple/zero PASS 6 test-crypto-tlssession /qcrypto/tlssession/altname3 ==7108==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7108==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffcca330000; bottom 0x7fd5237fe000; size: 0x0027a6b32000 (170300481536) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 22 ahci-test /x86_64/ahci/io/pio/lba48/simple/low ==7114==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7114==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffe7e69a000; bottom 0x7fab855fe000; size: 0x0052f909c000 (356365484032) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 23 ahci-test /x86_64/ahci/io/pio/lba48/simple/high ==7120==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7120==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffcc6639000; bottom 0x7fd4361fe000; size: 0x00289043b000 (174219046912) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 24 ahci-test /x86_64/ahci/io/pio/lba48/double/zero PASS 7 test-crypto-tlssession /qcrypto/tlssession/altname4 ==7126==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7126==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffe812b3000; bottom 0x7f11d77fe000; size: 0x00eca9ab5000 (1016458858496) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 25 ahci-test /x86_64/ahci/io/pio/lba48/double/low PASS 8 test-crypto-tlssession /qcrypto/tlssession/altname5 ==7132==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7132==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffd8dc67000; bottom 0x7f846affe000; size: 0x007922c69000 (520274481152) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 26 ahci-test /x86_64/ahci/io/pio/lba48/double/high PASS 9 test-crypto-tlssession /qcrypto/tlssession/altname6 ==7138==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 10 test-crypto-tlssession /qcrypto/tlssession/wildcard1 ==7138==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffdbf424000; bottom 0x7f42c0dfe000; size: 0x00bafe626000 (803131777024) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 11 test-crypto-tlssession /qcrypto/tlssession/wildcard2 PASS 12 test-crypto-tlssession /qcrypto/tlssession/wildcard3 PASS 27 ahci-test /x86_64/ahci/io/pio/lba48/long/zero PASS 13 test-crypto-tlssession /qcrypto/tlssession/wildcard4 ==7144==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7144==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffcd50c2000; bottom 0x7f73cd724000; size: 0x00890799e000 (588538044416) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 14 test-crypto-tlssession /qcrypto/tlssession/wildcard5 PASS 28 ahci-test /x86_64/ahci/io/pio/lba48/long/low ==7150==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7150==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fff1aab5000; bottom 0x7f5a3e17c000; size: 0x00a4dc939000 (708075294720) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 29 ahci-test /x86_64/ahci/io/pio/lba48/long/high PASS 15 test-crypto-tlssession /qcrypto/tlssession/wildcard6 PASS 16 test-crypto-tlssession /qcrypto/tlssession/cachain ==7156==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-qga -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-qga" PASS 30 ahci-test /x86_64/ahci/io/pio/lba48/short/zero ==7170==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-qga /qga/sync-delimited PASS 2 test-qga /qga/sync PASS 3 test-qga /qga/ping --- PASS 15 test-qga /qga/invalid-cmd PASS 16 test-qga /qga/invalid-args PASS 17 test-qga /qga/fsfreeze-status ==7176==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 32 ahci-test /x86_64/ahci/io/pio/lba48/short/high ==7185==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 18 test-qga /qga/blacklist PASS 19 test-qga /qga/config PASS 20 test-qga /qga/guest-exec PASS 21 test-qga /qga/guest-exec-invalid PASS 33 ahci-test /x86_64/ahci/io/dma/lba28/fragmented ==7203==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 34 ahci-test /x86_64/ahci/io/dma/lba28/retry PASS 22 test-qga /qga/guest-get-osinfo PASS 23 test-qga /qga/guest-get-host-name --- MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-util-filemonitor -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-util-filemonitor" PASS 1 test-util-filemonitor /util/filemonitor MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-util-sockets -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-util-sockets" ==7210==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-util-sockets /util/socket/is-socket/bad PASS 2 test-util-sockets /util/socket/is-socket/good PASS 3 test-util-sockets /socket/fd-pass/name/good --- PASS 5 test-authz-list /auth/list/explicit/deny PASS 6 test-authz-list /auth/list/explicit/allow MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-authz-listfile -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-authz-listfile" ==7233==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-authz-listfile /auth/list/complex PASS 2 test-authz-listfile /auth/list/default/deny PASS 3 test-authz-listfile /auth/list/default/allow --- PASS 4 test-io-channel-file /io/channel/pipe/sync PASS 5 test-io-channel-file /io/channel/pipe/async MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-io-channel-tls -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-io-channel-tls" ==7259==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 37 ahci-test /x86_64/ahci/io/dma/lba28/simple/high ==7311==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-io-channel-tls /qio/channel/tls/basic MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-io-channel-command -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-io-channel-command" PASS 38 ahci-test /x86_64/ahci/io/dma/lba28/double/zero --- PASS 3 test-base64 /util/base64/not-nul-terminated PASS 4 test-base64 /util/base64/invalid-chars MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-crypto-pbkdf -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-crypto-pbkdf" ==7325==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-crypto-pbkdf /crypto/pbkdf/rfc3962/sha1/iter1 PASS 2 test-crypto-pbkdf /crypto/pbkdf/rfc3962/sha1/iter2 PASS 3 test-crypto-pbkdf /crypto/pbkdf/rfc3962/sha1/iter1200a --- PASS 3 test-crypto-afsplit /crypto/afsplit/sha256/big PASS 4 test-crypto-afsplit /crypto/afsplit/sha1/1000 MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-crypto-xts -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-crypto-xts" ==7346==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 1 test-crypto-xts /crypto/xts/t-1-key-32-ptx-32/basic PASS 2 test-crypto-xts /crypto/xts/t-1-key-32-ptx-32/split PASS 3 test-crypto-xts /crypto/xts/t-1-key-32-ptx-32/unaligned --- PASS 3 test-logging /logging/logfile_write_path PASS 4 test-logging /logging/logfile_lock_path MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-replication -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-replication" ==7371==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7364==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7364==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffff9821000; bottom 0x7f244817b000; size: 0x00dbb16a6000 (943574376448) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 1 test-replication /replication/primary/read PASS 2 test-replication /replication/primary/write PASS 41 ahci-test /x86_64/ahci/io/dma/lba28/long/zero ==7380==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7380==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffdc6759000; bottom 0x7f3811b23000; size: 0x00c5b4c36000 (849141260288) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 3 test-replication /replication/primary/start --- PASS 5 test-replication /replication/primary/do_checkpoint PASS 6 test-replication /replication/primary/get_error_all PASS 42 ahci-test /x86_64/ahci/io/dma/lba28/long/low ==7387==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7387==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffe785f0000; bottom 0x7f855b57b000; size: 0x00791d075000 (520178061312) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 7 test-replication /replication/secondary/read PASS 43 ahci-test /x86_64/ahci/io/dma/lba28/long/high ==7394==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 8 test-replication /replication/secondary/write PASS 44 ahci-test /x86_64/ahci/io/dma/lba28/short/zero ==7400==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 45 ahci-test /x86_64/ahci/io/dma/lba28/short/low ==7406==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 46 ahci-test /x86_64/ahci/io/dma/lba28/short/high ==7413==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7371==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffcefee0000; bottom 0x7ff98278b000; size: 0x00036d755000 (14721306624) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 47 ahci-test /x86_64/ahci/io/dma/lba48/simple/zero PASS 9 test-replication /replication/secondary/start ==7437==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 48 ahci-test /x86_64/ahci/io/dma/lba48/simple/low ==7443==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 49 ahci-test /x86_64/ahci/io/dma/lba48/simple/high ==7449==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 50 ahci-test /x86_64/ahci/io/dma/lba48/double/zero ==7455==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 51 ahci-test /x86_64/ahci/io/dma/lba48/double/low PASS 10 test-replication /replication/secondary/stop ==7461==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 52 ahci-test /x86_64/ahci/io/dma/lba48/double/high ==7467==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7467==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffe783a7000; bottom 0x7f31761fd000; size: 0x00cd021aa000 (880503595008) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 53 ahci-test /x86_64/ahci/io/dma/lba48/long/zero ==7474==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7474==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffe0ef50000; bottom 0x7f01a037b000; size: 0x00fc6ebd5000 (1084189659136) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 54 ahci-test /x86_64/ahci/io/dma/lba48/long/low PASS 11 test-replication /replication/secondary/continuous_replication ==7481==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7481==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffcca9ff000; bottom 0x7f2c3eb23000; size: 0x00d08bedc000 (895700811776) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 55 ahci-test /x86_64/ahci/io/dma/lba48/long/high ==7488==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 56 ahci-test /x86_64/ahci/io/dma/lba48/short/zero ==7494==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 12 test-replication /replication/secondary/do_checkpoint PASS 57 ahci-test /x86_64/ahci/io/dma/lba48/short/low ==7500==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 13 test-replication /replication/secondary/get_error_all PASS 58 ahci-test /x86_64/ahci/io/dma/lba48/short/high MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-bufferiszero -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-bufferiszero" ==7506==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 59 ahci-test /x86_64/ahci/io/ncq/simple ==7515==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 60 ahci-test /x86_64/ahci/io/ncq/retry ==7521==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 61 ahci-test /x86_64/ahci/flush/simple ==7527==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 62 ahci-test /x86_64/ahci/flush/retry ==7533==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7539==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 63 ahci-test /x86_64/ahci/flush/migrate ==7547==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7553==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 64 ahci-test /x86_64/ahci/migrate/sanity ==7561==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7567==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 65 ahci-test /x86_64/ahci/migrate/dma/simple ==7575==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7581==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 66 ahci-test /x86_64/ahci/migrate/dma/halted ==7589==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7595==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 67 ahci-test /x86_64/ahci/migrate/ncq/simple ==7603==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7609==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 68 ahci-test /x86_64/ahci/migrate/ncq/halted ==7617==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 69 ahci-test /x86_64/ahci/cdrom/eject ==7622==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 70 ahci-test /x86_64/ahci/cdrom/dma/single ==7628==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 71 ahci-test /x86_64/ahci/cdrom/dma/multi ==7634==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 72 ahci-test /x86_64/ahci/cdrom/pio/single ==7640==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7640==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffe5dae4000; bottom 0x7fa7999fe000; size: 0x0056c40e6000 (372656463872) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 PASS 73 ahci-test /x86_64/ahci/cdrom/pio/multi ==7646==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 74 ahci-test /x86_64/ahci/cdrom/pio/bcl MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img tests/qtest/hd-geo-test -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="hd-geo-test" PASS 1 hd-geo-test /x86_64/hd-geo/ide/none ==7660==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 2 hd-geo-test /x86_64/hd-geo/ide/drive/cd_0 ==7666==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 3 hd-geo-test /x86_64/hd-geo/ide/drive/mbr/blank ==7672==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 4 hd-geo-test /x86_64/hd-geo/ide/drive/mbr/lba ==7678==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 5 hd-geo-test /x86_64/hd-geo/ide/drive/mbr/chs ==7684==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 6 hd-geo-test /x86_64/hd-geo/ide/device/mbr/blank ==7690==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 7 hd-geo-test /x86_64/hd-geo/ide/device/mbr/lba PASS 1 test-bufferiszero /cutils/bufferiszero MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-uuid -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="test-uuid" --- PASS 136 ptimer-test /ptimer/periodic policy=no_immediate_trigger,no_immediate_reload, PASS 137 ptimer-test /ptimer/on_the_fly_mode_change policy=no_immediate_trigger,no_immediate_reload, PASS 138 ptimer-test /ptimer/on_the_fly_period_change policy=no_immediate_trigger,no_immediate_reload, ==7696==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 139 ptimer-test /ptimer/on_the_fly_freq_change policy=no_immediate_trigger,no_immediate_reload, PASS 140 ptimer-test /ptimer/run_with_period_0 policy=no_immediate_trigger,no_immediate_reload, PASS 141 ptimer-test /ptimer/run_with_delta_0 policy=no_immediate_trigger,no_immediate_reload, --- PASS 21 test-qgraph /qgraph/test_two_test_same_interface PASS 22 test-qgraph /qgraph/test_test_in_path PASS 23 test-qgraph /qgraph/test_double_edge ==7712==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 9 hd-geo-test /x86_64/hd-geo/ide/device/user/chs ==7720==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 10 hd-geo-test /x86_64/hd-geo/ide/device/user/chst ==7726==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7730==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7734==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7738==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7742==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7746==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7750==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7754==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7757==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 11 hd-geo-test /x86_64/hd-geo/override/ide ==7764==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7768==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7772==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7776==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7780==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7784==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7788==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7792==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7795==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 12 hd-geo-test /x86_64/hd-geo/override/scsi ==7802==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7806==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7810==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7814==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7818==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7822==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7826==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7830==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7833==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 13 hd-geo-test /x86_64/hd-geo/override/scsi_2_controllers ==7840==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7844==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7848==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7852==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7855==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 14 hd-geo-test /x86_64/hd-geo/override/virtio_blk ==7862==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7866==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7869==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 15 hd-geo-test /x86_64/hd-geo/override/zero_chs ==7876==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7880==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7884==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7888==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7891==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 16 hd-geo-test /x86_64/hd-geo/override/scsi_hot_unplug ==7898==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7902==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7906==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7910==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==7913==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! PASS 17 hd-geo-test /x86_64/hd-geo/override/virtio_hot_unplug MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img tests/qtest/boot-order-test -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="boot-order-test" PASS 1 boot-order-test /x86_64/boot-order/pc --- Could not access KVM kernel module: No such file or directory qemu-system-x86_64: -accel kvm: failed to initialize kvm: No such file or directory qemu-system-x86_64: falling back to tcg ==7982==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! Looking for expected file 'tests/data/acpi/pc/FACP' Using expected file 'tests/data/acpi/pc/FACP' --- Using expected file 'tests/data/acpi/pc/HPET' Looking for expected file 'tests/data/acpi/pc/WAET' ** ERROR:/tmp/qemu-test/src/tests/qtest/bios-tables-test.c:367:load_expected_aml: assertion failed: (exp_sdt.aml_file) ERROR - Bail out! ERROR:/tmp/qemu-test/src/tests/qtest/bios-tables-test.c:367:load_expected_aml: assertion failed: (exp_sdt.aml_file) make: *** [/tmp/qemu-test/src/tests/Makefile.include:632: check-qtest-x86_64] Error 1 make: *** Waiting for unfinished jobs.... Traceback (most recent call last): File "./tests/docker/docker.py", line 664, in <module> --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=1d34807a851b48708093f12f049854a9', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-rir7t007/src/docker-src.2020-03-11-14.33.18.26628:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=1d34807a851b48708093f12f049854a9 make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-rir7t007/src' make: *** [docker-run-test-debug@fedora] Error 2 real 27m28.893s user 0m8.326s The full log is available at http://patchew.org/logs/20200311170826.79419-1-liran.alon@oracle.com/testing.asan/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
© 2016 - 2024 Red Hat, Inc.