[v1] hw/acpi: correct field sequence in SPCR table

[PATCH] hw/acpi: correct field sequence in SPCR table

Posted by Heinrich Schuchardt 1 week ago

On LoongArch and RISC-V invalid SPCR tables are created:

Terrminal Type : 00
Language : 03

The correct values are:

Terrminal 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

Fixes: 7dd0b070fa09 ("hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location")
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 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 4b37405088..a999320e61 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2115,10 +2115,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);
     /* PCI Device ID  */
     build_append_int_noprefix(table_data, f->pci_device_id, 2);
     /* PCI Vendor ID */
-- 
2.53.0

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

Posted by Michael S. Tsirkin 1 week ago

the fix is correct, thanks!
something more to improve:


On Thu, Mar 26, 2026 at 01:19:47PM +0100, Heinrich Schuchardt wrote:
> On LoongArch and RISC-V invalid SPCR tables are created:
> 
> Terrminal Type : 00
> Language : 03
> 
> The correct values are:
> 
> Terrminal 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

should mention revision you are consulting.

> 
> Fixes: 7dd0b070fa09 ("hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>


BTW I think the link to the spec should move to build_spcr.

And also:
/*      
 * Serial Port Console Redirection Table (SPCR)
 * Rev: 1.07
 */     

is probably wrong:
1.07	Fixed incorrect description in Stop Bits field. Undo accidental removal of Flow Control field. Edited formatting.

it's likely not the earliest version we adhere to.


> ---
>  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 4b37405088..a999320e61 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2115,10 +2115,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);
>      /* PCI Device ID  */
>      build_append_int_noprefix(table_data, f->pci_device_id, 2);
>      /* PCI Vendor ID */
> -- 
> 2.53.0

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

Posted by Heinrich Schuchardt 1 week ago

On 3/26/26 14:41, Michael S. Tsirkin wrote:
> the fix is correct, thanks!
> something more to improve:
> 
> 
> On Thu, Mar 26, 2026 at 01:19:47PM +0100, Heinrich Schuchardt wrote:
>> On LoongArch and RISC-V invalid SPCR tables are created:
>>
>> Terrminal Type : 00
>> Language : 03
>>
>> The correct values are:
>>
>> Terrminal 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
> 
> should mention revision you are consulting.

Revision 1.10 is the only version that is published at this site. But we 
do not adhere to one specific revision. See below.

The bug exists for any revision from 1.00 to today's 1.10.

According to the revision history version 0.95 removed language codes.
Version 1.0 
(https://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/SPCR_Tbl-v100.doc) 
called the language field "Reserved, must be 0".
The sequence of the two fields considered in this patch did not change 
since then.

> 
>>
>> Fixes: 7dd0b070fa09 ("hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location")
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> 
> 
> BTW I think the link to the spec should move to build_spcr.
> 
> And also:
> /*
>   * Serial Port Console Redirection Table (SPCR)
>   * Rev: 1.07
>   */
> 
> is probably wrong:
> 1.07	Fixed incorrect description in Stop Bits field. Undo accidental removal of Flow Control field. Edited formatting.
> 
> it's likely not the earliest version we adhere to.

The revision level differs between LoongArch, ARM, and RISC-V in our 
code for no obvious reason.

The LoongArch and the ARM code call build_spcr() with rev = 2 which 
implies specification revision <= 1.07. This matches the code comment.

The RISC-V code calls build_spcr() with rev = 4 which implies 
specification revision >= 1.09. We mention 1.10 in a comment.

Best regards

Heinrich

> 
> 
>> ---
>>   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 4b37405088..a999320e61 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -2115,10 +2115,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);
>>       /* PCI Device ID  */
>>       build_append_int_noprefix(table_data, f->pci_device_id, 2);
>>       /* PCI Vendor ID */
>> -- 
>> 2.53.0
>