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.64]) by mx.groups.io with SMTP id smtpd.web12.36090.1623073065643668507 for ; Mon, 07 Jun 2021 06:37:45 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@amd.com header.s=selector1 header.b=wr2cL365; 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.64, mailfrom: brijesh.singh@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Ma1aBqrn71Rc0VPR2GlTlaPThfq4p1LiQedXDKtgjM6hJ11GSgr9h5utnxDH6LLaq2s+Dx6nyw4bKicso4tEK2S+ONI0bOGoOBmDpWigy/Q+QPpm1mV0oTRUFVrK4By3bJWYr7VMvr/45K0tJt3vQk1piQjk7Vjfs8VEfX8EvqOLW4VCratNsO+RwNLfLNRMF719H3EMAXfsRsS4ZGhlmtNxBjIWUGhg9GV2Xmz7oWH9AvCP/4pwGukl9MXAQjTKXL0DpMRMauKVJSc9qlbS8JUwHMsjiUKz1KkZOzudGACx/TzdW01XrI36SBbNO5IXRMPob+kn0TW5EDxH+LMEcg== 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=0GVEFOUTBFRXVRT2q3lUGZEe91ve46RWpX/BeOVq5u0=; b=Y5iJ9tbHZ5FVqdJLN+LhXHcI0k47kX0vp/hKPKCtJ8J3/bTj+XJsqw+NqxkxL0KbfmeESQ4ZOihPnlmjHx6r26KxXxegqpGBujAxc9va3lS+x8bgC/Dbq1e9gJwXuyUiIrtfHVQ7wDevhKfnxwehD7n12JbXbxMQk7MegoubZ5Fnl+Ha8oj7CtE4rsjPcM3IREac8zW4GtqyXdthzNskBz7ukzZjvEIeCJWtkHP9nFTcFU+EH3lR1lLzNPNrCpfYIBhGFOKkHCNnWrhgiI9aBDJG+mHgmejNzP9qogJJbI4DSpNXcOpQjCW0yOx73pqlCWFPQC2R6/UPm6860I1qcw== 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=0GVEFOUTBFRXVRT2q3lUGZEe91ve46RWpX/BeOVq5u0=; b=wr2cL365kc6VMyxcYgC8t60Gg2Q6+T7E+/czLSfwBbq0wFHyXktlk4hzkKqH3+vvTMUGKkPdR/kh7kv5JFXtElFW3zmeyMCrAH9DQeezwjedak2zetQmC32SaLH3T6M9oGylWQWJpM9Pq6szKZrTHnDjnnmJemgBhOfXfwGOtQE= Authentication-Results: kernel.org; dkim=none (message not signed) header.d=none;kernel.org; dmarc=none action=none header.from=amd.com; Received: from SN6PR12MB2718.namprd12.prod.outlook.com (2603:10b6:805:6f::22) by SN6PR12MB2830.namprd12.prod.outlook.com (2603:10b6:805:e0::27) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4195.21; Mon, 7 Jun 2021 13:37:43 +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.4195.030; Mon, 7 Jun 2021 13:37:42 +0000 CC: brijesh.singh@amd.com, devel@edk2.groups.io, James Bottomley , Min Xu , Jiewen Yao , Tom Lendacky , Jordan Justen , Erdem Aktas , Eric Dong , Ray Ni , Rahul Kumar , Ard Biesheuvel Subject: Re: [edk2-devel] [PATCH RFC v3 04/22] OvmfPkg/MemEncryptSevLib: extend Es Workarea to include hv features To: Laszlo Ersek References: <20210526231118.12946-1-brijesh.singh@amd.com> <20210526231118.12946-5-brijesh.singh@amd.com> From: "Brijesh Singh" Message-ID: Date: Mon, 7 Jun 2021 08:37:40 -0500 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.10.2 In-Reply-To: X-Originating-IP: [70.112.153.56] X-ClientProxiedBy: SN1PR12CA0059.namprd12.prod.outlook.com (2603:10b6:802:20::30) 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 (70.112.153.56) by SN1PR12CA0059.namprd12.prod.outlook.com (2603:10b6:802:20::30) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4195.15 via Frontend Transport; Mon, 7 Jun 2021 13:37:41 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 20a44bc6-653a-452c-9c1e-08d929b96d33 X-MS-TrafficTypeDiagnostic: SN6PR12MB2830: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:9508; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: KddokU6QP63YTUS5W8rOb/St5j/ukJxwE8EFkUuZU4YuduEigUgL+cdpUmo1T6gtYuEmH12rWvqcx1f1d+3Ux+1Ilm0b8cSsqK7ZALhG/ZXCQe/zkSpc2szu7ERADsw2k14hkJ4515qqOjmLRcyhH5O26Rp37My6aHp9mIWwnFAb2MRMJi3jeOaKLkD9uyW72iDg5cFCp/kiuSPgUYybroPfBqaW+z+YSgL3HLxqdpMer4jaPOgIMgx4SJMIHZEricPyuD8ZzzJyIqkHHnJSvlSOyCqH7hG8jmne74x8CmLirmK1BUREl+jPGH1fOBFt6CdnUP6OmOGxRIp+O1VLkhGLCsAnaz6Yo8/lMjSoqE/iXiV3arsID1TArhOP5o6uuzR5MqI+h9rHmybgr726P3pWLBhY33mVkqyuXIa2ky64gHAprqE2mEbXx/cLs0M0VYF7xZzyKB1VOuC5z/n8l4QDCTeKMYccXJ3JZh+TKicMuoq1z6CUcRjcdMBvSHeTqe3V4MbU2yFUaN7PXWQJhAJwe8GPGxvlvGK/hzOv2VbQxbPO9UgevqrjufSEFnBQEBi9eZoAjPywoHODT9YHUqb6wndX1LRE3G+5uyHwJVzssN4gUw3SECt5S3Cd5dhXAKL+c7xOW0wizyKYTobIVoTGrMoxZcAI241uGHCy/ondCGVCosaq6wN9IS/V2LDTn56hTsoEcI4ZLyzNj0rbuv1J4yldDKle/gotqv2fYS4xgePqu4Pdpg6+xIg7KJTKNrN8V+P9VFRn4LlzTHUvGUGZe09wIOImGf4/Ib8GY/WIEthrd6mYf8mZ1g51fIqDAYwuVXFtbMuB2rLewGWt9Q== 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)(366004)(346002)(39850400004)(396003)(136003)(376002)(86362001)(54906003)(6486002)(316002)(31686004)(38100700002)(5660300002)(53546011)(30864003)(6506007)(52116002)(6512007)(186003)(8676002)(26005)(6916009)(2906002)(66946007)(66476007)(66556008)(83380400001)(8936002)(16526019)(31696002)(966005)(478600001)(956004)(2616005)(44832011)(36756003)(4326008)(7416002)(38350700002)(19627235002)(45080400002)(45980500001)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?us-ascii?Q?a1dvx8NreBXtQb2Pzs/KiDCHp8sp6lAIdWyul7FWschlL4PcnRWObXCrnKfM?= =?us-ascii?Q?FHysQzRZKPRktl4AbrH0dlLoUi+0LQ5CYU34Cc3CEvSCO5B1Dufwhd696gYY?= =?us-ascii?Q?6ypfpGogemj75Nf7+znR2wqXZug7GOONbrSVSqWF5hOnERAkNzXymdgT6jOM?= =?us-ascii?Q?9+q1tP5NF7XYaLKBuYIPQnXOeDKgJuxfQJ9uGDlPXHOW4PFKmc/2IiX7RbU7?= =?us-ascii?Q?TjN69k7/NbhAlT3GjaG/dWJNyAnxGjmGSYFv5bvUwGfb5y+mru45pxm7NLw/?= =?us-ascii?Q?Ed8YhLVXz2KgXMDnc6EmY6cQ9TMBUQQSfRvMS+YkD/oOYPsGC2wDUKGKJNf7?= =?us-ascii?Q?atsLGIx2/CVfqhn8Xtu+eNFhWDp+xcbUizXPzmhhGaRJp+6q5fdNuc4bBVbX?= =?us-ascii?Q?svlrSI4GhBbvSX5Q0hnB4bufxefBxMq+j2/3p4/pcfMQuoITkHq+Gtvp69Jd?= =?us-ascii?Q?LGsV8GBILCqWR263wRFcRfVCYwdb0qyZ/0+RtfmdL8aLjgR0IksP/+0k1rDJ?= =?us-ascii?Q?h9X0rzYN3ZDAh8NlabQFvWtVta9rTn1Y3cRPp3SZF9F+HEChcxmEL4UZnO8n?= =?us-ascii?Q?WxOTveQ56d+a2uuvMgXGllmfR2XWiyq+EzVB9TSR/CglTQTFfoZ9UrRnEiWS?= =?us-ascii?Q?NGALJQ7oDFfvkNCyrDA+9t8L4W5QiPR5DHqMv0obpl2nuejNuRPiJrWRpoZx?= =?us-ascii?Q?xppwB00LeOT4gSf8y+PlPYo3zVf4y4yEcisIRWqH4kV7+x9vBJeJW0tO4Grr?= =?us-ascii?Q?XTmRY8Paq1OD6eEtvhKx9Gkm32iph7lMAtUdai8xOJ6QTxoSrNRx+cWy5UrN?= =?us-ascii?Q?xrgUn7S7/8WSUvs0kIV/uu5ToLCMYdMKAwul909b0z+5IrIMaMuQrwF0SWvu?= =?us-ascii?Q?xNcDh9yyQ9VQ3Np+sBcz4CiApgYpfxa9wQ2VvnNBYZ1bDhqwD/GzJCigw7lq?= =?us-ascii?Q?G+P/HxI2UDWVQ6HP0wlIp0UzxNn6Q9y2fZB3rt0gCdrGOswzdLsNDx4ZBjdB?= =?us-ascii?Q?TzbcD5on3S8uxJ2V7kFq5VyKyL0xAU1XuLKAyQJkVP9PJBq3cLdoTUq/NXTN?= =?us-ascii?Q?9hdbOEtGNTsLTlI+HA+F/K/sQQ8wMPUEnTvvS7aihq+9JyxuAoubZBYGQulG?= =?us-ascii?Q?4wrowJtE9c9uqD9XfXnHP1KaVLJwcLNxexQ229n7zj9Ub8rfhNjTPyw788gn?= =?us-ascii?Q?wGRbXsaYA6UnJ5pINIbwajE8HKGqvL8ESwRMBtJXDs22BKwcVQ1iojfnqBCf?= =?us-ascii?Q?MUzdO2rI57EH1GbTI5eNmWpDEqxt8drBGpJIhO1l7gFJCHLBZRYi94XBlE6o?= =?us-ascii?Q?3DgR04zwWCMEUysl4Zms+d79?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 20a44bc6-653a-452c-9c1e-08d929b96d33 X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2718.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Jun 2021 13:37:42.8982 (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: gRZgL3AoPkIHkuzcidGIiEeqd+e/9pbDqKMbbIunPLVnXUaLLSP9hODOnExCIMQX4ELJr5S91sSvO50X2iTdlA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR12MB2830 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Content-Language: en-US Hi Laszlo, On 6/7/21 6:54 AM, Laszlo Ersek wrote: > Hi Brijesh, > > On 05/27/21 01:11, Brijesh Singh wrote: >> BZ: https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2F= bugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=3D04%7C01%7Cbrij= esh.singh%40amd.com%7C107d760e06824b1b6d6d08d929aaffd3%7C3dd8961fe4884e608e= 11a82d994e183d%7C0%7C0%7C637586636747299845%7CUnknown%7CTWFpbGZsb3d8eyJWIjo= iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdat= a=3DKQ42j9fOtsJfCh2iziap1v%2BBqC%2FHfR%2BR9ZbO3JHAJaM%3D&reserved=3D0 >> >> The GHCB Version 2 introduces advertisement of features that are support= ed >> by the hypervisor. The features value is saved in the SevEs workarea. Sa= ve >> the value in the PCD for the later use. >> >> Cc: James Bottomley >> Cc: Min Xu >> Cc: Jiewen Yao >> Cc: Tom Lendacky >> Cc: Jordan Justen >> Cc: Ard Biesheuvel >> Cc: Laszlo Ersek >> Cc: Erdem Aktas >> Signed-off-by: Brijesh Singh >> --- >> OvmfPkg/PlatformPei/PlatformPei.inf | 1 + >> OvmfPkg/Include/Library/MemEncryptSevLib.h | 2 + >> OvmfPkg/PlatformPei/AmdSev.c | 26 +++++ >> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 122 +++++++++++++++++++++ >> OvmfPkg/ResetVector/ResetVector.nasmb | 1 + >> 5 files changed, 152 insertions(+) > (1) Please split this patch: the PlatformPei changes should be in the sec= ond patch. Noted. > >> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/P= latformPei.inf >> index bc1dcac48343..3256ccfe88d8 100644 >> --- a/OvmfPkg/PlatformPei/PlatformPei.inf >> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf >> @@ -111,6 +111,7 @@ [Pcd] >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize >> gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled >> gUefiCpuPkgTokenSpaceGuid.PcdSevSnpIsEnabled >> + gUefiCpuPkgTokenSpaceGuid.PcdGhcbHypervisorFeatures >> =20 >> [FixedPcd] >> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress >> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Includ= e/Library/MemEncryptSevLib.h >> index 24507de55c5d..dd1c97d4a9a3 100644 >> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h >> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h >> @@ -55,6 +55,8 @@ typedef struct _SEC_SEV_ES_WORK_AREA { >> UINT64 RandomData; >> =20 >> UINT64 EncryptionMask; >> + >> + UINT64 HypervisorFeatures; >> } SEC_SEV_ES_WORK_AREA; >> =20 >> // >> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c >> index 67b78fd5fa36..81e40e0889aa 100644 >> --- a/OvmfPkg/PlatformPei/AmdSev.c >> +++ b/OvmfPkg/PlatformPei/AmdSev.c >> @@ -43,6 +43,27 @@ AmdSevSnpInitialize ( >> ASSERT_RETURN_ERROR (PcdStatus); >> } >> =20 >> +/** >> + >> + Function to set the PcdHypervisorFeatures. >> +**/ >> +STATIC >> +VOID >> +AmdSevHypervisorFeatures ( >> + VOID >> + ) >> +{ >> + SEC_SEV_ES_WORK_AREA *SevEsWorkArea; >> + RETURN_STATUS PcdStatus; >> + >> + SevEsWorkArea =3D (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWor= kAreaBase); >> + >> + PcdStatus =3D PcdSet64S (PcdGhcbHypervisorFeatures, SevEsWorkArea->Hy= pervisorFeatures); >> + ASSERT_RETURN_ERROR (PcdStatus); >> + >> + DEBUG ((DEBUG_INFO, "GHCB Hypervisor Features=3D0x%Lx\n", SevEsWorkAr= ea->HypervisorFeatures)); > (2) Overlong line. > > Please avoid basic mistakes like this, even in an RFC series. I will split it, but all the checkpatch and CI stuff passed on my branch before the submission. > > >> +} >> + >> /** >> =20 >> Initialize SEV-ES support if running as an SEV-ES guest. >> @@ -73,6 +94,11 @@ AmdSevEsInitialize ( >> PcdStatus =3D PcdSetBoolS (PcdSevEsIsEnabled, TRUE); >> ASSERT_RETURN_ERROR (PcdStatus); >> =20 >> + // >> + // Set the hypervisor features PCD. >> + // >> + AmdSevHypervisorFeatures (); >> + >> // >> // Allocate GHCB and per-CPU variable pages. >> // Since the pages must survive across the UEFI to OS transition >> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVe= ctor/Ia32/PageTables64.asm >> index 6838cdeec9c3..75e63d2a0561 100644 >> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm >> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm >> @@ -62,6 +62,16 @@ BITS 32 >> %define GHCB_CPUID_REGISTER_SHIFT 30 >> %define CPUID_INSN_LEN 2 >> =20 >> +; GHCB SEV Information MSR protocol >> +%define GHCB_SEV_INFORMATION_REQUEST 2 >> +%define GHCB_SEV_INFORMATION_RESPONSE 1 > (3) These macro names do not match the ones in "MdePkg/Include/Register/A= md/Fam17Msr.h" (GHCB_INFO_SEV_INFO_GET, GHCB_INFO_SEV_INFO, respectively). > > They don't *have* to match, technically speaking, but one goal of using m= acros for magic numbers is so we can grep the source for them. The macros j= ust below (for values 128 and 129) do match the header file. Noted, I will match them. > >> + >> +; GHCB Hypervisor features MSR protocol >> +%define GHCB_HYPERVISOR_FEATURES_REQUEST 128 >> +%define GHCB_HYPERVISOR_FEATURES_RESPONSE 129 >> + >> +; GHCB request to terminate protocol values >> +%define GHCB_GENERAL_TERMINATE_REQUEST 255 > (4) Not only does this macro name not match the one in the header file (w= hich is GHCB_INFO_TERMINATE_REQUEST), even the value is wrong. The header f= ile has > > #define GHCB_INFO_TERMINATE_REQUEST 256 > > and I checked the GHCBv2 spec too; there is no operation defined with opc= ode 255. > Ah good catch. Thanks >> =20 >> ; Check if Secure Encrypted Virtualization (SEV) features are enabled. >> ; >> @@ -86,6 +96,13 @@ CheckSevFeatures: >> ; will set it to 1. >> mov byte[SEV_ES_WORK_AREA_SNP], 0 >> =20 >> + ; Set the Hypervisor features field in the workarea to zero to comm= unicate >> + ; to the hypervisor features to the SEC phase. The hypervisor featu= re is >> + ; filled during the call to CheckHypervisorFeatures. >> + mov eax, 0 >> + mov dword[SEV_ES_WORK_AREA_HYPERVISOR_FEATURES], eax >> + mov dword[SEV_ES_WORK_AREA_HYPERVISOR_FEATURES + 4], eax >> + >> ; >> ; Set up exception handlers to check for SEV-ES >> ; Load temporary RAM stack based on PCDs (see SevEsIdtVmmComm for >> @@ -225,6 +242,106 @@ IsSevEsEnabled: >> SevEsDisabled: >> OneTimeCallRet IsSevEsEnabled >> =20 >> +; The version 2 of GHCB specification added the support to query the hy= pervisor features. >> +; If the GHCB version is >=3D2 then read the hypervisor features. >> +; >> +; Modified: EAX, EBX, ECX, EDX >> +; >> +CheckHypervisorFeatures: > (5) Arguably this label name should contain "Sev". Noted. > >> + ; Get the SEV Information >> + ; Setup GHCB MSR >> + ; GHCB_MSR[11:0] =3D SEV information request >> + ; >> + mov edx, 0 >> + mov eax, GHCB_SEV_INFORMATION_REQUEST >> + mov ecx, 0xc0010130 >> + wrmsr >> + >> + ; >> + ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 3= 2-bit >> + ; mode, so work around this by temporarily switching to 64-bit mode= . >> + ; >> +BITS 64 >> + rep vmmcall >> +BITS 32 >> + >> + ; >> + ; SEV Information Response GHCB MSR >> + ; GHCB_MSR[63:48] =3D Maximum protocol version >> + ; GHCB_MSR[47:32] =3D Minimum protocol version >> + ; GHCB_MSR[11:0] =3D SEV information response >> + ; >> + mov ecx, 0xc0010130 >> + rdmsr >> + and eax, 0xfff >> + cmp eax, GHCB_SEV_INFORMATION_RESPONSE >> + jnz TerminateSevGuestLaunch > (6) Before modifying the ResetVector module like this, please insert a re= factoring patch as follows: > > - A new SEV-specific assembly include file should be introduced. The majo= rity of the "OvmfPkg/ResetVector/Ia32/PageTables64.asm" file now deals with= SEV aspects, but the file-top comment still says "Sets the CR3 register fo= r 64-bit paging". It's high time that we move SEV stuff to a file with a na= me that references SEV. > > - We now have five (5) invocations of the GHCB MSR protocol in this file,= and every one of them open-codes the same setup, the same 0xc0010130 MSR c= onstant, the same retval check logic with possible guest termination, the s= ame "rep vmmcall" workaround for the 32-bit limitation of NASM, and so on. = this file is now borderline unreadable. At the minimum, please introduce a = function-like NASM macro with two arguments (=3D the request & response opc= odes), and extract as much as possible. > Okay, I will try moving all the SEV specific stuff outside this file. > >> + shr edx, 16 >> + cmp edx, 2 >> + jl CheckHypervisorFeaturesDone >> + >> + ; Get the hypervisor features >> + ; Setup GHCB MSR >> + ; GHCB_MSR[11:0] =3D Hypervisor features request >> + ; >> + mov edx, 0 >> + mov eax, GHCB_HYPERVISOR_FEATURES_REQUEST >> + mov ecx, 0xc0010130 >> + wrmsr >> + >> + ; >> + ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 3= 2-bit >> + ; mode, so work around this by temporarily switching to 64-bit mode= . >> + ; >> +BITS 64 >> + rep vmmcall >> +BITS 32 >> + >> + ; >> + ; Hypervisor features reponse >> + ; GHCB_MSR[63:12] =3D Features bitmap >> + ; GHCB_MSR[11:0] =3D Hypervisor features response >> + ; >> + mov ecx, 0xc0010130 >> + rdmsr >> + mov ebx, eax >> + and eax, 0xfff >> + cmp eax, GHCB_HYPERVISOR_FEATURES_RESPONSE >> + jnz TerminateSevGuestLaunch >> + >> + shr ebx, 12 >> + mov dword[SEV_ES_WORK_AREA_HYPERVISOR_FEATURES], ebx >> + mov dword[SEV_ES_WORK_AREA_HYPERVISOR_FEATURES + 4], edx > (7) According to the spec, the FEATURES bitmap is a contiguous bitmap of = 52 bits. The way the EDX:EAX qword is shifted right by 12 bits above is inc= orrect. The EAX half is shifted OK (through EBX), but the EDX half is not s= hifted down by 12 bits at all, it is simply stored to the most significant = dword of the "HypervisorFeatures" field. This basically inserts a 12 bit wi= de gap in the FEATURES bitmap. Noted, I will fix it. > >> + >> + jmp CheckHypervisorFeaturesDone >> +TerminateSevGuestLaunch: >> + ; >> + ; Setup GHCB MSR >> + ; GHCB_MSR[23:16] =3D 0 >> + ; GHCB_MSR[15:12] =3D 0 >> + ; GHCB_MSR[11:0] =3D Terminate Request >> + ; >> + mov edx, 0 >> + mov eax, GHCB_GENERAL_TERMINATE_REQUEST > (8) The "MdePkg/Include/Register/Amd/Fam17Msr.h" header file introduces G= HCB_TERMINATE_GHCB, GHCB_TERMINATE_GHCB_GENERAL, GHCB_TERMINATE_GHCB_PROTOC= OL. Can we use some of those here (with a separate, but matching, NASM macr= o of course)? See SevEsProtocolFailure() in "OvmfPkg/Sec/SecMain.c". Sure, while moving SEV bits in a separate file I will try to define a matching macros for it. > > >> + mov ecx, 0xc0010130 >> + wrmsr >> + >> + ; >> + ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 3= 2-bit >> + ; mode, so work around this by temporarily switching to 64-bit mode= . >> + ; >> +BITS 64 >> + rep vmmcall >> +BITS 32 >> + >> +TerminateSevGuestLaunchHlt: >> + cli >> + hlt >> + jmp TerminateSevGuestLaunchHlt >> + >> +CheckHypervisorFeaturesDone: >> + OneTimeCallRet CheckHypervisorFeatures >> + >> ; >> ; Modified: EAX, EBX, ECX, EDX >> ; >> @@ -328,6 +445,11 @@ clearGhcbMemoryLoop: >> mov dword[ecx * 4 + GHCB_BASE - 4], eax >> loop clearGhcbMemoryLoop >> =20 >> + ; >> + ; It is SEV-ES guest, query the Hypervisor features >> + ; >> + OneTimeCall CheckHypervisorFeatures >> + >> SetCr3: >> ; >> ; Set CR3 now that the paging structures are available >> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector= /ResetVector.nasmb >> index 1971557b1c00..5beba3ecb290 100644 >> --- a/OvmfPkg/ResetVector/ResetVector.nasmb >> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb >> @@ -76,6 +76,7 @@ >> %define SEV_ES_WORK_AREA_SNP (FixedPcdGet32 (PcdSevEsWorkAreaBase) + = 1) >> %define SEV_ES_WORK_AREA_RDRAND (FixedPcdGet32 (PcdSevEsWorkAreaBase)= + 8) >> %define SEV_ES_WORK_AREA_ENC_MASK (FixedPcdGet32 (PcdSevEsWorkAreaBas= e) + 16) >> + %define SEV_ES_WORK_AREA_HYPERVISOR_FEATURES (FixedPcdGet32 (PcdSevEs= WorkAreaBase) + 24) >> %define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBa= se) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize)) >> %include "Ia32/Flat32ToFlat64.asm" >> %include "Ia32/PageTables64.asm" >> > (9) And, I'm arriving at the following assertion only now unfortunately, = after spending about 4 hours on reviewing this patch, and the history of SE= C_SEV_ES_WORK_AREA. > > I assert that the "SEC_SEV_ES_WORK_AREA.HypervisorFeatures" field should = not exist. The only read site is here, in "OvmfPkg/PlatformPei". I choose to cache as much as I could during the reset vector time so that we avoid the need for VmgExit later. But now I understand that you prefer to query those information when they are needed and not use the SevEsWorkArea as a cache for other information. Also, I was trying to avoid the cases where the malicious hypervisor changing the feature value after the GHCB negotiation is completed.=C2=A0 e.g, during the reset vector they give us one feature value and change them during SEC or PEI or DXE instances run. They can't break the security of SNP by doing so, but I thought its good to avoid querying the features on every phase. > > Instead, we should have a new MemEncryptSevLib function that outputs the = FEATURES bitmask. It should be similar to MemEncryptSevGetEncryptionMask(),= but it should return a RETURN_STATUS, and produce the FEATURES bitmask thr= ough an output parameter. This is one of thing which I noticed last week, we are actually creating a circular dependency. The MemEncryptSevLib provides the routines which are used by the VmgExitLib. The MemEncryptSevLib should *not* depend on the VmgExitLib. If we extend the MemEncryptSevLib to provide a routine to query the features then we will be required to include the VmgExitLib. The patch #13 extends the MemEncryptSevLib to provide functions for the page validation and it requires the VmgExitLib. I am trying to see what I can do to not create this circular dependency and still keep code reuse. Thanks Brijesh > > The SEC instance of the function should return RETURN_UNSUPPORTED. > > The PEI instance should use the GHCB MSR protocol, with the help of the A= smCpuId(), AsmWriteMsr64(), AsmReadMsr64() and AsmVmgExit() BaseLib functio= ns. > > The DXE instance should read back the PCD. > > And so the OvmfPkg/ResetVector hunks should be dropped from this patch. > > Thanks, > Laszlo >