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

["Dionna Glaze" <dionnaglaze@google.com>]

>
> 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)