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

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

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