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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, May 16, 2023 at 10:10:07AM +0200, Roger Pau Monné wrote:
> 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.stabellini@xxxxxxx>
> > > >>
> > > >> 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@xxxxxxx>
> > > >> ---
> > > >>  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')

Just realized this will require some more work so that the guest
(dom0) provided RSDP is at least revision 2.  You will need to adjust
the field and recalculate the checksum if needed.

Thanks, Roger.