Re: [edk2-devel] [PATCH 1/2] OvmfPkg/PlatformInitLib: update PlatformScanOrAdd64BitE820Ram documentation

["Gerd Hoffmann" <kraxel@redhat.com>]

> > +  @param[out] MaxAddress  If MaxAddress is not NULL, then MaxAddress holds
> > +                          the highest exclusive >=4GB RAM address on output.
> > +                          If QEMU's fw_cfg E820 RAM map contains no RAM entry
> > +                          that starts outside of the 32-bit address range,
> > +                          then MaxAddress is exactly 4GB on output.
> >  
> >    @retval EFI_SUCCESS         The fw_cfg E820 RAM map was found and processed.
> >  
> 
> I've tried to review the function in its current state, but I don't
> understand the code either. Originally this function had two behaviors,
> reflected by its name as well ("Scan Or Add 64 Bit E820 Ram"), and its
> sole (NULL-able) parameter MaxAddress would switch between those two
> behaviors. The single "if" in the loop body, and the loop body in
> general was trivial -- and AddMemoryRangeHob() call on one branch, and a
> maximum search/comparison step on the other. Entry types other than
> EfiAcpiAddressRangeMemory were summarily ignored.
> 
> Now the function does many more things, especially at the end of this
> series. It does things for EfiAcpiAddressRangeReserved, but only when
> AddHighHob is TRUE. It implements a maximum search for LowMemory as
> well. The function name "Scan Or Add 64 Bit E820 Ram" has become a
> misnomer. It's not just the function comment block that is out of date,
> but the function's name too.
> 
> The function's initially simple structure can clealy not carry all its
> new tasks; I'm struggling to read the function definition. This is best
> shown by the multiple calls to the function in the code base, where we
> have a plethora of NULLs and TRUE/FALSE arguments, much obscuring the
> intended purpose of those calls.

Well, it's not *that* different from the original.  The implicit add-hob
request (via MaxAddress == NULL) has been replaced by an explicit bool.
MaxAddress works the same way it used to when non-NULL.
LowMemory has the same behavior (set to non-NULL to have the value returned).

Handling reservation hobs and reservation conflicts too doesn't fit in
that well indeed.

> The reason I originally wrote the function the way I did is that it
> would run in PEI. Small memory allocations go into HOBs in PEI, and
> cannot be freed (see FreePool() in
> "MdePkg/Library/PeiMemoryAllocationLib/MemoryAllocationLib.c"). Page
> allocations work, but I deemed that overkill, and there would be only
> two calls to this function anyway. Therefore, looping through the fw_cfg
> file twice, even using Port IO (for example in a SEV guest) would not be
> a big deal, not to mention when DMA would be available (the common case).
> 
> But that no longer holds. We have a bunch of calls now.

The code need to also work in SEC now.

Using a HOB should work too, and given that the number of e820 entries
is rather small (2-4 entries with 20 bytes each) it might not be that
much of a problem to have that permanently allocated.

I think a page allocator is not available in SEC.

> (1) Perform an initial set of checks, for the existence & proper size,
> of the fw_cfg file. Allocate the necessary number of pages, download the
> file, before the first scan. Implement all the scans based on the
> downloaded file, with separate, open-coded loops at every current call
> site. After the last use, release the pages.

Looks like the better option to me.

> (2) Alternatively, keep the current, outermost, checking and looping
> logic in the function, so that we not need dynamic memory just like
> before. However, the internals should be broken out, by taking a
> callback function pointer as parameter. The callback function would have
> two parameters: the E820 Entry just found, and a "VOID *Context" pointer.

Was thinking about that one, but I don't like passing around VOID
pointers and the overhead coming from the 4 callback functions.

Maybe I can pass around EFI_HOB_PLATFORM_INFO pointers instead.

take care,
  Gerd