On Mon, May 15, 2023 at 05:16:27PM -0700, Stefano Stabellini wrote:
> On Mon, 15 May 2023, Jan Beulich wrote:
> > On 15.05.2023 10:48, Roger Pau Monné wrote:
> > > On Fri, May 12, 2023 at 06:17:19PM -0700, Stefano Stabellini wrote:
> > >> From: Stefano Stabellini <stefano.stabell...@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.stabell...@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).
> >
> > I guess I'd prefer that last variant.
>
> I tried this approach (together with the second patch for necessity) and
> it worked.
>
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index fd2cbf68bc..11d6d1bc23 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -967,6 +967,10 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d,
> paddr_t madt_addr,
> goto out;
> }
> xsdt_paddr = rsdp->xsdt_physical_address;
> + if ( !xsdt_paddr )
> + {
> + xsdt_paddr = rsdp->rsdt_physical_address;
> + }
> acpi_os_unmap_memory(rsdp, sizeof(*rsdp));
> table = acpi_os_map_memory(xsdt_paddr, sizeof(*table));
> if ( !table )
To be slightly more consistent, could you use:
/*
* 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)
sdt_paddr = rsdp->xsdt_physical_address;
else
sdt_paddr = rsdp->rsdt_physical_address;
It was an oversight of mine to not check for the RSDP revision, as
RSDP < 2 will never have an XSDT. Also add:
Fixes: 1d74282c455f ('x86: setup PVHv2 Dom0 ACPI tables')
Thanks.
