[edk2-devel] [PATCH 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter
Brijesh Singh
brijesh.singh at amd.com
Wed May 12 16:35:16 UTC 2021
On 5/11/21 12:45 PM, Brijesh Singh wrote:
> 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=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc383d8fdc1264644760508d91473b003%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563309382960811%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=M3xHM2yU0m3VtPn1xGe1k5Wq0d6Vbdf9gMqDX1NxpgA%3D&reserved=0
>>>
>>> The Flush parameter is used to provide a hint whether the specified range
>>> 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 the
>>> Flush parameter from MemEncryptSev{Set,Clear}PageEncMask().
>> This looks good; it matches my request (1) from:
>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2021-May%2Fmsg00109.html&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc383d8fdc1264644760508d91473b003%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563309382960811%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=3TQrliMABDaa%2FtCN%2FvKewTmLfVRKIou2La5yRGGyzJY%3D&reserved=0
>>
>>> Cc: James Bottomley <jejb at linux.ibm.com>
>>> Cc: Min Xu <min.m.xu at intel.com>
>>> Cc: Jiewen Yao <jiewen.yao at intel.com>
>>> Cc: Tom Lendacky <thomas.lendacky at amd.com>
>>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
>>> Cc: Laszlo Ersek <lersek at redhat.com>
>>> Cc: Erdem Aktas <erdemaktas at google.com>
>>> Signed-off-by: Brijesh Singh <brijesh.singh at amd.com>
>>> ---
>>> 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/Include/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 memory
>>> region.
>>> - @param[in] Flush Flush the caches before clearing the bit
>>> - (mostly TRUE except MMIO addresses)
>>>
>>> @retval RETURN_SUCCESS The attributes were cleared for the
>>> 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
>>> );
>>>
>>> /**
>>> @@ -128,8 +125,6 @@ MemEncryptSevClearPageEncMask (
>>> address of a memory region.
>>> @param[in] NumPages The number of pages from start memory
>>> region.
>>> - @param[in] Flush Flush the caches before setting the bit
>>> - (mostly TRUE except MMIO addresses)
>>>
>>> @retval RETURN_SUCCESS The attributes were set for the memory
>>> region.
>>> @@ -142,8 +137,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/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
>>>
>>> @retval RETURN_SUCCESS The attributes were cleared for the
>>> 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
>>> );
>>>
>>> /**
>>> @@ -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
>>>
>>> @retval RETURN_SUCCESS The attributes were set for the memory
>>> region.
>>> @@ -99,8 +94,7 @@ EFIAPI
>>> InternalMemEncryptSevSetMemoryEncrypted (
>>> IN PHYSICAL_ADDRESS Cr3BaseAddress,
>>> IN PHYSICAL_ADDRESS PhysicalAddress,
>>> - IN UINTN Length,
>>> - IN BOOLEAN Flush
>>> + IN UINTN Length
>>> );
>>>
>>> /**
>>> 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 = 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
>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2021-May%2Fmsg00109.html&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc383d8fdc1264644760508d91473b003%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563309382960811%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=3TQrliMABDaa%2FtCN%2FvKewTmLfVRKIou2La5yRGGyzJY%3D&reserved=0>.
> Ah, I will fix in rev2.
>
>
>>> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>> index 49ffa2448811..b30628078f73 100644
>>> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>> @@ -252,8 +252,7 @@ IoMmuMap (
>>> Status = MemEncryptSevClearPageEncMask (
>>> 0,
>>> MapInfo->PlainTextAddress,
>>> - MapInfo->NumberOfPages,
>>> - TRUE
>>> + MapInfo->NumberOfPages
>>> );
>>> ASSERT_EFI_ERROR (Status);
>>> if (EFI_ERROR (Status)) {
>>> @@ -407,8 +406,7 @@ IoMmuUnmapWorker (
>>> Status = 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 memory
>>> region.
>>> - @param[in] Flush Flush the caches before clearing the bit
>>> - (mostly TRUE except MMIO addresses)
>>>
>>> @retval RETURN_SUCCESS The attributes were cleared for the
>>> 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 memory
>>> region.
>>> - @param[in] Flush Flush the caches before setting the bit
>>> - (mostly TRUE except MMIO addresses)
>>>
>>> @retval RETURN_SUCCESS The attributes were set for the memory
>>> 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 memory
>>> region.
>>> - @param[in] Flush Flush the caches before clearing the bit
>>> - (mostly TRUE except MMIO addresses)
>>>
>>> @retval RETURN_SUCCESS The attributes were cleared for the
>>> 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)
>>> );
>>> }
>>>
>>> @@ -63,8 +59,6 @@ MemEncryptSevClearPageEncMask (
>>> address of a memory region.
>>> @param[in] NumPages The number of pages from start memory
>>> region.
>>> - @param[in] Flush Flush the caches before setting the bit
>>> - (mostly TRUE except MMIO addresses)
>>>
>>> @retval RETURN_SUCCESS The attributes were set for the memory
>>> 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)
>>> );
>>> }
>>>
>>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.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 Mmio.
>>>
>>> @retval RETURN_SUCCESS The attributes were cleared for the
>>> 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;
>>>
>>> //
>>> // Set PageMapLevel4Entry to suppress incorrect compiler/analyzer warnings.
>>> //
>>> PageMapLevel4Entry = NULL;
>>>
>>> + //
>>> + // The cache need to flushed for the non-Mmio address range.
>>> + //
>>> + if (Mmio == TRUE) {
>>> + CacheFlush = FALSE;
>>> + } else {
>>> + CacheFlush = TRUE;
>>> + }
>>> +
>>> + //
>>> DEBUG ((
>>> DEBUG_VERBOSE,
>>> "%a:%a: Cr3Base=0x%Lx Physical=0x%Lx Length=0x%Lx Mode=%a CacheFlush=%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 = !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 internal function is used for clearing the mask for both system RAM
> 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 ?
I am going to drop the Mmio hunk from this patch to make it sane. I was
thinking ahead for the SEV-SNP support; For SEV and ES support, the
SetMemoryEncDec() does not need to know whether its called for Mmio or
RAM address. In SNP, the SetMemoryEncDec() will need to know this
information so that it can skip the page state change.
-Brijesh
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75076): https://edk2.groups.io/g/devel/message/75076
Mute This Topic: https://groups.io/mt/82665196/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list