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

[JeeHeng Sia]

> -----Original Message-----
> From: Alistair Francis <alistair23@gmail.com>
> Sent: Thursday, March 7, 2024 9:33 AM
> To: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Cc: JeeHeng Sia <jeeheng.sia@starfivetech.com>; qemu-arm@nongnu.org; 
> qemu-devel@nongnu.org; qemu-riscv@nongnu.org;
> mst@redhat.com; imammedo@redhat.com; anisinha@redhat.com; 
> peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> sunilvl@ventanamicro.com; palmer@dabbelt.com; alistair.francis@wdc.com; 
> bin.meng@windriver.com; liwei1518@gmail.com;
> zhiwei_liu@linux.alibaba.com
> Subject: Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation 
> to common location
> 
> 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.
I apologize for the oversight I've made that caused the trouble.
Thanks for the quick turnaround.
> 
> Alistair