Date:   Tue, 19 Oct 2021 15:56:52 -0400
From:   Alec Brown <alec.r.brown@...cle.com>
To:     Peter Stuge <peter@...ge.se>
Cc:     Heinrich Schuchardt <xypron.glpk@....de>,
        "coreboot@...eboot.org" <coreboot@...eboot.org>,
        "grub-devel@....org" <grub-devel@....org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "systemd-devel@...ts.freedesktop.org" 
        <systemd-devel@...ts.freedesktop.org>,
        "trenchboot-devel@...glegroups.com" 
        <trenchboot-devel@...glegroups.com>,
        "u-boot@...ts.denx.de" <u-boot@...ts.denx.de>,
        "x86@...nel.org" <x86@...nel.org>,
        "xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
        Aleksandr Burmashev <alexander.burmashev@...cle.com>,
        "allen.cryptic@...il.com" <allen.cryptic@...il.com>,
        "andrew.cooper3@...rix.com" <andrew.cooper3@...rix.com>,
        "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
        "ardb@...nel.org" <ardb@...nel.org>,
        "btrotter@...il.com" <btrotter@...il.com>,
        Daniel Kiper <daniel.kiper@...cle.com>,
        "dpsmith@...rtussolutions.com" <dpsmith@...rtussolutions.com>,
        Eric DeVolder <eric.devolder@...cle.com>,
        Eric Snowberg <eric.snowberg@...cle.com>,
        "frowand.list@...il.com" <frowand.list@...il.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "hun@...imensional.de" <hun@...imensional.de>,
        "james.dutton@...il.com" <james.dutton@...il.com>,
        "javierm@...hat.com" <javierm@...hat.com>,
        Joao Martins <joao.m.martins@...cle.com>,
        "jwerner@...omium.org" <jwerner@...omium.org>,
        Kanth Ghatraju <kanth.ghatraju@...cle.com>,
        Konrad Wilk <konrad.wilk@...cle.com>,
        "krystian.hebel@...eb.com" <krystian.hebel@...eb.com>,
        "leif@...iainc.co m" <leif@...iainc.com>,
        "lukasz.hawrylko@...el.com" <lukasz.hawrylko@...el.com>,
        "luto@...capital.net" <luto@...capital.net>,
        "michal.zygowski@...eb.com" <michal.zygowski@...eb.com>,
        "mjg59@...gle.com" <mjg59@...gle.com>,
        "mtottenh@...mai.com" <mtottenh@...mai.com>,
        "phcoder@...il.com" <phcoder@...il.com>,
        "piotr.krol@...eb.com" <piotr.krol@...eb.com>,
        "pjones@...hat.com" <pjones@...hat.com>,
        "pmenzel@...gen.mpg.de" <pmenzel@...gen.mpg.de>,
        "rasmus.villemoes@...vas.dk" <rasmus.villemoes@...vas.dk>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "roger.pau@...rix.com" <roger.pau@...rix.com>,
        Ross Philipson <ross.philipson@...cle.com>,
        "sjg@...omium.org" <sjg@...omium.org>,
        "trini@...sulko.com" <trini@...sulko.com>,
        "tyhicks@...ux.microsoft.com" <tyhicks@...ux.microsoft.com>,
        "ulrich.windl@...uni-regensburg.de" 
        <ulrich.windl@...uni-regensburg.de>,
        "wvervoorn@...an.com" <wvervoorn@...an.com>,
        "rharwood@...hat.com" <rharwood@...hat.com>
Subject: Re: [External] : Re: [SPECIFICATION RFC v3] The firmware and
 bootloader log specification


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

Powered by blists - more mailing lists