From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-DM6-obe.outbound.protection.outlook.com (NAM11-DM6-obe.outbound.protection.outlook.com [40.107.223.42]) by mx.groups.io with SMTP id smtpd.web10.5222.1627532969008149615 for ; Wed, 28 Jul 2021 21:29:29 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@amd.com header.s=selector1 header.b=2kGrEyYZ; 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.223.42, mailfrom: brijesh.singh@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=joia0e0w5NavLD02KMtVCdcA4uD09qkpdBDEbiHpqyUdHT8CaI/Z8QhKuZEPq687wYrjS6WrMX8oGruz+5bTGxpu8uQ0zzWf14i4aF3BVReSMGGRUmphnduY4drSVXx9mfGxaYFqo0iiOCEfU94LtgTJMakLKeInj78U5n3EistxsmHy/mj7kIwWWlU//vzLJwwAeFn908IZRbEVYchaX92VV1q3QW0ynyeCCa9Au9T63Lc1xt2ZYTtaY4c4Ij5b3Cc2zNnY/E1E88mU1gpJm4bSgXZfiUKwSa0ETFQjZdOqkT/NuIBTnRl5/jxoYQzVLADRAEBfTad21ZoMQbVVZA== 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=ha3gRFWW3mw4mnc18nH2nf6Jnv9CX95GmmRk27Vchvs=; b=U9bBgHuHW26B4Ng/Hxfim9N9iLcuAboa+K6bKG/q+DyepYsAJnRvhgVU69RVYgixjB5TZpG9MvEDfOZKsWLQw+UKQjQI/zrm19+HIGC+YXPo7Dh4OoKohxoTZ9sOqiHVbiu5EJKrY2pHwQCfXr3deQqx5EIN4bW5y/TEb4xFNfpX95sjS+H7AzKRyZ6Ity9CQBn1BnGSk92Fr5bqk4NBifopZnRRNKuZXP6DAAH7Pg4ZetGutKsPZ+GC8a5J3G02HXJ8ZHbOOZws8/pE6CvPyp5ExyvGTqg3g/1JP+IFumxfLIJfQT1xGcc/yp3I2ypSVGCFb391hPGS4y9eNRbNVw== 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=ha3gRFWW3mw4mnc18nH2nf6Jnv9CX95GmmRk27Vchvs=; b=2kGrEyYZpSIddLPSM5jPJsCmyRfjxIuBcv91IDJMLgmt4NJNTpF+1ZvJXydx366nGhx4i3c1y802y0Jh85XdvTmSv2mCUs3G9FyyiQc5CVEHUPHLcs+OI8hcDNj8P9WF3NOfEE+BaOehEYM/NmITCl3ON/1aNEBsdkGiLDHa9vM= 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 SA0PR12MB4432.namprd12.prod.outlook.com (2603:10b6:806:98::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4373.21; Thu, 29 Jul 2021 04:29:23 +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.021; Thu, 29 Jul 2021 04:29:23 +0000 Subject: Re: [edk2-devel] [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in ResetVector To: "Xu, Min M" , "Yao, Jiewen" , "devel@edk2.groups.io" CC: Ard Biesheuvel , "Justen, Jordan L" , Erdem Aktas , James Bottomley , Tom Lendacky References: <4E4F0C83-ED04-4CFC-BDE2-33825C106DB9@intel.com> <97ad36da-13cd-ccb1-48f3-17ee03934aa6@amd.com> <1e234d04-6348-5ac8-9c99-0557d6b44ef6@amd.com> From: "Brijesh Singh" Message-ID: Date: Wed, 28 Jul 2021 23:29:19 -0500 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 In-Reply-To: X-ClientProxiedBy: SN4PR0601CA0011.namprd06.prod.outlook.com (2603:10b6:803:2f::21) 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 Brijeshs-MacBook-Pro.local (165.204.77.11) by SN4PR0601CA0011.namprd06.prod.outlook.com (2603:10b6:803:2f::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4373.20 via Frontend Transport; Thu, 29 Jul 2021 04:29:21 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 77531ba1-1bc2-4e5d-916c-08d952497095 X-MS-TrafficTypeDiagnostic: SA0PR12MB4432: 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: EZ+LFscm0YeiR/xp2GlZIDmnIjAGBl1Ryb64gpCFPBEnh6UehEUXjbTMYoLyxcV7aq9uMZm0fDVNhYnl7tU6Eems0phxpsk4HUamSobc/AK8x2Y3aQ7NQ9zqf6w6F6pMIMCDNwRbBkR6D0v5zFzjghKn0ZCNz02B8xPvv00DlPv2676+Q2N0nz2rvCiaWEznSy7/p5+Piedc2tpBROztFSQlThlIQGGHCdsFe06tTMsNTTZX4HFs+DAgQ0adOzqUnRBhy6R3p5i6q7+hkaAHK2hOsINpIch8tADTLhjUjNJkkTpZecL13H8RZNdlSTrlnaIvAFxk805hdYmcZ3IGQ6jykzNzL0/VFPM6qhv+qVNReR/mjLROcx5IF867xD1h6zDl/1HrMUQKRfw7E2rT5/JLF6O3zHXtZMAJM37cuLJu2/CioP5KRCN5VWdDbo8vxNkyjl8wB0pensBTsijb5MDgIgBpGbOX4apdgbKNaxRbmpbQVrni7fzrDiXAauKYfJrBrggSVI9knqhpgp/nx2kYEpEULFjX6FkUa6KMHMLuai5s4uXzf54lObF9C3lY3W7SukApeGLzV1PldEcPAG4Ys0aj2ZlagMJsWekUFZchoODBaBvwd2xipvyIxFumXxzdGSXfTHf6TqdXxNu6/c2c1y9OzCIJtitXhTvXyQF7E0JzNtOGWApYCWWiFF+uCfDpxYJSedUoFtbzmt5PwkwolrKYjnKyJ+Kjyb0GcarNdQBqxBXEZJQRY4P1uq1/9JORR9gt6zt/a/nTFrkaUPx0r/x8lDlHhs6RI3bKD3iOf81nZ0Hy8t/Sgnr0IJCv4Vhml0EOpUZhItgwAkGAJN+DmPdo2h4zYnS1sv30wehk9YWpOY+19qKhPpOZnqZF 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)(136003)(346002)(396003)(376002)(366004)(39860400002)(66476007)(66556008)(36756003)(110136005)(52116002)(956004)(44832011)(2906002)(66946007)(6486002)(6512007)(966005)(2616005)(31686004)(186003)(6506007)(8676002)(31696002)(316002)(5660300002)(53546011)(8936002)(86362001)(83380400001)(45080400002)(30864003)(478600001)(38350700002)(4326008)(26005)(38100700002)(54906003)(45980500001)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?gVC7QdNjsFXJgqXX0mRLtSyRRuaIN9K/xePBW/DbVO3CI0Dfnkd9jhmIcW6+?= =?us-ascii?Q?TjDW+C0bvCM2gkh1q2CqDxfACEyZctzAHsBLF/tBlBW1Lni5DJyptrPrXGEa?= =?us-ascii?Q?htn+1TLvT5AS7i1BDL9CDiIl/1WtLYuQaRWU5Oqk0HVfGN32oo49nlOPWvPz?= =?us-ascii?Q?qREnPGHJ37XVivdX4ncLQ/6puFMWinFyUj1ht0p+ay7ZQP74EORdx7fkdmf3?= =?us-ascii?Q?dNVOFJZcQ/LL3guBZvtQIjg1UH2i+Z2jma90H/3/CIK89LSlLumQPTIuW++/?= =?us-ascii?Q?XRXFq43hVDajRndEQ9H0B7OuZhq4XL3CyLvzq2hSjK480BJWcnNpPZSUXLLO?= =?us-ascii?Q?k5WjfMKgMZ07kzm2CAX0c8hgcRRDZlc+9HVJcJV0RR8tnyfspJzhFstrXMCM?= =?us-ascii?Q?Ihca3HPxbfGs8apLPy6dlQc6uqs7QCZije0cTIHcwHgvvmynHmg2a/X+qFkS?= =?us-ascii?Q?rUQQt5/kH4CXe5g6XWFGoLJr8S/K+L2TcCCRxmvITvPWOkz1e0EPOR3yNR6e?= =?us-ascii?Q?yY0RR7AYGHHlEvNF4zOqgg06vXczN/a5iWKG/Iyf2jbjz9ELuM0/ImraakGf?= =?us-ascii?Q?UrQ8nHCJez7dUtucUtSLwrCL5kEvU6cMbGOE1ds36tqQMQoO/+jJhwksqlgG?= =?us-ascii?Q?ebSKQiJIZh845TAtfJ1tBM7LZju1gEs+hm0nTkcyJYHxbrey2LUT4qL7bdY7?= =?us-ascii?Q?3ZbEK2LwBdcdDEu7rHw5EFNWOJDHNowbeQiMJ67kny8F09SG5ZrnglAWfvje?= =?us-ascii?Q?HRSSVR/+NHoqYDoClrh9A4b9h9evAl9gJj24LWHy4BCWk/xL853iIetIUqDo?= =?us-ascii?Q?JPle0gvvwVQlRgHpWH2aGViO1W1+dTAqnJrIDnqrD73hukXiXJick+h9kBiP?= =?us-ascii?Q?fpO7+BXnW4s6l+XsLO1w8uY8YzrWh+w6opiJBDqeu7Sm2LFV3zLnBoIOiCbK?= =?us-ascii?Q?H+kzRkRGbs7OJyETw0GVzQRcTjJ5MMzmLMeAkVxUQskiW8Ff2j1a9f63JfVf?= =?us-ascii?Q?T5pN9xI/KSxlB0FhaWPihcs3WeJukosPMX1txvHFqQyOHZPJ5BUlTjURM6QO?= =?us-ascii?Q?yPBYWNMPHetcfBsj7X5gEhKIxn9vHtG53S5dfLafZc3ZD0SPqMgX0woePN95?= =?us-ascii?Q?nI9F/HCsu1rkxXcsHrZIy0Cv9iG5+ztzOi4x4r6X1XAvUXHnaNiHByFK9ahL?= =?us-ascii?Q?osdxGrczE+krCmP69VoInlux36+hD3oHRkvt2pG7hAjWrv/fHhK5+oBldOZ7?= =?us-ascii?Q?ZcctMv07ER+OIy5T/iRFhGK7XpjYO28NTLgokEgT0wFjFhPhlHV+sdx/CJvf?= =?us-ascii?Q?vSrsfJjUFzIAoWUfhV/+ShcE?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 77531ba1-1bc2-4e5d-916c-08d952497095 X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2718.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jul 2021 04:29:23.5989 (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: SbnvXHJZeBtqirrqB907Z8gWTCHsTCN5dV6fccOx96cyS7fCi8wcQ4a6iw8AfeqhKL1XV+nXAL9cW4Dq1eeD+w== X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR12MB4432 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Content-Language: en-US On 7/28/21 9:44 PM, Xu, Min M wrote: > Jiewen & Singh > > From the discussion I am thinking we have below rules to follow to the d= esign the structure > of TEE_WORK_AREA: > 1. Design should be flexible but not too complicated > 2. Reuse the current SEV_ES_WORK_AREA (PcdSevEsWorkAreaBase) as TEE_WORK= _AREA > 3. TEE_WORK_AREA should be initialized to all-0 at the beginning of Rese= tVecotr > 4. Reduce the changes to exiting code if possible > > So I try to make below conclusions below: (Please review) > 1. SEV_ES_WORK_AREA is used as the TEE_WORK_AREA by both TDX and SEV, ma= ybe in > the future it can be used by other CC technologies. > > 2. In MEMFD, add below initial value. So that TEE_WORK_AREA is guarantee= d to be cleared > in legacy guest. In TDX this memory region is initialized to be all-0 by= host VMM. In SEV the memory > region is cleared as well. > 0x00B000|0x001000 > gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGu= id.PcdSevEsWorkAreaSize > DATA =3D { > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 > } Hmm, I thought the contents of the data pages are controlled by the host VMM. If the backing pages are not zero filled then there is no guarantee that memory will be zero.=C2=A0 To verify it: 1. I applied your above change in OvmfPkgX86.fdt. I modified the DATA values from 0x00 -> 0xCC 2. Modified the SecMain.c to dump the SevEsWorkArea on entry And dump does not contain the 0xcc. And to confirm further,=C2=A0 I attached to the qemu with the GDB before t= he booting the OVMF, and modified the SevEsWorkArea with some garbage number=C2=A0 and this time the dump printed garbage value I put through th= e debugger. In summary, the OVMF to zero the workarea memory on the entry and we cannot rely on the DATA=3D{0x00, 0x00...} to zero the CCWorkArea. Did I miss something ? > > 3. The structure of TEE_WORK_AREA > The current SEV_ES_WORK_AREA is defined as below: > typedef struct { > UINT8 SevEsEnabled; > UINT8 Reserved1[7]; > [Others...] > } SEC_SEV_ES_WORK_AREA; > > So I think the TEE_WORK_AREA can be: > Byte[0] Type: > =090: legacy 1: SEV =092: TDX=20 > Byte[1] Subtype: > If Type is 0, then it is 0 > =09If Type is 1, then it is up to SEV's definition > =09If Type is 2, then it is up to TDX's definition > Byte[] other bytes are defined based on the Type/Subtype I personally like Yao Jiewen's struct definition, but I can also live with this one as well :). The only question I had was with his proposal was what if we remove the Length and Version fields. If the header length was fixed then life would be much easier in the ASM code.=C2=A0 > I check the code in SecMain.c. > SevEsIsEnabled() need updated to check SevEsWorkarea->SevEsEnabled =3D= =3D 1, not non-0. > @Brijesh Singh Is there any other code need update? As noted before, the SevEsWorkAreas is used to pass the information from the Sec to PEI phase. The workarea gets reused for totally different purpose after the PEI phase. thanks >> -----Original Message----- >> From: Yao, Jiewen >> Sent: Thursday, July 29, 2021 7:49 AM >> To: Brijesh Singh ; devel@edk2.groups.io; Xu, Mi= n M >> >> Cc: 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 >> >> Comment below: >> >>> -----Original Message----- >>> From: Brijesh Singh >>> Sent: Thursday, July 29, 2021 2:59 AM >>> 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 >>> >>> >>> >>> On 7/28/21 11:26 AM, Yao, Jiewen wrote: >>>> I would say it is NOT the best software practice to define 2 enable >>>> fields to >>> indicate one type. >>>> What if some software error, set both TdxEnable and SevEnable at same >> time? >>>> How do you detect such error? >>> Hmm, aren't we saying it is a software bug. In that case another bug >>> can also mess up the structure header. >> [Jiewen] Yes. I treat it as a software implementation bug. >> The key thing in software design is to avoid the developer making mista= ke, help >> debug issues, help maintain the code easily. >> >> >>>> If some code may check the SEV only, and some code may check TDX >>>> only, >>> then both code can run. That will be a disaster. The code is hard to >>> maintain and hard to debug. >>>> 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 not >>> scalable. >>>> The best software practice it to just define one field as >>>> enumeration. So 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. >>> I was trying to see if we can make it work without requiring any >>> changes to UefiCpuPkg etc (which uses the EsWorkArea). >> [Jiewen] I agree with you. >> And I think the priority should be: >> 1) make a good design following the best practice. >> 2) minimize the change. >> >> I don=E2=80=99t think two *enable* fields can satisfy #1. >> And I am open on how to do #2. (See rest comment below) >> >> >> >>> >>>> I think we can add "must zero" in the comment. But it does not mean >>>> there will >>> be no error during development. >>>> 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 as header. >>>> Since we are defining a common data structure, I think we can do >>>> some clean >>> up. >>>> Or if you have concern to change SEC_SEV_ES_WORK_AREA, we can define >>>> a >>> new CC WORK_AREA without touching SEV_SEV_ES_WORKING_AREA. >>> In your below structure, I assume the SEV or TDX probe will set the >>> Type only when it detects that feature is active. But which layer of >>> the software is going to set the type to zero to indicate its a legacy= guest ? >> [Jiewen] Good question. >> I expect some initialization function, such as InitCcWorkAreaSev, >> InitCcWorkAreaTdx. >> The default value Legacy shall be override in InitCcWorkArea after capa= bility >> check. >> PreMainFunctionHookXXX is common patter for all function. It just check= s the >> CcWorkArea. >> >> >> >>> typedef struct { >>> =09UINT8 HeaderVersion; // 0 >>> =09UINT8 HeadLength; // 4 >>> =09UINT8 Type; // 0 - legacy, 1 - SEV, 2 - TDX >>> =09UINT8 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 its active then set the type =3D MEM_ENCRYPT_SEV; and similarly the >>> Tdx hook will set the type=3DMEM_ENCRYPT_TDX. What if neither TDX or S= EV >>> is enabled. At this time we will depend on hypervisor to ensure that >>> value at that memory is zero. >> [Jiewen] I think we just let InitCcWorkAreaSev and InitCcWorkAreaTdx ov= erride >> the default value. >> InitCcWorkArea{Sev,Tdx}: >> =09// Detect Hardware Capability >> =09// If discovered, then set Type =3D {SEV,TDX} >> =09// Else, leave it as is >> >> >> >> >>> Additionally, do you see a need for the HeadLength field in this >>> structure ? In asm it is preferred if we can access the actual >>> workarea pointer at the fixed location instead of first reading the >>> HeadLength to determine how much it need to skip. >> [Jiewen] You are right. >> Length/Version is NOT absolutely necessary, if the header is simple eno= ugh. If >> you think we don=E2=80=99t need them, I am OK to remove them. >> I think only "Type" is mandatory, which tells us the category. >> I think "SubType" is useful, because we might want to know if it is SEV= , or SEV-ES, >> or SEV-SNP, and if it is TDX 1.0, or TDX.future. Please let me know you= r thought. >> >> >> One question on SEC_SEV_ES_WORK_AREA. Since you have SEV/SEV-ES/SEV- >> SNP, is this SEV_ES_WORK_AREA only for SEV_ES? Or it is also used in SE= V (no >> SEV_ES), and SEV_SNP ? >> For example, when we see SevEsEnable=3D0, how do we know it is SEV (no = SEV_ES) >> or legacy (no SEV, no SEV_ES)? Or we don=E2=80=99t need care in SEV (no= SEV_ES) case. >> >> >> >> >>> >>>> Thank you >>>> Yao Jiewen >>>> >>>> >>>>> -----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 >>>>> area 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= * >>>>>> fields. 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 >>>>>>> Brijesh 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; Ard Biesheuvel >>>>>>>>> ; Justen, Jordan L >>>>>>>>> ; Erdem Aktas >>>>>>>>> ; >>>>>>> James >>>>>>>>> Bottomley ; Tom Lendacky >>>>>>> >>>>>>>>> Subject: RE: [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in >>> ResetVector >>>>>>>>> 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 legacy, >>>>> SEV, >>>>>>>>> or >>>>>>>>>> TDX, right? >>>>>>>>> Right. We need somewhere to store this flag, either in a >>>>>>>>> Register or in >>>>>>> Memory. >>>>>>>>> If it is memory, then in Tdx the memory region should be >>>>>>>>> initialized 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 str= ucture. >>>>>>>>>>>> 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 = pattern. >>>>>>>>>>> >>>>>>>>>>> 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 >>>>>>>>>>> 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 >>>>>>>>>>> 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 >>> 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; >>>>>>> Are we reserving a new page for the TDX_WORK_AREA ? I am >>>>>>> wondering >>> why >>>>>>> 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 >>>>>>> without 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 phase 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 core does not need to know anything about >>>>>>> the workarea and they simply can read the PCDs. >>>>>>> >>>>>>> -Brijesh >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>> >>>>>=20 >>>>>