Re: [PATCH v2 1/3] IntelFrameworkPkg/LegacyBios.h: Add a macro to guarantee page 0 access

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 12/7/2017 1:40 PM, Jian J Wang wrote:
>> v2:
>> a. Fix a typo in expression in the macro ACCESS_PAGE0_CODE
>> b. Fix GCC49 build error
> 
> ue to the introduction of NULL pointer detection feature, page 0 will be
> disabled if the feature is enabled, which will cause legacy code failed to
> update legacy data in page 0. This macro is introduced to make sure the
> page 0 is enabled before those code and restore the original status of it
> afterwards.
> 
> Another reason to introduce this macro is to eliminate the dependency on
> the PcdNullPointerDetectionPropertyMask. Because this is a new PCD, it
> could cause some backward compatibility issue for some old packages.
> 
> This macro will simply check if the page 0 is disabled or not. If it's
> disabled, it will enable it before code updating page 0 and disable it
> afterwards. Otherwise, this macro will do nothing to page 0.
> 
> The usage of the macro will be look like (similar to DEBUG_CODE macro):
> 
>      ACCESS_PAGE0_CODE(
>        <code accessing page 0>
>      );
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>   IntelFrameworkPkg/Include/Protocol/LegacyBios.h | 34 +++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)
> 
> diff --git a/IntelFrameworkPkg/Include/Protocol/LegacyBios.h b/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
> index 641f101bce..6a5f5464e7 100644
> --- a/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
> +++ b/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
> @@ -1518,6 +1518,40 @@ struct _EFI_LEGACY_BIOS_PROTOCOL {
>     EFI_LEGACY_BIOS_BOOT_UNCONVENTIONAL_DEVICE  BootUnconventionalDevice;
>   };
>   
> +//
> +// Legacy BIOS needs to access memory in page 0 (0-4095), which is disabled if
> +// NULL pointer detection feature is enabled. Following macro can be used to
> +// enable/disable page 0 before/after accessing it.
> +//
> +#define ACCESS_PAGE0_CODE(statements)                           \
> +  do {                                                          \
> +    EFI_STATUS                            Status_;              \
> +    EFI_GCD_MEMORY_SPACE_DESCRIPTOR       Desc_;                \
> +                                                                \
> +    Desc_.Attributes = 0;                                       \
> +    Status_ = gDS->GetMemorySpaceDescriptor (0, &Desc_);        \
> +    ASSERT_EFI_ERROR (Status_);                                 \
> +    if ((Desc_.Attributes & EFI_MEMORY_RP) != 0) {              \
> +      Status_ = gDS->SetMemorySpaceAttributes (                 \
> +                      0,                                        \
> +                      EFI_PAGES_TO_SIZE(1),                     \
> +                      Desc_.Attributes & ~(UINT64)EFI_MEMORY_RP \
> +                      );                                        \
> +      ASSERT_EFI_ERROR (Status_);                               \
> +    }                                                           \
> +                                                                \
> +    statements;                                                 \

It's better to surrounded statements with {}.
So that when statements contains variable declaration, C compiler
doesn't complain.

> +                                                                \
> +    if ((Desc_.Attributes & EFI_MEMORY_RP) != 0) {              \
> +      Status_ = gDS->SetMemorySpaceAttributes (                 \
> +                      0,                                        \
> +                      EFI_PAGES_TO_SIZE(1),                     \
> +                      Desc_.Attributes                          \
> +                      );                                        \
> +      ASSERT_EFI_ERROR (Status_);                               \
> +    }                                                           \
> +  } while (FALSE)
> +
>   extern EFI_GUID gEfiLegacyBiosProtocolGuid;
>   
>   #endif
> 

With the above suggested changes,
   Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

-- 
Thanks,
Ray