[PATCH 0/2] OvmfPkg: check 64bit mmio window for resource conflicts

[PATCH 0/2] OvmfPkg: check 64bit mmio window for resource conflicts

[Gerd Hoffmann]

Gerd Hoffmann (2):
  OvmfPkg/PlatformInitLib: update PlatformScanOrAdd64BitE820Ram
    documentation
  OvmfPkg/PlatformInitLib: check 64bit mmio window for resource
    conflicts

 OvmfPkg/Library/PlatformInitLib/MemDetect.c | 63 +++++++++++++++++----
 1 file changed, 51 insertions(+), 12 deletions(-)

-- 
2.39.0

[PATCH 1/2] OvmfPkg/PlatformInitLib: update PlatformScanOrAdd64BitE820Ram documentation

[Gerd Hoffmann]

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

[PATCH 2/2] OvmfPkg/PlatformInitLib: check 64bit mmio window for resource conflicts

[Gerd Hoffmann]

Add new operation mode to PlatformScanOrAdd64BitE820Ram() which checks
whenever any reservations from the qemu e820 table overlap with the mmio
window.  Should that be the case move it to avoid the conflict.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/Library/PlatformInitLib/MemDetect.c | 47 ++++++++++++++++++---
 1 file changed, 41 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index 1255d6300fdd..4f5a04832a4e 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -126,6 +126,12 @@ PlatformQemuUc32BaseInitialization (
                           entries that start at or above 4GB.
                           It also produces HOBs for reserved entries.
 
+  @param[in out] PlatformInfoHob  If Lowmemory PlatformInfoHob is not NULL,
+                                  then PlatformScanOrAdd64BitE820Ram() checks
+                                  the 64bit PCI MMIO window against conflicts
+                                  with e820 reservations from qemu.  If needed
+                                  the MMIO window will be moved down.
+
   @param[out] LowMemory  If Lowmemory is not NULL, then Lowmemory MaxAddress
                          holds the amout of emory below 4G on output.
 
@@ -147,9 +153,10 @@ PlatformQemuUc32BaseInitialization (
 STATIC
 EFI_STATUS
 PlatformScanOrAdd64BitE820Ram (
-  IN BOOLEAN  AddHighHob,
-  OUT UINT64  *LowMemory OPTIONAL,
-  OUT UINT64  *MaxAddress OPTIONAL
+  IN BOOLEAN                    AddHighHob,
+  IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob OPTIONAL,
+  OUT UINT64                    *LowMemory OPTIONAL,
+  OUT UINT64                    *MaxAddress OPTIONAL
   )
 {
   EFI_STATUS            Status;
@@ -249,6 +256,33 @@ PlatformScanOrAdd64BitE820Ram (
           E820Entry.Length
           );
       }
+
+      if (PlatformInfoHob) {
+        UINT64  IntersectionBase = MAX (
+                                     E820Entry.BaseAddr,
+                                     PlatformInfoHob->PcdPciMmio64Base
+                                     );
+        UINT64  IntersectionEnd = MIN (
+                                    E820Entry.BaseAddr + E820Entry.Length,
+                                    PlatformInfoHob->PcdPciMmio64Base +
+                                    PlatformInfoHob->PcdPciMmio64Size
+                                    );
+
+        if (IntersectionBase < IntersectionEnd) {
+          // have overlap, so move mmio window down from end of address space
+          UINT64  NewBase = (PlatformInfoHob->PcdPciMmio64Base -
+                             PlatformInfoHob->PcdPciMmio64Size);
+
+          DEBUG ((
+            DEBUG_INFO,
+            "%a: move mmio: 0x%Lx => %Lx\n",
+            __FUNCTION__,
+            PlatformInfoHob->PcdPciMmio64Base,
+            NewBase
+            ));
+          PlatformInfoHob->PcdPciMmio64Base = NewBase;
+        }
+      }
     }
   }
 
@@ -340,7 +374,7 @@ PlatformGetSystemMemorySizeBelow4gb (
     return (UINT32)GetHighestSystemMemoryAddressFromPvhMemmap (TRUE);
   }
 
-  Status = PlatformScanOrAdd64BitE820Ram (FALSE, &LowerMemorySize, NULL);
+  Status = PlatformScanOrAdd64BitE820Ram (FALSE, NULL, &LowerMemorySize, NULL);
   if ((Status == EFI_SUCCESS) && (LowerMemorySize > 0)) {
     return (UINT32)LowerMemorySize;
   }
@@ -412,7 +446,7 @@ PlatformGetFirstNonAddress (
   // Otherwise, get the flat size of the memory above 4GB from the CMOS (which
   // can only express a size smaller than 1TB), and add it to 4GB.
   //
-  Status = PlatformScanOrAdd64BitE820Ram (FALSE, NULL, &FirstNonAddress);
+  Status = PlatformScanOrAdd64BitE820Ram (FALSE, NULL, NULL, &FirstNonAddress);
   if (EFI_ERROR (Status)) {
     FirstNonAddress = BASE_4GB + PlatformGetSystemMemorySizeAbove4gb ();
   }
@@ -648,6 +682,7 @@ PlatformDynamicMmioWindow (
     DEBUG ((DEBUG_INFO, "%a:   MMIO Space 0x%Lx (%Ld GB)\n", __func__, MmioSpace, RShiftU64 (MmioSpace, 30)));
     PlatformInfoHob->PcdPciMmio64Size = MmioSpace;
     PlatformInfoHob->PcdPciMmio64Base = AddrSpace - MmioSpace;
+    PlatformScanOrAdd64BitE820Ram (FALSE, PlatformInfoHob, NULL, NULL);
   } else {
     DEBUG ((DEBUG_INFO, "%a: using classic mmio window\n", __func__));
   }
@@ -960,7 +995,7 @@ PlatformQemuInitializeRam (
     // entries. Otherwise, create a single memory HOB with the flat >=4GB
     // memory size read from the CMOS.
     //
-    Status = PlatformScanOrAdd64BitE820Ram (TRUE, NULL, NULL);
+    Status = PlatformScanOrAdd64BitE820Ram (TRUE, NULL, NULL, NULL);
     if (EFI_ERROR (Status)) {
       UpperMemorySize = PlatformGetSystemMemorySizeAbove4gb ();
       if (UpperMemorySize != 0) {
-- 
2.39.0

Re: [PATCH 0/2] OvmfPkg: check 64bit mmio window for resource conflicts

[Ard Biesheuvel]

Hi Gerd,

On Fri, 6 Jan 2023 at 15:04, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>
>
> Gerd Hoffmann (2):
>   OvmfPkg/PlatformInitLib: update PlatformScanOrAdd64BitE820Ram
>     documentation
>   OvmfPkg/PlatformInitLib: check 64bit mmio window for resource
>     conflicts
>
>  OvmfPkg/Library/PlatformInitLib/MemDetect.c | 63 +++++++++++++++++----
>  1 file changed, 51 insertions(+), 12 deletions(-)
>

Mind adding a teeny bit of context to explain why this is needed? No
need to respin or anything - just want to make sure i understand the
problem and the solution.

Re: [PATCH 0/2] OvmfPkg: check 64bit mmio window for resource conflicts

[Gerd Hoffmann]

On Fri, Jan 06, 2023 at 03:14:53PM +0100, Ard Biesheuvel wrote:
> Hi Gerd,
> 
> On Fri, 6 Jan 2023 at 15:04, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >
> >
> > Gerd Hoffmann (2):
> >   OvmfPkg/PlatformInitLib: update PlatformScanOrAdd64BitE820Ram
> >     documentation
> >   OvmfPkg/PlatformInitLib: check 64bit mmio window for resource
> >     conflicts
> >
> >  OvmfPkg/Library/PlatformInitLib/MemDetect.c | 63 +++++++++++++++++----
> >  1 file changed, 51 insertions(+), 12 deletions(-)
> >
> 
> Mind adding a teeny bit of context to explain why this is needed? No
> need to respin or anything - just want to make sure i understand the
> problem and the solution.

qemu reserves some address space below 1TB when emulating amd processors
because that is the address range used by the amd iommu.  This avoids
placing the mmio window at a overlapping range.

https://bugzilla.tianocore.org/show_bug.cgi?id=4251

(guess I should add a link to the commit message ...)

take care,
  Gerd

Re: [PATCH 0/2] OvmfPkg: check 64bit mmio window for resource conflicts

[Yao, Jiewen]

I recommend also add detailed information in the code as comment.

That can help people who only reads the code directly.

Thank you
Yao, Jiewen

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Friday, January 6, 2023 11:32 PM
> To: Ard Biesheuvel <ardb@kernel.org>
> Cc: devel@edk2.groups.io; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> László Érsek <lersek@redhat.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Pawel Polawski <ppolawsk@redhat.com>;
> Oliver Steffen <osteffen@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [PATCH 0/2] OvmfPkg: check 64bit mmio window for resource
> conflicts
> 
> On Fri, Jan 06, 2023 at 03:14:53PM +0100, Ard Biesheuvel wrote:
> > Hi Gerd,
> >
> > On Fri, 6 Jan 2023 at 15:04, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > >
> > >
> > > Gerd Hoffmann (2):
> > >   OvmfPkg/PlatformInitLib: update PlatformScanOrAdd64BitE820Ram
> > >     documentation
> > >   OvmfPkg/PlatformInitLib: check 64bit mmio window for resource
> > >     conflicts
> > >
> > >  OvmfPkg/Library/PlatformInitLib/MemDetect.c | 63
> +++++++++++++++++----
> > >  1 file changed, 51 insertions(+), 12 deletions(-)
> > >
> >
> > Mind adding a teeny bit of context to explain why this is needed? No
> > need to respin or anything - just want to make sure i understand the
> > problem and the solution.
> 
> qemu reserves some address space below 1TB when emulating amd
> processors
> because that is the address range used by the amd iommu.  This avoids
> placing the mmio window at a overlapping range.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=4251
> 
> (guess I should add a link to the commit message ...)
> 
> take care,
>   Gerd

Re: [edk2-devel] [PATCH 2/2] OvmfPkg/PlatformInitLib: check 64bit mmio window for resource conflicts

[Pedro Falcato]

[-- Attachment #1: Type: text/plain, Size: 527 bytes --]

On Fri, Jan 6, 2023 at 2:04 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> Add new operation mode to PlatformScanOrAdd64BitE820Ram() which checks
> whenever any reservations from the qemu e820 table overlap with the mmio
> window.  Should that be the case move it to avoid the conflict.
>
Hi,

In what case could this happen? Does QEMU ever place e820 entries inside
the PCI MMIO window? Why?
(In fact, when does QEMU even add a reservation in the e820? I've never
seen one.)
Just checking for QemuOpenBoardPkg.

Thanks,
Pedro

[-- Attachment #2: Type: text/html, Size: 883 bytes --]

Re: [edk2-devel] [PATCH 2/2] OvmfPkg/PlatformInitLib: check 64bit mmio window for resource conflicts

[Gerd Hoffmann]

On Fri, Jan 06, 2023 at 08:39:38PM +0000, Pedro Falcato wrote:
> On Fri, Jan 6, 2023 at 2:04 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> > Add new operation mode to PlatformScanOrAdd64BitE820Ram() which checks
> > whenever any reservations from the qemu e820 table overlap with the mmio
> > window.  Should that be the case move it to avoid the conflict.
> >
> Hi,
> 
> In what case could this happen? Does QEMU ever place e820 entries inside
> the PCI MMIO window? Why?

Well, the *firmware* picks where it places the PCI MMIO window.  So it's
not qemu creating a conflict ;)

> (In fact, when does QEMU even add a reservation in the e820? I've never
> seen one.)

New enough qemu adds a reservation just below 1TB when running on AMD
processors (or emulating one with tcg).  That window is used by the
iommu.

take care,
  Gerd

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

[Laszlo Ersek]

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

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

[Gerd Hoffmann]

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

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

[Laszlo Ersek]

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
>