From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM03-CO1-obe.outbound.protection.outlook.com (NAM03-CO1-obe.outbound.protection.outlook.com [40.107.79.73]) by mx.groups.io with SMTP id smtpd.web11.30332.1574370718952432297 for ; Thu, 21 Nov 2019 13:11:59 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@amdcloud.onmicrosoft.com header.s=selector2-amdcloud-onmicrosoft-com header.b=mF73r6Y6; spf=none, err=SPF record not found (domain: amd.com, ip: 40.107.79.73, mailfrom: thomas.lendacky@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Gg+ubLtZxsPpMr6SEs2p71ufRlPEy+dnbjqqzh+o4ZnPcX1siHr+dFH08EZ1BGwIaCeWiZyAEfIs3zITqU/IVtMr39AVPwKR+VcmFyNrxbpNCeanbZkwIp8iPlo94vejl66s5HaUMqxe44OsngrwNPxuRWYt9FRHkqpXt7jy+BoxkUtJj+cMHf3/0kqukKngoNWHmKNAqvnp2Rq4UMWX8VQUG+mh1s/w0JHOHEpxd9P0qcmMeA37Xn/D0/QYadkMxZ3nbNmUJ21Uwg2gOEoh4EqZktVqugkc1qBN8opEXNltVY6/qS9djKdwhNAKJQbJOXKcSjewOxrHGuDahu+PfQ== 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=fe/Rs09lEFEUlyN6PqeM0Yh/OnPHYObvNxfYNagqGhc=; b=YNPfF8eNPXWljqFVZ7U1yCrSF7HWs2V0wAMSoZidpbhiKk8ytjfrSiimxtZOgZD56dbtrAnXKiqSoAAy/kErauqH1c5Bvf+KnRwWCU3WbjDjGSQEhZGrRWsHWr/1d2cvv8+iJOlbApQfl24xEmO3whE8krAOmDbQ7s3FISoZ3ztiiihWVw9C9UAm2d++PjX5+e7UNT+H7WYijctEJECm9eFZpd3AQQ41D7o2sO982/xalmfIxDxjJtnrQ7xLuyrMwPEpoOU2yNj8nebuBIDEFGWGd29NMCER0DF+FnweNzxheXfRc6Dk4YMIkI+l87LbjZ8LutQdJnGvXZC5FTlpTQ== 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=amdcloud.onmicrosoft.com; s=selector2-amdcloud-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=fe/Rs09lEFEUlyN6PqeM0Yh/OnPHYObvNxfYNagqGhc=; b=mF73r6Y6AaUZ4nJZS+4ycD3o6JHJdawLMffyQ+yy9g6RVaE0VcI/AlIA/BRVRwrXYEnMWN3RIbpJ9iY+vclPaW6DHFE798sEiTZAlRz95reMXwJTpuKWjn7ks41AhkAXq1geXPPHEeqpe/Uw81iFwi7MAQ9LrCXfrHiseF1D29E= Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=Thomas.Lendacky@amd.com; Received: from DM6PR12MB3163.namprd12.prod.outlook.com (20.179.71.154) by DM6PR12MB3946.namprd12.prod.outlook.com (10.255.174.155) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2474.19; Thu, 21 Nov 2019 21:11:55 +0000 Received: from DM6PR12MB3163.namprd12.prod.outlook.com ([fe80::dd0c:8e53:4913:8ef4]) by DM6PR12MB3163.namprd12.prod.outlook.com ([fe80::dd0c:8e53:4913:8ef4%5]) with mapi id 15.20.2474.019; Thu, 21 Nov 2019 21:11:55 +0000 Subject: Re: [edk2-devel] [RFC PATCH v3 32/43] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES is enabled To: devel@edk2.groups.io, lersek@redhat.com Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , Brijesh Singh References: <9a6a012c-9bdd-b3cb-65b3-79fda0158e88@redhat.com> From: "Lendacky, Thomas" Openpgp: preference=signencrypt Message-ID: <3b5f9e44-2d41-23ca-ae58-b445094e3e26@amd.com> Date: Thu, 21 Nov 2019 15:11:53 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 In-Reply-To: <9a6a012c-9bdd-b3cb-65b3-79fda0158e88@redhat.com> X-ClientProxiedBy: SN6PR01CA0022.prod.exchangelabs.com (2603:10b6:805:b6::35) To DM6PR12MB3163.namprd12.prod.outlook.com (2603:10b6:5:15e::26) Return-Path: thomas.lendacky@amd.com MIME-Version: 1.0 X-Originating-IP: [165.204.77.1] X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-HT: Tenant X-MS-Office365-Filtering-Correlation-Id: a452629c-4042-4548-8e98-08d76ec76fc5 X-MS-TrafficTypeDiagnostic: DM6PR12MB3946:|DM6PR12MB3946: X-MS-Exchange-PUrlCount: 4 X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:10000; X-Forefront-PRVS: 0228DDDDD7 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(4636009)(366004)(396003)(346002)(136003)(376002)(39860400002)(199004)(189003)(6512007)(6306002)(66556008)(50466002)(305945005)(7736002)(19627235002)(14444005)(23746002)(31696002)(6486002)(66476007)(66946007)(229853002)(6436002)(86362001)(8936002)(58126008)(54906003)(316002)(26005)(8676002)(81166006)(5660300002)(81156014)(31686004)(52116002)(65806001)(66066001)(186003)(2616005)(11346002)(478600001)(25786009)(47776003)(446003)(36756003)(76176011)(6116002)(3846002)(53546011)(2906002)(99286004)(386003)(6506007)(4326008)(65956001)(966005)(230700001)(14454004)(6246003)(213903007);DIR:OUT;SFP:1101;SCL:1;SRVR:DM6PR12MB3946;H:DM6PR12MB3163.namprd12.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX:1; Received-SPF: None (protection.outlook.com: amd.com does not designate permitted sender hosts) X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: bjtigSg8h5ZxOudNa1w7qamiuATl9ZF3hoGCKd4Pl81WN56lAKrr+HvKQlo0+8fLyOHWkimwBDETQKcNpF9FJMx6sEx6Kt7vKf9lqsnwaQTMiixBvk/MNJnjay8JlO2ZkEUcVZWMYscDOFrzPxgqk+3iPYKA97s3O+nmZORcgnhDXXLmJ8Bol1Us/2mngdZZAwQ+/UOWaTGnHc+RT7RQZFFYanPoRRaI2gF180PtLWp1Kv5/qyFOamDePnFcml4wohjoK28QHyePaUXeCkHuGO82m2+zmEBNLEJSOdkkbbDF0wjRXshKjrsN0BFbBakCAlqcfWj9asv/N5n08MPRapF42ocF5y6+ygJCw/VvMxaCWIiWFCC8jJDjhfGEdt6dVunqDMDSeeGFoetNPa6SlfO8Jv//J210xYa0LoAy0dqujqad9F0qfiEqE6DF2vXOdqmGN6FahNd+snoxa2ALxCQfnLYh9H5raKCdZmTuUYs= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: a452629c-4042-4548-8e98-08d76ec76fc5 X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Nov 2019 21:11:55.1238 (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: +A+2Phcxi8l7sM0PEFe83EZVfaXFOp1oCiubSxD/9aRb3lSfFzDMwoC+fUWrnY1euUAE5bVkUsUHlTMhYdKr4w== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB3946 Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/21/19 6:31 AM, Laszlo Ersek via Groups.Io wrote: > On 11/20/19 21:06, Lendacky, Thomas wrote: >> The flash detection routine will attempt to determine how the flash >> device behaves (e.g. ROM, RAM, Flash). But when SEV-ES is enabled and >> the flash device behaves as a ROM device (meaning it is marked read-only >> by the hypervisor), this check may result in an infinite nested page fault >> because of the attempted write. Since the instruction cannot be emulated >> when SEV-ES is enabled, the RIP is never advanced, resulting in repeated >> nested page faults. >> >> When SEV-ES is enabled, exit the flash detection early and assume that >> the FD behaves as Flash. This will result in QemuFlashWrite() being called >> to store EFI variables, which will also result in an infinite nested page >> fault when the write is performed. In this case, update QemuFlashWrite() >> to use the VmgMmioWrite function from the VmgExitLib library to have the >> hypervisor perform the write without having to emulate the instruction. > > (1) Please include the Bugzilla link in the commit message: > > https://bugzilla.tianocore.org/show_bug.cgi?id=2198 Yup, will do. I'm also missing the Cc:'s, so I'll add those as well. > >> >> Signed-off-by: Tom Lendacky >> --- >> OvmfPkg/OvmfPkgIa32.dsc | 1 + >> OvmfPkg/OvmfPkgIa32X64.dsc | 1 + >> OvmfPkg/OvmfPkgX64.dsc | 1 + >> .../FvbServicesRuntimeDxe.inf | 2 + >> .../QemuFlash.c | 38 +++++++++++++++++-- >> 5 files changed, 40 insertions(+), 3 deletions(-) >> >> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc >> index 56670eefde6b..ff2814c6246e 100644 >> --- a/OvmfPkg/OvmfPkgIa32.dsc >> +++ b/OvmfPkg/OvmfPkgIa32.dsc >> @@ -320,6 +320,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER] >> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf >> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf >> QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf >> + VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf >> >> [LibraryClasses.common.UEFI_DRIVER] >> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf >> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc >> index 9897e6889573..212952cfaacd 100644 >> --- a/OvmfPkg/OvmfPkgIa32X64.dsc >> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc >> @@ -325,6 +325,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER] >> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf >> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf >> QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf >> + VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf >> >> [LibraryClasses.common.UEFI_DRIVER] >> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf >> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >> index 59c4f9207fc3..8331fc0b663e 100644 >> --- a/OvmfPkg/OvmfPkgX64.dsc >> +++ b/OvmfPkg/OvmfPkgX64.dsc >> @@ -325,6 +325,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER] >> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf >> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf >> QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf >> + VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf >> >> [LibraryClasses.common.UEFI_DRIVER] >> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf >> index ca6326e833ed..0b7741ac07f8 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf >> @@ -38,6 +38,7 @@ [Sources] >> [Packages] >> MdePkg/MdePkg.dec >> MdeModulePkg/MdeModulePkg.dec >> + UefiCpuPkg/UefiCpuPkg.dec >> OvmfPkg/OvmfPkg.dec >> >> [LibraryClasses] >> @@ -52,6 +53,7 @@ [LibraryClasses] >> UefiBootServicesTableLib >> UefiDriverEntryPoint >> UefiRuntimeLib >> + VmgExitLib >> >> [Guids] >> gEfiEventVirtualAddressChangeGuid # ALWAYS_CONSUMED > > These hunks look good. > >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c >> index c81c58972bf2..4f3bf690fcad 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c >> @@ -9,7 +9,9 @@ >> >> #include >> #include >> +#include >> #include >> +#include >> >> #include "QemuFlash.h" >> >> @@ -80,6 +82,20 @@ QemuFlashDetected ( >> >> DEBUG ((EFI_D_INFO, "QEMU Flash: Attempting flash detection at %p\n", Ptr)); >> >> + if (MemEncryptSevEsIsEnabled()) { >> + // >> + // When SEV-ES is enabled, the check below can result in an infinite >> + // loop with respect to a nested page fault. When the FD behaves as >> + // a ROM, the nested page table entry is read-only. The check below > > (2) I suggest a slight change to the wording above. Please replace > > when the FD behaves as a ROM > > with > > when the memslot is mapped read-only > > (It's OK to make qemu / kvm references in this code: the name of the > file includes "Qemu".) Ok. > >> + // will cause a nested page fault that cannot be emulated, causing >> + // the instruction to retried over and over. For SEV-ES, assume that >> + // the FD does not behave as FLASH. > > (3) I would recommend > > For SEV-ES, acknowledge that the FD appears as ROM and not as FLASH, > but report FLASH anyway. How about if I expand on this to add "because FLASH behavior can be simulated using VMGEXIT.", too. > >> + // >> + DEBUG ((DEBUG_INFO, >> + "QEMU Flash: SEV-ES enabled, assuming FD behaves as FLASH\n")); >> + return TRUE; >> + } >> + >> OriginalUint8 = *Ptr; >> *Ptr = CLEAR_STATUS_CMD; >> ProbeUint8 = *Ptr; >> @@ -147,6 +163,21 @@ QemuFlashRead ( >> } >> >> >> +STATIC >> +VOID >> +QemuFlashPtrWrite ( >> + IN volatile UINT8 *Ptr, >> + IN UINT8 Value >> + ) >> +{ >> + if (MemEncryptSevEsIsEnabled()) { >> + VmgMmioWrite ((UINT8 *) Ptr, &Value, 1); >> + } else { >> + *Ptr = Value; >> + } >> +} >> + >> + >> /** >> Write to QEMU Flash >> > > (4) This change will break the "FvbServicesSmm.inf" build of this module. > > (Because, you have -- correctly -- added the VmgExitLib class to > "FvbServicesRuntimeDxe.inf" only.) > > Please: > > - create two implementations of the QemuFlashPtrWrite() function, one in > "QemuFlashDxe.c", and another in "QemuFlashSmm.c". > > - the former should be the function shown above, the latter should only > perform the direct assignment. > > - the '#include ' directive should also be moved > to "QemuFlashDxe.c". > > - the prototype for QemuFlashPtrWrite() should be added to "QemuFlash.h" > please. Ah, nice catch. I'll do that. Thanks, Tom > >> @@ -181,8 +212,9 @@ QemuFlashWrite ( >> // >> Ptr = QemuFlashPtr (Lba, Offset); >> for (Loop = 0; Loop < *NumBytes; Loop++) { >> - *Ptr = WRITE_BYTE_CMD; >> - *Ptr = Buffer[Loop]; >> + QemuFlashPtrWrite (Ptr, WRITE_BYTE_CMD); >> + QemuFlashPtrWrite (Ptr, Buffer[Loop]); >> + >> Ptr++; >> } >> >> @@ -190,7 +222,7 @@ QemuFlashWrite ( >> // Restore flash to read mode >> // >> if (*NumBytes > 0) { >> - *(Ptr - 1) = READ_ARRAY_CMD; >> + QemuFlashPtrWrite (Ptr - 1, READ_ARRAY_CMD); >> } >> >> return EFI_SUCCESS; >> > > With (1) and (4) fixed, and with (2) and (3) optionally fixed (i.e., if > you agree): > > Reviewed-by: Laszlo Ersek > > Thanks > Laszlo > > > >