On Thu, Mar 7, 2024 at 7:22 PM Daniel Henrique Barboza
<dbarb...@ventanamicro.com> wrote:
>
>
>
> On 3/7/24 00:45, Sunil V L wrote:
> > 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
> >> <dbarb...@ventanamicro.com> 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 <jeeheng....@starfivetech.com>
> >>>> ---
> >>>> 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?
>
> I don't recall the last time I had to deal with a new test being added after
> the
> code freeze. It's not a new feature, it increases test coverage and it might
> find
> bugs, so my guess is that it's fine.
>
> Alistair/Peter, any thoughts?
It's probably a post 9.0 thing, especially as I have been sick the
last week and am only just catching up.
Alistair
>
>
> Thanks,
>
> Daniel
>
>
>
>
> >
> > Thanks,
> > Sunil
