Re: [PATCH 1/2] xen/x86/pvh: use preset XSDT header for XSDT generation

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, May 12, 2023 at 06:17:19PM -0700, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@amd.com>
> 
> Xen always generates a XSDT table even if the firmware provided a RSDT
> table. Instead of copying the XSDT header from the firmware table (that
> might be missing), generate the XSDT header from a preset.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
>  xen/arch/x86/hvm/dom0_build.c | 32 +++++++++-----------------------
>  1 file changed, 9 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index 307edc6a8c..5fde769863 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -963,13 +963,18 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr,
>                                        paddr_t *addr)
>  {
>      struct acpi_table_xsdt *xsdt;
> -    struct acpi_table_header *table;
> -    struct acpi_table_rsdp *rsdp;
>      const struct acpi_table_desc *tables = acpi_gbl_root_table_list.tables;
>      unsigned long size = sizeof(*xsdt);
>      unsigned int i, j, num_tables = 0;
> -    paddr_t xsdt_paddr;
>      int rc;
> +    struct acpi_table_header header = {
> +        .signature    = "XSDT",
> +        .length       = sizeof(struct acpi_table_header),
> +        .revision     = 0x1,
> +        .oem_id       = "Xen",
> +        .oem_table_id = "HVM",

I think this is wrong, as according to the spec the OEM Table ID must
match the OEM Table ID in the FADT.

We likely want to copy the OEM ID and OEM Table ID from the RSDP, and
possibly also the other OEM related fields.

Alternatively we might want to copy and use the RSDT on systems that
lack an XSDT, or even just copy the header from the RSDT into Xen's
crafted XSDT, since the format of the RSDP and the XSDT headers are
exactly the same (the difference is in the size of the description
headers that come after).

> +        .oem_revision = 0,
> +    };

This wants to be initdata static const if we go down this route.

Thanks, Roger.