Re: [External] : Re: [SPECIFICATION RFC v3] The firmware and bootloader log specification

[Alec Brown <alec.r.brown@xxxxxxxxxx>]

On Tue, Sep 21, 2021 at 03:40:20PM +0000, Peter Stuge wrote:
> Alec Brown wrote:
> > Below is how the layout of these logs would store their data.
> > 
> > bf_log_header:
> >                +-------------------+
> > u32            | version           |
> > u32            | size              |
> > u8[64]         | producer          |
> > u8[64]         | log_format        |
> > u64            | flags             |
> > u64            | next_bflh_addr    |
> > u64            | log_addr          |
> > u32            | log_size          |
> >                +-------------------+
> 
> I suggest to include a .magic at least in bf_log_header and an
> .xor_checksum or .crc32 only in bf_log_header.
> 
> .magic doubles as endianess indicator when the structures are
> stored on movable media. (Pick an asymmetric magic bit pattern!)

This is something we will need to think about.

>  
> I suggest renaming .next_bflh_addr to .next_log_header and .log_addr
> to .log_buffer_addr.
> 
> I suggest to remove .size and .log_size:
> 
> The rationale for .size is "to allow for backward compatibility" but
> that seems redundant thanks to .version.
> 
> .log_size can be calculated from the subordinate data and is thus
> mostly an unneccessary source of potential inconsistency.

Looking back, I agree with removing .size since .version accomplishes the same
task. I'm not so sure on removing .log_size as it can be very convenient, and
.log_size won't need to be calculated every time someone wants to know the size
of the logs generated from the boot component.

> 
> 
> > bf_log_buffer:
> >                +-------------------+
> > u32            | version           |
> > u32            | size              |
> > u8[64]         | producer          |
> > u32            | next_msg_off      |
> > bf_log_msg[l]  | msgs              |
> >                +-------------------+
> 
> I suggest replacing .size and .next_msg_off with .messages containing l:
> 
> .size can then be calculated from .messages; again, reliably avoiding
> inconsistency between .size and .next_msg_off.
> 
> Allocated size doesn't seem useful if writers must anyway maintain state
> containing the starting address. If writers must be allowed to be completely
> stateless then maybe at least rename .size to .allocated_size and see below
> for discovery.
> 
> Having .messages also eliminates the need for an end-of-messages marker
> when the allocated space is not yet filled.
> 

After some thinking, it makes sense to replace the meaning of .size with the
meaning of .next_msg_off and removing .next_msg_off from the structure. This 
wouldn't be useful to store for the readers of the log.

> 
> > bf_log_msg:
> >                +-------------------+
> > u32            | size              |
> > u64            | ts_nsec           |
> > u32            | level             |
> > u32            | facility          |
> > u32            | msg_off           |
> > u8[n]          | type              |
> > u8[m]          | msg               |
> >                +-------------------+
> 
> It seems inconsistent that log_header.size and log_msg.size cover only
> the respective struct itself while log_buffer.size also covers all
> subordinate messages. Skipping all .size in this version fixes that.
> 
> And log_msg.size is not very useful since both .type and .msg have variable
> length; it's not possible to access .msg without scanning .type. Please at
> a minimum add .type_size but better yet replace .size with .type_size and
> .msg_size.
> 

You bring up some good points about the names for the fields and that they need
to be more consistent. By removing .size in bf_log_header, this should make it
more consistent.

> 
> > There is still the outstanding issue of how the logs will be sent to the 
> > OS. If
> > UEFI is used, we can use config tables. If ACPI or Device Tree is used, we 
> > can
> > use bf_log_header.next_bflh_addr to present the logs. If none of these 
> > platforms
> > are used, it becomes a lot trickier to solve this issue.
> > 
> > Any suggestions are much appreciated and will be taken into consideration.
> 
> Having bf_log_header.magic and some bf_log_header.$checksum, a strict rule
> for bf_log_header start address granularity and a strict maximum offset
> for the first header from top and/or bottom of memory allows to quickly
> discover a log in memory without explicit handover.
> 

This is something we'll have to think about some more. We aren't convinced about
using .magic for log discovery and are looking for a more explicit way of doing
this.

> 
> > LPC System Boot and Security Micro-conference on the 22nd of September
> > at 7:50 AM PDT (14:50 UTC).
> 
> Have fun! :)
> 
> 
> Heinrich Schuchardt wrote:
> > We already the EFI_TCG2_PROTOCOL and RFC 5424 (The syslog protocol).
> > Why do we need to start from scratch?
> 
> That's a good question. I guess noone wants to settle for a standard
> from somewhere else. ;)
> 
> I wouldn't mind if log_msg was a syslog transport, but I can understand
> if that's rejected because syslog messages require a lot of parsing for
> presentation while Alec's proposal seems focused on efficiency and simplicity.
> 
> It's also nice to be able to strictly mandate UTF-8 for all fields.
> (RFC 5424 allows MSG to be anything.)
> 
> 
> Kind regards
> 
> //Peter