Re: [edk2-devel] 回复: [PATCH V4 00/10] Introduce Lazy-accept for Tdx guest

["Min Xu" <min.m.xu@intel.com>]

The name of EDKII_MEMORY_ACCEPT_PROTOCOL indicates it is only used in edk2. It's not going to be included in UEFI spec, at least in current stage.
In our design of the protocol (EDKII_MEMORY_ACCEPT_PROTOCOL), it contains the function of EDKII_ACCEPT_MEMORY which is to accept the un-accepted memories based on the input address and size. We just want to make the protocol clear and straightforward.

> -----Original Message-----
> From: Dionna Amalie Glaze <dionnaglaze@google.com>
> Sent: Monday, October 10, 2022 11:51 PM
> To: Ni, Ray <ray.ni@intel.com>
> Cc: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io; Gao, Liming
> <gaoliming@byosoft.com.cn>; Gao, Zhichao <zhichao.gao@intel.com>; Aktas,
> Erdem <erdemaktas@google.com>; Gerd Hoffmann <kraxel@redhat.com>;
> James Bottomley <jejb@linux.ibm.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; Gao,
> Jiaqi <jiaqi.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Liu,
> Zhiguang <zhiguang.liu@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] 回复: [PATCH V4 00/10] Introduce Lazy-accept for
> Tdx guest
> 
> >
> > Can OS call AcceptMemory protocol for those ranges that are not accepted?
> >
> 
> AcceptMemory is not specified to avoid accepting previously accepted
> memory. As I understand it, AcceptMemory is purely a hardware abstraction
> layer for CC technologies inside the UEFI implementation.
> It additionally is not meant to modify address spaces. Address space
> modification happens around it. Gao has a point though, that the two could
> be combined. I'm not sure if it's particularly helpful to expose AcceptMemory
> to the OS. Exposing it I think would necessitate changing its semantics to be
> safer, e.g.,
> 
> Use the insight that AcceptMemory is only used to accept full or partial
> regions of unaccepted memory spaces:
> 
> /**
>  * Accepts memory in page granularity from the beginning of a pointed-to
> memory descriptor, and changes
>  * the type of the affected memory range to EfiConventionalMemory.
>  *
>  * @param[This] This A pointer to this protocol instance
>  * @param[AddressInSpace] An address within the memory descriptor from
> which to accept pages.
>  * @param[NumPages] The amount of EFI_PAGE_SIZE blocks of memory to
> accept from the memory
>  *  descriptor's beginning and convert into EfiConventionalMemory. If pages
> remain in the memory descriptor
>  * after acceptance, the remaining memory will start at the initial memory
> descriptor's
>  * start address + NumPages * EFI_PAGE_SIZE
>  * with type EfiUnacceptedMemory.
>  * The changes to the memory map affect only the memory descriptor
> named by AddressInSpace.
>  *
>  * Returns EFI_INVALID_PARAMETER if AddressInSpace names to a memory
> descriptor that is not
>  * EfiUnacceptedMemory, or if the named memory descriptor is not at least
> NumPages in size.
>  */
> EFI_STATUS EFIAPI AcceptFromMemorySpaceBeginning (
>   IN ProtocolType *This,
>   IN EFI_VIRTUAL_ADDRESS AddressInSpace,
>   IN UINTN NumPages
>   );
> 
> /**
>  * To be called by the OS loader to indicate that it supports and accepts
> responsibility for EfiUnacceptedMemory.
>  *
>  * Without calling this function, ExitBootServices will accept all unaccepted
> memory before returning. This
>  * behavior maintains safety for OSes that do not support unaccepted
> memory or know of this protocol.
>  */
> VOID EFIAPI DisableAcceptAllOnExitBootServices (IN ProtocolType *This);
> 
> I think this could be a fine protocol to expose to the OS loader since it would
> be safer written this way, albeit AcceptFromMemorySpaceBeginning is rather
> redundant for the behavior that the OS would need to implement if it calls
> the disable function.
> 
> I'm not too pleased about the naming behavior, but I also don't really like
> requiring the interface to only accept the start address of any particular
> memory descriptor. That's a matter of taste though. The implementation of
> the memory descriptor search would not be much more complicated than a
> couple inequality checks instead of a single equality check.
> 
> I don't think it's worth the effort in this interface to allow an arbitrary range
> that could split a single memory descriptor into at most three instead of at
> most two, since it is logic I don't think would be readily exercised. Given that
> we're talking about calling this function given knowledge of the MemoryMap,
> and that the MemoryMap should be condensed to not have separate
> memory descriptors that could be coalesced, I think the limitation that
> NumPages fits within the single descriptor is reasonable.
> 
> All this being said, what's the value of combining the protocols? One fewer
> header and guid? I honestly don't know since I haven't been around long
> enough to understand how these kinds of things evolve and create possible
> warts.
> 
> If it's just two two things though, I think a header and guid are worth
> avoiding confusion by exposing AcceptMemory unnecessarily.
> 
> > > -----Original Message-----
> > > From: Xu, Min M <min.m.xu@intel.com>
> > > Sent: Monday, October 10, 2022 11:08 AM
> > > To: devel@edk2.groups.io; Dionna Amalie Glaze
> > > <dionnaglaze@google.com>; Gao, Liming <gaoliming@byosoft.com.cn>
> > > Cc: Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray
> > > <ray.ni@intel.com>; Aktas, Erdem <erdemaktas@google.com>; 'Gerd
> Hoffmann'
> > > <kraxel@redhat.com>; 'James Bottomley' <jejb@linux.ibm.com>; Yao,
> > > Jiewen <jiewen.yao@intel.com>; 'Tom Lendacky'
> > > <thomas.lendacky@amd.com>; Gao, Jiaqi <jiaqi.gao@intel.com>; Wang,
> > > Jian J <jian.j.wang@intel.com>; Liu, Zhiguang
> > > <zhiguang.liu@intel.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Xu, Min M <min.m.xu@intel.com>
> > > Subject: RE: [edk2-devel] 回复: [PATCH V4 00/10] Introduce Lazy-accept
> > > for Tdx guest
> > >
> > > On October 10, 2022 10:28 AM, Gao Liming wrote:
> > > >
> > > > Min:
> > > >   I have no comments for new unaccepted resource type and
> > > > unaccepted
> > > gcd
> > > > type. In fact, they are mapping to UEFI EfiUnacceptedMemoryType.
> > > >
> > > >   For new protocol EfiMemoryAcceptProtocol, I see another patch
> > > > serial
> > > > https://edk2.groups.io/g/devel/message/94763 base on it to
> > > > introduce ACCEPT_ALL_UNACCEPTED_MEMORY_PROTOCOL. Can these
> two
> > > protocols
> > > > be combined into one?
> > > >
> > > EfiMemoryAcceptProtocol looks like this:
> > > typedef
> > > EFI_STATUS
> > > (EFIAPI *EDKII_ACCEPT_MEMORY)(
> > >   IN  EDKII_MEMORY_ACCEPT_PROTOCOL  *This,
> > >   IN  EFI_PHYSICAL_ADDRESS                          StartAddress,
> > >   IN  UINTN                                                         Size
> > >   );
> > > This protocol is called to accept the memory based on the input
> > > start address and size.
> > >
> > > While ACCEPT_ALL_UNACCEPTED_MEMORY_PROTOCOL looks like below:
> > > typedef
> > > EFI_STATUS
> > > (EFIAPI *BZ3987_DISABLE_ACCEPT_ALL_UNACCEPTED_MEMORY)(
> > >   IN  BZ3987_ACCEPT_ALL_UNACCEPTED_MEMORY_PROTOCOL  *This );
> > > According to its description
> > > (https://edk2.groups.io/g/devel/message/94768)
> > > this protocol is used to disable the behavior of accepting all
> > > unaccepted memory. And it is designed to be called by the OS loader, not
> EDK2 itself.
> > >
> > > I am afraid these 2 protocols cannot be combined into one.
> > >
> > > Dionna what's your thought?
> > >
> > > Thanks
> > > Min
> 
> 
> 
> --
> -Dionna Glaze, PhD (she/her)