Gerd Hoffmann <[email protected]> writes:
> On Fri, Oct 10, 2025 at 01:41:35PM +0200, Markus Armbruster wrote:
>> Gerd Hoffmann <[email protected]> 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!
