bootconfig: Update for the recent reports

Hello,

Here is the 2nd version of the bootconfig updates. There are several implementation changes and syntax fix/updates.

This version changed a bit in Kconfig and init/main.c.

- [1/8] Use pr_warn() for warning message. Remove unneeded "default n" line from Kconfig.
- [2/8][4/8] Update Kconfig comment. (data on initrd)

Thank you,

---

Masami Hiramatsu (8):
bootconfig: Set CONFIG_BOOT_CONFIG=n by default
bootconfig: Add bootconfig magic word for indicating bootconfig explicitly tools/bootconfig: Remove unneeded error message silencer
bootconfig: Remove unneeded checksum
bootconfig: Reject subkey and value on same parent key
bootconfig: Overwrite value on same key by default
bootconfig: Add append value operator support
bootconfig: Print array as multiple commands for legacy command line

 Documentation/admin-guide/bootconfig.rst     |   37 +++++++++++-
 include/linux/bootconfig.h                   |    3 +
 init/Kconfig                                 |    3 -
 init/main.c                                  |   54 +++++++-----------
 kernel/trace/Kconfig                         |    3 +
 lib/bootconfig.c                             |   44 +++++++++++----
 tools/bootconfig/include/linux/printk.h      |    5 --
 tools/bootconfig/main.c                      |   77 +++++++++++---------------
 tools/bootconfig/samples/bad-mixed-kv1.bconf |    3 +
 tools/bootconfig/samples/bad-mixed-kv2.bconf |    3 +
 tools/bootconfig/test-bootconfig.sh          |   31 +++++++++-
 11 files changed, 164 insertions(+), 99 deletions(-)
 create mode 100644 tools/bootconfig/samples/bad-mixed-kv1.bconf
 create mode 100644 tools/bootconfig/samples/bad-mixed-kv2.bconf

--
Masami Hiramatsu (Linaro) <[email protected]>

Set CONFIG_BOOT_CONFIG=n by default. This also warns user if CONFIG_BOOT_CONFIG=n but "bootconfig" is given in the kernel command line.

---
V2: Use pr_warn() for warning message.
Remove unneeded "default n" line from Kconfig.

---
 init/Kconfig         |    1 -
 init/main.c          |    8 ++++++++
 kernel/trace/Kconfig |    3 ++-
 3 files changed, 10 insertions(+), 2 deletions(-)

index 452bc1835cd4..e76e9241552c 100644
@@ -1227,7 +1227,6 @@ endif
 config BOOT_CONFIG
 	bool "Boot config support"
 	depends on BLK_DEV_INITRD
-	default y
 	help
 	  Extra boot config allows system admin to pass a config file as
 	  complemental extension of kernel cmdline when booting.

index f95b014a5479..ae4e37681247 100644
@@ -418,6 +418,14 @@ static void __init setup_boot_config(const char *cmdline)
 }
 #else
 #define setup_boot_config(cmdline)	do { } while (0)
+
+static int __init warn_bootconfig(char *str)
+{
+	pr_warn("WARNING: 'bootconfig' found on the kernel command line but CONFIG_BOOTCONFIG is not set.\n");
+	return 0;
+}
+early_param("bootconfig", warn_bootconfig);
+
 #endif
 
 /* Change NUL term back to "=", to make "param" the whole string. */

index 91e885194dbc..795c3e02d3f1 100644
@@ -143,7 +143,8 @@ if FTRACE
 
 config BOOTTIME_TRACING
 	bool "Boot-time Tracing support"
-	depends on BOOT_CONFIG && TRACING
+	depends on TRACING
+	select BOOT_CONFIG
 	default y
 	help
 	  Enable developer to setup ftrace subsystem via supplemental

Hi Hiramatsu-san,

Thanks for your patch!

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Yes, because if someone adds bootconfig to the command line they would be expecting their bootconfig to be read. If not, we should not fail silently.

Are you really concerned about a tiny __init function that gets freed after boot up?

-- Steve

Hi Steven,

If someone adds "ip=on" to the command line, they expect DHCP to work. Woops, you need CONFIG_IP_PNP for that.
If someone adds "nfsroot=..." to the command line, they expect the NFS root fielsystem to be mounted.
Guess how many options need to be enabled for that?

Perhaps we need CONFIG_COMMAND_NOT_FOUND?

    Kernel panic - not syncing: option "inspecial" not found.
    Did you mean:

option "imspecial" from section "mine"
option "urspecial" from section "yours"

Try enabling it with "make xconfig".

It's still part of the initial kernel image, and thus subject to boot loader and platform limitations.

Thanks!

Gr{oetje,eeting}s,

Geert

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

This isn't the same. It's more like having "ip=on" and ip configured, but nothing happening because the command line isn't being read.

And still in the noise of other aspects of the kernel. For little instances like this, there should be a CONFIG_TINY (I thought we had that?), otherwise it's going to be annoying. (Remember, I was fighting for not having a config option at all to disable CONFIG_BOOTCONFIG).

-- Steve

Add bootconfig magic word to the end of bootconfig on initrd image for indicating explicitly the bootconfig is there. Also tools/bootconfig treats wrong size or wrong checksum or parse error as an error, because if there is a bootconfig magic word, there must be a bootconfig.

The bootconfig magic word is "#BOOTCONFIG\n", 12 bytes word. Thus the block image of the initrd file with bootconfig is as follows.

[Initrd][bootconfig][size][csum][#BOOTCONFIG\n]

---
V2: Update Kconfig comment.

---
 Documentation/admin-guide/bootconfig.rst |   10 +++++--
 include/linux/bootconfig.h               |    3 ++
 init/Kconfig                             |    2 +
 init/main.c                              |    6 +++-
 tools/bootconfig/main.c                  |   43 ++++++++++++++++++++++--------
 tools/bootconfig/test-bootconfig.sh      |    2 +
 6 files changed, 49 insertions(+), 17 deletions(-)

index b342a6796392..5e7609936507 100644
@@ -102,9 +102,13 @@ Boot Kernel With a Boot Config
 ==============================
 
 Since the boot configuration file is loaded with initrd, it will be added
-to the end of the initrd (initramfs) image file. The Linux kernel decodes
-the last part of the initrd image in memory to get the boot configuration
-data.
+to the end of the initrd (initramfs) image file with size, checksum and
+12-byte magic word as below.
+
+[initrd][bootconfig][size(u32)][checksum(u32)][#BOOTCONFIG\n]
+
+The Linux kernel decodes the last part of the initrd image in memory to
+get the boot configuration data.
 Because of this "piggyback" method, there is no need to change or
 update the boot loader and the kernel image itself.
 

index 7e18c939663e..d11e183fcb54 100644
@@ -10,6 +10,9 @@
 #include <linux/kernel.h>
 #include <linux/types.h>
 
+#define BOOTCONFIG_MAGIC	"#BOOTCONFIG\n"
+#define BOOTCONFIG_MAGIC_LEN	12
+
 /* XBC tree node */
 struct xbc_node {
 	u16 next;

index e76e9241552c..6ffaf4940f3e 100644
@@ -1231,7 +1231,7 @@ config BOOT_CONFIG
 	  Extra boot config allows system admin to pass a config file as
 	  complemental extension of kernel cmdline when booting.
 	  The boot config file must be attached at the end of initramfs
-	  with checksum and size.
+	  with checksum, size and magic word.
 	  See <file:Documentation/admin-guide/bootconfig.rst> for details.
 
 	  If unsure, say Y.

index ae4e37681247..fe3ed874c748 100644
@@ -374,7 +374,11 @@ static void __init setup_boot_config(const char *cmdline)
 	if (!initrd_end)
 		goto not_found;
 
-	hdr = (u32 *)(initrd_end - 8);
+	data = (char *)initrd_end - BOOTCONFIG_MAGIC_LEN;
+	if (memcmp(data, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN))
+		goto not_found;
+
+	hdr = (u32 *)(data - 8);
 	size = hdr[0];
 	csum = hdr[1];
 

index 1de06de328e2..adafb7c50940 100755
@@ -49,7 +49,7 @@ xpass $BOOTCONF -a $TEMPCONF $INITRD
 new_size=$(stat -c %s $INITRD)
 
 echo "File size check"
-xpass test $new_size -eq $(expr $bconf_size + $initrd_size + 9)
+xpass test $new_size -eq $(expr $bconf_size + $initrd_size + 9 + 12)
 
 echo "Apply command repeat test"
 xpass $BOOTCONF -a $TEMPCONF $INITRD

Remove error message silent knob, we don't need it anymore because we can check if there is a bootconfig by checking the magic word.
If there is a magic word, but failed to load a bootconfig from initrd, there is a real problem.

---
 tools/bootconfig/include/linux/printk.h |    5 +----
 tools/bootconfig/main.c                 |    8 --------
 2 files changed, 1 insertion(+), 12 deletions(-)

index e978a63d3222..036e667596eb 100644
@@ -4,10 +4,7 @@
 
 #include <stdio.h>
 
-/* controllable printf */
-extern int pr_output;
-#define printk(fmt, ...)	\
-	(pr_output ? printf(fmt, ##__VA_ARGS__) : 0)
+#define printk(fmt, ...) printf(fmt, ##__VA_ARGS__)
 
 #define pr_err printk
 #define pr_warn	printk

index 742271f019a9..a9b97814d1a9 100644
@@ -14,8 +14,6 @@
 #include <linux/kernel.h>
 #include <linux/bootconfig.h>
 
-int pr_output = 1;
-
 static int xbc_show_array(struct xbc_node *node)
 {
 	const char *val;

@@ -227,13 +225,7 @@ int delete_xbc(const char *path)
 		return -errno;
 	}
 
-	/*
-	 * Suppress error messages in xbc_init() because it can be just a
-	 * data which concidentally matches the size and checksum footer.
-	 */
-	pr_output = 0;
 	size = load_xbc_from_initrd(fd, &buf);
-	pr_output = 1;
 	if (size < 0) {
 		ret = size;
 		pr_err("Failed to load a boot config from initrd: %d\n", ret);

Remove checksum of bootconfig because we already use a magic word to identify bootconfig. This checksum was used for checking whether there is a bootconfig at the end of initrd or not. Since we have a bootconfig magic word to identify the bootconfig data, we do not this checksum anymore.

Thus the block image of the initrd file with bootconfig is as follows.

[initrd][bootconfig][size(u32)][#BOOTCONFIG\n]

---
V2: Update Kconfig comment.

---
 Documentation/admin-guide/bootconfig.rst |    6 ++--
 init/Kconfig                             |    2 +
 init/main.c                              |   20 +-------------
 tools/bootconfig/main.c                  |   42 ++++++------------------------
 tools/bootconfig/test-bootconfig.sh      |    2 +
 5 files changed, 16 insertions(+), 56 deletions(-)

index 5e7609936507..48675052c963 100644
@@ -102,10 +102,10 @@ Boot Kernel With a Boot Config
 ==============================
 
 Since the boot configuration file is loaded with initrd, it will be added
-to the end of the initrd (initramfs) image file with size, checksum and
-12-byte magic word as below.
+to the end of the initrd (initramfs) image file with 4-byte size and 12-byte
+magic word as below.
 
-[initrd][bootconfig][size(u32)][checksum(u32)][#BOOTCONFIG\n]
+[initrd][bootconfig][size][#BOOTCONFIG\n]
 
 The Linux kernel decodes the last part of the initrd image in memory to
 get the boot configuration data.

index 6ffaf4940f3e..631ef2864608 100644
@@ -1231,7 +1231,7 @@ config BOOT_CONFIG
 	  Extra boot config allows system admin to pass a config file as
 	  complemental extension of kernel cmdline when booting.
 	  The boot config file must be attached at the end of initramfs
-	  with checksum, size and magic word.
+	  with size and magic word.
 	  See <file:Documentation/admin-guide/bootconfig.rst> for details.
 
 	  If unsure, say Y.

index fe3ed874c748..821c582af49b 100644
@@ -335,16 +335,6 @@ static char * __init xbc_make_cmdline(const char *key)
 	return new_cmdline;
 }
 
-u32 boot_config_checksum(unsigned char *p, u32 size)
-{
-	u32 ret = 0;
-
-	while (size--)
-		ret += *p++;
-
-	return ret;
-}
-
 static int __init bootconfig_params(char *param, char *val,
 				    const char *unused, void *arg)
 {

@@ -359,7 +349,7 @@ static int __init bootconfig_params(char *param, char *val,
 static void __init setup_boot_config(const char *cmdline)
 {
 	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
-	u32 size, csum;
+	u32 size;
 	char *data, *copy;
 	u32 *hdr;
 	int ret;

@@ -378,9 +368,8 @@ static void __init setup_boot_config(const char *cmdline)
 	if (memcmp(data, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN))
 		goto not_found;
 
-	hdr = (u32 *)(data - 8);
+	hdr = (u32 *)(data - sizeof(u32));
 	size = hdr[0];
-	csum = hdr[1];
 
 	if (size >= XBC_DATA_MAX) {
 		pr_err("bootconfig size %d greater than max size %d\n",

@@ -392,11 +381,6 @@ static void __init setup_boot_config(const char *cmdline)
 	if ((unsigned long)data < initrd_start)
 		goto not_found;
 
-	if (boot_config_checksum((unsigned char *)data, size) != csum) {
-		pr_err("bootconfig checksum failed\n");
-		return;
-	}
-
 	copy = memblock_alloc(size + 1, SMP_CACHE_BYTES);
 	if (!copy) {
 		pr_err("Failed to allocate memory for bootconfig\n");

index adafb7c50940..c5965eff62c5 100755
@@ -49,7 +49,7 @@ xpass $BOOTCONF -a $TEMPCONF $INITRD
 new_size=$(stat -c %s $INITRD)
 
 echo "File size check"
-xpass test $new_size -eq $(expr $bconf_size + $initrd_size + 9 + 12)
+xpass test $new_size -eq $(expr $bconf_size + $initrd_size + 5 + 12)
 
 echo "Apply command repeat test"
 xpass $BOOTCONF -a $TEMPCONF $INITRD

Hmm, I think the checksum is still good to have, in case of a corrupted file. Does it hurt to keep it?

-- Steve

No problem. We can drop this patch.
I thought this check was a bit redundant since (originally) this was introduced to identify the bootconfig.

Thank you,

Masami Hiramatsu <[email protected]>

Reject if a value node is mixed with subkey node on same parent key node.

A value node can not co-exist with subkey node under some key node, e.g.

key = value
key.subkey = another-value

This is not be allowed because bootconfig API is not designed to handle such case.

---
 Documentation/admin-guide/bootconfig.rst     |    7 +++++++
 lib/bootconfig.c                             |   16 ++++++++++++----
 tools/bootconfig/samples/bad-mixed-kv1.bconf |    3 +++
 tools/bootconfig/samples/bad-mixed-kv2.bconf |    3 +++
 4 files changed, 25 insertions(+), 4 deletions(-)
 create mode 100644 tools/bootconfig/samples/bad-mixed-kv1.bconf
 create mode 100644 tools/bootconfig/samples/bad-mixed-kv2.bconf

index 48675052c963..4a106584eb21 100644
@@ -62,6 +62,13 @@ Or more shorter, written as following::
 In both styles, same key words are automatically merged when parsing it
 at boot time. So you can append similar trees or key-values.
 
+Note that a sub-key and a value can not co-exist under a parent key.
+For example, following config is NOT allowed.::
+
+ foo = value1
+ foo.bar = value2 # !ERROR! subkey "bar" and value "value1" can NOT co-exist
+
+
 Comments
 --------
 

index 3ea601a2eba5..54ac623ca781 100644
@@ -533,7 +533,7 @@ struct xbc_node *find_match_node(struct xbc_node *node, char *k)
 
 static int __init __xbc_add_key(char *k)
 {
-	struct xbc_node *node;
+	struct xbc_node *node, *child;
 
 	if (!xbc_valid_keyword(k))
 		return xbc_parse_error("Invalid keyword", k);

@@ -543,8 +543,12 @@ static int __init __xbc_add_key(char *k)
 
 	if (!last_parent)	/* the first level */
 		node = find_match_node(xbc_nodes, k);
-	else
-		node = find_match_node(xbc_node_get_child(last_parent), k);
+	else {
+		child = xbc_node_get_child(last_parent);
+		if (child && xbc_node_is_value(child))
+			return xbc_parse_error("Subkey is mixed with value", k);
+		node = find_match_node(child, k);
+	}
 
 	if (node)
 		last_parent = node;

@@ -577,7 +581,7 @@ static int __init __xbc_parse_keys(char *k)
 static int __init xbc_parse_kv(char **k, char *v)
 {
 	struct xbc_node *prev_parent = last_parent;
-	struct xbc_node *node;
+	struct xbc_node *node, *child;
 	char *next;
 	int c, ret;
 

@@ -585,6 +589,10 @@ static int __init xbc_parse_kv(char **k, char *v)
 	if (ret)
 		return ret;
 
+	child = xbc_node_get_child(last_parent);
+	if (child && xbc_node_is_key(child))
+		return xbc_parse_error("Value is mixed with subkey", v);
+
 	c = __xbc_parse_value(&v, &next);
 	if (c < 0)
 		return c;

new file mode 100644
index 000000000000..1761547dd05c
@@ -0,0 +1,3 @@
+# value -> subkey pattern
+key = value
+key.subkey = another-value

new file mode 100644
index 000000000000..6b32e0c3878c
@@ -0,0 +1,3 @@
+# subkey -> value pattern
+key.subkey = value
+key = another-value

Currently, bootconfig does not overwrite existing value on same key, but add new value to the tail of an array. But this looks a bit confusing because similar syntax configuration always overwrite the value by default.

This changes the behavior to overwrite value on same key.

For example, if there are 2 entries

key = value
...
key = value2

Then, the key is updated as below.

key = value2

---
 Documentation/admin-guide/bootconfig.rst |   12 ++++++++++++
 lib/bootconfig.c                         |   18 ++++++++++++++----
 tools/bootconfig/test-bootconfig.sh      |   16 ++++++++++++++--
 3 files changed, 40 insertions(+), 6 deletions(-)

index 4a106584eb21..0c18e3f540e6 100644
@@ -62,6 +62,18 @@ Or more shorter, written as following::
 In both styles, same key words are automatically merged when parsing it
 at boot time. So you can append similar trees or key-values.
 
+Same-key Values
+---------------
+
+If two or more values or arraies share a same-key, the latter one remains.
+For example,::
+
+ foo = bar, baz
+ foo = qux
+
+In this case ``bar`` and ``baz`` is overwritten by ``qux``, and ``foo = qux``
+remains.
+
 Note that a sub-key and a value can not co-exist under a parent key.
 For example, following config is NOT allowed.::
 

index 54ac623ca781..9a094162ea3e 100644
@@ -578,10 +578,18 @@ static int __init __xbc_parse_keys(char *k)
 	return __xbc_add_key(k);
 }
 
+static void xbc_node_overwrite(struct xbc_node *node, char *v)
+{
+	unsigned long offset = v - xbc_data;
+
+	node->data = (u16)offset | XBC_VALUE;
+	node->next = 0;
+}
+
 static int __init xbc_parse_kv(char **k, char *v)
 {
 	struct xbc_node *prev_parent = last_parent;
-	struct xbc_node *node, *child;
+	struct xbc_node *child;
 	char *next;
 	int c, ret;
 

@@ -597,9 +605,11 @@ static int __init xbc_parse_kv(char **k, char *v)
 	if (c < 0)
 		return c;
 
-	node = xbc_add_sibling(v, XBC_VALUE);
-	if (!node)
-		return -ENOMEM;
+	if (!child) {
+		if (!xbc_add_sibling(v, XBC_VALUE))
+			return -ENOMEM;
+	} else
+		xbc_node_overwrite(child, v);
 
 	if (c == ',') {	/* Array */
 		c = xbc_parse_array(&next);

index c5965eff62c5..f6948790c693 100755
@@ -9,7 +9,7 @@ TEMPCONF=`mktemp temp-XXXX.bconf`
 NG=0
 
 cleanup() {
-  rm -f $INITRD $TEMPCONF
+  rm -f $INITRD $TEMPCONF $OUTFILE
   exit $NG
 }
 

@@ -71,7 +71,6 @@ printf " \0\0\0 \0\0\0" >> $INITRD
 $BOOTCONF -a $TEMPCONF $INITRD > $OUTFILE 2>&1
 xfail grep -i "failed" $OUTFILE
 xfail grep -i "error" $OUTFILE
-rm $OUTFILE
 
 echo "Max node number check"
 

@@ -96,6 +95,19 @@ truncate -s 32764 $TEMPCONF
 echo "\"" >> $TEMPCONF	# add 2 bytes + terminal ('\"\n\0')
 xpass $BOOTCONF -a $TEMPCONF $INITRD
 
+echo "Overwrite 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 "=== expected failure cases ==="
 for i in samples/bad-* ; do
   xfail $BOOTCONF -a $i $INITRD

Should we even allow this case? Or at the very least, some output should be made that a value is being overwritten.

-- Steve

Both are OK, but I like just making it error. At this moment, the bootconfig tool writes user-given bootconfig file to initrd (not reformat it). This means if user ignores the warning from bootconfig tool, they will see same warning on dmesg again.

Thank you,

Masami Hiramatsu <[email protected]>

OK, so you will be updating this patch?

FYI, I pulled in patches 1-3,5 and 8. I dropped patch 4, and wanted feedback from you on patch 6, and patch 7 depended on 6.

Feel free to update patch 6 and 7 on top of my git tree branch ftrace/urgent.

-- Steve

Yes.

OK, I'll send update soon.

Thank you,

Masami Hiramatsu <[email protected]>

Add append value operator "+=" support to bootconfig syntax. With this operator, user can add new value to the key as an entry of array instead of overwriting.
For example,

foo = bar
...
foo += baz

Then the key "foo" has "bar" and "baz" values as an array.

---
 Documentation/admin-guide/bootconfig.rst |    8 ++++++++
 lib/bootconfig.c                         |   14 ++++++++++----
 tools/bootconfig/test-bootconfig.sh      |   13 +++++++++++++
 3 files changed, 31 insertions(+), 4 deletions(-)

index 0c18e3f540e6..abe2432b8eec 100644
@@ -74,6 +74,14 @@ For example,::
 In this case ``bar`` and ``baz`` is overwritten by ``qux``, and ``foo = qux``
 remains.
 
+If you want to append the value to existing key as an array member,
+you can use ``+=`` operator. For example::
+
+ foo = bar, baz
+ foo += qux
+
+In this case, the key ``foo`` has ``bar``, ``baz`` and ``qux``.
+
 Note that a sub-key and a value can not co-exist under a parent key.
 For example, following config is NOT allowed.::
 

index 9a094162ea3e..4afc768489cd 100644
@@ -586,7 +586,7 @@ static void xbc_node_overwrite(struct xbc_node *node, char *v)
 	node->next = 0;
 }
 
-static int __init xbc_parse_kv(char **k, char *v)
+static int __init xbc_parse_kv(char **k, char *v, int op)
 {
 	struct xbc_node *prev_parent = last_parent;
 	struct xbc_node *child;

@@ -605,7 +605,7 @@ static int __init xbc_parse_kv(char **k, char *v)
 	if (c < 0)
 		return c;
 
-	if (!child) {
+	if (op == '+' || !child) {
 		if (!xbc_add_sibling(v, XBC_VALUE))
 			return -ENOMEM;
 	} else

@@ -781,7 +781,7 @@ int __init xbc_init(char *buf)
 
 	p = buf;
 	do {
-		q = strpbrk(p, "{}=;\n#");
+		q = strpbrk(p, "{}=+;\n#");
 		if (!q) {
 			p = skip_spaces(p);
 			if (*p != '\0')

@@ -792,8 +792,14 @@ int __init xbc_init(char *buf)
 		c = *q;
 		*q++ = '\0';
 		switch (c) {
+		case '+':
+			if (*q++ != '=') {
+				ret = xbc_parse_error("Wrong + operator", q - 2);
+				break;
+			}
+			/* Fall through */
 		case '=':
-			ret = xbc_parse_kv(&p, q);
+			ret = xbc_parse_kv(&p, q, c);
 			break;
 		case '{':
 			ret = xbc_open_brace(&p, q);

index f6948790c693..04f65548ca1e 100755
@@ -108,6 +108,19 @@ xfail grep -q "bar" $OUTFILE
 xfail grep -q "baz" $OUTFILE
 xpass grep -q "qux" $OUTFILE
 
+echo "Adding same-key values"
+cat > $TEMPCONF << EOF
+key = bar, baz
+key += qux
+EOF
+echo > $INITRD
+
+xpass $BOOTCONF -a $TEMPCONF $INITRD
+$BOOTCONF $INITRD > $OUTFILE
+xpass grep -q "bar" $OUTFILE
+xpass grep -q "baz" $OUTFILE
+xpass grep -q "qux" $OUTFILE
+
 echo "=== expected failure cases ==="
 for i in samples/bad-* ; do
   xfail $BOOTCONF -a $i $INITRD

Print arraied values as multiple same options for legacy kernel command line. With this rule, if the "kernel.*" and "init.*" array entries in bootconfig are printed out as multiple same options, e.g.

kernel {
console = "ttyS0,115200"
console += "tty0"
}

will be correctly converted to

console="ttyS0,115200" console="tty0"

in the kernel command line.

---
 init/main.c |   22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

index 821c582af49b..19a5577e0bb0 100644
@@ -268,7 +268,6 @@ static int __init xbc_snprint_cmdline(char *buf, size_t size,
 {
 	struct xbc_node *knode, *vnode;
 	char *end = buf + size;
-	char c = '\"';
 	const char *val;
 	int ret;
 

@@ -279,25 +278,20 @@ static int __init xbc_snprint_cmdline(char *buf, size_t size,
 			return ret;
 
 		vnode = xbc_node_get_child(knode);
-		ret = snprintf(buf, rest(buf, end), "%s%c", xbc_namebuf,
-				vnode ? '=' : ' ');
-		if (ret < 0)
-			return ret;
-		buf += ret;
-		if (!vnode)
+		if (!vnode) {
+			ret = snprintf(buf, rest(buf, end), "%s ", xbc_namebuf);
+			if (ret < 0)
+				return ret;
+			buf += ret;
 			continue;
-
-		c = '\"';
+		}
 		xbc_array_for_each_value(vnode, val) {
-			ret = snprintf(buf, rest(buf, end), "%c%s", c, val);
+			ret = snprintf(buf, rest(buf, end), "%s=\"%s\" ",
+				       xbc_namebuf, val);
 			if (ret < 0)
 				return ret;
 			buf += ret;
-			c = ',';
 		}
-		if (rest(buf, end) > 2)
-			strcpy(buf, "\" ");
-		buf += 2;
 	}
 
 	return buf - (end - size);