Add PlatformAddHobCB() callback function for use with
PlatformScanE820().  It adds HOBs for high memory and reservations (low
memory is handled elsewhere because there are some special cases to
consider).  This replaces calls to PlatformScanOrAdd64BitE820Ram() with
AddHighHobs = TRUE.

Also remove PlatformScanOrAdd64BitE820Ram() which is not used any more.

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

diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index 63329c4e796a..83a219581a1b 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -112,143 +112,6 @@ PlatformQemuUc32BaseInitialization (
   }
 }
 
-/**
-  Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map that start outside
-  of the 32-bit address range.
-
-  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()
-                          produces memory resource descriptor HOBs for RAM
-                          entries that start at or above 4GB.
-
-                          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.
-
-  @retval EFI_SUCCESS         The fw_cfg E820 RAM map was found and processed.
-
-  @retval EFI_PROTOCOL_ERROR  The RAM map was found, but its size wasn't a
-                              whole multiple of sizeof(EFI_E820_ENTRY64). No
-                              RAM entry was processed.
-
-  @return                     Error codes from QemuFwCfgFindFile(). No RAM
-                              entry was processed.
-**/
-STATIC
-EFI_STATUS
-PlatformScanOrAdd64BitE820Ram (
-  IN BOOLEAN  AddHighHob,
-  OUT UINT64  *LowMemory OPTIONAL,
-  OUT UINT64  *MaxAddress OPTIONAL
-  )
-{
-  EFI_STATUS            Status;
-  FIRMWARE_CONFIG_ITEM  FwCfgItem;
-  UINTN                 FwCfgSize;
-  EFI_E820_ENTRY64      E820Entry;
-  UINTN                 Processed;
-
-  Status = QemuFwCfgFindFile ("etc/e820", &FwCfgItem, &FwCfgSize);
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
-  if (FwCfgSize % sizeof E820Entry != 0) {
-    return EFI_PROTOCOL_ERROR;
-  }
-
-  if (LowMemory != NULL) {
-    *LowMemory = 0;
-  }
-
-  if (MaxAddress != NULL) {
-    *MaxAddress = BASE_4GB;
-  }
-
-  QemuFwCfgSelectItem (FwCfgItem);
-  for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) {
-    QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry);
-    DEBUG ((
-      DEBUG_VERBOSE,
-      "%a: Base=0x%Lx Length=0x%Lx Type=%u\n",
-      __FUNCTION__,
-      E820Entry.BaseAddr,
-      E820Entry.Length,
-      E820Entry.Type
-      ));
-    if (E820Entry.Type == EfiAcpiAddressRangeMemory) {
-      if (AddHighHob && (E820Entry.BaseAddr >= BASE_4GB)) {
-        UINT64  Base;
-        UINT64  End;
-
-        //
-        // Round up the start address, and round down the end address.
-        //
-        Base = ALIGN_VALUE (E820Entry.BaseAddr, (UINT64)EFI_PAGE_SIZE);
-        End  = (E820Entry.BaseAddr + E820Entry.Length) &
-               ~(UINT64)EFI_PAGE_MASK;
-        if (Base < End) {
-          PlatformAddMemoryRangeHob (Base, End);
-          DEBUG ((
-            DEBUG_VERBOSE,
-            "%a: PlatformAddMemoryRangeHob [0x%Lx, 0x%Lx)\n",
-            __FUNCTION__,
-            Base,
-            End
-            ));
-        }
-      }
-
-      if (MaxAddress || LowMemory) {
-        UINT64  Candidate;
-
-        Candidate = E820Entry.BaseAddr + E820Entry.Length;
-        if (MaxAddress && (Candidate > *MaxAddress)) {
-          *MaxAddress = Candidate;
-          DEBUG ((
-            DEBUG_VERBOSE,
-            "%a: MaxAddress=0x%Lx\n",
-            __FUNCTION__,
-            *MaxAddress
-            ));
-        }
-
-        if (LowMemory && (Candidate > *LowMemory) && (Candidate < BASE_4GB)) {
-          *LowMemory = Candidate;
-          DEBUG ((
-            DEBUG_VERBOSE,
-            "%a: LowMemory=0x%Lx\n",
-            __FUNCTION__,
-            *LowMemory
-            ));
-        }
-      }
-    } else if (E820Entry.Type == EfiAcpiAddressRangeReserved) {
-      if (AddHighHob) {
-        DEBUG ((
-          DEBUG_INFO,
-          "%a: Reserved: Base=0x%Lx Length=0x%Lx\n",
-          __FUNCTION__,
-          E820Entry.BaseAddr,
-          E820Entry.Length
-          ));
-        BuildResourceDescriptorHob (
-          EFI_RESOURCE_MEMORY_RESERVED,
-          0,
-          E820Entry.BaseAddr,
-          E820Entry.Length
-          );
-      }
-    }
-  }
-
-  return EFI_SUCCESS;
-}
-
 typedef VOID (*E820_SCAN_CALLBACK) (
   EFI_E820_ENTRY64       *E820Entry,
   EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
@@ -304,6 +167,41 @@ PlatformGetLowMemoryCB (
   }
 }
 
+/**
+  Create HOBs for reservations and RAM (except low memory).
+**/
+VOID
+PlatformAddHobCB (
+  IN     EFI_E820_ENTRY64       *E820Entry,
+  IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
+  )
+{
+  UINT64  Base = E820Entry->BaseAddr;
+  UINT64  End  = E820Entry->BaseAddr + E820Entry->Length;
+
+  switch (E820Entry->Type) {
+    case EfiAcpiAddressRangeMemory:
+      //
+      // Round up the start address, and round down the end address.
+      //
+      Base = ALIGN_VALUE (Base, (UINT64)EFI_PAGE_SIZE);
+      End  = End & ~(UINT64)EFI_PAGE_MASK;
+      if ((Base >= BASE_4GB) && (Base < End)) {
+        DEBUG ((DEBUG_INFO, "%a: HighMemory [0x%Lx, 0x%Lx]\n", __FUNCTION__, Base, End));
+        PlatformAddMemoryRangeHob (Base, End);
+      }
+
+      break;
+    case EfiAcpiAddressRangeReserved:
+      BuildResourceDescriptorHob (EFI_RESOURCE_MEMORY_RESERVED, 0, Base, End);
+      DEBUG ((DEBUG_INFO, "%a: Reserved [0x%Lx, 0x%Lx]\n", __FUNCTION__, Base, End));
+      break;
+    default:
+      DEBUG ((DEBUG_WARN, "%a: Type %d [0x%Lx, 0x%Lx] (NOT HANDLED)\n", __FUNCTION__, E820Entry->Type, Base, End));
+      break;
+  }
+}
+
 /**
   Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map, call the
   passed callback for each entry.
@@ -1048,7 +946,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 = PlatformScanE820 (PlatformAddHobCB, PlatformInfoHob);
     if (EFI_ERROR (Status)) {
       UpperMemorySize = PlatformGetSystemMemorySizeAbove4gb ();
       if (UpperMemorySize != 0) {
-- 
2.39.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98241): https://edk2.groups.io/g/devel/message/98241
Mute This Topic: https://groups.io/mt/96173192/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 1/10/23 09:21, Gerd Hoffmann wrote:
> Add PlatformAddHobCB() callback function for use with
> PlatformScanE820().  It adds HOBs for high memory and reservations (low
> memory is handled elsewhere because there are some special cases to
> consider).  This replaces calls to PlatformScanOrAdd64BitE820Ram() with
> AddHighHobs = TRUE.
>
> Also remove PlatformScanOrAdd64BitE820Ram() which is not used any more.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/Library/PlatformInitLib/MemDetect.c | 174 ++++----------------
>  1 file changed, 36 insertions(+), 138 deletions(-)
>
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index 63329c4e796a..83a219581a1b 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -112,143 +112,6 @@ PlatformQemuUc32BaseInitialization (
>    }
>  }
>
> -/**
> -  Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map that start outside
> -  of the 32-bit address range.
> -
> -  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()
> -                          produces memory resource descriptor HOBs for RAM
> -                          entries that start at or above 4GB.
> -
> -                          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.
> -
> -  @retval EFI_SUCCESS         The fw_cfg E820 RAM map was found and processed.
> -
> -  @retval EFI_PROTOCOL_ERROR  The RAM map was found, but its size wasn't a
> -                              whole multiple of sizeof(EFI_E820_ENTRY64). No
> -                              RAM entry was processed.
> -
> -  @return                     Error codes from QemuFwCfgFindFile(). No RAM
> -                              entry was processed.
> -**/
> -STATIC
> -EFI_STATUS
> -PlatformScanOrAdd64BitE820Ram (
> -  IN BOOLEAN  AddHighHob,
> -  OUT UINT64  *LowMemory OPTIONAL,
> -  OUT UINT64  *MaxAddress OPTIONAL
> -  )
> -{
> -  EFI_STATUS            Status;
> -  FIRMWARE_CONFIG_ITEM  FwCfgItem;
> -  UINTN                 FwCfgSize;
> -  EFI_E820_ENTRY64      E820Entry;
> -  UINTN                 Processed;
> -
> -  Status = QemuFwCfgFindFile ("etc/e820", &FwCfgItem, &FwCfgSize);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
> -  if (FwCfgSize % sizeof E820Entry != 0) {
> -    return EFI_PROTOCOL_ERROR;
> -  }
> -
> -  if (LowMemory != NULL) {
> -    *LowMemory = 0;
> -  }
> -
> -  if (MaxAddress != NULL) {
> -    *MaxAddress = BASE_4GB;
> -  }
> -
> -  QemuFwCfgSelectItem (FwCfgItem);
> -  for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) {
> -    QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry);
> -    DEBUG ((
> -      DEBUG_VERBOSE,
> -      "%a: Base=0x%Lx Length=0x%Lx Type=%u\n",
> -      __FUNCTION__,
> -      E820Entry.BaseAddr,
> -      E820Entry.Length,
> -      E820Entry.Type
> -      ));
> -    if (E820Entry.Type == EfiAcpiAddressRangeMemory) {
> -      if (AddHighHob && (E820Entry.BaseAddr >= BASE_4GB)) {
> -        UINT64  Base;
> -        UINT64  End;
> -
> -        //
> -        // Round up the start address, and round down the end address.
> -        //
> -        Base = ALIGN_VALUE (E820Entry.BaseAddr, (UINT64)EFI_PAGE_SIZE);
> -        End  = (E820Entry.BaseAddr + E820Entry.Length) &
> -               ~(UINT64)EFI_PAGE_MASK;
> -        if (Base < End) {
> -          PlatformAddMemoryRangeHob (Base, End);
> -          DEBUG ((
> -            DEBUG_VERBOSE,
> -            "%a: PlatformAddMemoryRangeHob [0x%Lx, 0x%Lx)\n",
> -            __FUNCTION__,
> -            Base,
> -            End
> -            ));
> -        }
> -      }
> -
> -      if (MaxAddress || LowMemory) {
> -        UINT64  Candidate;
> -
> -        Candidate = E820Entry.BaseAddr + E820Entry.Length;
> -        if (MaxAddress && (Candidate > *MaxAddress)) {
> -          *MaxAddress = Candidate;
> -          DEBUG ((
> -            DEBUG_VERBOSE,
> -            "%a: MaxAddress=0x%Lx\n",
> -            __FUNCTION__,
> -            *MaxAddress
> -            ));
> -        }
> -
> -        if (LowMemory && (Candidate > *LowMemory) && (Candidate < BASE_4GB)) {
> -          *LowMemory = Candidate;
> -          DEBUG ((
> -            DEBUG_VERBOSE,
> -            "%a: LowMemory=0x%Lx\n",
> -            __FUNCTION__,
> -            *LowMemory
> -            ));
> -        }
> -      }
> -    } else if (E820Entry.Type == EfiAcpiAddressRangeReserved) {
> -      if (AddHighHob) {
> -        DEBUG ((
> -          DEBUG_INFO,
> -          "%a: Reserved: Base=0x%Lx Length=0x%Lx\n",
> -          __FUNCTION__,
> -          E820Entry.BaseAddr,
> -          E820Entry.Length
> -          ));
> -        BuildResourceDescriptorHob (
> -          EFI_RESOURCE_MEMORY_RESERVED,
> -          0,
> -          E820Entry.BaseAddr,
> -          E820Entry.Length
> -          );
> -      }
> -    }
> -  }
> -
> -  return EFI_SUCCESS;
> -}
> -
>  typedef VOID (*E820_SCAN_CALLBACK) (
>    EFI_E820_ENTRY64       *E820Entry,
>    EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
> @@ -304,6 +167,41 @@ PlatformGetLowMemoryCB (
>    }
>  }
>
> +/**
> +  Create HOBs for reservations and RAM (except low memory).
> +**/
> +VOID
> +PlatformAddHobCB (
> +  IN     EFI_E820_ENTRY64       *E820Entry,
> +  IN OUT EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
> +  )
> +{
> +  UINT64  Base = E820Entry->BaseAddr;
> +  UINT64  End  = E820Entry->BaseAddr + E820Entry->Length;

(1) To my understanding, the edk2 coding style forbids initialization of
variables with automatic storage duration ("non-static locals").

> +
> +  switch (E820Entry->Type) {
> +    case EfiAcpiAddressRangeMemory:

(Side comment: I'm sure you've run this through Uncrustify -- so that's
a coding style change then. What I remember is that "switch" and "case"
were supposed to start in the same column.)

> +      //
> +      // Round up the start address, and round down the end address.
> +      //
> +      Base = ALIGN_VALUE (Base, (UINT64)EFI_PAGE_SIZE);
> +      End  = End & ~(UINT64)EFI_PAGE_MASK;
> +      if ((Base >= BASE_4GB) && (Base < End)) {

(2) This is not faithful conversion. The original code first compares
the base against 4GB, then rounds it up; this variant reverses the order
of those steps. I don't think if it will ever matter, but this is a
refactoring, so let's stick with the original logic (not hard to do).


> +        DEBUG ((DEBUG_INFO, "%a: HighMemory [0x%Lx, 0x%Lx]\n", __FUNCTION__, Base, End));

(3) I've not noticed before, but I'm noticing now: before this series,
the DEBUG levels (or more precisely, "debug masks") for the messages
emitted by PlatformScanOrAdd64BitE820Ram() were inconsistent. Most of
them were DEBUG_VERBOSE (per original intent), but then commit
bf65d7ee8842 ("OvmfPkg/PlatformInitLib: pass through reservations from
qemu", 2022-12-23) introduced a new branch with DEBUG_INFO. From the
perspective of this series, that's a preexistent inconsistency.

Is that worth fixing up at first, separately? (Changing it to
DEBUG_VERBOSE.)

(4) Also relating to logging. The current patch set seems to move all
DEBUG masks here to DEBUG_INFO. The uniformity is welcome, but I'm
unsure if DEBUG_INFO is justified. Writes to the QEMU debug console are
slow; are we sure we want these logs emitted in a defult build of OVMF?

(5) Still about logging. Before this series, the
PlatformScanOrAdd64BitE820Ram() function would log each E820 entry
before investigating them. (And, based on the investigation, further
messages may be logged.) With the whole series applied however, as far
as I can tell, we'll never get a complete dump of the E820 map, because
PlatformScanE820() doesn't log entries at all, and the callbacks only
log entries that prove "interesting" for them.

Is this intentional? I think it may make debugging harder. I didn't
notice it under patch#1, but I think we should add generic
(unconditional) logging there.

(6) *Yet more* logging-related observations. The original log message
uses a bracket "[" on the left hand side of the logged intervals, and a
parenthesis ")" on the RHS, to express that the RHS limit is exclusive:

            "%a: PlatformAddMemoryRangeHob [0x%Lx, 0x%Lx)\n",

This detail is lost in this patch (in all three DEBUG messages); both
sides use brackets.

(7) Sorry, I'm getting tired and my observations are starting to fall
apart. Anyway -- I think all the actual callback functions should be
STATIC.

(8) Furthermore, *perhaps* we should put E820 in their names somewhere
(I don't insist at all), instead of the "Platform" prefix -- these
functions are not public PlatformInitLib APIs.



> +        PlatformAddMemoryRangeHob (Base, End);
> +      }
> +
> +      break;

(9) Empty line intentional?

> +    case EfiAcpiAddressRangeReserved:
> +      BuildResourceDescriptorHob (EFI_RESOURCE_MEMORY_RESERVED, 0, Base, End);
> +      DEBUG ((DEBUG_INFO, "%a: Reserved [0x%Lx, 0x%Lx]\n", __FUNCTION__, Base, End));
> +      break;
> +    default:
> +      DEBUG ((DEBUG_WARN, "%a: Type %d [0x%Lx, 0x%Lx] (NOT HANDLED)\n", __FUNCTION__, E820Entry->Type, Base, End));

(10) I meant to ask earlier -- what is now the maximum line length?

I notice, in ".pytool/Plugin/UncrustifyCheck/uncrustify.cfg":

> # Code width / line splitting
> #code_width                      =120     # TODO: This causes non-deterministic behaviour in some cases when code wraps
> ls_code_width                   =false

Does that mean that 120 is considered a soft limit? (I used to ask for
80 chars under OvmfPkg, but there's no need to stick with that anymore.)

(11) "E820Entry->Type" is an enumeration type; per UEFI 2.10, 2.3.1 Data
Types, the UEFI build environment is supposed to make the compiler pick
INT32 or UINT32. (This is from Mantis ticket 913.)

Now, as long as QEMU sticks with the small set of enumeration constants
we define in "OvmfPkg/Include/IndustryStandard/E820.h", it's all fine,
and we can indeed print "E820Entry->Type" with both %d and %u,
interchangeably. I used "%u" in the original logging because I find the
printing of unexpected values as positive integers rather than negative
ones more tolerable. I don't mind %d as long as we're conscious of it.

Back to the patch:

On 1/10/23 09:21, Gerd Hoffmann wrote:
> +      break;
> +  }
> +}
> +
>  /**
>    Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map, call the
>    passed callback for each entry.
> @@ -1048,7 +946,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 = PlatformScanE820 (PlatformAddHobCB, PlatformInfoHob);
>      if (EFI_ERROR (Status)) {
>        UpperMemorySize = PlatformGetSystemMemorySizeAbove4gb ();
>        if (UpperMemorySize != 0) {

Looks OK.

Thanks,
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98252): https://edk2.groups.io/g/devel/message/98252
Mute This Topic: https://groups.io/mt/96173192/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> > +        DEBUG ((DEBUG_INFO, "%a: HighMemory [0x%Lx, 0x%Lx]\n", __FUNCTION__, Base, End));
> 
> (3) I've not noticed before, but I'm noticing now: before this series,
> the DEBUG levels (or more precisely, "debug masks") for the messages
> emitted by PlatformScanOrAdd64BitE820Ram() were inconsistent. Most of
> them were DEBUG_VERBOSE (per original intent), but then commit
> bf65d7ee8842 ("OvmfPkg/PlatformInitLib: pass through reservations from
> qemu", 2022-12-23) introduced a new branch with DEBUG_INFO. From the
> perspective of this series, that's a preexistent inconsistency.
> 
> Is that worth fixing up at first, separately? (Changing it to
> DEBUG_VERBOSE.)
> 
> (4) Also relating to logging. The current patch set seems to move all
> DEBUG masks here to DEBUG_INFO. The uniformity is welcome, but I'm
> unsure if DEBUG_INFO is justified. Writes to the QEMU debug console are
> slow; are we sure we want these logs emitted in a defult build of OVMF?
> 
> (5) Still about logging. Before this series, the
> PlatformScanOrAdd64BitE820Ram() function would log each E820 entry
> before investigating them. (And, based on the investigation, further
> messages may be logged.) With the whole series applied however, as far
> as I can tell, we'll never get a complete dump of the E820 map, because
> PlatformScanE820() doesn't log entries at all, and the callbacks only
> log entries that prove "interesting" for them.
> 
> Is this intentional? I think it may make debugging harder. I didn't
> notice it under patch#1, but I think we should add generic
> (unconditional) logging there.

Yes, all intentional.

I've dropped all logging from PlatformScanE820, we run that multiple
times and logging the e820 map there would make it show up multiple
times in the logs.  Instead I'm logging at the places where the code
actually does something with the values (set LowMemory, add HOB, ...),
which should give us good insights into the code flow in the logs.

I've turned them on by default because the logging should be less
verbose than it used to be and also because I've found myself flipping
these from VERBOSE to INFO frequently when debugging something.

> (6) *Yet more* logging-related observations. The original log message
> uses a bracket "[" on the left hand side of the logged intervals, and a
> parenthesis ")" on the RHS, to express that the RHS limit is exclusive:
> 
>             "%a: PlatformAddMemoryRangeHob [0x%Lx, 0x%Lx)\n",
> 
> This detail is lost in this patch (in all three DEBUG messages); both
> sides use brackets.

Oh, I wasn't aware of that.  It looked like a typo to me so I "fixed" it
along the way.  I think I prefer to stick with the (inclusive) brackets
and print "End - 1" then so it actually is inclusive.

> (7) Sorry, I'm getting tired and my observations are starting to fall
> apart. Anyway -- I think all the actual callback functions should be
> STATIC.

Yes.  PlatformScanE820() is static too.  Isn't there a gcc warning which
complains about non-static functions without prototype in some header?
Seems this is not turned on for edk2, I'm somehow used to gcc throwing
warnings on that.

> (8) Furthermore, *perhaps* we should put E820 in their names somewhere
> (I don't insist at all), instead of the "Platform" prefix -- these
> functions are not public PlatformInitLib APIs.

There is a simple reason for the naming:
I love 'grep ^Platform' on edk2 log files ;)

> > +      DEBUG ((DEBUG_WARN, "%a: Type %d [0x%Lx, 0x%Lx] (NOT HANDLED)\n", __FUNCTION__, E820Entry->Type, Base, End));
> 
> (10) I meant to ask earlier -- what is now the maximum line length?
> 
> I notice, in ".pytool/Plugin/UncrustifyCheck/uncrustify.cfg":
> 
> > # Code width / line splitting
> > #code_width                      =120     # TODO: This causes non-deterministic behaviour in some cases when code wraps
> > ls_code_width                   =false
> 
> Does that mean that 120 is considered a soft limit? (I used to ask for
> 80 chars under OvmfPkg, but there's no need to stick with that anymore.)

Oh, 120.  I already wondered why uncrustify didn't wrap that one, I
didn't expect the limit being higher than 100 which seems to be the new
standard in many projects.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98282): https://edk2.groups.io/g/devel/message/98282
Mute This Topic: https://groups.io/mt/96173192/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 1/11/23 09:06, Gerd Hoffmann wrote:
>>> +        DEBUG ((DEBUG_INFO, "%a: HighMemory [0x%Lx, 0x%Lx]\n", __FUNCTION__, Base, End));
>>
>> (3) I've not noticed before, but I'm noticing now: before this series,
>> the DEBUG levels (or more precisely, "debug masks") for the messages
>> emitted by PlatformScanOrAdd64BitE820Ram() were inconsistent. Most of
>> them were DEBUG_VERBOSE (per original intent), but then commit
>> bf65d7ee8842 ("OvmfPkg/PlatformInitLib: pass through reservations from
>> qemu", 2022-12-23) introduced a new branch with DEBUG_INFO. From the
>> perspective of this series, that's a preexistent inconsistency.
>>
>> Is that worth fixing up at first, separately? (Changing it to
>> DEBUG_VERBOSE.)
>>
>> (4) Also relating to logging. The current patch set seems to move all
>> DEBUG masks here to DEBUG_INFO. The uniformity is welcome, but I'm
>> unsure if DEBUG_INFO is justified. Writes to the QEMU debug console are
>> slow; are we sure we want these logs emitted in a defult build of OVMF?
>>
>> (5) Still about logging. Before this series, the
>> PlatformScanOrAdd64BitE820Ram() function would log each E820 entry
>> before investigating them. (And, based on the investigation, further
>> messages may be logged.) With the whole series applied however, as far
>> as I can tell, we'll never get a complete dump of the E820 map, because
>> PlatformScanE820() doesn't log entries at all, and the callbacks only
>> log entries that prove "interesting" for them.
>>
>> Is this intentional? I think it may make debugging harder. I didn't
>> notice it under patch#1, but I think we should add generic
>> (unconditional) logging there.
> 
> Yes, all intentional.
> 
> I've dropped all logging from PlatformScanE820, we run that multiple
> times and logging the e820 map there would make it show up multiple
> times in the logs.  Instead I'm logging at the places where the code
> actually does something with the values (set LowMemory, add HOB, ...),
> which should give us good insights into the code flow in the logs.
> 
> I've turned them on by default because the logging should be less
> verbose than it used to be and also because I've found myself flipping
> these from VERBOSE to INFO frequently when debugging something.

I think the last part should be replaced by you just building with
DEBUG_VERBOSE :), carrying perhaps a few patches for the DSC files for
re-disabling DEBUG_VERBOSE for a number of unjustifiably chatty modules
(I can give you a list); *regardless*, I think the above is all good,
just please mention it somewhere in the commit messages.

(Best would be to change the debug levels after the conversion,
separately, but I don't want to get on your nerves.)

> 
>> (6) *Yet more* logging-related observations. The original log message
>> uses a bracket "[" on the left hand side of the logged intervals, and a
>> parenthesis ")" on the RHS, to express that the RHS limit is exclusive:
>>
>>             "%a: PlatformAddMemoryRangeHob [0x%Lx, 0x%Lx)\n",
>>
>> This detail is lost in this patch (in all three DEBUG messages); both
>> sides use brackets.
> 
> Oh, I wasn't aware of that.  It looked like a typo to me so I "fixed" it
> along the way.  I think I prefer to stick with the (inclusive) brackets
> and print "End - 1" then so it actually is inclusive.

Works for me -- as long as you won't be searching the firmware logs for
the *exclusive end* hex strings :) :) :)

(Now of course you can satisfy both goals, as long as you don't print
(End-1) with "0x%Lx", but print (End) with "0x%Lx - 1"! In fact, you may
have meant the latter already, above.)

> 
>> (7) Sorry, I'm getting tired and my observations are starting to fall
>> apart. Anyway -- I think all the actual callback functions should be
>> STATIC.
> 
> Yes.  PlatformScanE820() is static too.  Isn't there a gcc warning which
> complains about non-static functions without prototype in some header?
> Seems this is not turned on for edk2, I'm somehow used to gcc throwing
> warnings on that.

Haha, good catch -- it's intentionally not turned on in edk2. Namely,
some version of MSVC (maybe even bleeding edge ones, I don't know) screw
up source-level debugging for such functions that are marked STATIC, as
far as I remember. This is why, in core packages, you'll see an
inexplicably low number of STATIC functions. So the MSVC shortcoming (if
I can call it that) actively conflicts with the nice gcc warning you
mention. The gcc warning is a no-brainer for when you can actually
enable it.

> 
>> (8) Furthermore, *perhaps* we should put E820 in their names somewhere
>> (I don't insist at all), instead of the "Platform" prefix -- these
>> functions are not public PlatformInitLib APIs.
> 
> There is a simple reason for the naming:
> I love 'grep ^Platform' on edk2 log files ;)

That sounds convincing enough, thanks. Log analysis is important, we
should support it!

> 
>>> +      DEBUG ((DEBUG_WARN, "%a: Type %d [0x%Lx, 0x%Lx] (NOT HANDLED)\n", __FUNCTION__, E820Entry->Type, Base, End));
>>
>> (10) I meant to ask earlier -- what is now the maximum line length?
>>
>> I notice, in ".pytool/Plugin/UncrustifyCheck/uncrustify.cfg":
>>
>>> # Code width / line splitting
>>> #code_width                      =120     # TODO: This causes non-deterministic behaviour in some cases when code wraps
>>> ls_code_width                   =false
>>
>> Does that mean that 120 is considered a soft limit? (I used to ask for
>> 80 chars under OvmfPkg, but there's no need to stick with that anymore.)
> 
> Oh, 120.  I already wondered why uncrustify didn't wrap that one, I
> didn't expect the limit being higher than 100 which seems to be the new
> standard in many projects.
> 
> take care,
>   Gerd
> 

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98296): https://edk2.groups.io/g/devel/message/98296
Mute This Topic: https://groups.io/mt/96173192/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-