[PATCH v2 0/3] hw: acpi: add UART clock freq to AArch64 SPCR

[PATCH v2 0/3] hw: acpi: add UART clock freq to AArch64 SPCR

[Vadim Chichikalyuk]

This series upgrades the ACPI SPCR table for AArch64 virt to revision 4,
adding the UART clock frequency, which was not previously available via ACPI.

Regarding the NamespaceString: following the example of the SPCR table
for RISC-V, I've set it to "." (indicating that there is no device
corresponding to the UART in the ACPI namespace). However, looking
at build_dsdt(), the UART is actually added to the DSDT, as
"\_SB.COM0" (same as RISC-V). Should the NamespaceString be that instead?

Thanks,
Vadim

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

Vadim Chichikalyuk (3):
  tests: acpi: whitelist expected blobs
  hw: arm: acpi: upgrade SPCR table to revision 4
  tests: acpi: update expected blobs

 hw/arm/virt-acpi-build.c          |  14 ++++++++------
 tests/data/acpi/aarch64/virt/SPCR | Bin 80 -> 90 bytes
 2 files changed, 8 insertions(+), 6 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH v2 1/3] 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..a60794a3f6 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",
-- 
2.39.5 (Apple Git-154)

[PATCH v2 2/3] hw: arm: acpi: upgrade SPCR table to revision 4

[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 4, but is currently at revision 2.

Bump the revision to 4, add the clock frequency of 24 MHz and
complete the other new fields.

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

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index b01fc4f8ef..f685668c5e 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -536,6 +536,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 static void
 spcr_setup(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {
+    const char name[] = ".";
     AcpiSpcrData serial = {
         .interface_type = 3,       /* ARM PL011 UART */
         .base_addr.id = AML_AS_SYSTEM_MEMORY,
@@ -559,13 +560,14 @@ spcr_setup(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         .pci_function = 0,
         .pci_flags = 0,
         .pci_segment = 0,
+        .uart_clk_freq = 24000000,
+        .precise_baudrate = 0,
+        .namespace_string_length = sizeof(name),
+        .namespace_string_offset = 88,
     };
-    /*
-     * Passing NULL as the SPCR Table for Revision 2 doesn't support
-     * NameSpaceString.
-     */
-    build_spcr(table_data, linker, &serial, 2, vms->oem_id, vms->oem_table_id,
-               NULL);
+
+    build_spcr(table_data, linker, &serial, 4, vms->oem_id, vms->oem_table_id,
+               name);
 }
 
 /*
-- 
2.39.5 (Apple Git-154)

[PATCH v2 3/3] tests: acpi: update expected blobs

[Vadim Chichikalyuk]

Previous patch changed the SPCR ACPI table for AArch64 virt.
Hexdump diff:
@@ -1 +1 @@
-00000000  53 50 43 52 50 00 00 00  02 b1 42 4f 43 48 53 20
+00000000  53 50 43 52 5a 00 00 00  04 78 42 4f 43 48 53 20
@@ -5,2 +5,3 @@
-00000040  ff ff ff ff 00 00 00 00  00 00 00 00 00 00 00 00
-00000050
+00000040  ff ff ff ff 00 00 00 00  00 00 00 00 00 36 6e 01
+00000050  00 00 00 00 02 00 58 00  2e 00
+0000005a

Signed-off-by: Vadim Chichikalyuk <chichikalyuk@gmail.com>
---
 tests/data/acpi/aarch64/virt/SPCR           | Bin 80 -> 90 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..6df47f5c95dfbad2a0b060151c0733bdbc9f7392 100644
GIT binary patch
delta 30
kcmWHD;tCFM4vJ!6U|^}3$mPsymd6NWGcYkkFz7J=09{iA+W-In

delta 20
acmazF;0g|K4hmpkU|`xfk;|DG$N&H@<O9(F

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index a60794a3f6..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",
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2 0/3] hw: acpi: add UART clock freq to AArch64 SPCR

[Peter Maydell]

On Sat, 9 Aug 2025 at 22:11, Vadim Chichikalyuk <chichikalyuk@gmail.com> wrote:
>
> This series upgrades the ACPI SPCR table for AArch64 virt to revision 4,
> adding the UART clock frequency, which was not previously available via ACPI.

So, what's the rationale here? QEMU's UART doesn't care about
clocks at all. We provide a clock in the DTB because the kernel
refuses to boot if you don't, but if the ACPI spec didn't even
have a way to pass the clock frequency until rev 4 this obviously
isn't a problem on that side.

thanks
-- PMM

Re: [PATCH v2 2/3] hw: arm: acpi: upgrade SPCR table to revision 4

[Peter Maydell]

On Sat, 9 Aug 2025 at 22:11, 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 4, but is currently at revision 2.
>
> Bump the revision to 4, add the clock frequency of 24 MHz and
> complete the other new fields.

This commit message should say why we need to do this,
i.e. why we can't continue to do what we've always done
and not tell the guest about a clock frequency which is
entirely imaginary in any case.

> Signed-off-by: Vadim Chichikalyuk <chichikalyuk@gmail.com>
> ---
>  hw/arm/virt-acpi-build.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index b01fc4f8ef..f685668c5e 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -536,6 +536,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  static void
>  spcr_setup(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
> +    const char name[] = ".";
>      AcpiSpcrData serial = {
>          .interface_type = 3,       /* ARM PL011 UART */
>          .base_addr.id = AML_AS_SYSTEM_MEMORY,
> @@ -559,13 +560,14 @@ spcr_setup(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          .pci_function = 0,
>          .pci_flags = 0,
>          .pci_segment = 0,
> +        .uart_clk_freq = 24000000,

This is a number which ought to match the existing one in create_fdt()
in hw/arm/virt.c. If we need it in two different files we should
#define it as a constant in include/hw/arm/virt.h.

> +        .precise_baudrate = 0,
> +        .namespace_string_length = sizeof(name),
> +        .namespace_string_offset = 88,

What is this magic number 88 for? The spec says that it's
the offset from the beginning of the structure to wherever
the namespace string is put, but we don't decide that,
the build_spcr() function does. The build_spcr() function
knows what the offset of the name string is, so it should
write this field; the callers shouldn't have to.

>      };

thanks
-- PMM

Re: [PATCH v2 0/3] hw: acpi: add UART clock freq to AArch64 SPCR

[Vadim Chichikalyuk]

> On 18 Aug 2025, at 18:26, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> So, what's the rationale here? QEMU's UART doesn't care about
> clocks at all. We provide a clock in the DTB because the kernel
> refuses to boot if you don't, but if the ACPI spec didn't even
> have a way to pass the clock frequency until rev 4 this obviously
> isn't a problem on that side.

The rationale is the same – just as the (Linux?) kernel expects 
a clock frequency if a UART is present in the DTB, guests using 
ACPI may expect a valid value for the clock frequency in the
SPCR record when one is present.

Since we already have to have a notional value in the DTB, it
makes sense to expose the same value via ACPI, too, for 
consistency. The RISC-V virt machine, for example, has it both
in its DTB and ACPI DSDT.

I’ll fix the other patches per your comments if you think this
is worth adding.

Thanks,
Vadim

Re: [PATCH v2 0/3] hw: acpi: add UART clock freq to AArch64 SPCR

[Peter Maydell]

On Mon, 25 Aug 2025 at 00:00, Vadim Chichikalyuk <chichikalyuk@gmail.com> wrote:
>
>
> > On 18 Aug 2025, at 18:26, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > So, what's the rationale here? QEMU's UART doesn't care about
> > clocks at all. We provide a clock in the DTB because the kernel
> > refuses to boot if you don't, but if the ACPI spec didn't even
> > have a way to pass the clock frequency until rev 4 this obviously
> > isn't a problem on that side.
>
> The rationale is the same – just as the (Linux?) kernel expects
> a clock frequency if a UART is present in the DTB, guests using
> ACPI may expect a valid value for the clock frequency in the
> SPCR record when one is present.

But the ACPI spec prior to rev 4 doesn't even provide a
way to specify the clock frequency, so why would a guest
expect it, and how could it do so?

thanks
-- PMM