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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98124): https://edk2.groups.io/g/devel/message/98124
Mute This Topic: https://groups.io/mt/96093485/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98173): https://edk2.groups.io/g/devel/message/98173
Mute This Topic: https://groups.io/mt/96093485/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> > + @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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98180): https://edk2.groups.io/g/devel/message/98180
Mute This Topic: https://groups.io/mt/96093485/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.