[edk2-devel] [PATCH] ArmVirtPkg/XenAcpiPlatformDxe: Install FACS table from DT

[edk2-devel] [PATCH] ArmVirtPkg/XenAcpiPlatformDxe: Install FACS table from DT

[David Woodhouse]

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

From: David Woodhouse <dwmw@amazon.co.uk>

The FACS may still exist when the reduced hardware flag is set in FADT;
it is optional. Since it contains the hardware signature field which
indicates that a hibernated system should boot cleanly instead of
attempting to resume, a platform may choose to expose it. Propagate it
correctly.

Also avoid a NULL pointer dereference if the platform doesn't provide
a DSDT.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 .../XenAcpiPlatformDxe/XenAcpiPlatformDxe.c   | 37 +++++++++++++++----
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c b/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
index 32c8b1e94ed2..3e6e5cb367d4 100644
--- a/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
+++ b/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
@@ -128,10 +128,12 @@ InstallXenArmTables (
   EFI_ACPI_DESCRIPTION_HEADER                   *Xsdt;
   EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE     *FadtTable;
   EFI_ACPI_DESCRIPTION_HEADER                   *DsdtTable;
+  EFI_ACPI_3_0_FIRMWARE_ACPI_CONTROL_STRUCTURE  *FacsTable;
 
   XenAcpiRsdpStructurePtr = NULL;
   FadtTable               = NULL;
   DsdtTable               = NULL;
+  FacsTable               = NULL;
   TableHandle             = 0;
   NumberOfTableEntries    = 0;
 
@@ -191,6 +193,8 @@ InstallXenArmTables (
         FadtTable = (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *)
                     (UINTN)CurrentTablePointer;
         DsdtTable = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)FadtTable->Dsdt;
+        FacsTable = (EFI_ACPI_3_0_FIRMWARE_ACPI_CONTROL_STRUCTURE *)
+                    (UINTN) FadtTable->FirmwareCtrl;
       }
     }
   }
@@ -198,14 +202,31 @@ InstallXenArmTables (
   //
   // Install DSDT table.
   //
-  Status = AcpiProtocol->InstallAcpiTable (
-                           AcpiProtocol,
-                           DsdtTable,
-                           DsdtTable->Length,
-                           &TableHandle
-                           );
-  if (EFI_ERROR (Status)) {
-    return Status;
+  if (DsdtTable != NULL) {
+    Status = AcpiProtocol->InstallAcpiTable (
+               AcpiProtocol,
+               DsdtTable,
+               DsdtTable->Length,
+               &TableHandle
+               );
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+  }
+
+  //
+  // Install FACS table.
+  //
+  if (FacsTable != NULL) {
+    Status = AcpiProtocol->InstallAcpiTable (
+               AcpiProtocol,
+               FacsTable,
+               FacsTable->Length,
+               &TableHandle
+               );
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
   }
 
   return EFI_SUCCESS;
-- 
2.34.1




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115381): https://edk2.groups.io/g/devel/message/115381
Mute This Topic: https://groups.io/mt/104329932/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [edk2-devel] [PATCH] ArmVirtPkg/XenAcpiPlatformDxe: Install FACS table from DT

[Laszlo Ersek]

On 2/13/24 11:50, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> The FACS may still exist when the reduced hardware flag is set in FADT;
> it is optional. Since it contains the hardware signature field which
> indicates that a hibernated system should boot cleanly instead of
> attempting to resume, a platform may choose to expose it. Propagate it
> correctly.
> 
> Also avoid a NULL pointer dereference if the platform doesn't provide
> a DSDT.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  .../XenAcpiPlatformDxe/XenAcpiPlatformDxe.c   | 37 +++++++++++++++----
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c b/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
> index 32c8b1e94ed2..3e6e5cb367d4 100644
> --- a/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
> +++ b/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
> @@ -128,10 +128,12 @@ InstallXenArmTables (
>    EFI_ACPI_DESCRIPTION_HEADER                   *Xsdt;
>    EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE     *FadtTable;
>    EFI_ACPI_DESCRIPTION_HEADER                   *DsdtTable;
> +  EFI_ACPI_3_0_FIRMWARE_ACPI_CONTROL_STRUCTURE  *FacsTable;
>  
>    XenAcpiRsdpStructurePtr = NULL;
>    FadtTable               = NULL;
>    DsdtTable               = NULL;
> +  FacsTable               = NULL;
>    TableHandle             = 0;
>    NumberOfTableEntries    = 0;
>  
> @@ -191,6 +193,8 @@ InstallXenArmTables (
>          FadtTable = (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *)
>                      (UINTN)CurrentTablePointer;
>          DsdtTable = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)FadtTable->Dsdt;
> +        FacsTable = (EFI_ACPI_3_0_FIRMWARE_ACPI_CONTROL_STRUCTURE *)
> +                    (UINTN) FadtTable->FirmwareCtrl;
>        }
>      }
>    }
> @@ -198,14 +202,31 @@ InstallXenArmTables (
>    //
>    // Install DSDT table.
>    //
> -  Status = AcpiProtocol->InstallAcpiTable (
> -                           AcpiProtocol,
> -                           DsdtTable,
> -                           DsdtTable->Length,
> -                           &TableHandle
> -                           );
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> +  if (DsdtTable != NULL) {
> +    Status = AcpiProtocol->InstallAcpiTable (
> +               AcpiProtocol,
> +               DsdtTable,
> +               DsdtTable->Length,
> +               &TableHandle
> +               );
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +  }
> +
> +  //
> +  // Install FACS table.
> +  //
> +  if (FacsTable != NULL) {
> +    Status = AcpiProtocol->InstallAcpiTable (
> +               AcpiProtocol,
> +               FacsTable,
> +               FacsTable->Length,
> +               &TableHandle
> +               );
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
>    }
>  
>    return EFI_SUCCESS;

I'm not a fan of InstallXenArmTables() not rolling back tables upon a
failure, but that wart predates this patch.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Can you please ping, after the edk2-stable202402 release, so we merge this?

Or -- given the NULL ptr deref fix in particular -- does this qualify
for merging during the hard feature freeze?

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115436): https://edk2.groups.io/g/devel/message/115436
Mute This Topic: https://groups.io/mt/104329932/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg/XenAcpiPlatformDxe: Install FACS table from DT

[Laszlo Ersek]

On 2/13/24 11:50, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> The FACS may still exist when the reduced hardware flag is set in FADT;
> it is optional. Since it contains the hardware signature field which
> indicates that a hibernated system should boot cleanly instead of
> attempting to resume, a platform may choose to expose it. Propagate it
> correctly.
> 
> Also avoid a NULL pointer dereference if the platform doesn't provide
> a DSDT.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  .../XenAcpiPlatformDxe/XenAcpiPlatformDxe.c   | 37 +++++++++++++++----
>  1 file changed, 29 insertions(+), 8 deletions(-)

Merged as commit ba96acd963e7, via
<https://github.com/tianocore/edk2/pull/5407>. (I needed to "uncrustify"
the code first.)

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115935): https://edk2.groups.io/g/devel/message/115935
Mute This Topic: https://groups.io/mt/104329932/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-