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

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

[Masami Hiramatsu]

Hi,

Here is a seires of value-override operator support for bootconfig.

Currently, the bootconfig syntax supports normal assignment operator
("=") and value append operator ("+="), but there is no way to
override (or update) existing value.

This patch adds the value override operator (":=") to update the
existing value with another value. For example,

key.word = value1
key.word := value2, value3

In this case, the 2nd "key.word" config overwrites the 1st one.
Thus this is equal to;

key.word = value2, value3

Note that it is still not allowed to override a subkey with
a value, and a value with a subkey. This means the following
case is not allowed.

key.word = value1
key := value2 # Error!


With this change, if the bootloader wants to change some value
in the default bootconfig, it doesn't need to parse the existing
bootconfig, but it can just append the new configs at the tail
of the bootconfig and update the footer (size, checksum and magic
word).

Thank you,

---

Masami Hiramatsu (3):
      lib/bootconfig: Add override operator support
      tools/bootconfig: Add testcases for value override operator
      Documentation: bootconfig: Add bootconfig override operator


 Documentation/admin-guide/bootconfig.rst     |   11 +++++++++
 lib/bootconfig.c                             |   33 ++++++++++++++++++--------
 tools/bootconfig/samples/bad-override.bconf  |    3 ++
 tools/bootconfig/samples/bad-override2.bconf |    3 ++
 tools/bootconfig/samples/good-override.bconf |    6 +++++
 tools/bootconfig/test-bootconfig.sh          |   13 ++++++++++
 6 files changed, 59 insertions(+), 10 deletions(-)
 create mode 100644 tools/bootconfig/samples/bad-override.bconf
 create mode 100644 tools/bootconfig/samples/bad-override2.bconf
 create mode 100644 tools/bootconfig/samples/good-override.bconf

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH 1/3] lib/bootconfig: Add override operator support

[Masami Hiramatsu]

Add the value override operator (":=") support to the bootconfig.

This value override operator will be useful for the bootloaders
which will only update the existing bootconfig according to the
bootloader boot options.

Without this override operator, the bootloader needs to parse
the existing bootconfig and update it. However, with this
assignment, it can just append the updated (partial) bootconfig
text at the tail of existing one without parsing it.
(Of course, it must update the size, checksum and magic,
 but that will be done easily)

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 lib/bootconfig.c |   33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 912ef4921398..a5f701161f6b 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -329,22 +329,30 @@ const char * __init xbc_node_find_next_key_value(struct xbc_node *root,
 
 /* XBC parse and tree build */
 
+static int __init xbc_init_node(struct xbc_node *node, char *data, u32 flag)
+{
+	unsigned long offset = data - xbc_data;
+
+	if (WARN_ON(offset >= XBC_DATA_MAX))
+		return -EINVAL;
+
+	node->data = (u16)offset | flag;
+	node->child = 0;
+	node->next = 0;
+
+	return 0;
+}
+
 static struct xbc_node * __init xbc_add_node(char *data, u32 flag)
 {
 	struct xbc_node *node;
-	unsigned long offset;
 
 	if (xbc_node_num == XBC_NODE_MAX)
 		return NULL;
 
 	node = &xbc_nodes[xbc_node_num++];
-	offset = data - xbc_data;
-	node->data = (u16)offset;
-	if (WARN_ON(offset >= XBC_DATA_MAX))
+	if (xbc_init_node(node, data, flag) < 0)
 		return NULL;
-	node->data |= flag;
-	node->child = 0;
-	node->next = 0;
 
 	return node;
 }
@@ -603,7 +611,9 @@ static int __init xbc_parse_kv(char **k, char *v, int op)
 	if (c < 0)
 		return c;
 
-	if (!xbc_add_sibling(v, XBC_VALUE))
+	if (op == ':' && child) {
+		xbc_init_node(child, v, XBC_VALUE);
+	} else if (!xbc_add_sibling(v, XBC_VALUE))
 		return -ENOMEM;
 
 	if (c == ',') {	/* Array */
@@ -787,7 +797,7 @@ int __init xbc_init(char *buf, const char **emsg, int *epos)
 
 	p = buf;
 	do {
-		q = strpbrk(p, "{}=+;\n#");
+		q = strpbrk(p, "{}=+;:\n#");
 		if (!q) {
 			p = skip_spaces(p);
 			if (*p != '\0')
@@ -798,9 +808,12 @@ int __init xbc_init(char *buf, const char **emsg, int *epos)
 		c = *q;
 		*q++ = '\0';
 		switch (c) {
+		case ':':
 		case '+':
 			if (*q++ != '=') {
-				ret = xbc_parse_error("Wrong '+' operator",
+				ret = xbc_parse_error(c == '+' ?
+						"Wrong '+' operator" :
+						"Wrong ':' operator",
 							q - 2);
 				break;
 			}

[PATCH 2/3] tools/bootconfig: Add testcases for value override operator

[Masami Hiramatsu]

Add some testcases and examples for value override operator.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/bootconfig/samples/bad-override.bconf  |    3 +++
 tools/bootconfig/samples/bad-override2.bconf |    3 +++
 tools/bootconfig/samples/good-override.bconf |    6 ++++++
 tools/bootconfig/test-bootconfig.sh          |   13 +++++++++++++
 4 files changed, 25 insertions(+)
 create mode 100644 tools/bootconfig/samples/bad-override.bconf
 create mode 100644 tools/bootconfig/samples/bad-override2.bconf
 create mode 100644 tools/bootconfig/samples/good-override.bconf

diff --git a/tools/bootconfig/samples/bad-override.bconf b/tools/bootconfig/samples/bad-override.bconf
new file mode 100644
index 000000000000..fde6c561512e
--- /dev/null
+++ b/tools/bootconfig/samples/bad-override.bconf
@@ -0,0 +1,3 @@
+key.subkey = value
+# We can not override pre-defined subkeys with value
+key := value
diff --git a/tools/bootconfig/samples/bad-override2.bconf b/tools/bootconfig/samples/bad-override2.bconf
new file mode 100644
index 000000000000..688587cb023c
--- /dev/null
+++ b/tools/bootconfig/samples/bad-override2.bconf
@@ -0,0 +1,3 @@
+key = value
+# We can not override pre-defined value with subkey
+key.subkey := value
diff --git a/tools/bootconfig/samples/good-override.bconf b/tools/bootconfig/samples/good-override.bconf
new file mode 100644
index 000000000000..7d31d5f8fbd8
--- /dev/null
+++ b/tools/bootconfig/samples/good-override.bconf
@@ -0,0 +1,6 @@
+# Override the value
+key.word = 1,2,4
+key.word := 2,3
+
+# No pre-defined key
+key.new.word := "new"
diff --git a/tools/bootconfig/test-bootconfig.sh b/tools/bootconfig/test-bootconfig.sh
index 3c2ab9e75730..56284b98d8f0 100755
--- a/tools/bootconfig/test-bootconfig.sh
+++ b/tools/bootconfig/test-bootconfig.sh
@@ -117,6 +117,19 @@ xpass grep -q "bar" $OUTFILE
 xpass grep -q "baz" $OUTFILE
 xpass grep -q "qux" $OUTFILE
 
+echo "Override same-key values"
+cat > $TEMPCONF << EOF
+key = bar, baz
+key := qux
+EOF
+echo > $INITRD
+
+xpass $BOOTCONF -a $TEMPCONF $INITRD
+$BOOTCONF $INITRD > $OUTFILE
+xfail grep -q "bar" $OUTFILE
+xfail grep -q "baz" $OUTFILE
+xpass grep -q "qux" $OUTFILE
+
 echo "Double/single quotes test"
 echo "key = '\"string\"';" > $TEMPCONF
 $BOOTCONF -a $TEMPCONF $INITRD

[PATCH 3/3] Documentation: bootconfig: Add bootconfig override operator

[Masami Hiramatsu]

Add a sentence about bootconfig override operator (":=") to
bootconfig.rst.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Documentation/admin-guide/bootconfig.rst |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
index d6b3b77a4129..a22024f9175e 100644
--- a/Documentation/admin-guide/bootconfig.rst
+++ b/Documentation/admin-guide/bootconfig.rst
@@ -71,6 +71,16 @@ For example,::
  foo = bar, baz
  foo = qux  # !ERROR! we can not re-define same key
 
+If you want to update the value, you must use the override operator
+``:=`` explicitly. For example::
+
+ foo = bar, baz
+ foo := qux
+
+then, the ``qux`` is assigned to ``foo`` key. This is useful for
+overriding the default value by adding (partial) custom bootconfigs
+without parsing the default bootconfig.
+
 If you want to append the value to existing key as an array member,
 you can use ``+=`` operator. For example::
 
@@ -84,6 +94,7 @@ For example, following config is NOT allowed.::
 
  foo = value1
  foo.bar = value2 # !ERROR! subkey "bar" and value "value1" can NOT co-exist
+ foo.bar := value2 # !ERROR! even with the override operator, this is NOT allowed.
 
 
 Comments

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

[Steven Rostedt]

On Thu, 16 Jul 2020 01:00:21 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> With this change, if the bootloader wants to change some value
> in the default bootconfig, it doesn't need to parse the existing
> bootconfig, but it can just append the new configs at the tail
> of the bootconfig and update the footer (size, checksum and magic
> word).

I wonder if we should support multiple bootconfigs instead of updating
the size/checksum/magic?

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.

-- Steve

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

[Masami Hiramatsu]

On Wed, 15 Jul 2020 16:45:04 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 16 Jul 2020 01:00:21 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > With this change, if the bootloader wants to change some value
> > in the default bootconfig, it doesn't need to parse the existing
> > bootconfig, but it can just append the new configs at the tail
> > of the bootconfig and update the footer (size, checksum and magic
> > word).
> 
> I wonder if we should support multiple bootconfigs instead of updating
> the size/checksum/magic?
> 
> 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.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Steven Rostedt]

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?

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.

-- Steve

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

[Masami Hiramatsu]

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.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Masami Hiramatsu]

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>