On Thu, Mar 07, 2024 at 11:33:25AM +1000, Alistair Francis wrote: > On Thu, Mar 7, 2024 at 4:59 AM Daniel Henrique Barboza > <[email protected]> wrote: > > > > Hi, > > > > This patch break check-qtest, most specifically 'bios-table'test', for > > aarch64. > > I found this while running riscv-to-apply.next in the Gitlab pipeline. > > > > > > Here's the output: > > > > $ make -j && QTEST_QEMU_BINARY=./qemu-system-aarch64 V=1 > > ./tests/qtest/bios-tables-test > > TAP version 13 > > # random seed: R02Sf0f2fa0a3fac5d540b1681c820621b7d > > # starting QEMU: exec ./qemu-system-aarch64 -qtest > > unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev > > socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control > > -display none -audio none -machine none -accel qtest > > 1..8 > > # Start of aarch64 tests > > # Start of acpi tests > > # starting QEMU: exec ./qemu-system-aarch64 -qtest > > unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev > > socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control > > -display none -audio none -machine virt -accel tcg -nodefaults -nographic > > -drive if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on > > -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on > > -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 -cpu > > cortex-a57 -smbios type=4,max-speed=2900,current-speed=2700 -accel qtest > > acpi-test: Warning! SPCR binary file mismatch. Actual > > [aml:/tmp/aml-9G53J2], Expected [aml:tests/data/acpi/virt/SPCR]. > > See source file tests/qtest/bios-tables-test.c for instructions on how to > > update expected files. > > acpi-test: Warning! SPCR mismatch. Actual [asl:/tmp/asl-SR53J2.dsl, > > aml:/tmp/aml-9G53J2], Expected [asl:/tmp/asl-4Z33J2.dsl, > > aml:tests/data/acpi/virt/SPCR]. > > > > The diff is here: > > > > --- /tmp/asl-4Z33J2.dsl 2024-03-06 15:40:24.879879348 -0300 > > +++ /tmp/asl-SR53J2.dsl 2024-03-06 15:40:24.877879347 -0300 > > @@ -1,57 +1,49 @@ > > /* > > * Intel ACPI Component Architecture > > * AML/ASL+ Disassembler version 20220331 (64-bit version) > > * Copyright (c) 2000 - 2022 Intel Corporation > > > > (...) > > > > [000h 0000 4] Signature : "SPCR" [Serial Port > > Console Redirection Table] > > -[004h 0004 4] Table Length : 00000050 > > +[004h 0004 4] Table Length : 0000004F > > [008h 0008 1] Revision : 02 > > -[009h 0009 1] Checksum : B1 > > +[009h 0009 1] Checksum : B2 > > [00Ah 0010 6] Oem ID : "BOCHS " > > > > (...) > > > > -[042h 0066 2] PCI Vendor ID : FFFF > > +[042h 0066 2] PCI Vendor ID : 00FF > > > > > > After inspecting the common helper and what the original ARM code was doing > > I found out that we're missing something down there: > > > > > > On 1/15/24 22:09, Sia Jee Heng wrote: > > > RISC-V should also generate the SPCR in a manner similar to ARM. > > > Therefore, instead of replicating the code, relocate this function > > > to the common AML build. > > > > > > Signed-off-by: Sia Jee Heng <[email protected]> > > > --- > > > hw/acpi/aml-build.c | 51 ++++++++++++++++++++++++++++ > > > hw/arm/virt-acpi-build.c | 68 +++++++++++++++---------------------- > > > include/hw/acpi/acpi-defs.h | 33 ++++++++++++++++++ > > > include/hw/acpi/aml-build.h | 4 +++ > > > 4 files changed, 115 insertions(+), 41 deletions(-) > > > > > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > > > index af66bde0f5..f3904650e4 100644 > > > --- a/hw/acpi/aml-build.c > > > +++ b/hw/acpi/aml-build.c > > > @@ -1994,6 +1994,57 @@ static void build_processor_hierarchy_node(GArray > > > *tbl, uint32_t flags, > > > } > > > } > > > > > > +void build_spcr(GArray *table_data, BIOSLinker *linker, > > > + const AcpiSpcrData *f, const uint8_t rev, > > > + const char *oem_id, const char *oem_table_id) > > > +{ > > > + AcpiTable table = { .sig = "SPCR", .rev = rev, .oem_id = oem_id, > > > + .oem_table_id = oem_table_id }; > > > + > > > + acpi_table_begin(&table, table_data); > > > + /* Interface type */ > > > + build_append_int_noprefix(table_data, f->interface_type, 1); > > > + /* Reserved */ > > > + build_append_int_noprefix(table_data, 0, 3); > > > + /* Base Address */ > > > + build_append_gas(table_data, f->base_addr.id, f->base_addr.width, > > > + f->base_addr.offset, f->base_addr.size, > > > + f->base_addr.addr); > > > + /* Interrupt type */ > > > + build_append_int_noprefix(table_data, f->interrupt_type, 1); > > > + /* IRQ */ > > > + build_append_int_noprefix(table_data, f->pc_interrupt, 1); > > > + /* Global System Interrupt */ > > > + build_append_int_noprefix(table_data, f->interrupt, 4); > > > + /* Baud Rate */ > > > + build_append_int_noprefix(table_data, f->baud_rate, 1); > > > + /* Parity */ > > > + build_append_int_noprefix(table_data, f->parity, 1); > > > + /* Stop Bits */ > > > + build_append_int_noprefix(table_data, f->stop_bits, 1); > > > + /* Flow Control */ > > > + build_append_int_noprefix(table_data, f->flow_control, 1); > > > > Here. We're missing the "Language" entry. > > > > > > This diff fixes the broken test: > > > > > > $ git diff > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > > index f3904650e4..6d4517cfbe 100644 > > --- a/hw/acpi/aml-build.c > > +++ b/hw/acpi/aml-build.c > > @@ -2024,6 +2024,8 @@ void build_spcr(GArray *table_data, BIOSLinker > > *linker, > > build_append_int_noprefix(table_data, f->stop_bits, 1); > > /* Flow Control */ > > build_append_int_noprefix(table_data, f->flow_control, 1); > > + /* Language */ > > + build_append_int_noprefix(table_data, f->language, 1); > > /* Terminal Type */ > > build_append_int_noprefix(table_data, f->terminal_type, 1); > > /* PCI Device ID */ > > Thanks! I added this to the patch > > > > > > > > > As a side note, it seems like 'bios-table-test' isn't being run for RISC-V. > > Not sure if this > > is intentional or a foresight. > > I think you are looking for oversight :) > > I think it's just an oversight, not intentional. > Adding these test cases is WIP. Drew had already pointed this. Myself and Haibo are looking at it. But I doubt patches can be sent before 9.0 deadlines. Can we add them post 9.0?
Thanks, Sunil
