From: Alistair Francis <alistair23@gmail.com>
To: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Cc: Sia Jee Heng <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
Date: Thu, 7 Mar 2024 11:33:25 +1000 [thread overview]
Message-ID: <CAKmqyKPE-ERHShmAJQH3VNc7XqOfOeKYYyCRswJSUwf7hqm1xA@mail.gmail.com> (raw)
In-Reply-To: <49d0d8d3-c7fd-4039-aeed-6c31ba16557a@ventanamicro.com>
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.
Alistair
next prev parent reply other threads:[~2024-03-07 1:35 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-16 1:09 [PATCH v2 0/2] RISC-V: ACPI: Enable SPCR Sia Jee Heng
2024-01-16 1:09 ` [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location Sia Jee Heng
2024-02-15 3:26 ` Alistair Francis
2024-03-06 18:57 ` Daniel Henrique Barboza
2024-03-06 20:00 ` Daniel Henrique Barboza
2024-03-07 1:33 ` Alistair Francis [this message]
2024-03-07 1:42 ` JeeHeng Sia
2024-03-07 3:45 ` Sunil V L
2024-03-07 9:22 ` Daniel Henrique Barboza
2024-03-20 4:41 ` Alistair Francis
2024-01-16 1:09 ` [PATCH v2 2/2] hw/riscv/virt-acpi-build.c: Generate SPCR table Sia Jee Heng
2024-02-15 3:29 ` Alistair Francis
2024-02-15 9:54 ` [PATCH v2 0/2] RISC-V: ACPI: Enable SPCR Alistair Francis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKmqyKPE-ERHShmAJQH3VNc7XqOfOeKYYyCRswJSUwf7hqm1xA@mail.gmail.com \
--to=alistair23@gmail.com \
--cc=alistair.francis@wdc.com \
--cc=anisinha@redhat.com \
--cc=bin.meng@windriver.com \
--cc=dbarboza@ventanamicro.com \
--cc=imammedo@redhat.com \
--cc=jeeheng.sia@starfivetech.com \
--cc=liwei1518@gmail.com \
--cc=mst@redhat.com \
--cc=palmer@dabbelt.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=shannon.zhaosl@gmail.com \
--cc=sunilvl@ventanamicro.com \
--cc=zhiwei_liu@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).