Re: [PATCH v4 1/2] hw/acpi: correct field sequence in SPCR table

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jun 29, 2026 at 08:12:12AM +0200, Heinrich Schuchardt wrote:
> On 6/29/26 02:58, Alistair Francis wrote:
> > On Mon, Jun 22, 2026 at 9:18 PM Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> > > 
> > > On LoongArch and RISC-V invalid SPCR tables are created:
> > > 
> > >      Terminal Type : 00
> > >      Language : 03
> > > 
> > > The correct values are:
> > > 
> > >      Terminal Type : 03
> > >      Language : 00
> > > 
> > > This is due to commit 7dd0b070fa09 ("hw/arm/virt-acpi-build.c: Migrate
> > > SPCR creation to common location") that swapped the fields.
> > > 
> > > See the specification of the table in
> > > https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/serial-port-console-redirection-table
> > > 
> > > This page shows version 1.10. But the sequence of the fields was not changed
> > > since version 1.0.
> > > 
> > > Our LoongArch and ARM code uses version 1.07 of the specification.
> > > Our RISC-V code uses version 1.10 of the specification.
> > > 
> > > Fixes: 7dd0b070fa09 ("hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location")
> > > Origin: https://lore.kernel.org/qemu-devel/20260326121947.51200-1-heinrich.schuchardt@canonical.com/T/#u
> > > Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/2146419
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > ---
> > > v4:
> > >          Reproduce the test data changes using rebuild-expected-aml.sh
> > >          creating the same diff as v3 but in two separate patches.
> > > v3:
> > >          Update test data
> > > v2:
> > >          Mention the different specification versions used by our code
> > >          https://lore.kernel.org/qemu-devel/20260326144835.67911-1-heinrich.schuchardt@canonical.com/
> > > v1:
> > >          https://lore.kernel.org/qemu-devel/20260326121947.51200-1-heinrich.schuchardt@canonical.com/T/#u
> > > ---
> > >   hw/acpi/aml-build.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > index 9b3cdd3781..990abc64cd 100644
> > > --- a/hw/acpi/aml-build.c
> > > +++ b/hw/acpi/aml-build.c
> > > @@ -2144,10 +2144,10 @@ 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);
> > > +    /* Language */
> > > +    build_append_int_noprefix(table_data, f->language, 1);
> > 
> > Won't this break tests? Don't you need to disable the test first (then
> > re-enable it and fix it in patch 3)
> 
> Hello Alistair,
> 
> It would be helpful if /tests/qtest/bios-tables-test.c would clearly point
> out if changes to tests/qtest/bios-tables-test-allowed-diff.h and their
> reversion should be in the submitted patch series or not and what the
> acceptance criteria for the patch series are.
> 

1. add to allowed
2. code change
3. update expected
4. remove from allowed

1+2 and 3+4 can be combined


> I was not able to identify this from the existing text. So I eliminated
> those redundant changes.
> 
> Best regards
> 
> Heinrich

they are not redundant - they are to keep bisect working.
i don't know how to make it clearer.

> > 
> > Alistair
> > 
> > >       /* PCI Device ID  */
> > >       build_append_int_noprefix(table_data, f->pci_device_id, 2);
> > >       /* PCI Vendor ID */
> > > --
> > > 2.53.0
> > > 
> 
> 
>