Re: [edk2-devel] [PATCH v8 0/7] Add safe unaccepted memory behavior

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 11/8/22 10:09, Yao, Jiewen wrote:
> Hi Tom/Dionna
> I think this patch is addressing https://bugzilla.tianocore.org/show_bug.cgi?id=3987.
> 
> For patch 1~3, I am OK for the API which we already agreed, such as EfiMemoryAcceptProtocol and EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES.
> I have given Acked by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> For patch 4, it changed the behavior to accept all. I don’t believe it should be 4. It should be the latest one. (please correct me if I am wrong.)
> 
> For patch 5~6, I cannot give R-B for BZ3987_MEMORY_ACCEPTANCE_PROTOCAL proposed here because it does not address my concern. Please refer to the discussion in the Bugzilla.
> 
> 
> If you want to split the patch series and submit 1~3 as standalone one, that will match SEV to TDX on lazy accept part. I believe we may merge them after we get R-B from right person.

Just FYI, without the last half of this series, I believe that TDX will 
only be accepting 4GB of memory when using OvmfPkgX64.dsc (see 
OvmfPkg/Library/PlatformInitLib/IntelTdx.c), while SNP will be accepting 
all memory. This would seem to present a problem for TDX with 
OvmfPkgX64.dsc if the OS does not have support for unaccepted memory.

Thanks,
Tom

> 
> Thank you
> Yao, Jiewen
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
>> Lendacky, Thomas via groups.io
>> Sent: Tuesday, November 8, 2022 10:38 PM
>> To: Dionna Glaze <dionnaglaze@google.com>; devel@edk2.groups.io
>> Cc: Ard Biescheuvel <ardb@kernel.org>; Xu, Min M <min.m.xu@intel.com>;
>> Gerd Hoffmann <kraxel@redhat.com>; James Bottomley
>> <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Aktas, Erdem
>> <erdemaktas@google.com>; Andrew Fish <afish@apple.com>; Kinney,
>> Michael D <michael.d.kinney@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v8 0/7] Add safe unaccepted memory
>> behavior
>>
>> On 10/24/22 15:41, Dionna Glaze wrote:
>>> These seven patches build on the lazy-accept patch series
>>>
>>> "Introduce Lazy-accept for Tdx guest"
>>
>> Since the above series was accepted into the EDK2 tree, can this series
>> also be pulled in so that both TDX and SNP can support unaccepted
>> memory
>> in the same release?
>>
>> Thanks,
>> Tom
>>
>>>
>>> by adding SEV-SNP support for the MemoryAccept protocol, and
>>> importantly making eager memory acceptance the default behavior.
>>>
>>> We implement a standardized event group from UEFI v2.9,
>>> EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES, since it provides
>> exactly
>>> the right invocation point for eagerly accepting memory if eager
>>> acceptance has not been disabled.
>>>
>>> To make use of this event group, we add a new driver that is meant to
>>> carry behavior that is needed for all confidential compute technologies,
>>> not just specific platforms, CocoDxe. In CocoDxe we implement the
>>> default safe behavior to accept all unaccepted memory and invalidate
>>> the MemoryMap on ExitBootServices.
>>>
>>> To allow the OS loader to prevent the eager acceptance, we add a new
>>> protocol, up for standardization, AcceptAllUnacceptedMemoryProtocol.
>>> This protocol has one interface, Disable(). The OS loader can inform the
>>> UEFI that it supports the unaccepted memory type and accepts the
>>> responsibility to accept it.
>>>
>>> All images that support unaccepted memory must now locate and call
>> this
>>> new BZ3987_ACCEPT_ALL_UNACCEPTED_MEMORY_PROTOCOL and call
>> the Disable
>>> function.
>>>
>>> Changes since v7:
>>>    - Rebased onto lazy accept v4 patch series, so memory accept protocol
>>>      has the EDKII prefix, and the unaccepted memory type has the BZ3937
>>>      prefix.
>>>    - Removed a bad #include to a header removed in v7.
>>>    - Renamed the protocol to BZ3987_MEMORY_ACCEPTANCE_PROTOCOL
>> as per the
>>>      discussion on the buganizer issue.
>>>    - Uncrustify formatting
>>>
>>> Changes since v6:
>>>    - Added implementation of
>> EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES.
>>>    - Changed callback protocol of v5 to instead use the standardized event
>>>      group for before_exit_boot_services.
>>>
>>> Changes since v5:
>>>    - Generic callback protocol moved to MdeModulePkg
>>>    - Removed use of EFI_WARN_STALE_DATA and added comment that the
>> callback
>>>      should only return EFI_SUCCESS or EFI_INVALID_PARAMETER.
>>>    - Removed errant log statement and fixed formatting.
>>>
>>> Changes since v4:
>>>    - Commit message wording
>>>    - Replaced direct change to DxeMain with a more generic callback
>>>      protocol.
>>>    - Implemented the direct change as an instance of the callback protocol
>>>      from a new CocoDxe driver.
>>>    - Replaced "enable" protocol with a "disable" protocol, since the name
>>>      was confusing. The AcceptAllUnacceptedMemory protocol directly
>> names
>>>      the behavior that is disabling.
>>>
>>> Changes since v3:
>>>    - "DxeMain accepts all memory" patch split into 3 to make each patch
>>>      affect only one package at a time.
>>>
>>> Changes since v2:
>>>    - Removed the redundant memory accept interface and added the accept
>>>      behavior to the DXE implementation of
>>>      MemEncryptSevSnpPreValidateSystemRam.
>>>    - Fixed missing #include in >=4GB patch.
>>>
>>> Changes since v1:
>>>    - Added a patch to classify SEV-SNP memory above 4GB unaccepted.
>>>    - Fixed style problems in EfiMemoryAcceptProtocol implementation.
>>>
>>> Cc: Ard Biescheuvel <ardb@kernel.org>
>>> Cc: "Min M. Xu" <min.m.xu@intel.com>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: James Bottomley <jejb@linux.ibm.com>
>>> Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Erdem Aktas <erdemaktas@google.com>
>>> Cc: Andrew Fish <afish@apple.com>
>>> Cc: "Michael D. Kinney" <michael.d.kinney@intel.com>
>>>
>>> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
>>>
>>> Dionna Glaze (7):
>>>     OvmfPkg: Realize EfiMemoryAcceptProtocol in AmdSevDxe
>>>     MdePkg: Add EFI_EVENT_BEFORE_EXIT_BOOT_SERVICES_GUID
>>>     MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices
>>>     OvmfPkg: Introduce CocoDxe driver
>>>     MdePkg: Introduce the MemoryAcceptance protocol
>>>     OvmfPkg: Implement AcceptAllUnacceptedMemory in CocoDxe
>>>     OvmfPkg/PlatformPei: SEV-SNP make >=4GB unaccepted
>>>
>>>    MdeModulePkg/Core/Dxe/DxeMain.inf                                  |   1 +
>>>    MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c                            |   6 +
>>>    MdePkg/Include/Guid/EventGroup.h                                   |   5 +
>>>    MdePkg/Include/Protocol/MemoryAcceptance.h                         |  40
>> +++++
>>>    MdePkg/MdePkg.dec                                                  |   8 +-
>>>    OvmfPkg/AmdSev/AmdSevX64.dsc                                       |   1 +
>>>    OvmfPkg/AmdSev/AmdSevX64.fdf                                       |   1 +
>>>    OvmfPkg/AmdSevDxe/AmdSevDxe.c                                      |  55 ++++++-
>>>    OvmfPkg/AmdSevDxe/AmdSevDxe.inf                                    |   3 +
>>>    OvmfPkg/CocoDxe/CocoDxe.c                                          | 174
>> ++++++++++++++++++++
>>>    OvmfPkg/CocoDxe/CocoDxe.inf                                        |  46 ++++++
>>>    OvmfPkg/IntelTdx/IntelTdxX64.dsc                                   |   1 +
>>>    OvmfPkg/IntelTdx/IntelTdxX64.fdf                                   |   1 +
>>>
>> OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.
>> c |  24 ++-
>>>    OvmfPkg/OvmfPkgIa32X64.dsc                                         |   1 +
>>>    OvmfPkg/OvmfPkgIa32X64.fdf                                         |   1 +
>>>    OvmfPkg/OvmfPkgX64.dsc                                             |   1 +
>>>    OvmfPkg/OvmfPkgX64.fdf                                             |   1 +
>>>    OvmfPkg/PlatformPei/AmdSev.c                                       |   5 +
>>>    19 files changed, 366 insertions(+), 9 deletions(-)
>>>    create mode 100644 MdePkg/Include/Protocol/MemoryAcceptance.h
>>>    create mode 100644 OvmfPkg/CocoDxe/CocoDxe.c
>>>    create mode 100644 OvmfPkg/CocoDxe/CocoDxe.inf
>>>
>>
>>
>> 
>>
>