Re: [PATCH v2 3/4] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB

["Gerd Hoffmann" <kraxel@redhat.com>]

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