acpi: Add Windows ACPI Emulated Device Table (WAET)

Commit Message

Liran Alon March 11, 2020, 5:08 p.m. UTC

From: Elad Gabay <[e-mail]>

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 <[e-mail]>
Co-developed-by: Liran Alon <[e-mail]>
Signed-off-by: Liran Alon <[e-mail]>
---
 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(+)

Comments

[e-mail] March 11, 2020, 6:59 p.m. UTC | #1

Patchew URL: https://patchew.org/QEMU/[e-mail]/



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/[e-mail]/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to [e-mail]

[e-mail] March 11, 2020, 7 p.m. UTC | #2

Patchew URL: https://patchew.org/QEMU/[e-mail]/



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/[e-mail]/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to [e-mail]

Liran Alon March 11, 2020, 7:08 p.m. UTC | #3

On 11/03/2020 20:59, [e-mail] wrote:
> Patchew URL: https://urldefense.com/v3/__https://patchew.org/QEMU/[e-mail]/__;!!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

Michael S. Tsirkin March 11, 2020, 8:24 p.m. UTC | #4

On Wed, Mar 11, 2020 at 09:08:56PM +0200, Liran Alon wrote:
> 
> On 11/03/2020 20:59, [e-mail] wrote:
> > Patchew URL: https://urldefense.com/v3/__https://patchew.org/QEMU/[e-mail]/__;!!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

Michael S. Tsirkin March 11, 2020, 8:36 p.m. UTC | #5

Thanks for the patch! Some questions/comments:

On Wed, Mar 11, 2020 at 07:08:26PM +0200, Liran Alon wrote:
> From: Elad Gabay <[e-mail]>
> 
> 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 <[e-mail]>
> Co-developed-by: Liran Alon <[e-mail]>
> Signed-off-by: Liran Alon <[e-mail]>
> ---
>  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

Liran Alon March 11, 2020, 11:20 p.m. UTC | #6

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 <[e-mail]>
>>
>> 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 <[e-mail]>
>> Co-developed-by: Liran Alon <[e-mail]>
>> Signed-off-by: Liran Alon <[e-mail]>
>> ---
>>   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

Liran Alon March 12, 2020, 1:31 a.m. UTC | #7

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

Michael S. Tsirkin March 12, 2020, 6:12 a.m. UTC | #8

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 <[e-mail]>
> > > 
> > > 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 <[e-mail]>
> > > Co-developed-by: Liran Alon <[e-mail]>
> > > Signed-off-by: Liran Alon <[e-mail]>
> > > ---
> > >   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
>

Michael S. Tsirkin March 12, 2020, 6:27 a.m. UTC | #9

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.

Liran Alon March 12, 2020, 11:30 a.m. UTC | #10

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 <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>> Co-developed-by: Liran Alon <[e-mail]>
>>>> Signed-off-by: Liran Alon <[e-mail]>
>>>> ---
>>>>    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

Michael S. Tsirkin March 12, 2020, 12:19 p.m. UTC | #11

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 <[e-mail]>
> > > > > 
> > > > > 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 <[e-mail]>
> > > > > Co-developed-by: Liran Alon <[e-mail]>
> > > > > Signed-off-by: Liran Alon <[e-mail]>
> > > > > ---
> > > > >    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)

Liran Alon March 12, 2020, 12:55 p.m. UTC | #12

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 <[e-mail]>
>>>>>>
>>>>>> 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 <[e-mail]>
>>>>>> Co-developed-by: Liran Alon <[e-mail]>
>>>>>> Signed-off-by: Liran Alon <[e-mail]>
>>>>>> ---
>>>>>>     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

Igor Mammedov March 12, 2020, 4:27 p.m. UTC | #13

On Wed, 11 Mar 2020 19:08:26 +0200
Liran Alon <[e-mail]> wrote:

> From: Elad Gabay <[e-mail]>
> 
> 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 <[e-mail]>
> Co-developed-by: Liran Alon <[e-mail]>
> Signed-off-by: Liran Alon <[e-mail]>
> ---
>  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;

Igor Mammedov March 12, 2020, 4:35 p.m. UTC | #14

On Thu, 12 Mar 2020 14:55:50 +0200
Liran Alon <[e-mail]> 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 <[e-mail]>
> >>>>>>
> >>>>>> 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 <[e-mail]>
> >>>>>> Co-developed-by: Liran Alon <[e-mail]>
> >>>>>> Signed-off-by: Liran Alon <[e-mail]>
> >>>>>> ---
> >>>>>>     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
> 
>

Michael S. Tsirkin March 12, 2020, 5:09 p.m. UTC | #15

On Thu, Mar 12, 2020 at 05:27:45PM +0100, Igor Mammedov wrote:
> On Wed, 11 Mar 2020 19:08:26 +0200
> Liran Alon <[e-mail]> wrote:
> 
> > From: Elad Gabay <[e-mail]>
> > 
> > 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 <[e-mail]>
> > Co-developed-by: Liran Alon <[e-mail]>
> > Signed-off-by: Liran Alon <[e-mail]>
> > ---
> >  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;

Liran Alon March 12, 2020, 5:28 p.m. UTC | #16

On 12/03/2020 18:27, Igor Mammedov wrote:
> On Wed, 11 Mar 2020 19:08:26 +0200
> Liran Alon <[e-mail]> 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

Liran Alon March 12, 2020, 6:48 p.m. UTC | #17

On 12/03/2020 18:35, Igor Mammedov wrote:
> On Thu, 12 Mar 2020 14:55:50 +0200
> Liran Alon <[e-mail]> 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.

Michael S. Tsirkin March 12, 2020, 7:47 p.m. UTC | #18

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 <[e-mail]> 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.

Liran Alon March 12, 2020, 9:17 p.m. UTC | #19

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 <[e-mail]> 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

Igor Mammedov March 13, 2020, 9:35 a.m. UTC | #20

On Thu, 12 Mar 2020 20:48:48 +0200
Liran Alon <[e-mail]> wrote:

> On 12/03/2020 18:35, Igor Mammedov wrote:
> > On Thu, 12 Mar 2020 14:55:50 +0200
> > Liran Alon <[e-mail]> 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.  
>

Igor Mammedov March 13, 2020, 9:36 a.m. UTC | #21

On Thu, 12 Mar 2020 13:09:51 -0400
"Michael S. Tsirkin" <[e-mail]> 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 <[e-mail]> wrote:
> >   
> > > From: Elad Gabay <[e-mail]>
> > > 
> > > 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 <[e-mail]>
> > > Co-developed-by: Liran Alon <[e-mail]>
> > > Signed-off-by: Liran Alon <[e-mail]>
> > > ---
> > >  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 
[...]

Igor Mammedov March 13, 2020, 10:05 a.m. UTC | #22

On Thu, 12 Mar 2020 19:28:31 +0200
Liran Alon <[e-mail]> wrote:

> On 12/03/2020 18:27, Igor Mammedov wrote:
> > On Wed, 11 Mar 2020 19:08:26 +0200
> > Liran Alon <[e-mail]> 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).

[...]

Liran Alon March 13, 2020, 2:23 p.m. UTC | #23

On 13/03/2020 12:05, Igor Mammedov wrote:
> On Thu, 12 Mar 2020 19:28:31 +0200
> Liran Alon <[e-mail]> wrote:
>
>> On 12/03/2020 18:27, Igor Mammedov wrote:
>>> On Wed, 11 Mar 2020 19:08:26 +0200
>>> Liran Alon <[e-mail]> 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

Michael S. Tsirkin March 13, 2020, 3:26 p.m. UTC | #24

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" <[e-mail]> 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 <[e-mail]> wrote:
> > >   
> > > > From: Elad Gabay <[e-mail]>
> > > > 
> > > > 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 <[e-mail]>
> > > > Co-developed-by: Liran Alon <[e-mail]>
> > > > Signed-off-by: Liran Alon <[e-mail]>
> > > > ---
> > > >  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.

Igor Mammedov March 16, 2020, 1:26 p.m. UTC | #25

On Fri, 13 Mar 2020 11:26:45 -0400
"Michael S. Tsirkin" <[e-mail]> 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" <[e-mail]> 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 <[e-mail]> wrote:
> > > >     
> > > > > From: Elad Gabay <[e-mail]>
> > > > > 
> > > > > 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 <[e-mail]>
> > > > > Co-developed-by: Liran Alon <[e-mail]>
> > > > > Signed-off-by: Liran Alon <[e-mail]>
> > > > > ---
> > > > >  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.

Patch

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;