> > + @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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98179): https://edk2.groups.io/g/devel/message/98179
Mute This Topic: https://groups.io/mt/96093485/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
