From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM12-DM6-obe.outbound.protection.outlook.com (NAM12-DM6-obe.outbound.protection.outlook.com [40.107.243.70]) by mx.groups.io with SMTP id smtpd.web08.185.1620328741400912111 for ; Thu, 06 May 2021 12:19:01 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@amd.com header.s=selector1 header.b=h5dRpkMt; 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.243.70, mailfrom: brijesh.singh@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=jhRW/CfmkwHM5rihAPWyWJ/83RRw/la3tBIzeaWc2TCFBHCpXZ9aDVCb8JWgeo1p0Lg7uhkB+TmBBH2Xm2Q35aYXdru7G9wxguI8qUCcXhb+6+lpi2wyiVKWk4UZuDoMhpZqj36idkxb9pTjvhwsQgfDr+Wjx6FRLvYMmqNQjMQbftX5GWpfbCCrlrTlf0XAqcs0T6nOCATkV/CZlJtjwxvsdp/MwDsuzvvzTinIFrpVVvO26LlHMDZbRqnKE8i8qRQZo6LVG1kv0b17x7F/VegmMiUZbbJbUm+P8TqfGbCzR7RvKSbYBbgzSreKhvSQ7kcGWbPxT3Y2T+IndljC9A== 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=OJjO/3jLFh395MLDiG27mnv1TcwioNHNpB0FWOU6TQg=; b=AxIiGqkzmxi1W+uERdXC2mVz2zCe3DZ8xvTDMSju0xPQcIkXh4hspKB2UTI4WKaRr43epW/4xelr79UwVdHAxiES/2d8Tjibcr9ngPan+CRWilpLch3jf6Ljhv6r+/ok5PNO7DtlCcoxPoHsZ/SwIVgyWT/M4A1uZdW/5e6/BCcoIiX6ri1inwrxD3uq/woUFqVehFzMpw9UpWOtsdCCXvn6HsjRsOwhMFlTmTtyCsglSB819AklCG+vNaStKD/8LpKfKRTm56YHU3LGt8NVeZ0kInpltNmPTID/xENNC2/83VWR5zZY661GOw1QJWduwOrHhy8TP+id8d4uqBiYog== 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=OJjO/3jLFh395MLDiG27mnv1TcwioNHNpB0FWOU6TQg=; b=h5dRpkMtPwcy1TgGbekXO2KhgykYRmq79x33IITIiLn1LmzW0LFPW+TnogDnJXglBV+hVfUnwB9JU300ok4zy2B+PvFfROgV8gF2PTM0+jllKf2DjLVMuboUgRQXimURXx7nK31ZV5B5f6SBQSr9eKxkQGQEUpmTIm1FuAb1iBY= 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 SN1PR12MB2365.namprd12.prod.outlook.com (2603:10b6:802:2e::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4108.27; Thu, 6 May 2021 19:18:59 +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.4108.026; Thu, 6 May 2021 19:18:59 +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 RFC v2 06/28] OvmfPkg/BaseMemEncryptSevLib: Introduce MemEncryptSevClearMmioPageEncMask() To: devel@edk2.groups.io, lersek@redhat.com References: <20210430115148.22267-1-brijesh.singh@amd.com> <20210430115148.22267-7-brijesh.singh@amd.com> From: "Brijesh Singh" Message-ID: Date: Thu, 6 May 2021 14:18:56 -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: X-Originating-IP: [70.112.153.56] X-ClientProxiedBy: SN4PR0501CA0011.namprd05.prod.outlook.com (2603:10b6:803:40::24) 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 SN4PR0501CA0011.namprd05.prod.outlook.com (2603:10b6:803:40::24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4129.11 via Frontend Transport; Thu, 6 May 2021 19:18:58 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 115e35aa-9387-4e71-1b23-08d910c3cc9c X-MS-TrafficTypeDiagnostic: SN1PR12MB2365: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:2331; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: Z7uP5QDh8AwB90KvJc71xRcaNhXi69FzAe7Vw8nBbhip3FjoSWUMu7PPZ67lzGHO5F7TaMV1ZLHg/pK6BzteCE8CIO88YVE/DiNSpD1xe4hFpz1BBIu4TVQSR/NpZ2CKYddm15H1xkQCxQJeLlzkz8pnJDn/XlC4NfybbeQNs7fe/mCczCBHoLgoy5kqna5EfzfCUofD2cNoYX99oKl5/KORLB763VfNlGCjEa9cnvlI1z8deuMQetJhLrgqJs7hNgfP5CSEUg38GKA/2wn1RDbqeOTJb7TWFsxjyHK1f6Kg2KHBm7motywaRpT+BNHltzDXUn3I3Eg1IgBsiFYRKot6mxSiUQuHoZZtjJ+S3AaNanzMDY6wDnFmsVyyYQoGRLCTeg0CeXWohql+wI3aAVJ5vshkISRx6mdfaL7CdJCabh8r6gIj/9ji7Bh78Cmm9NgVcclLKnrBhJQTQsoFOwXoazFucbfBaTHfcv0wmvOSX0Oy588vCt9VF/TsnOl9UF3Zt7blCbh9UzPq9I5S9AQkEOQ7mHlhUpuxWcCtaypwTtITrDljzdkHIInyvAkTNKjOwq7DCvERh/tW/TDmojjLvpOFUCeHqOpqdeE5SMjGHYQh/vQ2Zroq84cYvAV6l1WZMM32oA/czsIQBbHB6mliPhh6OtnSm1XjP/oS66iLOdoyMBWt+h0hNkOScFbiOir9U3BjXiUhjY7xN3WKo1fRlSI5WTamUmkjOtPUX9Ojv37ye+If3cRpIAoktRBNzbNpifZwyY8s3aypDO1YKxSmqGlSptdqkGpQnj1IFKB7nEbB0ABFMQmZTedcDArX5/t+gF5RLyChHqwe/JGMIA== 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)(39860400002)(346002)(396003)(376002)(366004)(31686004)(316002)(45080400002)(6486002)(6512007)(956004)(86362001)(30864003)(44832011)(36756003)(45954011)(53546011)(54906003)(2906002)(31696002)(83380400001)(66946007)(52116002)(2616005)(186003)(6506007)(16526019)(26005)(8676002)(4326008)(66556008)(8936002)(66476007)(966005)(38100700002)(478600001)(5660300002)(38350700002)(213903007)(43740500002)(45980500001);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?eTE4K2l2VG1WYURVSHFIQjV0eVZkSGNoQ1BJZUplWE5lU09HUnFBQnhnY1lk?= =?utf-8?B?QkdMZTlNOU1IZGU0WFZqK0pXNThDLzQ4WTRqK1I2MnJkUWR2bXZIMkNDOHhm?= =?utf-8?B?R2ZFMWM5N3hJblhuSXlrVHFOWkJhVExkWDA2aGNHMnRZL3FpZU05dnkyQnAr?= =?utf-8?B?VEY0b0VrSk9UM2h3WGxRWWQ4UWR1V1gxQzRJK0dNeHdFcEdaeVI1YklIckRC?= =?utf-8?B?ZDZkZVpldERoY1QvU2ZmcTRUTnpGUGZGdGxNNTdzdm9iclMzaGoyTSttc1Zl?= =?utf-8?B?K09LL2FZR09BM0ZIcXhSRGFVT2x3U0R2MVlwODNUKzY0UFFsbkd4djc0NTdW?= =?utf-8?B?ZXhvaVM2dkFLS2NkdmMvZmJUVkxPZ1N2WkRoNDF1R2N4TlVLb2FWMjdZUEJ0?= =?utf-8?B?eTBQQ0pReXBkV245Q1B4dlhia2hXUm5DOTNkZFczMEMwdktRaU1UN0w5TnZ0?= =?utf-8?B?cURoejN4VFh4UG1uTUtZUDllZFBHNk5FYjVQMUVuMGV3aE1LYWRrOWx4TDN1?= =?utf-8?B?NDBTUXhLMk1Odm9ySEdVVW1GWGlMdkMxNFZNTk41T2MrQ0l4QmhDc3ZYTEVs?= =?utf-8?B?dzdnQ0hOZ1hIZXpudUZWRVJQbEhRbk1TdEJUZ3I4cnY4am0yOTQ3eXpPdU9G?= =?utf-8?B?aGl1R2I3SnlQOVBjc044UnVzMW5iYndBdVBZVmxrcEo0dmdzM2ZrRDVQdEpS?= =?utf-8?B?a1dNYklxbDdhTHdoeXVEdVRUU0F5Zi9OV0dsM1dzdGJ4TTBCYm5QN1ZIK3ZW?= =?utf-8?B?YUJ1Z0dRbWZFSm1xaTFOaW5VT0kzTEFLN3FzQ0t6bmZEdWdvY1lkT1lvNVF0?= =?utf-8?B?cDNEcXQwaGpFaWl3REdBU1RqN2VKSWVXVmljc2lJeTh3Vml3MjgrTlZSNlJ3?= =?utf-8?B?Zk5SbTRXZGRRN2Vyc2dPemdsQ3RCRXpKYU4yczFqMHJTcXRMTEhtQUp4MmlD?= =?utf-8?B?OWdLMTViWFNuL0lIS1RYU3NOTUowT2FLc1dWNnJTQ1JrVDlSM20yZllLNWhz?= =?utf-8?B?NUhFYjZDUDV3Q3ZkUnhOam9pL0xteWpsbFM1OXNYeTdLK3VtdXJrM1RxVTBX?= =?utf-8?B?Skl1T0hNV1lPQWRiM2pDUUJnMHFLczRNUnNsejNNOFpTUWJjR0lCeDliUjFu?= =?utf-8?B?V01pcHZQeTFmd21hUFFzZW9BQmVBbDlSWFF4MU5OMVB1TGZFQ2Z6REpTcXUw?= =?utf-8?B?cXBXT1dTQmdabFRjK2JPUm9hQjE2ODRtQlBBOEJKdzhuUkxEU09GUUVacVVo?= =?utf-8?B?ZGdEWEZJd2FEdStEekNaRDUxcnUyNzVJMWdRNjVzU29RZTI2UzF0UEg0TEZD?= =?utf-8?B?eEExVWlqV2NkZUlqWmNsc1ZEelBTMUhkOTBSbncveUt6SUdSWWd6YTYreENI?= =?utf-8?B?SDVNa291SEpIK2N4RERMcXJVc3JqZWVFYTl4WWppMXhJS0dOY29mWTV0YU9D?= =?utf-8?B?SnlXZkx5L3ZxNHN0dFVrbGpPTmZjZ0Z1a1R6eGd4WnBYTGFrNGdkUWtNdUJQ?= =?utf-8?B?enM4Y1FFRjFZcHFIYk9wdzArc3c1KzJDdXNWTVFXSWZlVGQ0ZnNBRVVHTmJE?= =?utf-8?B?VlNJVCtzY0dsWnd0RmxaTDZRR0F0Tys4V2tBaHhKMVJHMnR4VjZJSWVlV3VD?= =?utf-8?B?U1lKcktyS3V3Wk9QRk9udGdvN3pQanZLU2s4TnVWM0pGbVBtWWRoMDJrSmZT?= =?utf-8?B?TEtmcERKRW9VV1dvaXlxanQzekdhYTJRM1FqV2kxSkVZM2d0NFJLa1pPeEZG?= =?utf-8?Q?f1fBpinyOCYueLxEhsbtOnxaDBJRT/uCWK9cz+f?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 115e35aa-9387-4e71-1b23-08d910c3cc9c X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2718.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 May 2021 19:18:59.3531 (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: OrxyHx40wZ8J9P5NTzDeSa9EJ+Sb8+WY6dk9QcE56AH3WQd9C6MCfl8nHBDPT1ids7kFdlaZGLCJpFGJ+cKeNQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR12MB2365 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US On 5/6/21 5:39 AM, Laszlo Ersek via groups.io wrote: > On 04/30/21 13:51, Brijesh Singh wrote: >> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7C2719bcebf85d4e5228e508d9107b449d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637558943897655383%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=YrgkAA3Kp6v2eJuovVho1I3kR%2FXAA5D%2BE2vaUKyFO68%3D&reserved=0 >> >> The MemEncryptSevClearMmioPageEncMask() helper can be used for clearing >> the memory encryption mask for the Mmio region from the current page >> table context. > The commit message, and five comments in the patch, say "current page > table context". However, Cr3BaseAddress is taken explicitly. > > I realize the files being modified in this patch already make similarly > incorrect statements, but I'd like if we avoided adding more. > > (1) Please just drop "current page table context" from the mentioned six > locations. (Explaining that Cr3BaseAddress=0 means "current CR3" is of > course valid.) > > Thanks Laszlo, I will go through the feedback on this patch and address them in next rev. -Brijesh >> 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 | 25 +++++++++++ >> OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 31 ++++++++++++++ >> OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 33 +++++++++++++++ >> OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 44 ++++++++++++++++++-- >> OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h | 23 ++++++++++ >> 5 files changed, 153 insertions(+), 3 deletions(-) >> >> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h >> index 99f15a7d12..c19f92afc6 100644 >> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h >> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h >> @@ -203,4 +203,29 @@ MemEncryptSevGetAddressRangeState ( >> IN UINTN Length >> ); >> >> +/** >> + This function clears memory encryption bit for the mmio region specified by >> + BaseAddress and NumPages from the current page table context. >> + >> + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use >> + current CR3) >> + @param[in] BaseAddress The physical address that is the start >> + address of a mmio region. >> + @param[in] NumPages The number of pages from start memory >> + region. >> + >> + @retval RETURN_SUCCESS The attributes were cleared for the >> + memory region. >> + @retval RETURN_INVALID_PARAMETER Number of pages is zero. >> + @retval RETURN_UNSUPPORTED Clearing the memory encryption attribute >> + is not supported >> +**/ >> +RETURN_STATUS >> +EFIAPI >> +MemEncryptSevClearMmioPageEncMask ( >> + IN PHYSICAL_ADDRESS Cr3BaseAddress, >> + IN PHYSICAL_ADDRESS BaseAddress, >> + IN UINTN NumPages >> + ); >> + >> #endif // _MEM_ENCRYPT_SEV_LIB_H_ >> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c >> index 12a5bf495b..4e8a997d42 100644 >> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c >> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c >> @@ -111,3 +111,34 @@ MemEncryptSevGetAddressRangeState ( >> // >> return MemEncryptSevAddressRangeEncrypted; >> } >> + >> +/** >> + This function clears memory encryption bit for the mmio region specified by >> + BaseAddress and NumPages from the current page table context. >> + >> + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use >> + current CR3) >> + @param[in] BaseAddress The physical address that is the start >> + address of a mmio region. >> + @param[in] NumPages The number of pages from start memory >> + region. >> + >> + @retval RETURN_SUCCESS The attributes were cleared for the >> + memory region. >> + @retval RETURN_INVALID_PARAMETER Number of pages is zero. >> + @retval RETURN_UNSUPPORTED Clearing the memory encryption attribute >> + is not supported >> +**/ >> +RETURN_STATUS >> +EFIAPI >> +MemEncryptSevClearMmioPageEncMask ( >> + IN PHYSICAL_ADDRESS Cr3BaseAddress, >> + IN PHYSICAL_ADDRESS BaseAddress, >> + IN UINTN NumPages >> + ) >> +{ >> + // >> + // Memory encryption bit is not accessible in 32-bit mode >> + // >> + return RETURN_UNSUPPORTED; >> +} >> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c >> index 4fea6a6be0..6786573aea 100644 >> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c >> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c >> @@ -118,3 +118,36 @@ MemEncryptSevGetAddressRangeState ( >> Length >> ); >> } >> + >> +/** >> + This function clears memory encryption bit for the mmio region specified by >> + BaseAddress and NumPages from the current page table context. >> + >> + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use >> + current CR3) >> + @param[in] BaseAddress The physical address that is the start >> + address of a mmio region. >> + @param[in] NumPages The number of pages from start memory >> + region. >> + >> + @retval RETURN_SUCCESS The attributes were cleared for the >> + memory region. >> + @retval RETURN_INVALID_PARAMETER Number of pages is zero. >> + @retval RETURN_UNSUPPORTED Clearing the memory encryption attribute >> + is not supported >> +**/ >> +RETURN_STATUS >> +EFIAPI >> +MemEncryptSevClearMmioPageEncMask ( >> + IN PHYSICAL_ADDRESS Cr3BaseAddress, >> + IN PHYSICAL_ADDRESS BaseAddress, >> + IN UINTN NumPages >> + ) >> +{ >> + return InternalMemEncryptSevClearMmioPageEncMask ( >> + Cr3BaseAddress, >> + BaseAddress, >> + EFI_PAGES_TO_SIZE (NumPages) >> + ); >> + >> +} >> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c >> index d3455e812b..3bcc92f2e9 100644 >> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c >> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c >> @@ -557,6 +557,7 @@ EnableReadOnlyPageWriteProtect ( >> @param[in] Mode Set or Clear mode >> @param[in] CacheFlush Flush the caches before applying the >> encryption mask >> + @param[in] IsMmio The address is Mmio address. >> >> @retval RETURN_SUCCESS The attributes were cleared for the >> memory region. > (2) The parameter's name in the documentation ("IsMmio") does not match > the actual parameter name ("Mmio"). > > >> @@ -572,7 +573,8 @@ SetMemoryEncDec ( >> IN PHYSICAL_ADDRESS PhysicalAddress, >> IN UINTN Length, >> IN MAP_RANGE_MODE Mode, >> - IN BOOLEAN CacheFlush >> + IN BOOLEAN CacheFlush, >> + IN BOOLEAN Mmio >> ) >> { >> PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry; > The "Mmio" parameter is not used for anything in this patch. It's very > confusing. > > (3) Please remove the addition of the Mmio parameter from this patch. > Please introduce the Mmio parameter only when it is utilized -- as far > as I can tell from a quick git-blame, that's patch #26 > ("OvmfPkg/MemEncryptSevLib: Change the page state in the RMP table"). > > As a result, in this patch, InternalMemEncryptSevClearMmioPageEncMask() > will effectively become a slight simplification of > InternalMemEncryptSevSetMemoryDecrypted() -- rather than forwarding > "Flush" for "CacheFlush", it will pass constant FALSE for "CacheFlush". > > > (4) Please mention the above fact (= last paragraph above) in the commit > message. > > >> @@ -852,7 +854,8 @@ InternalMemEncryptSevSetMemoryDecrypted ( >> PhysicalAddress, >> Length, >> ClearCBit, >> - Flush >> + Flush, >> + FALSE >> ); >> } >> >> @@ -888,6 +891,41 @@ InternalMemEncryptSevSetMemoryEncrypted ( >> PhysicalAddress, >> Length, >> SetCBit, >> - Flush >> + Flush, >> + FALSE >> + ); >> +} >> + >> +/** >> + This function clears memory encryption bit for the Mmio region specified by >> + PhysicalAddress and Length from the current page table context. >> + >> + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use >> + current CR3) >> + @param[in] PhysicalAddress The physical address that is the start >> + address of a mmio region. >> + @param[in] Length The length of memory region >> + >> + @retval RETURN_SUCCESS The attributes were cleared for the >> + memory region. >> + @retval RETURN_INVALID_PARAMETER Number of pages is zero. > This function does not take a NumPages parameter but a Length parameter. > > (5) Please update the RETURN_INVALID_PARAMETER comment accordingly -- in > the header file as well. > > >> + @retval RETURN_UNSUPPORTED Clearing the memory encyrption attribute >> + is not supported >> +**/ >> +RETURN_STATUS >> +EFIAPI >> +InternalMemEncryptSevClearMmioPageEncMask ( >> + IN PHYSICAL_ADDRESS Cr3BaseAddress, >> + IN PHYSICAL_ADDRESS PhysicalAddress, >> + IN UINTN Length >> + ) >> +{ >> + return SetMemoryEncDec ( >> + Cr3BaseAddress, >> + PhysicalAddress, >> + Length, >> + ClearCBit, >> + FALSE, >> + TRUE >> ); >> } >> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h >> index fe2a0b2826..99ee7ea0e8 100644 >> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h >> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h >> @@ -126,4 +126,27 @@ InternalMemEncryptSevGetAddressRangeState ( >> IN UINTN Length >> ); >> >> +/** >> + This function clears memory encryption bit for the Mmio region specified by >> + PhysicalAddress and Length from the current page table context. >> + >> + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use >> + current CR3) >> + @param[in] PhysicalAddress The physical address that is the start >> + address of a mmio region. >> + @param[in] Length The length of memory region >> + >> + @retval RETURN_SUCCESS The attributes were cleared for the >> + memory region. >> + @retval RETURN_INVALID_PARAMETER Number of pages is zero. > This function does not take a NumPages parameter but a Length parameter. > > (6) Please update the RETURN_INVALID_PARAMETER comment accordingly -- > same as in the C file. > > >> + @retval RETURN_UNSUPPORTED Clearing the memory encyrption attribute >> + is not supported >> +**/ >> +RETURN_STATUS >> +EFIAPI >> +InternalMemEncryptSevClearMmioPageEncMask ( >> + IN PHYSICAL_ADDRESS Cr3BaseAddress, >> + IN PHYSICAL_ADDRESS PhysicalAddress, >> + IN UINTN Length >> + ); >> #endif >> > Please carefully audit the rest of the series for comment blocks. Such > issues render reviews inefficient. > > Thanks > Laszlo > > > > > >