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

[Jonathan Cameron via <qemu-devel@nongnu.org>]

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