Re: [PATCH v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.04.2024 21:18, Daniel P. Smith wrote:
> From: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> 
> Xen always generates as XSDT table even if the firmware provided an RSDT 
> table.
> Copy the RSDT header from the firmware table, adjusting the signature, for the
> XSDT table when not provided by the firmware.
> 
> Fixes: 1d74282c455f ('x86: setup PVHv2 Dom0 ACPI tables')

Especially with this tag the description really wants to say what the problem
is that is being solved here. XSDT having superseded RSDT for ages, it seems
unlikely that there might be systems around that are new enough to run PVH
Dom0, yet come without an XSDT. I can only guess ...

> Suggested-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> ---
> 
> This patch is used for development and testing of hyperlaunch using the QEMU
> emulator.

... that for whatever reason qemu doesn't surface an XSDT (at which point a
follow-on question would be why this wouldn't want dealing with in qemu
instead).

> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -1077,7 +1077,16 @@ static int __init pvh_setup_acpi_xsdt(struct domain 
> *d, paddr_t madt_addr,
>          rc = -EINVAL;
>          goto out;
>      }
> -    xsdt_paddr = rsdp->xsdt_physical_address;
> +    /*
> +     * Note the header is the same for both RSDT and XSDT, so it's fine to
> +     * copy the native RSDT header to the Xen crafted XSDT if no native
> +     * XSDT is available.
> +     */
> +    if ( rsdp->revision > 1 && rsdp->xsdt_physical_address )
> +        xsdt_paddr = rsdp->xsdt_physical_address;
> +    else
> +        xsdt_paddr = rsdp->rsdt_physical_address;
> +
>      acpi_os_unmap_memory(rsdp, sizeof(*rsdp));
>      table = acpi_os_map_memory(xsdt_paddr, sizeof(*table));
>      if ( !table )
> @@ -1089,6 +1098,9 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, 
> paddr_t madt_addr,
>      xsdt->header = *table;
>      acpi_os_unmap_memory(table, sizeof(*table));
>  
> +    /* In case the header is an RSDT copy, blindly ensure it has an XSDT sig 
> */
> +    xsdt->header.signature[0] = 'X';

This is in no way "blindly". The size of the table elements being different
between RSDT and XSDT makes it mandatory to have the correct signature. Else
the consumer of the struct is going to be misled.

Jan