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

["Laszlo Ersek" <lersek@redhat.com>]

On 1/9/23 11:15, Gerd Hoffmann wrote:
>>> +  @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.

Sigh, yes. Thanks for the reminder.

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

Right.

Another option (because the lifetime of the E820 table from QEMU is
strongly limited, as far as the firmware is concerned) is perhaps just
to have

  EFI_E820_ENTRY64 E820Entries[32];

somewhere higher up the call stack, as a local variable, and to pass it
along with the platform info hob wherever it is needed. Then, once this
"outer" function completes, the E820 map goes away too.

... Arguably, that would require changes to "PlatformInitLib.h" as well,
and would introduce quite a bit of churn in clients of PlatformInitLib.

Embedding the above fixed-size, 32-element array into
EFI_HOB_PLATFORM_INFO might be simplest.

Laszlo

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