Re: [PATCH v3] hw/uefi: add "info firmware-log" + "query-firmware-log" monitor commands

[Markus Armbruster <armbru@redhat.com>]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On Fri, Oct 10, 2025 at 01:41:35PM +0200, Markus Armbruster wrote:
>> Gerd Hoffmann <kraxel@redhat.com> writes:
>> 
>> > Starting with the edk2-stable202508 tag OVMF (and ArmVirt too) have
>> > optional support for logging to a memory buffer.  There is guest side
>> > support -- for example in linux kernels v6.17+ -- to read that buffer.
>> > But that might not helpful if your guest stops booting early enough that
>> > guest tooling can not be used yet.  So host side support to read that
>> > log buffer is a useful thing to have.
>> >
>> > This patch implements both qmp and hmp monitor commands to read the
>> > firmware log.
>> 
>> So this is just for EDK2, at least for now.
>
> Yes.
>
>> > +    char                 FirmwareVersion[128];
>> > +} MEM_DEBUG_LOG_HDR;
>> 
>> I understand this is a (close to) literal copy from EDK2, and adjusting
>> it to QEMU style would be a bad idea.
>> 
>> > +
>> > +
>> > +/* ----------------------------------------------------------------------- */
>> > +/* qemu monitor command                                                    */
>> > +
>> > +typedef struct {
>> > +    uint64_t             Magic1;
>> > +    uint64_t             Magic2;
>> > +} MEM_DEBUG_LOG_MAGIC;
>> 
>> Unusual capitalization for a typedef name.  Why?  To emphasize the
>> relation to MEM_DEBUG_LOG_HDR?
>
> Yes.

Okay.

>> > +    if (header.DebugLogSize > MiB) {
>> > +        /* default size is 128k (32 pages), allow up to 1M */
>> > +        error_setg(errp, "firmware log: log buffer is too big");
>> 
>> [*] We limit the buffer to 1MiB.  No objection to the size.
>> 
>> What do you mean by "default" in "default size"?  Is the size
>> configurable in EDK2?
>
> Yes, there is an option for that.
>
>> Should we try to cope more gracefully with oversized log buffers?  It's
>> a ring buffer.  What about silently reading the latest 1MiB then?
>> Behaves just as if the ring buffer was 1MiB.
>
> See below.
>
>> > +# @log: Firmware debug log, in base64 encoding.
>> 
>> Can this have a partial line at the beginning and/or the end?
>
> Yes.

Partial lines can be troublesome, in particular when complete lines
start with a prefix in a known format.  If avoiding them isn't
practical, we should at least document.

>> > +#
>> > +# Since: 10.2
>> > +##
>> > +{ 'struct': 'FirmwareLog',
>> > +  'data': { '*version': 'str',
>> > +            '*log': 'str' } }
>> 
>> These aren't actually optional with the current code.  See [**] above.
>> I guess you make them optional just in case some other firmware can
>> provide only one of them.
>
> We could also make both mandatory.  There is always the option to return
> an empty string ...

Yes.  Loses the distinction between "firmware doesn't support this" and
"firmware supports this, but it happens to be empty right now".  Do we
care?

>> > +##
>> > +# @query-firmware-log:
>> > +#
>> > +# Find firmware memory log buffer in guest memory, return content.
>> 
>> Should we mention this is implemented only for EDK2 at this time?
>> 
>> Have you considered an optional size argument to retrieve the tail of
>> the log?
>
> I'll have a look.  If we implement the 1MB cap suggested above we would
> get that (almost) for free.
>
> take care,
>   Gerd

Thanks!