[edk2-devel] [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in ResetVector

Wed Jul 28 00:40:34 UTC 2021

On July 27, 2021 8:31 PM, Brijesh Singh wrote:
> On 7/27/21 6:51 AM, Xu, Min M wrote:
> > On July 27, 2021 6:57 PM, Brijesh Singh wrote:
> >> Hi Min,
> >>
> >> This refactoring is already done by the SNP patch series.
> >>
> >>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk
> >>
> 2.groups.io%2Fg%2Fdevel%2Fmessage%2F77336%3Fp%3D%2C%2C%2C20%
> 2C0%2C0%2
> >>
> C0%3A%3ACreated%2C%2Cpost&data=04%7C01%7Cbrijesh.singh%40a
> md.com%
> >>
> 7C22b61f2ff5bb48348b0608d950f4d7c5%7C3dd8961fe4884e608e11a82d994
> e183d
> >> %7C0%7C0%7C637629834792320372%7CUnknown%7CTWFpbGZsb3d8ey
> JWIjoiMC4wLjA
> >>
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&s
> data=
> >>
> tMGpR4a2uZTTR%2FsciTN0oeca2mZ32GfX3K78lA5BWas%3D&reserved
> =0
> >> erid%3A5969970,20,2,20,83891510
> >>
> >> It appears that you are also pulling in some of TDX logic inside the
> >> AMDSev.asm such as
> >>
> >> ;
> >> +PostJump64BitAndLandHereSev:
> >> +
> >> +    ;
> >> +    ; If it is Tdx guest, jump to exit point directly.
> >> +    ; This is because following code may access the memory region which
> has
> >> +    ; not been accepted. It is not allowed in Tdx guests.
> >> +    ;
> >> +    mov     eax, dword[TDX_WORK_AREA]
> >> +    cmp     eax, 0x47584454             ; 'TDXG'
> >> +    jz      GoodCompare
> >>
> >> Why we are referring the TDX workarea inside the AmdSev.asm ?
> > See my explanation in the above comments. In Tdx guests memory region
> > cannot be accessed unless it is accepted by guest or initialized by
> > the host VMM. In PostJump64BitAndLandHereSev there is access to
> > dword[SEV_ES_WORK_AREA_RDRAND] which is not initialized by host
> VMM.
> > If this code will not be executed in Tdx guest, then the above check is not
> needed. I need your help to confirm it.
> >
> > There are similar Tdx check in my patch of AmdSev.asm. For example in
> > CheckSevFeatures byte[SEV_ES_WORK_AREA] is used to record the SEV-ES
> > flag. This memory region is not initialized by host VMM either. So in Tdx it
> will trigger error.
> >
> > Another solution is that the memory region used by SEV in ResetVector
> > are added Into Tdx metadata so that host VMM will initialize those
> > memory region when It creates the Td guest. What's your opinion?
> 
> I am not full versed on TDX yet and sorry I am not able to follow you
> question completely to provide any advice. With SEV and SEV-ES, a guest can
> access the memory without going through the validation process, but with
> the SEV-SNP, the page need to be validated (aka accepted) before the access.
TDX has the same requirement.
> In SNP series, we ensure that the data pages used in the reset vector are pre-
> validated during the VM creation time -- this allows us to access the pages
> without going through accept process. If I follow you correctly on your
> metadata comment then it is similar to saying is pre-validate these range of
> pages used in the reset vector code (that include GHCB page, Page table
> pages etc), right ?
That's right. Tdx metadata describes the memory region which host VMM initialized
during the VM creation time.

In the current patch-set, below memory region are described in Tdx metadata.
 - TdMailbox (PcdOvmfSecGhcbBackupBase)
 - TdHob(PcdOvmfSecGhcbBase)
 - TdExtraPage(PcdOvmfSecGhcbPageTableBase)
 - OvmfPageTable (PcdOvmfSecPageTablesBase)
These memory regions are initialized by host VMM so they can be accessed in ResetVector in Tdx guests.

In the SEV codes, I find some memory is accessed as well. CheckSevFeatures is the example.
In CheckSevFeatures byte[SEV_ES_WORK_AREA] (PcdSevEsWorkAreaBase) is used to record/check
if it is SEV. So if this function is called in Tdx guest, then error is triggered.

What I am concerned is that, in the current pattern:
====================
	OneTimeCall   PreMainFunctionHookSev
	OneTimeCall   PreMainFunctionHookTdx
MainFunction:
	XXXXXX
	OneTimeCall   PostMainFunctionHookSev
	OneTimeCall   PostMainFunctionHookTdx
====================
The TEE function need implement a TEE check function (such as IsSev, or IsTdx). 
Tdx call CPUID(0x21) to determine if it is tdx guest in the very beginning of ResetVector. Then 'TDXG' is set
in TDX_WORK_AREA. 
SEV does the similar work which call CheckSevFeatures to set SEV_ES_WORK_AREA to 1.
After that both TDX and SEV read the above WORK_AREA to check it is TDX or SEV or legacy guest.

In Tdx the access to SEV_ES_WORK_AREA will trigger error because SEV_ES_WORK_AREA is initialized by host VMM.
In SEV-SNP I am afraid the access to TDX_WORK_AREA will trigger error too.

I am wondering if TDX and SEV can use the same memory region (for example, TEE_WORK_AREA) as the work area?
So that this work area is guaranteed to be initialized in both TDX and SEV. Structure of the TEE_WORK_AREA may
look like this:
  typedef struct {
      UINT8  Flag[4];         'TDXG' or 'SEVG' or all-0
      UINT8  Others[];
  } TEE_WORK_AREA;

> 
> For SEV-SNP, see this patch
> 
> https://edk2.groups.io/g/devel/message/77342?p=,,,20,0,0,0::Created,,post
> erid%3A5969970,20,2,20,83891520
> 
> A VMM (qemu) looks for the range of page it need to prevalidate before the
> boot, the range is provided through the GUID (SevSnpBootBlock).
> 
> >> I will take out my refactoring patch outside of the SNP series and
> >> submit it so that you can build on top of. This will simplify review process.
> >>
> > Thank you very much for the refactoring.  I will refine my patch based on it.
> >> thanks
> >>
> >>

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78258): https://edk2.groups.io/g/devel/message/78258
Mute This Topic: https://groups.io/mt/84476064/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-