The UART clock frequency field of the SPCR table was added in revision 3.
Currently, build_spcr() treats revision 3 tables the same as revision 2 and
only includes this field in revision 4 tables.

Fix build_spcr() to include the clock frequency field in revision 3 tables.
Per the specification, this is the only change between revisions 2 and 3.

Signed-off-by: Vadim Chichikalyuk <chichikalyuk@gmail.com>
---
 hw/acpi/aml-build.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 1e685f982f..9855d5f053 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2123,20 +2123,22 @@ void build_spcr(GArray *table_data, BIOSLinker *linker,
     build_append_int_noprefix(table_data, f->pci_flags, 4);
     /* PCI Segment */
     build_append_int_noprefix(table_data, f->pci_segment, 1);
-    if (rev < 4) {
+    if (rev < 3) {
         /* Reserved */
         build_append_int_noprefix(table_data, 0, 4);
     } else {
         /* UartClkFreq */
         build_append_int_noprefix(table_data, f->uart_clk_freq, 4);
-        /* PreciseBaudrate */
-        build_append_int_noprefix(table_data, f->precise_baudrate, 4);
-        /* NameSpaceStringLength */
-        build_append_int_noprefix(table_data, f->namespace_string_length, 2);
-        /* NameSpaceStringOffset */
-        build_append_int_noprefix(table_data, f->namespace_string_offset, 2);
-        /* NamespaceString[] */
-        g_array_append_vals(table_data, name, f->namespace_string_length);
+        if (rev >= 4) {
+            /* PreciseBaudrate */
+            build_append_int_noprefix(table_data, f->precise_baudrate, 4);
+            /* NameSpaceStringLength */
+            build_append_int_noprefix(table_data, f->namespace_string_length, 2);
+            /* NameSpaceStringOffset */
+            build_append_int_noprefix(table_data, f->namespace_string_offset, 2);
+            /* NamespaceString[] */
+            g_array_append_vals(table_data, name, f->namespace_string_length);
+        }
     }
     acpi_table_end(linker, &table);
 }
-- 
2.39.5 (Apple Git-154)

On Fri, 18 Jul 2025 19:20:42 +0300
Vadim Chichikalyuk <chichikalyuk@gmail.com> wrote:

> The UART clock frequency field of the SPCR table was added in revision 3.
> Currently, build_spcr() treats revision 3 tables the same as revision 2 and
> only includes this field in revision 4 tables.

Given this isn't in the ACPI spec, I'd make sure you have a reference to the MS
documentation for this. I think it is this one:
https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/serial-port-console-redirection-table

> 
> Fix build_spcr() to include the clock frequency field in revision 3 tables.
> Per the specification, this is the only change between revisions 2 and 3.

Maybe say why this has never mattered - I think because no code actually uses
revision 3.  Prior to this series, arm-virt and loongarch were 2 and
riscv-virt was 4.

> 
> Signed-off-by: Vadim Chichikalyuk <chichikalyuk@gmail.com>
Code looks fine so with those additions to the description

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
>  hw/acpi/aml-build.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 1e685f982f..9855d5f053 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2123,20 +2123,22 @@ void build_spcr(GArray *table_data, BIOSLinker *linker,
>      build_append_int_noprefix(table_data, f->pci_flags, 4);
>      /* PCI Segment */
>      build_append_int_noprefix(table_data, f->pci_segment, 1);
> -    if (rev < 4) {
> +    if (rev < 3) {
>          /* Reserved */
>          build_append_int_noprefix(table_data, 0, 4);
>      } else {
>          /* UartClkFreq */
>          build_append_int_noprefix(table_data, f->uart_clk_freq, 4);
> -        /* PreciseBaudrate */
> -        build_append_int_noprefix(table_data, f->precise_baudrate, 4);
> -        /* NameSpaceStringLength */
> -        build_append_int_noprefix(table_data, f->namespace_string_length, 2);
> -        /* NameSpaceStringOffset */
> -        build_append_int_noprefix(table_data, f->namespace_string_offset, 2);
> -        /* NamespaceString[] */
> -        g_array_append_vals(table_data, name, f->namespace_string_length);
> +        if (rev >= 4) {
> +            /* PreciseBaudrate */

Obviously historical, but this does seem like a lot of unnecessary comments
given the clear naming of the input parameters!

> +            build_append_int_noprefix(table_data, f->precise_baudrate, 4);
> +            /* NameSpaceStringLength */
> +            build_append_int_noprefix(table_data, f->namespace_string_length, 2);
> +            /* NameSpaceStringOffset */
> +            build_append_int_noprefix(table_data, f->namespace_string_offset, 2);
> +            /* NamespaceString[] */
> +            g_array_append_vals(table_data, name, f->namespace_string_length);
> +        }
>      }
>      acpi_table_end(linker, &table);
>  }

> On 21 Jul 2025, at 12:39, Jonathan Cameron via <qemu-devel@nongnu.org> wrote:
> 
> On Fri, 18 Jul 2025 19:20:42 +0300
> Vadim Chichikalyuk <chichikalyuk@gmail.com> wrote:
> 
>> The UART clock frequency field of the SPCR table was added in revision 3.
>> Currently, build_spcr() treats revision 3 tables the same as revision 2 and
>> only includes this field in revision 4 tables.
> 
> Given this isn't in the ACPI spec, I'd make sure you have a reference to the MS
> documentation for this. I think it is this one:
> https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/serial-port-console-redirection-table

I’m going off of the following line in the revision history table on that MS page, 
suggesting there was a revision 3 with just the clock frequency added:
"Changed Table Revision to 3 and created field for UART Clock Frequency. Edited formatting."

> 
>> ---
>> hw/acpi/aml-build.c | 20 +++++++++++---------
>> 1 file changed, 11 insertions(+), 9 deletions(-)
>> 
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 1e685f982f..9855d5f053 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -2123,20 +2123,22 @@ void build_spcr(GArray *table_data, BIOSLinker *linker,
>>     build_append_int_noprefix(table_data, f->pci_flags, 4);
>>     /* PCI Segment */
>>     build_append_int_noprefix(table_data, f->pci_segment, 1);
>> -    if (rev < 4) {
>> +    if (rev < 3) {
>>         /* Reserved */
>>         build_append_int_noprefix(table_data, 0, 4);
>>     } else {
>>         /* UartClkFreq */
>>         build_append_int_noprefix(table_data, f->uart_clk_freq, 4);
>> -        /* PreciseBaudrate */
>> -        build_append_int_noprefix(table_data, f->precise_baudrate, 4);
>> -        /* NameSpaceStringLength */
>> -        build_append_int_noprefix(table_data, f->namespace_string_length, 2);
>> -        /* NameSpaceStringOffset */
>> -        build_append_int_noprefix(table_data, f->namespace_string_offset, 2);
>> -        /* NamespaceString[] */
>> -        g_array_append_vals(table_data, name, f->namespace_string_length);
>> +        if (rev >= 4) {
>> +            /* PreciseBaudrate */
> 
> Obviously historical, but this does seem like a lot of unnecessary comments
> given the clear naming of the input parameters!
> 

Absolutely, I’ll remove those that don’t add any useful information in v2.