| Message ID | 20240424191826.23656-1-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build | expand |
On 24.04.2024 21:18, Daniel P. Smith wrote: > From: Stefano Stabellini <stefano.stabellini@amd.com> > > 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@citrix.com> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > --- > > 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
On Wed, Apr 24, 2024 at 03:18:26PM -0400, Daniel P. Smith wrote: > From: Stefano Stabellini <stefano.stabellini@amd.com> > > Xen always generates as XSDT table even if the firmware provided an RSDT table. ^ only As providing an RSDT doesn't exclude from a XSDT also being provided. > Copy the RSDT header from the firmware table, adjusting the signature, for the > XSDT table when not provided by the firmware. Either here, or in the code comment below (or in both places), I would detail that this is required for QEMU, that only exposes RSDT but no XSDT. > Fixes: 1d74282c455f ('x86: setup PVHv2 Dom0 ACPI tables') > Suggested-by: Roger Pau Monné <roger.pau@citrix.com> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On Thu, Apr 25, 2024 at 08:12:09AM +0200, Jan Beulich wrote: > On 24.04.2024 21:18, Daniel P. Smith wrote: > > @@ -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. The "blindly" IMO refers to the fact that the table might already have the right signature, but this is not checked, IOW we could do: if ( xsdt->header.signature[0] == 'R' ) xsdt->header.signature[0] = 'X'; Regards, Roger.
On 25.04.2024 09:10, Roger Pau Monné wrote: > On Thu, Apr 25, 2024 at 08:12:09AM +0200, Jan Beulich wrote: >> On 24.04.2024 21:18, Daniel P. Smith wrote: >>> @@ -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. > > The "blindly" IMO refers to the fact that the table might already have > the right signature, but this is not checked, IOW we could do: > > if ( xsdt->header.signature[0] == 'R' ) > xsdt->header.signature[0] = 'X'; Right, and indeed I was seeing this as a possible further interpretation. Yet given multiple ways of reading this, I'm of the opinion that it wants adjusting. ", ... unconditionally ensure it has ... " may already do. Simply dropping "blindly" would too be okay with me. Jan
On Thu, Apr 25, 2024 at 09:26:59AM +0200, Jan Beulich wrote: > On 25.04.2024 09:10, Roger Pau Monné wrote: > > On Thu, Apr 25, 2024 at 08:12:09AM +0200, Jan Beulich wrote: > >> On 24.04.2024 21:18, Daniel P. Smith wrote: > >>> @@ -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. > > > > The "blindly" IMO refers to the fact that the table might already have > > the right signature, but this is not checked, IOW we could do: > > > > if ( xsdt->header.signature[0] == 'R' ) > > xsdt->header.signature[0] = 'X'; > > Right, and indeed I was seeing this as a possible further interpretation. > Yet given multiple ways of reading this, I'm of the opinion that it wants > adjusting. ", ... unconditionally ensure it has ... " may already do. > Simply dropping "blindly" would too be okay with me. FWIW, I'm fine with any of the proposed adjustments. Thanks, Roger.
Ping? I think this is a useful change, could we please have a new version with the proposed adjustments? Thanks, Roger. On Wed, Apr 24, 2024 at 03:18:26PM -0400, Daniel P. Smith wrote: > From: Stefano Stabellini <stefano.stabellini@amd.com> > > 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') > Suggested-by: Roger Pau Monné <roger.pau@citrix.com> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > --- > > This patch is used for development and testing of hyperlaunch using the QEMU > emulator. After dicussiong with Stefano, he was okay with me addressing Jan's > comment regarding the table signature and reposting for acceptance. > > Changes in v3: > - ensure the constructed XSDT table always has the correct signature > > --- > xen/arch/x86/hvm/dom0_build.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > index ac71d43a6b3f..781aac00fe72 100644 > --- 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'; > + > /* Add the custom MADT. */ > xsdt->table_offset_entry[0] = madt_addr; > > -- > 2.30.2 >
I sent v4 here: https://marc.info/?l=xen-devel&m=172610400409473 On Tue, 10 Sep 2024, Roger Pau Monné wrote: > Ping? > > I think this is a useful change, could we please have a new version > with the proposed adjustments? > > Thanks, Roger. > > On Wed, Apr 24, 2024 at 03:18:26PM -0400, Daniel P. Smith wrote: > > From: Stefano Stabellini <stefano.stabellini@amd.com> > > > > 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') > > Suggested-by: Roger Pau Monné <roger.pau@citrix.com> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > > --- > > > > This patch is used for development and testing of hyperlaunch using the QEMU > > emulator. After dicussiong with Stefano, he was okay with me addressing Jan's > > comment regarding the table signature and reposting for acceptance. > > > > Changes in v3: > > - ensure the constructed XSDT table always has the correct signature > > > > --- > > xen/arch/x86/hvm/dom0_build.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > > index ac71d43a6b3f..781aac00fe72 100644 > > --- 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'; > > + > > /* Add the custom MADT. */ > > xsdt->table_offset_entry[0] = madt_addr; > > > > -- > > 2.30.2 > > >
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c index ac71d43a6b3f..781aac00fe72 100644 --- 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'; + /* Add the custom MADT. */ xsdt->table_offset_entry[0] = madt_addr;