[PATCH] OvmfPkg: Fix SevMemoryAcceptance memory attributes

[PATCH] OvmfPkg: Fix SevMemoryAcceptance memory attributes

[Dionna Glaze]

The hard-coded attributes for the re-added memory space should instead
forward the replaced descriptor's capabilities, plus the
EFI_MEMORY_CPU_CRYPTO attribute.

Tested on Linux with efi=debug. Prior to this change, an 8GiB VM running
a kernel without unaccepted memory support shows this entry

efi: mem94: [Conventional|   |  |CC|  |  |  |  |  |  |   |  |  |  |  ]
range=[0x0000000100000000-0x000000023fffffff] (5120MB)

This does not have the cache capabilities one would expect for system
memory, UC|WC|WT|WB.

After this change, the same entry becomes

efi: mem94: [Conventional|   |  |CC|  |  |  |  |  |  |   |WB|WT|WC|UC]
range=[0x0000000100000000-0x000000023fffffff] (5120MB)

This has all the expected attributes.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Michael Roth <michael.roth@amd.com>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 OvmfPkg/AmdSevDxe/AmdSevDxe.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
index 6391d1f775..59d5ff759f 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
@@ -23,6 +23,10 @@
 #include <Pi/PrePiDxeCis.h>
 #include <Protocol/SevMemoryAcceptance.h>
 #include <Protocol/MemoryAccept.h>
+#include <Uefi/UefiSpec.h>
+
+// Present, initialized, tested bits defined in MdeModulePkg/Core/Dxe/DxeMain.h
+#define EFI_MEMORY_INTERNAL_MASK 0x0700000000000000ULL
 
 STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION  mSnpBootDxeTable = {
   SIGNATURE_32 ('A',                                    'M', 'D', 'E'),
@@ -78,6 +82,7 @@ AcceptAllMemory (
   UINTN                            NumEntries;
   UINTN                            Index;
   EFI_STATUS                       Status;
+  UINT64                           Capabilities;
 
   DEBUG ((DEBUG_INFO, "Accepting all memory\n"));
 
@@ -112,11 +117,14 @@ AcceptAllMemory (
       break;
     }
 
+    Capabilities = EFI_MEMORY_CPU_CRYPTO | Desc->Capabilities;
     Status = gDS->AddMemorySpace (
                     EfiGcdMemoryTypeSystemMemory,
                     Desc->BaseAddress,
                     Desc->Length,
-                    EFI_MEMORY_CPU_CRYPTO | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_RP
+                    // Allocable system memory resource capabilities as masked
+                    // in MdeModulePkg/Core/Dxe/Mem/Page.c:PromoteMemoryResource
+                    Capabilities & ~(EFI_MEMORY_INTERNAL_MASK | EFI_MEMORY_RUNTIME)
                     );
     if (EFI_ERROR (Status)) {
       break;
-- 
2.39.1.456.gfc5497dd1b-goog

Re: [PATCH] OvmfPkg: Fix SevMemoryAcceptance memory attributes

[Dionna Glaze]

>
> efi: mem94: [Conventional|   |  |CC|  |  |  |  |  |  |   |  |  |  |  ]
> range=[0x0000000100000000-0x000000023fffffff] (5120MB)
>
> This does not have the cache capabilities one would expect for system
> memory, UC|WC|WT|WB.
>
> After this change, the same entry becomes
>
> efi: mem94: [Conventional|   |  |CC|  |  |  |  |  |  |   |WB|WT|WC|UC]
> range=[0x0000000100000000-0x000000023fffffff] (5120MB)
>
> This has all the expected attributes.
>

This change is made given a request from Ard. The CC capability is not
applied to other system memory ranges that probably should also have
that capability, given that it's encrypted and accepted. I haven't
considered carefully where EFI_MEMORY_CPU_CRYPTO should be added to
conventional memory, given the acceptance happens before DXE
initializes. Perhaps
CoreConvertResourceDescriptorHobAttributesToCapabilities? This is more
of a question to Ard and Thomas.

-- 
-Dionna Glaze, PhD (she/her)

Re: [PATCH] OvmfPkg: Fix SevMemoryAcceptance memory attributes

[Ard Biesheuvel]

On Tue, 31 Jan 2023 at 20:18, Dionna Amalie Glaze
<dionnaglaze@google.com> wrote:
>
> >
> > efi: mem94: [Conventional|   |  |CC|  |  |  |  |  |  |   |  |  |  |  ]
> > range=[0x0000000100000000-0x000000023fffffff] (5120MB)
> >
> > This does not have the cache capabilities one would expect for system
> > memory, UC|WC|WT|WB.
> >
> > After this change, the same entry becomes
> >
> > efi: mem94: [Conventional|   |  |CC|  |  |  |  |  |  |   |WB|WT|WC|UC]
> > range=[0x0000000100000000-0x000000023fffffff] (5120MB)
> >
> > This has all the expected attributes.
> >
>
> This change is made given a request from Ard. The CC capability is not
> applied to other system memory ranges that probably should also have
> that capability, given that it's encrypted and accepted. I haven't
> considered carefully where EFI_MEMORY_CPU_CRYPTO should be added to
> conventional memory, given the acceptance happens before DXE
> initializes. Perhaps
> CoreConvertResourceDescriptorHobAttributesToCapabilities? This is more
> of a question to Ard and Thomas.
>

It's not clear to me whether the CC attribute applies to the host or
the guest. From the guest PoV, there is really no distinction, whereas
on the host, I could imagine that only CC capable memory can be used
for handing out to VMs.

The important thing for this conversion is that
- the type changes to ConventionalMemory
- EFI_MEMORY_WB gets set (along with the other ones set in the
capability), as otherwise, Linux will disregard this memory and not
use it at all.

The EFI_MEMORY_CPU_CRYPTO flag is currently only referenced by Linux
in code that prints the memory map for diagnostic purposes, and not
used anywhere else.

Re: [PATCH] OvmfPkg: Fix SevMemoryAcceptance memory attributes

[Dionna Glaze]

> >
> > This change is made given a request from Ard. The CC capability is not
> > applied to other system memory ranges that probably should also have
> > that capability, given that it's encrypted and accepted. I haven't
> > considered carefully where EFI_MEMORY_CPU_CRYPTO should be added to
> > conventional memory, given the acceptance happens before DXE
> > initializes. Perhaps
> > CoreConvertResourceDescriptorHobAttributesToCapabilities? This is more
> > of a question to Ard and Thomas.
> >
>
> It's not clear to me whether the CC attribute applies to the host or
> the guest. From the guest PoV, there is really no distinction, whereas
> on the host, I could imagine that only CC capable memory can be used
> for handing out to VMs.
>

That's a good point. The UEFI spec language is hard to interpret here.
Min or Jiewen, do you have more context on the EFI_MEMORY_CPU_CRYPTO
attribute?


-- 
-Dionna Glaze, PhD (she/her)

Re: [PATCH] OvmfPkg: Fix SevMemoryAcceptance memory attributes

[Ard Biesheuvel]

On Thu, 2 Feb 2023 at 21:42, Dionna Amalie Glaze <dionnaglaze@google.com> wrote:
>
> > >
> > > This change is made given a request from Ard. The CC capability is not
> > > applied to other system memory ranges that probably should also have
> > > that capability, given that it's encrypted and accepted. I haven't
> > > considered carefully where EFI_MEMORY_CPU_CRYPTO should be added to
> > > conventional memory, given the acceptance happens before DXE
> > > initializes. Perhaps
> > > CoreConvertResourceDescriptorHobAttributesToCapabilities? This is more
> > > of a question to Ard and Thomas.
> > >
> >
> > It's not clear to me whether the CC attribute applies to the host or
> > the guest. From the guest PoV, there is really no distinction, whereas
> > on the host, I could imagine that only CC capable memory can be used
> > for handing out to VMs.
> >
>
> That's a good point. The UEFI spec language is hard to interpret here.
> Min or Jiewen, do you have more context on the EFI_MEMORY_CPU_CRYPTO
> attribute?
>

To keep things moving, I've queued this up (as #4040) with the
EFI_MEMORY_CPU_CRYPTO flag dropped.

I don't think we need to add it here, given that EDK2 nor Linux ever
set or test this flag anywhere else, but if this changes, we can add
it back.