Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
On Thu, Mar 7, 2024 at 7:22 PM Daniel Henrique Barboza
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
> >> 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 4]Signature : "SPCR"[Serial Port
> >>> Console Redirection Table]
> >>> -[004h 0004 4] Table Length : 0050
> >>> +[004h 0004 4] Table Length : 004F
> >>>[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 :
> >>> +[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
> ---
> 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_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
Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
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
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 4]Signature : "SPCR"[Serial Port
Console Redirection Table]
-[004h 0004 4] Table Length : 0050
+[004h 0004 4] Table Length : 004F
[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 :
+[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
---
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_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
---
Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
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
> 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 4]Signature : "SPCR"[Serial Port
> > Console Redirection Table]
> > -[004h 0004 4] Table Length : 0050
> > +[004h 0004 4] Table Length : 004F
> > [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 :
> > +[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
> > > ---
> > > 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_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 */
> > > +
RE: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
> -Original Message- > From: Alistair Francis > Sent: Thursday, March 7, 2024 9:33 AM > To: Daniel Henrique Barboza > Cc: JeeHeng Sia ; qemu-...@nongnu.org; > qemu-devel@nongnu.org; qemu-ri...@nongnu.org; > m...@redhat.com; imamm...@redhat.com; anisi...@redhat.com; > peter.mayd...@linaro.org; shannon.zha...@gmail.com; > suni...@ventanamicro.com; pal...@dabbelt.com; alistair.fran...@wdc.com; > bin.m...@windriver.com; liwei1...@gmail.com; > zhiwei_...@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 > 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 4]Signature : "SPCR"[Serial Port > > Console Redirection Table] > > -[004h 0004 4] Table Length : 0050 > > +[004h 0004 4] Table Length : 004F > > [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 : > > +[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 > > > --- > > > 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, > > > } > > > } >
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
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 4]Signature : "SPCR"[Serial Port
> Console Redirection Table]
> -[004h 0004 4] Table Length : 0050
> +[004h 0004 4] Table Length : 004F
> [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 :
> +[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
> > ---
> > 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_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,
Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
On 3/6/24 15:57, Daniel Henrique Barboza wrote: 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. s/foresight/hindsight There's no 'make check' for what we want to say in the ML but hopefully there's a way to enable 'bios-table-test' for RISC-V :D Daniel
Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
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 4]Signature : "SPCR"[Serial Port Console
Redirection Table]
-[004h 0004 4] Table Length : 0050
+[004h 0004 4] Table Length : 004F
[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 :
+[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
---
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_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);
Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
On Tue, Jan 16, 2024 at 11:11 AM 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
Reviewed-by: Alistair Francis
Alistair
> ---
> 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_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);
> +/* Terminal Type */
> +build_append_int_noprefix(table_data, f->terminal_type, 1);
> +/* PCI Device ID */
> +build_append_int_noprefix(table_data, f->pci_device_id, 2);
> +/* PCI Vendor ID */
> +build_append_int_noprefix(table_data, f->pci_vendor_id, 2);
> +/* PCI Bus Number */
> +build_append_int_noprefix(table_data, f->pci_bus, 1);
> +/* PCI Device Number */
> +build_append_int_noprefix(table_data, f->pci_device, 1);
> +/* PCI Function Number */
> +build_append_int_noprefix(table_data, f->pci_function, 1);
> +/* PCI Flags */
> +build_append_int_noprefix(table_data, f->pci_flags, 4);
> +/* PCI Segment */
> +build_append_int_noprefix(table_data, f->pci_segment, 1);
> +/* Reserved */
> +build_append_int_noprefix(table_data, 0, 4);
> +
> +acpi_table_end(linker, );
> +}
> /*
> * ACPI spec, Revision 6.3
> * 5.2.29 Processor Properties Topology Table (PPTT)
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index a22a2f43a5..195767c0f0 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -431,48 +431,34 @@ build_iort(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> * Rev: 1.07
> */
> static void
> -build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> +spcr_setup(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> {
> -AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = vms->oem_id,
> -.oem_table_id = vms->oem_table_id };
> -
> -acpi_table_begin(, table_data);
> -
> -/* Interface Type */
> -build_append_int_noprefix(table_data, 3, 1); /* ARM PL011 UART */
> -build_append_int_noprefix(table_data, 0, 3); /* Reserved */
> -/* Base Address */
> -build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 32, 0, 3,
> - vms->memmap[VIRT_UART].base);
> -/* Interrupt Type */
> -build_append_int_noprefix(table_data,
> -(1 << 3) /* Bit[3] ARMH GIC interrupt */, 1);
> -build_append_int_noprefix(table_data, 0, 1); /* IRQ */
> -/* Global System Interrupt */
> -build_append_int_noprefix(table_data,
> - vms->irqmap[VIRT_UART] + ARM_SPI_BASE, 4);
> -build_append_int_noprefix(table_data, 3 /* 9600 */, 1); /* Baud Rate */
> -build_append_int_noprefix(table_data, 0 /* No Parity */, 1); /* Parity */
> -/* Stop Bits */
> -build_append_int_noprefix(table_data, 1 /* 1 Stop bit */, 1);
> -/* Flow Control */
> -build_append_int_noprefix(table_data,
> -(1 << 1) /* RTS/CTS hardware flow control */, 1);
> -/* Terminal Type */
> -build_append_int_noprefix(table_data, 0 /* VT100 */, 1);
> -
