Re: [PATCH V7 25/37] MdeModulePkg: EFER should not be changed in TDX

["Wang, Jian J" <jian.j.wang@intel.com>]

Hi Min,

I think the PCD should not be dynamic. Dynamic PCD is used for those features
which can be changed at boot time. But, for Intel processor, it should always
stay as FALSE. So there's no need to make it dynamic. FixedAtBuild should be fine.

Regards,
Jian

> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Monday, February 28, 2022 3:21 PM
> To: devel@edk2.groups.io
> Cc: Xu, Min M <min.m.xu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Wu, Hao A <hao.a.wu@intel.com>; Brijesh Singh <brijesh.singh@amd.com>;
> Aktas, Erdem <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH V7 25/37] MdeModulePkg: EFER should not be changed in TDX
> 
> RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429
> 
> In TDX IA32_ERER is RO to host VMM. It could not be changed.
> PcdIa32EferChangeAllowed is added in MdeModulePkg.dec and it is
> to be set to FALSE in Tdx guest.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          | 1 +
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 7 +++++++
>  MdeModulePkg/MdeModulePkg.dec                    | 5 +++++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index 19b8a4c8aefa..106b679b6bd0 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -117,6 +117,7 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable                  ##
> SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                            ##
> CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize                            ##
> CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdIa32EferChangeAllowed               ##
> CONSUMES
> 
>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ##
> SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index 0700f310b203..5c647c74e773 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -159,6 +159,13 @@ IsEnableNonExecNeeded (
>      return FALSE;
>    }
> 
> +  //
> +  // Intel TDX sets this flag to FALSE.
> +  //
> +  if (!PcdGetBool (PcdIa32EferChangeAllowed)) {
> +    return FALSE;
> +  }
> +
>    //
>    // XD flag (BIT63) in page table entry is only valid if IA32_EFER.NXE is set.
>    // Features controlled by Following PCDs need this feature to be enabled.
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec
> index 463e889e9a68..453f2a74b11d 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -2138,6 +2138,11 @@
>    # @Prompt GHCB Pool Size
>    gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize|0|UINT64|0x00030008
> 
> +  ## This dynamic PCD indicates if IA32_EFER can be changed. The default value
> is TRUE but in
> +  #  Intel TDX change of IA32_EFER is not allowed.
> +  # @Prompt The flag which indicates if IA32_EFER is allowed to be changed.
> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdIa32EferChangeAllowed|TRUE|BOOLEA
> N|0x00030009
> +
>  [PcdsDynamicEx]
>    ## This dynamic PCD enables the default variable setting.
>    #  Its value is the default store ID value. The default value is zero as Standard
> default.
> --
> 2.29.2.windows.2