On Tue, May 16, 2023 at 11:13:17AM +0200, Jan Beulich wrote:
> On 16.05.2023 10:24, Roger Pau Monné wrote:
> > 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.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')
> >
> > 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.
>
> We could also mandate ACPI version >= 2 for PVH Dom0.
Sorry, mentioned on IRC, the above is not required because the RSDP
provided to dom0 is already crafted by Xen and unconditionally set to
version == 2. There's no need to adjust the RSDP at all.
Thanks, Roger.
