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

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

On 1/6/23 15:04, Gerd Hoffmann wrote:
> Documentation of PlatformScanOrAdd64BitE820Ram() ran out of sync
> with the implementation.  Fix that.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/Library/PlatformInitLib/MemDetect.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index 0c4956852689..1255d6300fdd 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -121,15 +121,19 @@ PlatformQemuUc32BaseInitialization (
>    Find the highest exclusive >=4GB RAM address, or produce memory resource
>    descriptor HOBs for RAM entries that start at or above 4GB.
>  
> -  @param[out] MaxAddress  If MaxAddress is NULL, then PlatformScanOrAdd64BitE820Ram()
> +  @param[in] AddHighHob   If True then PlatformScanOrAdd64BitE820Ram()
>                            produces memory resource descriptor HOBs for RAM
>                            entries that start at or above 4GB.
> +                          It also produces HOBs for reserved entries.
>  
> -                          Otherwise, 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.
> +  @param[out] LowMemory  If Lowmemory is not NULL, then Lowmemory MaxAddress
> +                         holds the amout of emory below 4G on output.
> +

(1) The specification in the right hand side column is not aligned with
the other specs in the same column.

(2) Typo in "emory"

(3) Typo in "Lowmemory" (twice)

(4) "Lowmemory MaxAddress holds the amount ..." is probably another
typo, I don't understand it.

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

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.

So, I request that we please split this function up. There are two ways
to do that I guess:

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

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

typedef
VOID
(* ACCUMULATE_E820_ENTRY) (
  IN     CONST EFI_E820_ENTRY64 *E820Entry,
  IN OUT VOID                   *Context
  );

[The above need not be EFIAPI; so that omission is not an oversight.]

STATIC
EFI_STATUS
PlatformScanE820 (
  IN     ACCUMULATE_E820_ENTRY Accumulate,
  IN OUT VOID                  *Context
  )
{
  ...
  for (...) {
    ...
    Accumulate (&E820Entry, Context);
    ...
  }
  ...
}

Then the various call sites would pass in their own context pointers
(pointing to each's own pre-initialized context structure or even just
scalar variable), and compatible accumulator functions.

Quite a bit more code, but much cleaner, IMO. The documentation would
also be much-much simpler to get right.

(Right now, I'm proposing that the callback function return VOID. Later,
if it becomes necessary, the return type can be changed to EFI_STATUS,
and then a failure from the callback could -- if necessary -- abort the
outer loop, and make PlatformScanE820 return with a failure code early,
as well. But right now that's not needed.)

Thanks,
Laszlo