Re: [edk2-devel] [PATCH 34/35] UefiPayloadPkg/BlSupportPei: fix MMCONFIG assignment from XSDT

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 9/17/19 9:49 PM, Laszlo Ersek wrote:
> (This patch is unrelated to the rest of this series; its purpose is to
> enable building the UefiPayloadPkg DSC files with GCC.)
> 
> When building "UefiPayloadPkg/UefiPayloadPkgIa32.dsc" with GCC48 for the
> DEBUG target, the compiler reports that "Entry32" may be used
> uninitialized in ParseAcpiInfo(), in the XSDT branch.
> 
> Code inspection proves the compiler right. In the XSDT branch, the code
> from the RSDT branch must have been duplicated, and "Entry32" references
> were replaced with "Entry64" -- except where "MmCfgHdr" is assigned.
> 
> Fix this bug by introducing a helper variable called "Signature", so that
> we have to refer to "Entry32" or "Entry64" only once per loop body.
> 
> Cc: Benjamin You <benjamin.you@intel.com>
> Cc: Guo Dong <guo.dong@intel.com>
> Cc: Maurice Ma <maurice.ma@intel.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     build-tested only
> 
>  UefiPayloadPkg/BlSupportPei/BlSupportPei.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiPayloadPkg/BlSupportPei/BlSupportPei.c b/UefiPayloadPkg/BlSupportPei/BlSupportPei.c
> index 90433b609f22..22972453117a 100644
> --- a/UefiPayloadPkg/BlSupportPei/BlSupportPei.c
> +++ b/UefiPayloadPkg/BlSupportPei/BlSupportPei.c
> @@ -164,6 +164,7 @@ ParseAcpiInfo (
>    UINT64                                        *Entry64;
>    UINTN                                         Entry64Num;
>    UINTN                                         Idx;
> +  UINT32                                        *Signature;
>    EFI_ACPI_MEMORY_MAPPED_CONFIGURATION_BASE_ADDRESS_TABLE_HEADER *MmCfgHdr;
>    EFI_ACPI_MEMORY_MAPPED_ENHANCED_CONFIGURATION_SPACE_BASE_ADDRESS_ALLOCATION_STRUCTURE *MmCfgBase;
>  
> @@ -181,13 +182,14 @@ ParseAcpiInfo (
>      Entry32  = (UINT32 *)(Rsdt + 1);
>      Entry32Num = (Rsdt->Length - sizeof(EFI_ACPI_DESCRIPTION_HEADER)) >> 2;
>      for (Idx = 0; Idx < Entry32Num; Idx++) {
> -      if (*(UINT32 *)(UINTN)(Entry32[Idx]) == EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> -        Fadt = (EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *)(UINTN)(Entry32[Idx]);
> +      Signature = (UINT32 *)(UINTN)Entry32[Idx];
> +      if (*Signature == EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> +        Fadt = (EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *)Signature;
>          DEBUG ((DEBUG_INFO, "Found Fadt in Rsdt\n"));
>        }
>  
> -      if (*(UINT32 *)(UINTN)(Entry32[Idx]) == EFI_ACPI_5_0_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE) {
> -        MmCfgHdr = (EFI_ACPI_MEMORY_MAPPED_CONFIGURATION_BASE_ADDRESS_TABLE_HEADER *)(UINTN)(Entry32[Idx]);
> +      if (*Signature == EFI_ACPI_5_0_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE) {
> +        MmCfgHdr = (EFI_ACPI_MEMORY_MAPPED_CONFIGURATION_BASE_ADDRESS_TABLE_HEADER *)Signature;
>          DEBUG ((DEBUG_INFO, "Found MM config address in Rsdt\n"));
>        }
>  
> @@ -205,13 +207,14 @@ ParseAcpiInfo (
>      Entry64  = (UINT64 *)(Xsdt + 1);
>      Entry64Num = (Xsdt->Length - sizeof(EFI_ACPI_DESCRIPTION_HEADER)) >> 3;
>      for (Idx = 0; Idx < Entry64Num; Idx++) {
> -      if (*(UINT32 *)(UINTN)(Entry64[Idx]) == EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> -        Fadt = (EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *)(UINTN)(Entry64[Idx]);
> +      Signature = (UINT32 *)(UINTN)Entry64[Idx];
> +      if (*Signature == EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> +        Fadt = (EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *)Signature;
>          DEBUG ((DEBUG_INFO, "Found Fadt in Xsdt\n"));
>        }
>  
> -      if (*(UINT32 *)(UINTN)(Entry64[Idx]) == EFI_ACPI_5_0_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE) {
> -        MmCfgHdr = (EFI_ACPI_MEMORY_MAPPED_CONFIGURATION_BASE_ADDRESS_TABLE_HEADER *)(UINTN)(Entry32[Idx]);

Ah! Thanks GCC.

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> +      if (*Signature == EFI_ACPI_5_0_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE) {
> +        MmCfgHdr = (EFI_ACPI_MEMORY_MAPPED_CONFIGURATION_BASE_ADDRESS_TABLE_HEADER *)Signature;
>          DEBUG ((DEBUG_INFO, "Found MM config address in Xsdt\n"));
>        }
>  
>