On Fri, 29 May 2020 13:27:35 +0200
Daniel Kiper wrote:
> Below you can find my rough idea of the bootloader log format which is
> generic thing but initially will be used for TrenchBoot work. I
> discussed this proposal with Ross and Daniel S. So, the idea went
> through initial sanitization. Now I would like to take feedback from
> other folks too. So, please take a look and complain...
>
> In general we want to pass the messages produced by the bootloader to
> the OS kernel and finally to the userspace for further processing and
> analysis. Below is the description of the structures which will be
> used for this thing.
This should mention that this about having one contiguous block of
memory which contains a struct bootloader_log.
> struct bootloader_log_msgs
Why the plural with the trailing letter s in the struct name? This looks
like a single message to me, and should thus be signular without a
trailing letter s.
> {
> grub_uint32_t level;
> grub_uint32_t facility;
> char type[];
> char msg[];
> }
How would this work? How could a compiler know where msg starts? gcc
just reports "error: flexible array member not at end of struct" here
as there is no way of knowing.
Where are the sizes of type[] and msg[] defined?
Only implicitly by just having two NUL terminated strings right after
each other? That would mean you need to change the struct to
struct bootloader_log_msg
{
grub_uint32_t level;
grub_uint32_t facility;
char strings[];
}
and have anyone parsing this structure walk through all characters in
strings[] looking for NULs to eventually find out where the msg string
actually starts. This looks at least a bit ugly to me, unless you
absolutely need to save the last bit of code and data memory in the
bootloader.
To help find where the msg actually starts without needing to look for
a NUL character, you could add a struct member showing where the msg
string actually starts within strings[]:
struct bootloader_log_msg
{
grub_uint32_t level;
grub_uint32_t facility;
grub_uint32_t msg_start;
char strings[];
}
This msg_start value could be defined as an character offset into
strings[]. Then accessing msg->strings or >strings[0] would access
the type string, and >strings[msg_ofs] would access the message:
struct bootloader_log_msg *msg = ...;
printf("msg type: %s\n", >strings[0]);
printf("msg: %s\n", >strings[msg->msg_start]);
For quick parsing of the messages, having a "grub_uint32_t size"
struct member to quickly skip the current struct bootloader_log_msg
completely and jump directly to the next struct would be helpful,
regardless of how the strings are actually stored:
struct bootloader_log_msg
{
grub_uint32_t level;
grub_uint32_t facility;
grub_uint32_t size;
[ ... string storage ... ]
}
> struct bootloader_log
> {
> grub_uint32_t version;
> grub_uint32_t producer;
> grub_uint32_t size;
> grub_uint32_t next_off;
> bootloader_log_msgs msgs[];
> }
>
> The members of struct bootloader_log:
> - version: the bootloader log format version number, 1 for now,
> - producer: the producer/bootloader type; we can steal some values
> from linux/Documentation/x86/boot.rst:type_of_loader,
> - size: total size of the log buffer including the bootloader_log
> struct,
"size in bytes"?
> - next_off: offset in bytes, from start of the bootloader_log
> struct, of the next byte after the last log message in the msgs[];
> i.e. the offset of the next available log message slot,
It appears to me that next_off is only interesting for code which
appends log messages to that structure. For reading such a struct,
next_off is useless and could thus be a private variable inside that
bootloader which is not passed on to a next stage bootloader or an OS
kernel.
So specifying next_off as something other than a "reserved" uint32_t is
for when you have a chain of bootloaders which all append messages to
that buffer, and you want to avoid all the bootloaders having to parse
the messages from the previous bootloader's messages in order to find
out where to append messages. If that is the intention, this procedure
should at least be mentioned somewhere.
I am also missing any mention of memory alignment. With the number of
CPUs in the field which cannot directly read misaligned words
increasing, specifying a 4 byte or 8 byte alignment for these
structures could significantly reduce the code complexity for such
CPUs at the cost of a few bytes of memory.
And while on the topic of memory layout: With all these uint32_t
values, is this only for a 32bit protocol, or will this remain 32bit
even for otherwise 64bit code, or what is the plan here?
> - msgs: the array of log messages.
>
> The members of struct bootloader_log_msgs:
> - level: similar to syslog meaning; can be used to differentiate
> normal messages from debug messages; exact
