Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location

[Alistair Francis]

On Thu, Mar 7, 2024 at 7:22 PM Daniel Henrique Barboza
<dbarboza@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
> >> <dbarboza@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.sia@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