Re: [PATCH V12 14/47] UefiCpuPkg: Enable Tdx support in MpInitLib

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 3/29/22 18:46, Min Xu wrote:
> RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429
> 
> In TDVF BSP and APs are simplified. BSP is the vCPU-0, while the others
> are treated as APs.
> 
> So MP intialization is rather simple. ApWorker is not supported, BSP is
> always the working processor, while the APs are just in a
> wait-for-precedure state.
> 
> 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: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
>   UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |   3 +
>   UefiCpuPkg/Library/MpInitLib/MpIntelTdx.h     |  69 ++++++++++++
>   UefiCpuPkg/Library/MpInitLib/MpLib.c          |  63 ++++++++++-
>   UefiCpuPkg/Library/MpInitLib/MpLibTdx.c       | 106 ++++++++++++++++++
>   UefiCpuPkg/Library/MpInitLib/MpLibTdxNull.c   |  69 ++++++++++++
>   UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |   3 +
>   6 files changed, 308 insertions(+), 5 deletions(-)
>   create mode 100644 UefiCpuPkg/Library/MpInitLib/MpIntelTdx.h
>   create mode 100644 UefiCpuPkg/Library/MpInitLib/MpLibTdx.c
>   create mode 100644 UefiCpuPkg/Library/MpInitLib/MpLibTdxNull.c
> 

>     //
> @@ -2367,6 +2385,11 @@ MpInitLibWhoAmI (
>       return EFI_INVALID_PARAMETER;
>     }
>   
> +  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
> +    *ProcessorNumber = 0;
> +    return EFI_SUCCESS;
> +  }
> +

I've narrowed down this change as causing issues when booting multiple 
vCPU guests (regular and SEV). This issues consist of EfiAcquireLock() / 
EfiReleaseLock() ASSERTS and TPL level ASSERTS that occur during 
ExitBootServices when the APs are being parked by RelocateApLoop(). The 
PCD accesses use locking which is not SMP safe. I believe PCD calls are 
not supposed to be issued by APs because of this (or at least not APs 
executing in parallel).

This check is spread throughout the MpLib code and I didn't look to see 
how many of those calls can be done by an AP. I think the PCD usage can be 
reduced to getting the PcdConfidentialComputingGuestAttr value at init and 
caching it in a STATIC variable, but I would feel better if @Min Xu could 
verify that and submit a patch accordingly. Otherwise, maybe this TDX 
guest setting could be added to the CpuMpData (similar to the 
SevEsIsEnabled, etc. fields).

A future cleanup could cache PcdConfidentialComputingGuestAttr in 
CpuMpData and use CC_GUEST_IS_XXX() calls against that.

Thanks,
Tom

>     CpuMpData = GetCpuMpData ();
>   
>     return GetProcessorNumber (CpuMpData, ProcessorNumber);