[PATCH v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build

[PATCH v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build

[Daniel P. Smith]

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

Re: [PATCH v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build

[Jan Beulich]

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

Re: [PATCH v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build

[Roger Pau Monné]

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.

Re: [PATCH v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build

[Jan Beulich]

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

Re: [PATCH v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build

[Roger Pau Monné]

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.

Re: [PATCH v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build

[Roger Pau Monné]

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.

Re: [PATCH v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build

[Roger Pau Monné]

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
> 

Re: [PATCH v3] xen/x86/pvh: handle ACPI RSDT table in PVH Dom0 build

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2736 bytes --]

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
> > 
>