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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
