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

["Laszlo Ersek" <lersek@redhat.com>]

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