Re: [PATCH 0/3] bootconfig: Add value override operator

[Masami Hiramatsu <mhiramat@kernel.org>]

Hi Steve,

On Thu, 16 Jul 2020 10:27:03 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi Steve,
> 
> On Wed, 15 Jul 2020 20:02:12 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Thu, 16 Jul 2020 07:38:43 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > 
> > > > So the end of the initrd would have:
> > > > 
> > > >  [data][size/checksum/magic][more-data][size/checksum/magic]
> > > > 
> > > > 
> > > > And the kernel could do the following:
> > > > 
> > > >  1. read the end of the initrd for bootconfig
> > > >  2. If found parse the bootconfig data.
> > > >  3. look at the content before the bootconfig
> > > >  4. if another bootconfig exists, goto 2.
> > > >   
> > > 
> > > Yeah, that is possible. But since the total size of the bootconfig
> > > is limited to 32KB (this means data + 1st footer + more-data),
> > > I would like to give a chance of sanity check to the bootloader.
> > 
> > 
> > That's a limit of the size field, right?
> 
> If you mean the size field in the footer, no, it is u32.
> 
> To be honest, the size limitation came from the xbc_node data
> structure. To minimize the memory footprint, I decided to
> pack the data structure into 64bits with 4 fields.
> Each field has 16bits, and the data field needs 1 bit flag
> to distinguish the value and the key.
> Thus the maximum number of nodes can be expanded to 64K
> (but it is not feasible, maybe less than 8K will be a real
> size), but the data field (the offset from the bootconfig
> start address) is 15bits = 32KB long.
> Of course we can use the bitfield to tune it, but maybe current
> balance ( 512 node / 32KB ) is enough.
> 
> Note that if we expand the number of nodes, we need to pre-allocate
> the node data structure as a static data (in .bss) because parsing
> will be done before initializing memory management. 512 nodes means
> 4096B is already allocated. 8K node needs 64KB, and that will be
> not filled in most cases.
> 
> > The bootloader (and all tools including the kernel) could check for
> > multiple instances, and that would even increase the size of what can
> > be added. As each section would be 32KB max size, but there's no limit
> > to how many you have. All tools, bootconfig, the bootloader, and the
> > kernel can perform the checksum.
> 
> In fact, I previously considered the multi-section, but came to the
> conclusion that it wasn't much benefit for both Linux and the
> bootloaders.
> 
> If we support multi-section, we have to mix the section nodes on
> a single tree for overriding values, this means the data field must
> have a section number (and per-section starting address pointers),
> or an offset from the 1st section address.
> 
> And I think it is easy for the bootloader to just concatenate the
> data as below, because the data is already on memory.
> 
> [data][more-data][size/checksum/magic]
> 
> To support multiple-section, the bootloader will do
> 
> 0. load the bootconfig with the initrd from media
> 1. prepare the data
> 2. calculate the size and checksum of the data
> 3. append the data and footer right after the last footer
> 
> and to support single section,
> 
> 0. load the bootconfig with the initrd from media
> 1. prepare the data
> 2. calculate the size and checksum of the data
> 3. increment the size and the checksum 
>    (note that the size and checksum is already on memory)
> 4. append the data and footer right after the last data
> 
> Thus, I think there is no much benefit to support multiple sections.

What would you think?

I guess if we have other types of data appended to the initrd
as similar to the bootconfig, I think we should add the multiple
section support. But only for the bootconfig, we can just update
the bootconfig data as I suggested, since it keeps the code simple.
It might be a chiken-egg problem...

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>