Re: [PATCH v6 00/13] RISC-V: ACPI: Enable AIA, PLIC and update RHCT

[Daniel Henrique Barboza]

Sunil,


While doing unrelated work (running Gitlab on my series built on top of
current riscv-to-apply.next), I hit the following error:

https://gitlab.com/danielhb/qemu/-/jobs/5448178994

==========

4/257 ERROR:../tests/qtest/bios-tables-test.c:535:test_acpi_asl: assertion 
failed: (all_tables_match) ERROR
  4/257 qemu:qtest+qtest-i386 / qtest-i386/bios-tables-test                
ERROR           7.77s   killed by signal 6 SIGABRT
> > > G_TEST_DBUS_DAEMON=/builds/danielhb/qemu/tests/dbus-vmstate-daemon.sh 
> > > PYTHON=/builds/danielhb/qemu/build/pyvenv/bin/python3 MALLOC_PERTURB_=159 
> > > QTEST_QEMU_BINARY=./qemu-system-i386 
> > > /builds/danielhb/qemu/build/tests/qtest/bios-tables-test --tap -k

acpi-test: Warning! DSDT binary file mismatch. Actual [aml:/tmp/aml-IOYVD2], 
Expected [aml:tests/data/acpi/microvm/DSDT].
See source file tests/qtest/bios-tables-test.c for instructions on how to 
update expected files.
to see ASL diff between mismatched files install IASL, rebuild QEMU from 
scratch and re-run tests with V=1 environment variable set**
ERROR:../tests/qtest/bios-tables-test.c:535:test_acpi_asl: assertion failed: 
(all_tables_match)
(test program exited with status code -6)

(...)

Summary of Failures:
4571  4/257 qemu:qtest+qtest-i386 / qtest-i386/bios-tables-test                
ERROR           7.77s   killed by signal 6 SIGABRT
4572  7/257 qemu:qtest+qtest-aarch64 / qtest-aarch64/bios-tables-test          
ERROR          66.00s   killed by signal 6 SIGABRT
4573  Ok:                 247
4574  Expected Fail:      0
4575  Fail:               2
4576  Unexpected Pass:    0
4577  Skipped:            8
4578  Timeout:            0

==========

(qtest-aarch64/bios-tables-test' fails with the same error message as 
'qtest-i386/bios-tables-test')


To be sure, since I ran a hacked version of your patches in that Gitlab 
pipeline, I
removed your ACPI patches from riscv-to-apply.next, applied this new version, 
and then
tried to run the x86 qtest locally. It still fails, same error:

$ rm -rf build && ./configure --target-list=x86_64-softmmu
$ make -j && QTEST_QEMU_BINARY=./build/qemu-system-x86_64 
./build/tests/qtest/bios-tables-test

(...)
# starting QEMU: exec ./build/qemu-system-x86_64 -qtest 
unix:/tmp/qtest-508977.sock -qtest-log /dev/null -chardev 
socket,path=/tmp/qtest-508977.qmp,id=char0 -mon chardev=char0,mode=control 
-display none -audio none -machine microvm -accel kvm -accel tcg -net none  
-machine microvm,acpi=on,ioapic2=off,rtc=off -drive 
id=hd0,if=none,file=tests/acpi-test-disk-eyxyvN,format=raw -device 
virtio-blk-device,drive=hd0  -accel qtest
acpi-test: Warning! DSDT binary file mismatch. Actual [aml:/tmp/aml-H9IUD2], 
Expected [aml:tests/data/acpi/microvm/DSDT].
See source file tests/qtest/bios-tables-test.c for instructions on how to 
update expected files.
to see ASL diff between mismatched files install IASL, rebuild QEMU from 
scratch and re-run tests with V=1 environment variable set**
ERROR:../tests/qtest/bios-tables-test.c:535:test_acpi_asl: assertion failed: 
(all_tables_match)
not ok /x86_64/acpi/microvm - 
ERROR:../tests/qtest/bios-tables-test.c:535:test_acpi_asl: assertion failed: 
(all_tables_match)


I did a 'git bisect' and it pointed to the following patch:


$ git bisect good
e63248d45e8f8488706ed13a7b83266e578deafd is the first bad commit
commit e63248d45e8f8488706ed13a7b83266e578deafd
Author: Sunil V L <sunilvl@ventanamicro.com>
Date:   Thu Nov 2 22:32:13 2023 +0530

    hw/i386/acpi-microvm.c: Use common function to add virtio in DSDT
    
    With common function to add virtio in DSDT created now, update microvm
    code also to use it instead of duplicate code.
    
    Suggested-by: Andrew Jones <ajones@ventanamicro.com>
    Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
    Acked-by: Alistair Francis <alistair.francis@wdc.com>
    Acked-by: Michael S. Tsirkin <mst@redhat.com>

 hw/i386/acpi-microvm.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)
$

This suggests a problem in patch 03. I suggest doing a bisect on your own to be 
sure.

Unfortunately these tests will block the pipeline, forbidding us to merge it. 
I'm afraid
you'll need to fix it and send a v7.



Thanks,

Daniel


On 11/2/23 14:02, Sunil V L wrote:
> This series primarily enables external interrupt controllers (AIA and PLIC)
> in ACPI tables for RISC-V virt platform. It also updates RHCT with CMO and
> MMU related information.
>
> Below ECRs for these changes are approved by ASWG and will be
> available in next ACPI spec release.
>
> 1) MADT (AIA) - 
> https://drive.google.com/file/d/1oMGPyOD58JaPgMl1pKasT-VKsIKia7zR/view?usp=sharing
> 2) RHCT - 
> https://drive.google.com/file/d/1sKbOa8m1UZw1JkquZYe3F1zQBN1xXsaf/view?usp=sharing
>
> First two patches in this series are to migrate a couple of functions from
> ARM architecture to common code so that RISC-V doesn't need to duplicate
> the same.
>
> The patch set is based on Alistair's riscv-to-apply.next branch.
>
> These changes are also available in  riscv_acpi_b2_v6 branch at:
> https://github.com/vlsunil/qemu/
>
> Changes since v5:
>         1) Fixed the issue in PATCH 2 reported by Daniel found when built with
>            clang + --enable-debug.
>
> Changes since v4:
>         1) Updated copyright for new files as per SPDX format suggested by Drew.
>         2) Updated RINTC patch to avoid code duplication as suggested by Drew.
>         3) Moved mmu offset below cmo in MMU patch as suggested by Drew.
>         4) Updated tags.
>
> Changes since v3:
>         1) Addressed comments from Daniel and Drew.
>         2) Added a new patch in microvm to use common function for virtio in 
> DSDT.
>         3) Rebased to latest riscv-to-apply.next branch and added tags.
>
> Changes since v2:
>          1) Rebased to latest riscv-to-apply.next branch which needed
>             changing ext_icboz to ext_zicboz in CMO patch.
>          2) Fixed node type in MMU node.
>          3) Added latest tags.
>
> Changes since v1:
>          1) As per Igor's suggestion, migrated fw_cfg and virtio creation
>             functions to device specific file instead of generic aml-build.c.
>             Since ACPI is optional, new files are created and enabled for
>             build only when CONFIG_ACPI is enabled.
>          2) As per Igor's suggestion, properties are added to the GPEX PCI
>             host to indicate MMIO ranges. The platform fw can initialize
>             these to appropriate values and the DSDT generator can fetch
>             the information from the host bus itself. This makes the code
>             generic instead of machine specific.
>          3) Added PLIC patch from Haibo.
>          4) Rebased to latest riscv-to-apply.next and added RB tags as
>             appropriate.
> Sunil V L (13):
>    hw/arm/virt-acpi-build.c: Migrate fw_cfg creation to common location
>    hw/arm/virt-acpi-build.c: Migrate virtio creation to common location
>    hw/i386/acpi-microvm.c: Use common function to add virtio in DSDT
>    hw/riscv: virt: Make few IMSIC macros and functions public
>    hw/riscv/virt-acpi-build.c: Add AIA support in RINTC
>    hw/riscv/virt-acpi-build.c: Add IMSIC in the MADT
>    hw/riscv/virt-acpi-build.c: Add APLIC in the MADT
>    hw/riscv/virt-acpi-build.c: Add CMO information in RHCT
>    hw/riscv/virt-acpi-build.c: Add MMU node in RHCT
>    hw/pci-host/gpex: Define properties for MMIO ranges
>    hw/riscv/virt: Update GPEX MMIO related properties
>    hw/riscv/virt-acpi-build.c: Add IO controllers and devices
>    hw/riscv/virt-acpi-build.c: Add PLIC in MADT
>
>   hw/arm/virt-acpi-build.c        |  51 +----
>   hw/i386/acpi-microvm.c          |  15 +-
>   hw/nvram/fw_cfg-acpi.c          |  23 +++
>   hw/nvram/meson.build            |   1 +
>   hw/pci-host/gpex-acpi.c         |  13 ++
>   hw/pci-host/gpex.c              |  12 ++
>   hw/riscv/Kconfig                |   1 +
>   hw/riscv/virt-acpi-build.c      | 323 +++++++++++++++++++++++++++++---
>   hw/riscv/virt.c                 |  72 ++++---
>   hw/virtio/meson.build           |   1 +
>   hw/virtio/virtio-acpi.c         |  32 ++++
>   include/hw/nvram/fw_cfg_acpi.h  |  15 ++
>   include/hw/pci-host/gpex.h      |  28 ++-
>   include/hw/riscv/virt.h         |  26 +++
>   include/hw/virtio/virtio-acpi.h |  16 ++
>   15 files changed, 498 insertions(+), 131 deletions(-)
>   create mode 100644 hw/nvram/fw_cfg-acpi.c
>   create mode 100644 hw/virtio/virtio-acpi.c
>   create mode 100644 include/hw/nvram/fw_cfg_acpi.h
>   create mode 100644 include/hw/virtio/virtio-acpi.h