Re: [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in ResetVector


Yao, Jiewen
 

HI Min
I agree with Brijesh.

The basic rule is: SEV file shall never refer to TDX data structure. TDX file shall never refer to SEV data structure.
These code should be isolated clearly.

Do we still need that logic if we follow the new pattern?

Thank you
Yao Jiewen

-----Original Message-----
From: Brijesh Singh <brijesh.singh@amd.com>
Sent: Tuesday, July 27, 2021 8:31 PM
To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
Cc: brijesh.singh@amd.com; Ard Biesheuvel <ardb+tianocore@kernel.org>;
Justen, Jordan L <jordan.l.justen@intel.com>; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Yao,
Jiewen <jiewen.yao@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>
Subject: Re: [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in ResetVector


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%2Fedk2.gr
oups.io%2Fg%2Fdevel%2Fmessage%2F77336%3Fp%3D%2C%2C%2C20%2C0%2
C0%2C0%3A%3ACreated%2C%2Cpost&amp;data=04%7C01%7Cbrijesh.singh%4
0amd.com%7C22b61f2ff5bb48348b0608d950f4d7c5%7C3dd8961fe4884e608e1
1a82d994e183d%7C0%7C0%7C637629834792320372%7CUnknown%7CTWFpb
GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M
n0%3D%7C1000&amp;sdata=tMGpR4a2uZTTR%2FsciTN0oeca2mZ32GfX3K78lA
5BWas%3D&amp;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. 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 ?

For SEV-SNP, see this patch

https://edk2.groups.io/g/devel/message/77342?p=,,,20,0,0,0::Created,,posteri
d%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

Join devel@edk2.groups.io to automatically receive all group messages.