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.83]) by mx.groups.io with SMTP id smtpd.web08.445.1620755111076890159 for ; Tue, 11 May 2021 10:45:11 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@amd.com header.s=selector1 header.b=0MRrmDUR; 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.83, mailfrom: brijesh.singh@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=DkaCTRp7SFAS0AsLEEtEnekMuVCLV3NvlhNiIkWYm+lJIr8+71t4F8oo8RZWVvw9a+ebzXEFyyoHCx325C6TJHuqFROFaMua+Nve6ZhSKVGfe3FhJXsWeJvinzL48+FFvO5k5lD4wwVDWp35FMmcnC7lC+dYXBYguVqXfqwCRX9FbBhs2VUmj6qxAyrVkC1+eXqCP5eMMiznPnHIuG9MlPUapcw/8mZrQXsFzXl2u0/vobWz2db22KUUCnppSPc46PpajkTwBx7+KVBbnEGUC/uKGbP7Xg7RUJmQ0q+1HsL1RmQsv8nX7gCCj/pyTK/CC5oXbgoAKsm1dkC0vhZN4Q== 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=MacMx9fXINWOOUDZwgv/IeovUeNamgkO4mf3wwU9Vx8=; b=hPju28dKSrdUSBwRJDwKuV+Gg/hv8z+rmYtORVTgYxD6LypPi63PFD1pDbA8hX+nnUXROZQWKlpM3jBaLTylV5xfdpX2xK2D9m788vWbhGOqnQdvtltXMOq7gLfOWuaxV8BxQAmmEW4SFQypsr8bdHB6K5j6iWGnLYlYkUbdVLNNNhbB+7iKefWqZgx3NSI0eVeLLIKzfSw8yi4/m9FhmU3THKzbP46y0cJXU+SoOemyzSObr17f30gX6qCRIGHdnkNBUAzMHMrGA6aPXtZORm1vxbc4yYMWt6+d09Yjb4Z3VBaNnlRfjsqgMN8Mr+g/hpcdpdelkdcx1RzXA1foNQ== 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=MacMx9fXINWOOUDZwgv/IeovUeNamgkO4mf3wwU9Vx8=; b=0MRrmDUR95EQo661GqrAtcvP7VnV319dyqBZLPvIdxAIUXBdi2WJyGAf8oBTlpEudkD4NtzA+X6uBFxDiuq4VvRzzq6gG86cU2P5mV3dvQwNmpQ66j23ziWsDU2fbYrLVPQ+ZkINQ+OZhVptpPF024mXTVR3B0HgrYGOPpWlUR0= Authentication-Results: google.com; dkim=none (message not signed) header.d=none;google.com; dmarc=none action=none header.from=amd.com; Received: from SN6PR12MB2718.namprd12.prod.outlook.com (2603:10b6:805:6f::22) by SN1PR12MB2445.namprd12.prod.outlook.com (2603:10b6:802:31::24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4108.26; Tue, 11 May 2021 17:45:09 +0000 Received: from SN6PR12MB2718.namprd12.prod.outlook.com ([fe80::9898:5b48:a062:db94]) by SN6PR12MB2718.namprd12.prod.outlook.com ([fe80::9898:5b48:a062:db94%6]) with mapi id 15.20.4129.025; Tue, 11 May 2021 17:45:09 +0000 CC: brijesh.singh@amd.com, James Bottomley , Min Xu , Jiewen Yao , Tom Lendacky , Jordan Justen , Ard Biesheuvel , Erdem Aktas Subject: Re: [edk2-devel] [PATCH 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter To: Laszlo Ersek , devel@edk2.groups.io References: <20210507203838.23706-1-brijesh.singh@amd.com> <20210507203838.23706-14-brijesh.singh@amd.com> <3c85698e-0b5e-38b0-f752-30204cb78cad@redhat.com> From: "Brijesh Singh" Message-ID: <41c63c63-554d-1f7d-5d27-bd649261e897@amd.com> Date: Tue, 11 May 2021 12:45:07 -0500 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 In-Reply-To: <3c85698e-0b5e-38b0-f752-30204cb78cad@redhat.com> X-Originating-IP: [70.112.153.56] X-ClientProxiedBy: SA9PR13CA0071.namprd13.prod.outlook.com (2603:10b6:806:23::16) 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 SA9PR13CA0071.namprd13.prod.outlook.com (2603:10b6:806:23::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4129.12 via Frontend Transport; Tue, 11 May 2021 17:45:08 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 5cbe1097-fbf2-4b14-18d2-08d914a48504 X-MS-TrafficTypeDiagnostic: SN1PR12MB2445: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:8882; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: cKhSRYzcna3ON+Irq8SRcRyyZ4CbcSWa2+Yr70oZU68GaQyYkfJMeiQYuXhZJS2Uu3qYx36LYS59fVczqY+HDDwusKZ//xiXs2NghdS0OqwVbOw3YyP1xfDa6In5P3/C3iKehiWcvp3oTWCIG40kxV4RHhFQv0ODybfNcPpSb0XLRdL4g/MQon1fFoyblxrRW1sbvtWJN0IosGQoFU9Re69V0ZUl4en2PsIwNarBwWuGLvwcY3o14N13Xq4gAWRIQFs4CsEBG+GnkBpy+0akYdX4JX4gG20TyKpcBz1baj6cJO1Ndl7QHOkv7O/04WVbiYC5vfAlUYe1LFE90Bc72wrKeFHD9V94gi9CeedtGlXKVLQtAEiuU5B7M5KsTLRcYR7Z/NXVivBRAZ6CrqbJkqGm+Cjo6837Yibs/JPuJ6hx0sZccVFvfgrXcNK79tgXNU/meUNlDI8LKOfI7sz0EhFn9xvyvj65bXKzbd+x7MpT3Lzpm7RxtxcCM5mK027C51ocO1C6xR/gTO7gVjWKhxTKT7PvsUzl8T6vkAJmvOt5+U/ISTnNKeDLOKvRzqB2jqV55cLyDsEiu7Nr/OgL2AelwyozPEKG4lh9vTsT2Ufm8gaTOhnmh9pMiS0qGP2W4N/z6SqYTkwOtop+252wf/2wC5sY/og+L+bwnh39R1APRwQxi4Jf7KUPlJ3G9AcmMqCQca/F6G1OrzbEZvfZkhO3s09+623HUyxLOLBQW6Z3s7v1Da4iPpTx4erH3up1QVV/HsliHozw38mbXAYjuw9gpM6HYhMaLuNl+7jU3LejuRYPgLmdLDw/2721WdA/vNhu+/vrDznr7/bwvZjm6Q== 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)(39860400002)(376002)(136003)(346002)(396003)(366004)(45080400002)(6486002)(54906003)(2616005)(8936002)(478600001)(19627235002)(53546011)(6506007)(44832011)(30864003)(2906002)(966005)(8676002)(5660300002)(956004)(52116002)(83380400001)(86362001)(31686004)(66946007)(36756003)(26005)(38100700002)(16526019)(38350700002)(31696002)(4326008)(316002)(186003)(6512007)(66476007)(66556008)(213903007)(43740500002)(45980500001);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?us-ascii?Q?MfjUGxbw27dPxkSK6hERDad5MqXnwTx8Ky3N3WN7HMW48Ih0qFltjZpF2+vE?= =?us-ascii?Q?ZdA5+lrgT1XPlvgpstK6RhDtTpq5B/JxtxNf9LCiy8qVEhGQBDY9123xMceB?= =?us-ascii?Q?+NZCLAqFhuRrGgvaGUYr8TY4S/ld+o/3ZW/i7+I/h4ejSZrBt2hNspx/5Hrr?= =?us-ascii?Q?Bgt/iqPN/dxQVURVCSM9rzurPuBzpoYZHm3beoYWy4SxSt6BzvRUZkTaP2hu?= =?us-ascii?Q?q9AUw8FBq7SNJxr3khBp/mdeIMV15P36KiAOc5MRSJOOJYtaXqyrWtqIjS27?= =?us-ascii?Q?zo1KBpp8rQiEQnAdwWk8bqiaEwCOQF1zf7YWKhfv0eOQILum7VigGlWz1zI2?= =?us-ascii?Q?2HpsCZL94JZ24ckQu1GNrCBSRK5+ocubEogfkNYu8pAv1E4JOcBMkiY3ZMtG?= =?us-ascii?Q?1mZ0gqCRlur900eciJjVCxTM5dikvlI6XPX2SoJ9292mKPiAd0ErnzwLrpzw?= =?us-ascii?Q?pGx/rcbcTAM4eld/Z2kL/R6HHbjQkOcAL1UUnUKbnaVKs6+QDXdR4/Ulcpgp?= =?us-ascii?Q?NW3lPA0+xnKcR3MZwbpvW7q2Ltoy0gW+tT+UfAOJ1LZFQ7yL4f3yy6CFf++Z?= =?us-ascii?Q?JgdfICN3ylW+yK4cnGkjtf0tJDhCDySMX8U8gE5Kv05QzqOJ+Kyt13Fcbosl?= =?us-ascii?Q?0VwHLLtR+9VnwrnkxxpTQqVLkKRf2qkBIXYFrux3dwjYdjmEt6jn+TLlA9Fr?= =?us-ascii?Q?2hsOmIALD5okZbByvtyegsftLoAJi0ps+mm9xZsL9lQpSorcryy6dJlMGuLD?= =?us-ascii?Q?ZaGdS88wU+7LskX7G5IZrLY1W0CnmVHw+xPsLn2ojkGi26mZnBJ2Y5c4je6z?= =?us-ascii?Q?fuMTwzXo19hjwNtMF6Jqt8NPHsTD3XXp2L75hVnRUphu7PIc2R6Qj5I2IUBe?= =?us-ascii?Q?iyDGDUKL7eWaQ6GPp7cECqZEfCkHlCzed6b7SjB2UnE9ahcTIauliDPu+70j?= =?us-ascii?Q?JHJwza2Ml8uRKA0pJqe/LSMX1FMZPVhA27MlMatlmbPPXuIL7eERe0Olowfi?= =?us-ascii?Q?jaUcbAe382jiwJQS4JuPCd4bsZXIUc6IUjSO7JEr0D25OKauTTxllC8mjM4g?= =?us-ascii?Q?/erE32vyosR4MUNusNY4omFNIhfgJ99metrdBZG4vXA9Gt/Jf58jYJqqEr6D?= =?us-ascii?Q?HXXpg2sblhwDODiAt+chnCPgYyX2WmID0sq6pN436KV823RYSFBYfc8P5wP+?= =?us-ascii?Q?etwzo3140Ol8igYg8MPAqI3plzfw+JBY4vwbueZoI+q7RhDaiu9jKtlJVsrb?= =?us-ascii?Q?bHhSJsM8cKUAkdTn5J5jjsUiJVzX0wsOtugONv4mITxmMV+4Cxz+aHpHCiQv?= =?us-ascii?Q?KQMWkp9lv5nI8mxxFArKzsOX?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 5cbe1097-fbf2-4b14-18d2-08d914a48504 X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2718.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 May 2021 17:45:08.9835 (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: CpFpSXmXcOKggwcD7raj9UemI8BBn4nBFfgn3T/GKJFNvGg6G9O8HkgLRzNYKMf9BY9Hj6iPcClFzi9m0XnRXw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR12MB2445 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Content-Language: en-US On 5/11/21 6:55 AM, Laszlo Ersek wrote: > I don't fully understand the updates in this patch: > > On 05/07/21 22:38, 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%7Cc383d8fdc1264644760508d91473b003%7C3dd8961fe4884e608e= 11a82d994e183d%7C0%7C0%7C637563309382960811%7CUnknown%7CTWFpbGZsb3d8eyJWIjo= iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdat= a=3DM3xHM2yU0m3VtPn1xGe1k5Wq0d6Vbdf9gMqDX1NxpgA%3D&reserved=3D0 >> >> The Flush parameter is used to provide a hint whether the specified rang= e >> is Mmio address. Now that we have a dedicated helper to clear the >> memory encryption mask for the Mmio address range, its safe to remove th= e >> Flush parameter from MemEncryptSev{Set,Clear}PageEncMask(). > This looks good; it matches my request (1) from: > > https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Flistm= an.redhat.com%2Farchives%2Fedk2-devel-archive%2F2021-May%2Fmsg00109.html&am= p;data=3D04%7C01%7Cbrijesh.singh%40amd.com%7Cc383d8fdc1264644760508d91473b0= 03%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563309382960811%7CUnknow= n%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI= 6Mn0%3D%7C1000&sdata=3D3TQrliMABDaa%2FtCN%2FvKewTmLfVRKIou2La5yRGGyzJY%= 3D&reserved=3D0 > >> 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/Include/Library/MemEncryptSevLib.h | 10 ++---- >> .../BaseMemEncryptSevLib/X64/VirtualMemory.h | 10 ++---- >> OvmfPkg/AmdSevDxe/AmdSevDxe.c | 3 +- >> OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 6 ++-- >> .../Ia32/MemEncryptSevLib.c | 10 ++---- >> .../X64/MemEncryptSevLib.c | 16 +++------- >> .../X64/PeiDxeVirtualMemory.c | 32 +++++++++++-------- >> .../X64/SecVirtualMemory.c | 8 ++--- >> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 3 +- >> OvmfPkg/PlatformPei/AmdSev.c | 3 +- >> 10 files changed, 35 insertions(+), 66 deletions(-) >> >> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Includ= e/Library/MemEncryptSevLib.h >> index b91490d5d44d..76d06c206c8b 100644 >> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h >> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h >> @@ -100,8 +100,6 @@ MemEncryptSevIsEnabled ( >> address of a memory region. >> @param[in] NumPages The number of pages from start me= mory >> region. >> - @param[in] Flush Flush the caches before clearing = the bit >> - (mostly TRUE except MMIO addresse= s) >> =20 >> @retval RETURN_SUCCESS The attributes were cleared for t= he >> memory region. >> @@ -114,8 +112,7 @@ EFIAPI >> MemEncryptSevClearPageEncMask ( >> IN PHYSICAL_ADDRESS Cr3BaseAddress, >> IN PHYSICAL_ADDRESS BaseAddress, >> - IN UINTN NumPages, >> - IN BOOLEAN Flush >> + IN UINTN NumPages >> ); >> =20 >> /** >> @@ -128,8 +125,6 @@ MemEncryptSevClearPageEncMask ( >> address of a memory region. >> @param[in] NumPages The number of pages from start me= mory >> region. >> - @param[in] Flush Flush the caches before setting t= he bit >> - (mostly TRUE except MMIO addresse= s) >> =20 >> @retval RETURN_SUCCESS The attributes were set for the m= emory >> region. >> @@ -142,8 +137,7 @@ EFIAPI >> MemEncryptSevSetPageEncMask ( >> IN PHYSICAL_ADDRESS Cr3BaseAddress, >> IN PHYSICAL_ADDRESS BaseAddress, >> - IN UINTN NumPages, >> - IN BOOLEAN Flush >> + IN UINTN NumPages >> ); >> =20 >> =20 >> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/= OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h >> index 8dc39e647b90..21bbbd1c4f9c 100644 >> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h >> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h >> @@ -58,8 +58,6 @@ InternalGetMemEncryptionAddressMask ( >> @param[in] PhysicalAddress The physical address that is the = start >> address of a memory region. >> @param[in] Length The length of memory region >> - @param[in] Flush Flush the caches before applying = the >> - encryption mask >> =20 >> @retval RETURN_SUCCESS The attributes were cleared for t= he >> memory region. >> @@ -72,8 +70,7 @@ EFIAPI >> InternalMemEncryptSevSetMemoryDecrypted ( >> IN PHYSICAL_ADDRESS Cr3BaseAddress, >> IN PHYSICAL_ADDRESS PhysicalAddress, >> - IN UINTN Length, >> - IN BOOLEAN Flush >> + IN UINTN Length >> ); >> =20 >> /** >> @@ -85,8 +82,6 @@ InternalMemEncryptSevSetMemoryDecrypted ( >> @param[in] PhysicalAddress The physical address that is the = start >> address of a memory region. >> @param[in] Length The length of memory region >> - @param[in] Flush Flush the caches before applying = the >> - encryption mask >> =20 >> @retval RETURN_SUCCESS The attributes were set for the m= emory >> region. >> @@ -99,8 +94,7 @@ EFIAPI >> InternalMemEncryptSevSetMemoryEncrypted ( >> IN PHYSICAL_ADDRESS Cr3BaseAddress, >> IN PHYSICAL_ADDRESS PhysicalAddress, >> - IN UINTN Length, >> - IN BOOLEAN Flush >> + IN UINTN Length >> ); >> =20 >> /** >> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe= .c >> index 80831b81facf..41e4b291d070 100644 >> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c >> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c >> @@ -120,8 +120,7 @@ AmdSevDxeEntryPoint ( >> Status =3D MemEncryptSevClearPageEncMask ( >> 0, // Cr3BaseAddress -- use current CR3 >> MapPagesBase, // BaseAddress >> - MapPagesCount, // NumPages >> - TRUE // Flush >> + MapPagesCount // NumPages >> ); >> if (EFI_ERROR (Status)) { >> DEBUG ((DEBUG_ERROR, "%a: MemEncryptSevClearPageEncMask(): %r\n", > (1) You missed my comment (2) in > . Ah, I will fix in rev2. > >> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoM= mu.c >> index 49ffa2448811..b30628078f73 100644 >> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c >> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c >> @@ -252,8 +252,7 @@ IoMmuMap ( >> Status =3D MemEncryptSevClearPageEncMask ( >> 0, >> MapInfo->PlainTextAddress, >> - MapInfo->NumberOfPages, >> - TRUE >> + MapInfo->NumberOfPages >> ); >> ASSERT_EFI_ERROR (Status); >> if (EFI_ERROR (Status)) { >> @@ -407,8 +406,7 @@ IoMmuUnmapWorker ( >> Status =3D MemEncryptSevSetPageEncMask ( >> 0, >> MapInfo->PlainTextAddress, >> - MapInfo->NumberOfPages, >> - TRUE >> + MapInfo->NumberOfPages >> ); >> ASSERT_EFI_ERROR (Status); >> if (EFI_ERROR (Status)) { >> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.= c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c >> index 169d3118e44f..be260e0d1014 100644 >> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c >> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c >> @@ -25,8 +25,6 @@ >> address of a memory region. >> @param[in] NumPages The number of pages from start me= mory >> region. >> - @param[in] Flush Flush the caches before clearing = the bit >> - (mostly TRUE except MMIO addresse= s) >> =20 >> @retval RETURN_SUCCESS The attributes were cleared for t= he >> memory region. >> @@ -39,8 +37,7 @@ EFIAPI >> MemEncryptSevClearPageEncMask ( >> IN PHYSICAL_ADDRESS Cr3BaseAddress, >> IN PHYSICAL_ADDRESS BaseAddress, >> - IN UINTN NumPages, >> - IN BOOLEAN Flush >> + IN UINTN NumPages >> ) >> { >> // >> @@ -59,8 +56,6 @@ MemEncryptSevClearPageEncMask ( >> address of a memory region. >> @param[in] NumPages The number of pages from start me= mory >> region. >> - @param[in] Flush Flush the caches before setting t= he bit >> - (mostly TRUE except MMIO addresse= s) >> =20 >> @retval RETURN_SUCCESS The attributes were set for the m= emory >> region. >> @@ -73,8 +68,7 @@ EFIAPI >> MemEncryptSevSetPageEncMask ( >> IN PHYSICAL_ADDRESS Cr3BaseAddress, >> IN PHYSICAL_ADDRESS BaseAddress, >> - IN UINTN NumPages, >> - IN BOOLEAN Flush >> + IN UINTN NumPages >> ) >> { >> // >> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c= b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c >> index a2bf698bcde7..a57e8fd37fa7 100644 >> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c >> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c >> @@ -27,8 +27,6 @@ >> address of a memory region. >> @param[in] NumPages The number of pages from start me= mory >> region. >> - @param[in] Flush Flush the caches before clearing = the bit >> - (mostly TRUE except MMIO addresse= s) >> =20 >> @retval RETURN_SUCCESS The attributes were cleared for t= he >> memory region. >> @@ -41,15 +39,13 @@ EFIAPI >> MemEncryptSevClearPageEncMask ( >> IN PHYSICAL_ADDRESS Cr3BaseAddress, >> IN PHYSICAL_ADDRESS BaseAddress, >> - IN UINTN NumPages, >> - IN BOOLEAN Flush >> + IN UINTN NumPages >> ) >> { >> return InternalMemEncryptSevSetMemoryDecrypted ( >> Cr3BaseAddress, >> BaseAddress, >> - EFI_PAGES_TO_SIZE (NumPages), >> - Flush >> + EFI_PAGES_TO_SIZE (NumPages) >> ); >> } >> =20 >> @@ -63,8 +59,6 @@ MemEncryptSevClearPageEncMask ( >> address of a memory region. >> @param[in] NumPages The number of pages from start me= mory >> region. >> - @param[in] Flush Flush the caches before setting t= he bit >> - (mostly TRUE except MMIO addresse= s) >> =20 >> @retval RETURN_SUCCESS The attributes were set for the m= emory >> region. >> @@ -77,15 +71,13 @@ EFIAPI >> MemEncryptSevSetPageEncMask ( >> IN PHYSICAL_ADDRESS Cr3BaseAddress, >> IN PHYSICAL_ADDRESS BaseAddress, >> - IN UINTN NumPages, >> - IN BOOLEAN Flush >> + IN UINTN NumPages >> ) >> { >> return InternalMemEncryptSevSetMemoryEncrypted ( >> Cr3BaseAddress, >> BaseAddress, >> - EFI_PAGES_TO_SIZE (NumPages), >> - Flush >> + EFI_PAGES_TO_SIZE (NumPages) >> ); >> } >> =20 >> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemor= y.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c >> index a18d336a8789..ad1021bd3e43 100644 >> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c >> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c >> @@ -555,8 +555,7 @@ EnableReadOnlyPageWriteProtect ( >> address of a memory region. >> @param[in] Length The length of memory region >> @param[in] Mode Set or Clear mode >> - @param[in] CacheFlush Flush the caches before applying = the >> - encryption mask >> + @param[in] Mmio The physical address range is Mmi= o. >> =20 >> @retval RETURN_SUCCESS The attributes were cleared for t= he >> memory region. >> @@ -572,7 +571,7 @@ SetMemoryEncDec ( >> IN PHYSICAL_ADDRESS PhysicalAddress, >> IN UINTN Length, >> IN MAP_RANGE_MODE Mode, >> - IN BOOLEAN CacheFlush >> + IN BOOLEAN Mmio >> ) >> { >> PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry; >> @@ -585,12 +584,23 @@ SetMemoryEncDec ( >> UINT64 AddressEncMask; >> BOOLEAN IsWpEnabled; >> RETURN_STATUS Status; >> + BOOLEAN CacheFlush; >> =20 >> // >> // Set PageMapLevel4Entry to suppress incorrect compiler/analyzer war= nings. >> // >> PageMapLevel4Entry =3D NULL; >> =20 >> + // >> + // The cache need to flushed for the non-Mmio address range. >> + // >> + if (Mmio =3D=3D TRUE) { >> + CacheFlush =3D FALSE; >> + } else { >> + CacheFlush =3D TRUE; >> + } >> + >> + // >> DEBUG (( >> DEBUG_VERBOSE, >> "%a:%a: Cr3Base=3D0x%Lx Physical=3D0x%Lx Length=3D0x%Lx Mode=3D%a C= acheFlush=3D%u\n", > (2) The calculation of "CacheFlush" from "Mmio" is awkward. First, we > don't compare BOOLEANs against TRUE or FALSE, BOOLEANs just stand alone > in controlling expression (or otherwise "logical") context. Second, why > not just write: > > CacheFlush =3D !Mmio; > > But even so... > > (3) ... The introduction of the "Mmio" parameter is inexplicable to me. > It apparently replaces CacheFlush (with inverse meaning), but neither > the commit message, nor the (RFCv2 -> PATCH) changelog, explain why this > replacement makes sense. The=C2=A0 internal function is used for clearing the mask for both system R= AM as well as Mmio, so we need a way to tell the internal function that call is for the Mmio range. I thought making all the changes in a single file makes sense but I see it can get harder for the review. I guess I could split the work in two patches 1) Drop the cache flush param from high level MemEncryptSev{Set,Clear}PageEncMask and don't touch anything in the SetMemoryEncDec() 2) Rename the Flush parameter to Mmio in the SetMemoryEncDec() Does it makes sense to you ? > > The SetMemoryEncDec() function is an internal function (not a library > class API), so this change doesn't necessarily conflict with the commit > message -- but having this change in this particular patch (the last > patch in the series) seems unjustified. > > In the previous version, we updated two SetMemoryEncDec() call sites: in > InternalMemEncryptSevSetMemoryDecrypted() and > InternalMemEncryptSevSetMemoryEncrypted(), we replaced the forwarding of > "Flush" with TRUE constants. > > In this version, we update *three* SetMemoryEncDec() call sites: > > - in InternalMemEncryptSevSetMemoryDecrypted() and > InternalMemEncryptSevSetMemoryEncrypted(), we replace the forwarding of > "Flush" with FALSE constants, > > - and in InternalMemEncryptSevClearMmioPageEncMask(), we replace the > *constant* FALSE with TRUE. > > I think this very last point -- regarding > InternalMemEncryptSevClearMmioPageEncMask() -- shows that the > replacement of CacheFlush with Mmio, at this point in the series, is > unwarranted. > > Minimally, this replacement / negation should be a separate patch, but > even then, I think we'd need a defensible purpose (which is not clear to > me at this point); *plus*, the "re-calculation" of CacheFlush inside > SetMemoryEncDec() from Mmio feels like a cop-out. It's only being done > to save some additional replacements in the patch, but it leaves us with > a stricly worse -- harder to understand -- function. If you really need > this replacement / negation, then please do it in a separate patch, with > a good commit message; furthermore, please replace CacheFlush > *completely*, in SetMemoryEncDec() -- please don't reintroduce it. > > Thanks, > Laszlo > > >> @@ -828,8 +838,6 @@ SetMemoryEncDec ( >> @param[in] PhysicalAddress The physical address that is the = start >> address of a memory region. >> @param[in] Length The length of memory region >> - @param[in] Flush Flush the caches before applying = the >> - encryption mask >> =20 >> @retval RETURN_SUCCESS The attributes were cleared for t= he >> memory region. >> @@ -842,8 +850,7 @@ EFIAPI >> InternalMemEncryptSevSetMemoryDecrypted ( >> IN PHYSICAL_ADDRESS Cr3BaseAddress, >> IN PHYSICAL_ADDRESS PhysicalAddress, >> - IN UINTN Length, >> - IN BOOLEAN Flush >> + IN UINTN Length >> ) >> { >> =20 >> @@ -852,7 +859,7 @@ InternalMemEncryptSevSetMemoryDecrypted ( >> PhysicalAddress, >> Length, >> ClearCBit, >> - Flush >> + FALSE >> ); >> } >> =20 >> @@ -865,8 +872,6 @@ InternalMemEncryptSevSetMemoryDecrypted ( >> @param[in] PhysicalAddress The physical address that is the = start >> address of a memory region. >> @param[in] Length The length of memory region >> - @param[in] Flush Flush the caches before applying = the >> - encryption mask >> =20 >> @retval RETURN_SUCCESS The attributes were set for the m= emory >> region. >> @@ -879,8 +884,7 @@ EFIAPI >> InternalMemEncryptSevSetMemoryEncrypted ( >> IN PHYSICAL_ADDRESS Cr3BaseAddress, >> IN PHYSICAL_ADDRESS PhysicalAddress, >> - IN UINTN Length, >> - IN BOOLEAN Flush >> + IN UINTN Length >> ) >> { >> return SetMemoryEncDec ( >> @@ -888,7 +892,7 @@ InternalMemEncryptSevSetMemoryEncrypted ( >> PhysicalAddress, >> Length, >> SetCBit, >> - Flush >> + FALSE >> ); >> } >> =20 >> @@ -921,6 +925,6 @@ InternalMemEncryptSevClearMmioPageEncMask ( >> PhysicalAddress, >> Length, >> ClearCBit, >> - FALSE >> + TRUE >> ); >> } >> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c= b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c >> index bca5e3febb1b..24d19d3ca161 100644 >> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c >> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c >> @@ -42,8 +42,6 @@ InternalGetMemEncryptionAddressMask ( >> @param[in] PhysicalAddress The physical address that is the = start >> address of a memory region. >> @param[in] Length The length of memory region >> - @param[in] Flush Flush the caches before applying = the >> - encryption mask >> =20 >> @retval RETURN_SUCCESS The attributes were cleared for t= he >> memory region. >> @@ -56,8 +54,7 @@ EFIAPI >> InternalMemEncryptSevSetMemoryDecrypted ( >> IN PHYSICAL_ADDRESS Cr3BaseAddress, >> IN PHYSICAL_ADDRESS PhysicalAddress, >> - IN UINTN Length, >> - IN BOOLEAN Flush >> + IN UINTN Length >> ) >> { >> // >> @@ -89,8 +86,7 @@ EFIAPI >> InternalMemEncryptSevSetMemoryEncrypted ( >> IN PHYSICAL_ADDRESS Cr3BaseAddress, >> IN PHYSICAL_ADDRESS PhysicalAddress, >> - IN UINTN Length, >> - IN BOOLEAN Flush >> + IN UINTN Length >> ) >> { >> // >> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/Ovm= fPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >> index fdf2380974fa..c7cc5b0389c8 100644 >> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >> @@ -283,8 +283,7 @@ SmmCpuFeaturesSmmRelocationComplete ( >> Status =3D MemEncryptSevSetPageEncMask ( >> 0, // Cr3BaseAddress -- use current CR3 >> MapPagesBase, // BaseAddress >> - MapPagesCount, // NumPages >> - TRUE // Flush >> + MapPagesCount // NumPages >> ); >> if (EFI_ERROR (Status)) { >> DEBUG ((DEBUG_ERROR, "%a: MemEncryptSevSetPageEncMask(): %r\n", >> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c >> index dddffdebda4b..a8bf610022ba 100644 >> --- a/OvmfPkg/PlatformPei/AmdSev.c >> +++ b/OvmfPkg/PlatformPei/AmdSev.c >> @@ -72,8 +72,7 @@ AmdSevEsInitialize ( >> DecryptStatus =3D MemEncryptSevClearPageEncMask ( >> 0, >> GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount), >> - 1, >> - TRUE >> + 1 >> ); >> ASSERT_RETURN_ERROR (DecryptStatus); >> } >>