[PATCH 0/4] hw: acpi: support SPCR rev. 3 & UART clock freq in ARM SPCR

[PATCH 0/4] hw: acpi: support SPCR rev. 3 & UART clock freq in ARM SPCR

[Vadim Chichikalyuk]

This series fixes erroneous building of ACPI SPCR tables in hw/acpi/aml-build.c, 
where the UART clock frequency field is omitted in revision 3 tables despite
being present since revision 3 of the specification.

The last three patches update the SPCR table for the AArch64 virt machine to
revision 3, exposing the UART clock frequency, which was not previously available
via ACPI, to the guest.

Thanks,
Vadim

Signed-off-by: Vadim Chichikalyuk <chichikalyuk@gmail.com>

Vadim Chichikalyuk (4):
  hw: acpi: add support for SPCR revision 3
  tests: acpi: whitelist expected blobs
  hw: arm: acpi: add UART clock frequency to SPCR table
  tests: acpi: update expected blobs

 hw/acpi/aml-build.c               |  20 +++++++++++---------
 hw/arm/virt-acpi-build.c          |   5 +++--
 tests/data/acpi/aarch64/virt/SPCR | Bin 80 -> 80 bytes
 3 files changed, 14 insertions(+), 11 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH 1/4] hw: acpi: add support for SPCR revision 3

[Vadim Chichikalyuk]

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)

[PATCH 2/4] tests: acpi: whitelist expected blobs

[Vadim Chichikalyuk]

Signed-off-by: Vadim Chichikalyuk <chichikalyuk@gmail.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..2a30472d57 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,2 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/aarch64/virt/SPCR",
\ No newline at end of file
-- 
2.39.5 (Apple Git-154)

[PATCH 3/4] hw: arm: acpi: add UART clock frequency to SPCR table

[Vadim Chichikalyuk]

On the ARM virt machine, there is currently no way to programmatically
discover the frequency of the UART reference clock solely through the use of
UEFI/ACPI (without the DTB). The SPCR table can include this information
as of revision 3.

Bump the revision to 3 and add the clock frequency of 24 MHz to the table.

Signed-off-by: Vadim Chichikalyuk <chichikalyuk@gmail.com>
---
 hw/arm/virt-acpi-build.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index b01fc4f8ef..029cbb37f7 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -559,12 +559,13 @@ spcr_setup(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         .pci_function = 0,
         .pci_flags = 0,
         .pci_segment = 0,
+        .uart_clk_freq = 24000000, /* 24MHz */
     };
     /*
-     * Passing NULL as the SPCR Table for Revision 2 doesn't support
+     * Passing NULL as the SPCR Table for Revision 3 doesn't support
      * NameSpaceString.
      */
-    build_spcr(table_data, linker, &serial, 2, vms->oem_id, vms->oem_table_id,
+    build_spcr(table_data, linker, &serial, 3, vms->oem_id, vms->oem_table_id,
                NULL);
 }
 
-- 
2.39.5 (Apple Git-154)

[PATCH 4/4] tests: acpi: update expected blobs

[Vadim Chichikalyuk]

Previous patch changed the SPCR ACPI table for AArch64 virt:
@@ -15,2 +15,2 @@
-[008h 0008 001h]                    Revision : 02
-[009h 0009 001h]                    Checksum : B1
+[008h 0008 001h]                    Revision : 03
+[009h 0009 001h]                    Checksum : 0B
@@ -49 +49 @@
-[04Ch 0076 004h]                    Reserved : 00000000
+[04Ch 0076 004h]                    Reserved : 016E3600
@@ -53 +53 @@
-0000: 53 50 43 52 50 00 00 00 02 B1 42 4F 43 48 53 20  // SPCRP.....BOCHS
+0000: 53 50 43 52 50 00 00 00 03 0B 42 4F 43 48 53 20  // SPCRP.....BOCHS
@@ -57 +57 @@
-0040: FF FF FF FF 00 00 00 00 00 00 00 00 00 00 00 00  // ................
+0040: FF FF FF FF 00 00 00 00 00 00 00 00 00 36 6E 01  // .............6n.

In a revision 3 SPCR table, the "Reserved" field is the UART clock frequency
field.

Signed-off-by: Vadim Chichikalyuk <chichikalyuk@gmail.com>
---
 tests/data/acpi/aarch64/virt/SPCR           | Bin 80 -> 80 bytes
 tests/qtest/bios-tables-test-allowed-diff.h |   1 -
 2 files changed, 1 deletion(-)

diff --git a/tests/data/acpi/aarch64/virt/SPCR b/tests/data/acpi/aarch64/virt/SPCR
index cf0f2b75226515097c08d2e2016a83a4f08812ba..76ac417fbdc4dc6a473c51b82164f40bc5320c58 100644
GIT binary patch
delta 20
bcmWFt;0g|K4hmpkU|{B+$mPszmd6MHD|7?L

delta 20
acmWFt;0g|K4hmpkU|`xfk;|DG$N&H@90SJy

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 2a30472d57..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,2 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/aarch64/virt/SPCR",
\ No newline at end of file
-- 
2.39.5 (Apple Git-154)

Re: [PATCH 1/4] hw: acpi: add support for SPCR revision 3

[Jonathan Cameron via]

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);
>  }

Re: [PATCH 2/4] tests: acpi: whitelist expected blobs

[Jonathan Cameron via]

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

> Signed-off-by: Vadim Chichikalyuk <chichikalyuk@gmail.com>
> ---
>  tests/qtest/bios-tables-test-allowed-diff.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index dfb8523c8b..2a30472d57 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1 +1,2 @@
>  /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/aarch64/virt/SPCR",
> \ No newline at end of file
Fix that.

Re: [PATCH 3/4] hw: arm: acpi: add UART clock frequency to SPCR table

[Jonathan Cameron via]

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

> On the ARM virt machine, there is currently no way to programmatically
> discover the frequency of the UART reference clock solely through the use of
> UEFI/ACPI (without the DTB). The SPCR table can include this information
> as of revision 3.
> 
> Bump the revision to 3 and add the clock frequency of 24 MHz to the table.

Maybe add something on why you aren't just skipping forwards to 4 and filling
in the rest of the stuff?

> 
> Signed-off-by: Vadim Chichikalyuk <chichikalyuk@gmail.com>

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

> ---
>  hw/arm/virt-acpi-build.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index b01fc4f8ef..029cbb37f7 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -559,12 +559,13 @@ spcr_setup(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          .pci_function = 0,
>          .pci_flags = 0,
>          .pci_segment = 0,
> +        .uart_clk_freq = 24000000, /* 24MHz */
>      };
>      /*
> -     * Passing NULL as the SPCR Table for Revision 2 doesn't support
> +     * Passing NULL as the SPCR Table for Revision 3 doesn't support
>       * NameSpaceString.
>       */
> -    build_spcr(table_data, linker, &serial, 2, vms->oem_id, vms->oem_table_id,
> +    build_spcr(table_data, linker, &serial, 3, vms->oem_id, vms->oem_table_id,
>                 NULL);
>  }
>  

Re: [PATCH 4/4] tests: acpi: update expected blobs

[Jonathan Cameron via]

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

> Previous patch changed the SPCR ACPI table for AArch64 virt:
> @@ -15,2 +15,2 @@
> -[008h 0008 001h]                    Revision : 02
> -[009h 0009 001h]                    Checksum : B1
> +[008h 0008 001h]                    Revision : 03
> +[009h 0009 001h]                    Checksum : 0B
> @@ -49 +49 @@
> -[04Ch 0076 004h]                    Reserved : 00000000
> +[04Ch 0076 004h]                    Reserved : 016E3600

Hmm. No support in acpi-tools?  Old version maybe?

Looks like v4 support was added in this pull request last year
https://github.com/acpica/acpica/pull/931

Jonathan


> @@ -53 +53 @@
> -0000: 53 50 43 52 50 00 00 00 02 B1 42 4F 43 48 53 20  // SPCRP.....BOCHS
> +0000: 53 50 43 52 50 00 00 00 03 0B 42 4F 43 48 53 20  // SPCRP.....BOCHS
> @@ -57 +57 @@
> -0040: FF FF FF FF 00 00 00 00 00 00 00 00 00 00 00 00  // ................
> +0040: FF FF FF FF 00 00 00 00 00 00 00 00 00 36 6E 01  // .............6n.
> 
> In a revision 3 SPCR table, the "Reserved" field is the UART clock frequency
> field.
> 
> Signed-off-by: Vadim Chichikalyuk <chichikalyuk@gmail.com>
> ---
>  tests/data/acpi/aarch64/virt/SPCR           | Bin 80 -> 80 bytes
>  tests/qtest/bios-tables-test-allowed-diff.h |   1 -
>  2 files changed, 1 deletion(-)
> 
> diff --git a/tests/data/acpi/aarch64/virt/SPCR b/tests/data/acpi/aarch64/virt/SPCR
> index cf0f2b75226515097c08d2e2016a83a4f08812ba..76ac417fbdc4dc6a473c51b82164f40bc5320c58 100644
> GIT binary patch
> delta 20
> bcmWFt;0g|K4hmpkU|{B+$mPszmd6MHD|7?L
> 
> delta 20
> acmWFt;0g|K4hmpkU|`xfk;|DG$N&H@90SJy
> 
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index 2a30472d57..dfb8523c8b 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1,2 +1 @@
>  /* List of comma-separated changed AML files to ignore */
> -"tests/data/acpi/aarch64/virt/SPCR",
> \ No newline at end of file

Re: [PATCH 1/4] hw: acpi: add support for SPCR revision 3

[Vadim Chichikalyuk]

> 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.

Re: [PATCH 3/4] hw: arm: acpi: add UART clock frequency to SPCR table

[Vadim Chichikalyuk]

[-- Attachment #1: Type: text/plain, Size: 1430 bytes --]


> On 21 Jul 2025, at 12:41, Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> On Fri, 18 Jul 2025 19:20:44 +0300
> Vadim Chichikalyuk <chichikalyuk@gmail.com> wrote:
> 
>> On the ARM virt machine, there is currently no way to programmatically
>> discover the frequency of the UART reference clock solely through the use of
>> UEFI/ACPI (without the DTB). The SPCR table can include this information
>> as of revision 3.
>> 
>> Bump the revision to 3 and add the clock frequency of 24 MHz to the table.
> 
> Maybe add something on why you aren't just skipping forwards to 4 and filling
> in the rest of the stuff?

Haven’t really considered that – you’re right, might as well upgrade it to revision 4.

Based on build_dsdt() and acpi_dsdt_add_uart(), the NamespaceString would be 
“\_SB.COM0”, right? Although, for some reason, it’s just “.” for RISC-V virt (indicating
that there isn’t a corresponding device in the ACPI namespace), despite there being 
entries for the UART in the DSDT, just as for ARM?

The relevant lines, for reference (hw/arm/virt-acpi-build.c):

scope = aml_scope("\\_SB");
acpi_dsdt_add_uart(scope, &memmap[VIRT_UART0], (irqmap[VIRT_UART0] + ARM_SPI_BASE), 0);

static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap, uint32_t uart_irq) {
    Aml *dev = aml_device("COM%d", uartidx);
    ...
}


Thanks for the review,
Vadim

[-- Attachment #2: Type: text/html, Size: 2538 bytes --]