From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg: Save PcdConfidentialComputingGuestAttr in mCcGuestAttr To: Min Xu ,devel@edk2.groups.io From: "Ni, Ray" X-Originating-Location: California, US (192.55.55.52) X-Originating-Platform: Windows Chrome 101 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Wed, 04 May 2022 18:41:02 -0700 References: In-Reply-To: Message-ID: <21893.1651714862231437175@groups.io> Content-Type: multipart/alternative; boundary="NHC2CTrtAmLiDX1eQLOl" --NHC2CTrtAmLiDX1eQLOl Content-Type: text/plain; charset="utf-8"; markup=markdown Content-Transfer-Encoding: quoted-printable On Mon, May 2, 2022 at 03:03 PM, Min Xu wrote: > > The reason why C global variable cannot be used in PEIM is that in some > scenario PEIM is executed in FLASH so that the value of C global variable > cannot be kept in different calls. But I don't think it is a problem in t= his > situation. > 1. This global variable is to keep the PcdConfidentialComputingGuestAttri= bute > in mCcGuestAttr. So if this is Tdx guest, then the global variable can be= kept > (CC_GUEST_IS_TDX (mCcGuestAttr) =3D=3D TRUE). > 2. If this is Non-Td guest, then even the global variable cannot be kept, > CC_GUEST_IS_TDX (mCcGuestAttr) is FALSE. So mCcGuestAttr can still work. Directly writing to flash area might cause some side effects. Usually we need to avoid that. >=20 > There is another solution that we can use CcProbe (which is in > MdePkg/CcProbeLib). CcProbe checks the work area to fetch the guest type.= It > calls FixedPcdGet32 (PcdOvmfWorkAreaBase) so there is no SMP safe issue i= n > PcdLib. It seems ok. But it's unclear to me how the work area is built. >=20 > As to a new field in CPU_MP_DATA, Tdx guest doesn't create CPU_MP_DATA ob= ject > in MpInitLibInitialize, instead it return right after it detects the work= ing > guest is Tdx guest. So this fix will be more complicated than above 2 > solutions. I agree it'll be more complicated to let the TDX version of MpLib use the C= PU_MP_DATA. But, I am very curious why not leave the MpInitLib untouched. The solution = is: 1. Platform FDF includes two CpuMpPeim drivers. One links to PeiMpInitLib, = the other links to MpInitLibUp. Change platform DSC to let first PEIM depend on "FullMpPpi" Change platform DSC to let second PEIM depend on "UpMpPpi" 2. SEC code passes "FullMpPpi" to PEI Core when it's not a TDX boot. It pas= ses "UpMpPpi" when it's a TDX boot. I think this solution doesn't even change the MP code. Or saying in another= way, MP and TDX are decoupled. I prefer this solution. >=20 > Ray, What's your thought? > --NHC2CTrtAmLiDX1eQLOl Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable

On Mon, May 2, 2022 at 03:03 PM, Min Xu wrote:

The reason why C global variable cannot be used in PEIM is that in some scenario PEIM is executed in FLASH so that the value of C global variable cannot be kept in different calls. But I don't think it is a problem in thi= s situation. 1. This global variable is to keep the PcdConfidentialComputingGuestAttribu= te in mCcGuestAttr. So if this is Tdx guest, then the global variable can be k= ept (CC_GUEST_IS_TDX (mCcGuestAttr) =3D=3D TRUE). 2. If this is Non-Td guest, then even the global variable cannot be kept, CC_GUEST_IS_TDX (mCcGuestAttr) is FALSE. So mCcGuestAttr can still work.

Directly writing to flash area might cause some side effects. Usually we need to avoid that.

There is another solution that we can use CcProbe (which is in MdePkg/CcProbeLib). CcProbe checks the work area to fetch the guest type. I= t calls FixedPcdGet32 (PcdOvmfWorkAreaBase) so there is no SMP safe issue in PcdLib.

It seems ok. But it's unclear to me how the work area is built.

As to a new field in CPU_MP_DATA, Tdx guest doesn't create CPU_MP_DATA o= bject in MpInitLibInitialize, instead it return right after it detects the workin= g guest is Tdx guest. So this fix will be more complicated than above 2 solutions.

I agree it'll be more complicated to let the TDX version of MpLib use th= e CPU_MP_DATA. But, I am very curious why not leave the MpInitLib untouched. The solution = is: 1. Platform FDF includes two CpuMpPeim drivers. One links to PeiMpInitLib, = the other links to MpInitLibUp. Change platform DSC to let first PEIM depend on "FullMpPpi" Change platform DSC to let second PEIM depend on "UpMpPpi" 2. SEC code passes "FullMpPpi" to PEI Core when it's not a TDX bo= ot. It passes "UpMpPpi" when it's a TDX boot.

I think this solution doesn't even change the MP code. Or saying in anot= her way, MP and TDX are decoupled. I prefer this solution.

Ray, What's your thought?

--NHC2CTrtAmLiDX1eQLOl--