From: Alistair Francis <alistair23@gmail.com>
To: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Cc: Sunil V L <sunilvl@ventanamicro.com>,
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, 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: Wed, 20 Mar 2024 14:41:33 +1000 [thread overview]
Message-ID: <CAKmqyKNaLLBqFnd+BoL+OK6REr95rOP6D5MNWR=hS+dqSYRQtg@mail.gmail.com> (raw)
In-Reply-To: <b50a1d69-4978-4aa9-a30b-24f5c8f8d306@ventanamicro.com>
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
next prev parent reply other threads:[~2024-03-20 4:42 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
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 [this message]
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='CAKmqyKNaLLBqFnd+BoL+OK6REr95rOP6D5MNWR=hS+dqSYRQtg@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 read-only IMAP folder(s) and NNTP newsgroup(s).