From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-CO1-obe.outbound.protection.outlook.com (NAM11-CO1-obe.outbound.protection.outlook.com [40.107.220.43]) by mx.groups.io with SMTP id smtpd.web12.2385.1627498719162252949 for ; Wed, 28 Jul 2021 11:58:39 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@amd.com header.s=selector1 header.b=L8dClMn/; spf=permerror, err=parse error for token &{10 18 %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email}: invalid domain name (domain: amd.com, ip: 40.107.220.43, mailfrom: brijesh.singh@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=KRaGaWRz8JXMXUmRFWTl09Tr8t5nbyS454K91IxECTeASF9XKw50qv9Fh4JSjCn/ibKXNdbc52A6f4sAjElc9z+kwiRZACiwhIiU+lS25q7VfyElUm6hPftFg7LM1xZyIi8CqgE9jfWWJxUaEvP3H2AXVNadkecFYlgRLUIytGXWyTZ/PSjwyBLpRkoC42EbYxb/Kx5tTTxWhs6F6+tZ1KjRewwtMyTRwGcrvDZSQlSzmMSkAhe0LQnqbVwPfzojpghlz1TLwpFvBPb1S5zBrb2dGB9zs7JgY29GyhqL87RupTVZnrJ+r3Uw78VDLWdWvDmoAvQ35XYWINhWjmr4mQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=VOTmdoEII4D46JYWtvVnYovfrq6+OJbO6IG84LqVv7M=; b=Nw6t1T4r95oUrjs2VDScrSwkbe9qr56k0UMOeWCjTRTW4aDgmHx0tFi3T0Nf7ugYWy+C6NRrEG8iwN8N8Vmh2XoYXeMJCCqWWlJ21BZpa77GOIXq3AYh376i61rZnaJAp+Oocr18XAS0tjQepJwRzhh23GAjYm8s/XNDO4rjUKGJMlj4DabDowsf/3DKN3TLBX1fG8Z9ODVjCJLW/tV3K7VklWHqNluxkNJ+hPX8PJ0hm3Sj4goDmvmUXR5yW0ctofL0hTPkIjdzpcAUONkpsDxe1uDBW9p2v0jk+Yn7Mh9lfrs2uvgqNlhVO8fCf1BJozvVHrIsSUCUEmZ7Ai/wtA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=VOTmdoEII4D46JYWtvVnYovfrq6+OJbO6IG84LqVv7M=; b=L8dClMn/d70/nKF2anlqMMN3FwZ4fa4zdazGVikypOlCgpc4k9nq3c7SSc3yTHfxnbnfzpghVy5uv9E8bKGIV0FkIlq0+LElDmK3fEQGNZiev0YPRRYPgIhuvoOGOYdD4o6At5NonxzetbVccmMmuhLLOqalTgdVv2vFtLIaK+U= Authentication-Results: amd.com; dkim=none (message not signed) header.d=none;amd.com; dmarc=none action=none header.from=amd.com; Received: from SN6PR12MB2718.namprd12.prod.outlook.com (2603:10b6:805:6f::22) by SN1PR12MB2413.namprd12.prod.outlook.com (2603:10b6:802:2b::29) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4352.28; Wed, 28 Jul 2021 18:58:36 +0000 Received: from SN6PR12MB2718.namprd12.prod.outlook.com ([fe80::a8a9:2aac:4fd1:88fa]) by SN6PR12MB2718.namprd12.prod.outlook.com ([fe80::a8a9:2aac:4fd1:88fa%3]) with mapi id 15.20.4373.019; Wed, 28 Jul 2021 18:58:36 +0000 CC: brijesh.singh@amd.com, Ard Biesheuvel , "Justen, Jordan L" , Erdem Aktas , James Bottomley , Tom Lendacky Subject: Re: [edk2-devel] [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in ResetVector To: "Yao, Jiewen" , "devel@edk2.groups.io" , "Xu, Min M" References: <4E4F0C83-ED04-4CFC-BDE2-33825C106DB9@intel.com> <97ad36da-13cd-ccb1-48f3-17ee03934aa6@amd.com> From: "Brijesh Singh" Message-ID: <1e234d04-6348-5ac8-9c99-0557d6b44ef6@amd.com> Date: Wed, 28 Jul 2021 13:58:34 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 In-Reply-To: X-ClientProxiedBy: SN4PR0501CA0153.namprd05.prod.outlook.com (2603:10b6:803:2c::31) To SN6PR12MB2718.namprd12.prod.outlook.com (2603:10b6:805:6f::22) Return-Path: brijesh.singh@amd.com MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [10.236.31.95] (165.204.77.1) by SN4PR0501CA0153.namprd05.prod.outlook.com (2603:10b6:803:2c::31) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4394.8 via Frontend Transport; Wed, 28 Jul 2021 18:58:35 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 45dc9843-51dd-45ea-b4e5-08d951f9b436 X-MS-TrafficTypeDiagnostic: SN1PR12MB2413: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:10000; X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: oBZhaPGjE47yuoOqKczCQRd+EAtLEkrBtpO9pZ5kEbxqnvrqHDO5VHtcKc6Yh8AhBW67cRhn+N94OZv08UJc5AUWtCff8tBdSlp8eNtPVNuVTfT0SMj4SjfAS32M/QBy76tsi79XAvwhnAeckvi5qXjuGK768LRPcSa9IUubsliI25av0CPa2slG+qFVimHjzE9r3yDNP5ZFAMnAj+OzmpdV1RgKWd4ExTI80wg7vCvOU7+MAZKzeHbaWiDg1S9N9jjnS5ae5NDmLm6vuka2mHvbztr+z9K9y3wV5usSEgJ01TlvXg8iPSp+TgAjL5ROYS0+XPUm/1oNiP96aaiCN9PWUifXvH+Y9GuLnawHJZLxGHHIWo++U9Qa8AYVs2d/WCgxBc5uu3n0EZhIn2gu/mkznWqyV7HKN8hnGb5NL1ol820S4wN3xO3VLLxRfZGDlnvvID7Erq+VW/5PI0oDMdlH8NTAiO+iFscL6pxYh3vTCDFdDDh0kkdO+ktovJJpwOszfA6QCNFDm6dQP301Vx+H7FlGbtbJjNs7boCqSU2Mgbgu5gYKbROte/OsSUw1RMHqhFDWxsxuChAwIIHFCb68UNbQNKDR2EjZSI+WXmeRtjkUJ8E3mEU3xyCxFAhHPl8D7iKxt9c6jsVsVQSe/pU8zVnab8J/x+It4BFyq5KAGJsTsNvMXLbO0a/9093wdNLqnLBqjAvmvx3I7OWQO5jhsFhdH/i/cnaLQhEqSyyRAQIMItIiEHwig8g//oybHkzy69UtD3giwHAsTyE6BocDfoPIIZCXcmhkIIC6k1dgcIJ+cVVSploPZaoLIbFe2A0MddYXoT/RMFqn78fupM6dGHWO4YGPtigC0tjN6XQ= X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:SN6PR12MB2718.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(396003)(346002)(376002)(39860400002)(136003)(366004)(316002)(478600001)(66556008)(31696002)(86362001)(2906002)(38350700002)(966005)(16576012)(45080400002)(66476007)(8936002)(66946007)(83380400001)(38100700002)(8676002)(26005)(186003)(52116002)(6486002)(956004)(30864003)(44832011)(110136005)(5660300002)(54906003)(2616005)(4326008)(53546011)(36756003)(31686004)(43740500002)(45980500001);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?fVTUTshgT9LsiKUO1JYisg8DpuOMaJo3aVD1KKQ9+lnzzA7SOQ6qnFyRkpwB?= =?us-ascii?Q?VYiiezdJUB3XhXGBUpRJD6N8lHhfwJGgPtWW5bYhhJGXKuW3ArOstMAIo3E3?= =?us-ascii?Q?TQXe0lJ22yMi2SxnrLdCjvKavMHbVG/pF/UbDOsX3hAK+S4pEl4L6QTINAgn?= =?us-ascii?Q?IjdXlAtRoZ3hVZN/3Nx93RsEBydi1f0D7XpzJt5JcX8dF2CYz1gaJvEysT15?= =?us-ascii?Q?GuttnbYiA7Moz6v5JhaVyi1cwgiXpz7KBW1Yl/KIuflhn+PeMrTfQPJEQdyL?= =?us-ascii?Q?fJGcylyA4etrH3DNgtajv5XfwvuWmph6rxegRxPzsss+u5k53mrI7SwbyPH7?= =?us-ascii?Q?HRuGUT9rZFzgpi5j0ghvX3IB7Fl/UJUANSMoKnyFR7aS0ao2BfSe0tx7RX0g?= =?us-ascii?Q?uvwKAyqBMyVBbTb0dHTaA4zd4iu7f/tC6ZhL22pIT5DVWNTxpR4y/NDZECOZ?= =?us-ascii?Q?4aZmz4T3clp0/PIQyB0QPmJjOXdzlpzTz/tLSBHm4pR0KCc9BtTOIIo20cxk?= =?us-ascii?Q?o49WZmQen0DGmLBgVlu+rL9lekxCC4eFnQJVJrWMBt5Y6Zbmkq0pvNv3ObnM?= =?us-ascii?Q?SyoZseWy86g5GqaEW/owhfkJAzp+3Sh1RBv5l80PNwkupctovg3B4wMQCsnj?= =?us-ascii?Q?rjO5BcyrfDTQuHYO0EZCCulk2hpTRH4NhtVWpqvYzu/RSzsP2eLuieXjsBFG?= =?us-ascii?Q?gOEn6fCLPYPlfUcjkECeaw/NINFSNzUF8NBiz7HkUJlUauLGRaGpaOWLgYB7?= =?us-ascii?Q?RiyshwVRZHX/z9vuM0a+xUKMLl2nhnuVZMDczcV5KHF/mF2alH5qW+l06VAW?= =?us-ascii?Q?7LwGrwJaAPuE8IagomJeL/PqQH8Y2at4Ab3K2edy2KQhAbBgRENYSdddhprW?= =?us-ascii?Q?LXzZr8FzrAzJf1bXJVZduUgmimt8ayhLNmDq98JNT7w0K7KvUgXMfCpLcSth?= =?us-ascii?Q?sE1w/bTsAAzS+9LCLvCMLcAsCs8Ze0Kst0jaCPqyurdeFmbxue3N7/NLtEGl?= =?us-ascii?Q?Ojvcrdsl/PivNgK87R6JpGlYG2WA5M+m2BA1x94nQBw6+5uNZ7Ke+zxSzkTc?= =?us-ascii?Q?B1y0D+3FQB9D+2OxHxQ9PVOIBO7DL+CdA13KPS6Em+eCz/gtNQqEX3jb/Kyr?= =?us-ascii?Q?5ajoJuwJmTEkGcw2eZNCxeKk5Gu8RtfW/5IJz9DJAsJKo2B8mRaVQvrtzjfq?= =?us-ascii?Q?4wcXz+3Vz32KbgK58vvLLFBW0G9Q4wYhs9/i9itwctboZP8ayxAbki6n/qx7?= =?us-ascii?Q?1sL7L+95RSFVNvIJ6tPmKQVR74OStyrZHCA2ZzxQoQtj2fv5tlGzgoHK5CAz?= =?us-ascii?Q?D0Kd08Y5Z7Nk5elm/EPkIRmp?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 45dc9843-51dd-45ea-b4e5-08d951f9b436 X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2718.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jul 2021 18:58:36.4694 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: aKmIxzlciENlW4JKCoheODvpWsbU6zJwjV1djpZT/7nzfHS7YbPIkcee2xXDQqBKJ3VgoA4X2HONpjshX5+w+A== X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR12MB2413 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 7/28/21 11:26 AM, Yao, Jiewen wrote: > I would say it is NOT the best software practice to define 2 enable fiel= ds to indicate one type. >=20 > What if some software error, set both TdxEnable and SevEnable at same ti= me? > How do you detect such error? Hmm, aren't we saying it is a software bug. In that case another bug can= =20 also mess up the structure header. >=20 > If some code may check the SEV only, and some code may check TDX only, t= hen both code can run. That will be a disaster. The code is hard to maintai= n and hard to debug. >=20 > Another consideration is: what if someone wants to set the third type? > Do we want to reserve the 3rd byte? To indicate the third type? It is no= t scalable. >=20 > The best software practice it to just define one field as enumeration. S= o any software can only set Tdx or Sev. The result is consistent, no matter= you check the SEV at first or TDX at first. > If there is 3rd bytes, we just need assign the 3rd value to it, without = impact any other field. >=20 I was trying to see if we can make it work without requiring any changes= =20 to UefiCpuPkg etc (which uses the EsWorkArea). >=20 > I think we can add "must zero" in the comment. But it does not mean ther= e will be no error during development. >=20 > UNION is not a type safe structure. Usually, the consumer of UNION shall= refer to a common field to know what the type of union is - I treat that a= s header. >=20 > Since we are defining a common data structure, I think we can do some cl= ean up. > Or if you have concern to change SEC_SEV_ES_WORK_AREA, we can define a n= ew CC WORK_AREA without touching SEV_SEV_ES_WORKING_AREA. >=20 In your below structure, I assume the SEV or TDX probe will set the Type= =20 only when it detects that feature is active. But which layer of the=20 software is going to set the type to zero to indicate its a legacy guest ? typedef struct { UINT8 HeaderVersion; // 0 UINT8 HeadLength; // 4 UINT8 Type; // 0 - legacy, 1 - SEV, 2 - TDX UINT8 SubType; // Type specific sub type, if needed. } CC_COMMON_WORK_AREA_HEADER; i.e In this call sequence: OneTimeCall PreMainFunctionHookSev OneTimeCall PreMainFunctionHookTdx .... The PreMainFunctionHookSev will detect whether SEV is active or not. If=20 its active then set the type =3D MEM_ENCRYPT_SEV; and similarly the Tdx=20 hook will set the type=3DMEM_ENCRYPT_TDX. What if neither TDX or SEV is=20 enabled. At this time we will depend on hypervisor to ensure that value=20 at that memory is zero. Additionally, do you see a need for the HeadLength field in this=20 structure ? In asm it is preferred if we can access the actual workarea=20 pointer at the fixed location instead of first reading the HeadLength to= =20 determine how much it need to skip. > Thank you > Yao Jiewen >=20 >=20 >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of Brijesh >> Singh via groups.io >> Sent: Wednesday, July 28, 2021 11:59 PM >> To: Yao, Jiewen ; devel@edk2.groups.io; Xu, Min M >> >> Cc: brijesh.singh@amd.com; Ard Biesheuvel ; >> Justen, Jordan L ; Erdem Aktas >> ; James Bottomley ; Tom >> Lendacky >> Subject: Re: [edk2-devel] [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in >> ResetVector >> >> Hi Yao Jiewen, >> >> I guess I am still trying to figure out why we need the header in the >> work area. Can't we do something like this: >> >> typedef struct { >> =09UINT8 SevEsEnabled; >> >> =09// If Es is enabled then this field must be zeroed >> =09UINT8 MustBeZero; >> >> =09UINT8 Reserved1[6]; >> >> =09UINT64 RandomData; >> >> =09UINT64 EncryptionMask; >> } SEC_SEV_ES_WORK_AREA; >> >> typedef struct { >> =09// If Tdx is enabled then it must be zeroed >> =09UINT8 MustBeZero >> >> =09UINT8 TdxEnabled; >> >> =09UINT8 Reserved2[6]; >> =09.... >> >> } TX_WORK_AREA; >> >> typedef union { >> =09SEC_SEV_ES_WORK_AREA SevEsWorkArea; >> =09TDX_WORK_AREA TdxWorkArea; >> } CC_WORK_AREA; >> >> I am trying to minimize the changes to the existing code. The SEV and >> TDX probe logic should ensure that if the feature is detected, then it >> must clear the MustBeZero'ed field. >> >> Basically, we already have a 64-bit value reserved in the SevEsWork are= a >> and currently only one byte is used and second byte can be detected for >> the TDX. Basically the which encryption technology is active the >> definition of the work area will change. >> >> Am I missing something ? >> >> Thanks >> >> On 7/28/21 10:22 AM, Yao, Jiewen wrote: >>> Hi Brijesh >>> Thanks! >>> >>> I think if we want to reuse this, we need rename the data structure. >>> >>> First, we should use a generic name. >>> >>> Second, I don=E2=80=99t think it is good idea to define two *enable* f= ields. Since only >> one of them should be enabled, we should use 1 field as enumeration. >>> >>> Third, we should hide the SEV specific and TDX specific definition in = CC >> common work area. >>> >>> If we agree to use a common work area, I recommend below: >>> >>> typedef struct { >>> UINT8 HeaderVersion; // 0 >>> UINT8 HeadLength; // 4 >>> UINT8 Type; // 0 - legacy, 1 - SEV, 2 - TDX >>> UINT8 SubType; // Type specific sub type, if needed. >>> } CC_COMMON_WORK_AREA_HEADER; >>> >>> typedef struct { >>> CC_COMMON_WORK_AREA_HEADER Header; >>> // reset is valid if Type =3D=3D 1 >>> UINT8 Reserved1[4]; >>> UINT64 RandomData; >>> UINT64 EncryptionMask; >>> } SEC_SEV_ES_WORK_AREA; >>> >>> typedef struct { >>> CC_COMMON_WORK_AREA_HEADER Header; >>> // reset is valid if Type =3D=3D 2 >>> UINT8 TdxSpecific[]; // TBD >>> } TDX_WORK_AREA; >>> >>> Thank you >>> Yao Jiewen >>> >>>> -----Original Message----- >>>> From: devel@edk2.groups.io On Behalf Of Brijes= h >>>> Singh via groups.io >>>> Sent: Wednesday, July 28, 2021 10:34 PM >>>> To: Yao, Jiewen ; Xu, Min M >>>> Cc: brijesh.singh@amd.com; devel@edk2.groups.io; Ard Biesheuvel >>>> ; Justen, Jordan L ; >>>> Erdem Aktas ; James Bottomley >>>> ; Tom Lendacky >>>> Subject: Re: [edk2-devel] [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in >>>> ResetVector >>>> >>>> Hi Jiewen and Min, >>>> >>>> See my comments below. >>>> >>>> >>>> On 7/28/21 2:54 AM, Yao, Jiewen wrote: >>>>> Yes. I am thinking the same thing. >>>>> >>>>> [CC Flag memory location] >>>>> 1) A general purpose register, such as EBP. >>>>> >>>>> 2) A global variable, such as >>>>> .data >>>>> TeeFlags: DD 0 >>>>> >>>>> 3) A fixed region in stack, such as >>>>> dword[STACK_TOP - 4] >>>>> >>>>> 4) A new CC common fixed region, such as >>>>> dword[CC_COMMON_FLAGS] >>>>> >>>>> 5) A fixed region piggyback on existing CC working area, such as >>>>> dword[CC_WORKING_AREA] >>>>> >>>>> Hi Brijesh/Min >>>>> Any preference? >>>>> >>>>> [CC Indicator Flags] >>>>> Proposal: UINT8[4] >>>>> >>>>> Byte [0] Version: 0 >>>>> byte [1] Length: 4 >>>>> byte [2] Type: >>>>> =090: legacy >>>>> =091: SEV >>>>> =092: TDX >>>>> byte [3] Sub Type: >>>>> =09If Type is 0 (legacy), then >>>>> =09 0: legacy >>>>> =09If Type is 1 (SEV), then >>>>> =09 0: SEV >>>>> =09 1: SEV-ES >>>>> =09 2: SEV-SNP >>>>> =09If Type is 2 (TDX), then >>>>> =09 0: TDX 1.0 >>>>> >>>>> Thank you >>>>> Yao Jiewen >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Xu, Min M >>>>>> Sent: Wednesday, July 28, 2021 2:58 PM >>>>>> To: Yao, Jiewen >>>>>> Cc: Brijesh Singh ; devel@edk2.groups.io; Ar= d >>>>>> Biesheuvel ; Justen, Jordan L >>>>>> ; Erdem Aktas ; >>>> James >>>>>> Bottomley ; Tom Lendacky >>>> >>>>>> Subject: RE: [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in ResetVecto= r >>>>>> >>>>>> On July 28, 2021 2:05 PM, Yao, Jiewen wrote: >>>>>>> It does not necessary to be a working area. >>>>>>> >>>>>>> We just need a common TEE flag to indicate if the system run in le= gacy, >> SEV, >>>>>> or >>>>>>> TDX, right? >>>>>> Right. We need somewhere to store this flag, either in a Register o= r in >>>> Memory. >>>>>> If it is memory, then in Tdx the memory region should be initialize= d by host >>>> VMM. >>>>>>> >>>>>>> thank you! >>>>>>> Yao, Jiewen >>>>>>> >>>>>>> >>>>>>>> =E5=9C=A8 2021=E5=B9=B47=E6=9C=8828=E6=97=A5=EF=BC=8C=E4=B8=8B=E5= = =8D=881:07=EF=BC=8CXu, Min M >> =E5=86=99 >>>> =E9=81=93=EF=BC=9A >>>>>>>> >>>>>>>> =EF=BB=BFOn July 27, 2021 8:46 PM, Yao, Jiewen wrote: >>>>>>>>> HI Min >>>>>>>>> I agree with Brijesh. >>>>>>>>> >>>>>>>>> The basic rule is: SEV file shall never refer to TDX data struct= ure. >>>>>>>>> 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? >>>>>>>> I have replied to Brijesh's mail about the concern of the new pat= tern. >>>>>>>> >>>>>>>> I have some concern in the current pattern: >>>>>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>>>>>>> OneTimeCall PreMainFunctionHookSev >>>>>>>> OneTimeCall PreMainFunctionHookTdx >>>>>>>> MainFunction: >>>>>>>> XXXXXX >>>>>>>> OneTimeCall PostMainFunctionHookSev >>>>>>>> OneTimeCall PostMainFunctionHookTdx >>>>>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>>>>>>> The TEE function need implement a TEE check function (such as IsS= ev, 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. SE= V >>>> does >>>>>>> the similar work which call CheckSevFeatures to set SEV_ES_WORK_AR= EA >> to >>>> 1. >>>>>>>> >>>>>>>> After that both TDX and SEV read the above WORK_AREA to check if = 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 *NOT* initialized by host VMM. >>>>>>>> In SEV-SNP I am afraid the access to TDX_WORK_AREA will trigger e= rror >>>> 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 TD= X 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; >>>>>>>>> >>>> >>>> Are we reserving a new page for the TDX_WORK_AREA ? I am wondering wh= y >>>> can't we use the SEV_ES_WORK_AREA instead of wasting space in the >> MEMFD. >>>> >>>> The SEV_ES_WORK_AREA layout looks like this: >>>> >>>> typedef struct _SEC_SEV_ES_WORK_AREA { >>>> UINT8 SevEsEnabled; >>>> UINT8 Reserved1[7]; >>>> >>>> UINT64 RandomData; >>>> >>>> UINT64 EncryptionMask; >>>> } SEC_SEV_ES_WORK_AREA; >>>> >>>> There is reserved bit after the SevEsEnabled and one byte can be used >>>> for the TdxEnabled; >>>> >>>> typedef struct _SEC_SEV_ES_WORK_AREA { >>>> UINT8 SevEsEnabled; >>>> UINT8 TdxEnabled; >>>> UINT8 Reserved2[6]; >>>> >>>> UINT64 RandomData; >>>> >>>> UINT64 EncryptionMask; >>>> } SEC_SEV_ES_WORK_AREA; >>>> >>>> The SEV_ES_WORK_AREA can be treated as a TEE_WORK_AREA and we can >> be >>>> pull out from MemEncrypSevLib.h to CcWorkAreaLib.h and rename the >>>> structure (if needed). >>>> >>>> Both the SEV-SNP and TEE host-VMM accepts the TEE_WORK_AREA before >>>> booting the guest to ensure that its safe to access the memory withou= t >>>> going through the accept/validation process. >>>> >>>> In case of the TDX, the reset vector code sets the TdxEnabled on the >>>> entry. In case of the SEV, the workarea is valid from SEC to PEI phas= e >>>> only and it gets reused for other purposes. The PEI phase set the Pcd= 's >>>> (such as SevEsEnabled or SevEnabled etc) so that Dxe or other EDK2 co= re >>>> does not need to know anything about the workarea and they simply can >>>> read the PCDs. >>>> >>>> -Brijesh >>>> >>>> >>>> >>>> >>> >> >> >>=20 >> >=20