Message-ID: <20210921154021.19289.qmail@stuge.se>
Date:   Tue, 21 Sep 2021 15:40:20 +0000
From:   Peter Stuge <peter@...ge.se>
To:     Alec Brown <alec.r.brown@...cle.com>,
        Heinrich Schuchardt <xypron.glpk@....de>
Cc:     "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: [SPECIFICATION RFC v3] The firmware and bootloader log
 specification

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!)

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.


> 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.


> 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.


> 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.


> 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