[PATCH v2 0/3] Memconsole changes for new coreboot format

[PATCH v2 0/3] Memconsole changes for new coreboot format

[Julius Werner]

This series enhances the memconsole driver to work well with the new
persistent ring buffer console introduced in coreboot with
https://review.coreboot.org/#/c/18301. It needs to be applied on top of
the other memconsole patches currently queued up in char-misc-next
(ending in 049a59db34e).

Julius Werner (3):
  firmware: google: memconsole: Make memconsole interface more flexible
  firmware: google: memconsole: Escape unprintable characters
  firmware: google: memconsole: Adapt to new coreboot ring buffer format

 drivers/firmware/google/memconsole-coreboot.c   | 51 +++++++++++++++++++++----
 drivers/firmware/google/memconsole-x86-legacy.c | 18 +++++++--
 drivers/firmware/google/memconsole.c            | 46 +++++++++++++++-------
 drivers/firmware/google/memconsole.h            |  7 ++--
 4 files changed, 96 insertions(+), 26 deletions(-)

-- 
2.12.2

[PATCH v2 1/3] firmware: google: memconsole: Make memconsole interface more flexible

[Julius Werner]

This patch redesigns the interface between the generic memconsole driver
and its implementations to become more flexible than a flat memory
buffer with unchanging bounds. This allows memconsoles like coreboot's
to include lines that were added by runtime firmware after the driver
was initialized. Since the console log size is thus no longer static,
this means that the /sys/firmware/log file has to become unseekable.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 drivers/firmware/google/memconsole-coreboot.c   | 12 +++++++++---
 drivers/firmware/google/memconsole-x86-legacy.c | 18 +++++++++++++++---
 drivers/firmware/google/memconsole.c            | 14 ++++++--------
 drivers/firmware/google/memconsole.h            |  7 ++++---
 4 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/google/memconsole-coreboot.c
index 21210144def7..b6234e61004d 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -33,6 +33,14 @@ struct cbmem_cons {
 
 static struct cbmem_cons __iomem *cbmem_console;
 
+static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count)
+{
+	return memory_read_from_buffer(buf, count, &pos,
+				       cbmem_console->buffer_body,
+				       min(cbmem_console->buffer_cursor,
+					   cbmem_console->buffer_size));
+}
+
 static int memconsole_coreboot_init(phys_addr_t physaddr)
 {
 	struct cbmem_cons __iomem *tmp_cbmc;
@@ -50,9 +58,7 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
 	if (!cbmem_console)
 		return -ENOMEM;
 
-	memconsole_setup(cbmem_console->buffer_body,
-		min(cbmem_console->buffer_cursor, cbmem_console->buffer_size));
-
+	memconsole_setup(memconsole_coreboot_read);
 	return 0;
 }
 
diff --git a/drivers/firmware/google/memconsole-x86-legacy.c b/drivers/firmware/google/memconsole-x86-legacy.c
index 529078c62488..1b556e944599 100644
--- a/drivers/firmware/google/memconsole-x86-legacy.c
+++ b/drivers/firmware/google/memconsole-x86-legacy.c
@@ -49,6 +49,15 @@ struct biosmemcon_ebda {
 	};
 } __packed;
 
+static char *memconsole_baseaddr;
+static size_t memconsole_length;
+
+static ssize_t memconsole_read(char *buf, loff_t pos, size_t count)
+{
+	return memory_read_from_buffer(buf, count, &pos, memconsole_baseaddr,
+				       memconsole_length);
+}
+
 static void found_v1_header(struct biosmemcon_ebda *hdr)
 {
 	pr_info("memconsole: BIOS console v1 EBDA structure found at %p\n",
@@ -57,7 +66,9 @@ static void found_v1_header(struct biosmemcon_ebda *hdr)
 		hdr->v1.buffer_addr, hdr->v1.start,
 		hdr->v1.end, hdr->v1.num_chars);
 
-	memconsole_setup(phys_to_virt(hdr->v1.buffer_addr), hdr->v1.num_chars);
+	memconsole_baseaddr = phys_to_virt(hdr->v1.buffer_addr);
+	memconsole_length = hdr->v1.num_chars;
+	memconsole_setup(memconsole_read);
 }
 
 static void found_v2_header(struct biosmemcon_ebda *hdr)
@@ -68,8 +79,9 @@ static void found_v2_header(struct biosmemcon_ebda *hdr)
 		hdr->v2.buffer_addr, hdr->v2.start,
 		hdr->v2.end, hdr->v2.num_bytes);
 
-	memconsole_setup(phys_to_virt(hdr->v2.buffer_addr + hdr->v2.start),
-			 hdr->v2.end - hdr->v2.start);
+	memconsole_baseaddr = phys_to_virt(hdr->v2.buffer_addr + hdr->v2.start);
+	memconsole_length = hdr->v2.end - hdr->v2.start;
+	memconsole_setup(memconsole_read);
 }
 
 /*
diff --git a/drivers/firmware/google/memconsole.c b/drivers/firmware/google/memconsole.c
index 94e200ddb4fa..166f07c68c02 100644
--- a/drivers/firmware/google/memconsole.c
+++ b/drivers/firmware/google/memconsole.c
@@ -22,15 +22,15 @@
 
 #include "memconsole.h"
 
-static char *memconsole_baseaddr;
-static size_t memconsole_length;
+static ssize_t (*memconsole_read_func)(char *, loff_t, size_t);
 
 static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
 			       struct bin_attribute *bin_attr, char *buf,
 			       loff_t pos, size_t count)
 {
-	return memory_read_from_buffer(buf, count, &pos, memconsole_baseaddr,
-				       memconsole_length);
+	if (WARN_ON_ONCE(!memconsole_read_func))
+		return -EIO;
+	return memconsole_read_func(buf, pos, count);
 }
 
 static struct bin_attribute memconsole_bin_attr = {
@@ -38,16 +38,14 @@ static struct bin_attribute memconsole_bin_attr = {
 	.read = memconsole_read,
 };
 
-void memconsole_setup(void *baseaddr, size_t length)
+void memconsole_setup(ssize_t (*read_func)(char *, loff_t, size_t))
 {
-	memconsole_baseaddr = baseaddr;
-	memconsole_length = length;
+	memconsole_read_func = read_func;
 }
 EXPORT_SYMBOL(memconsole_setup);
 
 int memconsole_sysfs_init(void)
 {
-	memconsole_bin_attr.size = memconsole_length;
 	return sysfs_create_bin_file(firmware_kobj, &memconsole_bin_attr);
 }
 EXPORT_SYMBOL(memconsole_sysfs_init);
diff --git a/drivers/firmware/google/memconsole.h b/drivers/firmware/google/memconsole.h
index 190fc03a51ae..ff1592dc7d1a 100644
--- a/drivers/firmware/google/memconsole.h
+++ b/drivers/firmware/google/memconsole.h
@@ -18,13 +18,14 @@
 #ifndef __FIRMWARE_GOOGLE_MEMCONSOLE_H
 #define __FIRMWARE_GOOGLE_MEMCONSOLE_H
 
+#include <linux/types.h>
+
 /*
  * memconsole_setup
  *
- * Initialize the memory console from raw (virtual) base
- * address and length.
+ * Initialize the memory console, passing the function to handle read accesses.
  */
-void memconsole_setup(void *baseaddr, size_t length);
+void memconsole_setup(ssize_t (*read_func)(char *, loff_t, size_t));
 
 /*
  * memconsole_sysfs_init
-- 
2.12.2

[PATCH v2 2/3] firmware: google: memconsole: Escape unprintable characters

[Julius Werner]

Recent improvements in coreboot's memory console allow it to contain
logs from more than one boot as long as the information persists in
memory. Since trying to persist a memory buffer across reboots often
doesn't quite work perfectly it is now more likely for random bit flips
to occur in the console. With the current implementation this can lead
to stray control characters that cause weird effects for the most common
use cases (such as just dumping the console with 'cat').

This patch changes the memconsole driver to replace unprintable
characters with '?' by default. It also adds a new /sys/firmware/rawlog
node next to the existing /sys/firmware/log for use cases where it's
desired to read the raw characters.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 drivers/firmware/google/memconsole.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/google/memconsole.c b/drivers/firmware/google/memconsole.c
index 166f07c68c02..807fcd4f7f85 100644
--- a/drivers/firmware/google/memconsole.c
+++ b/drivers/firmware/google/memconsole.c
@@ -15,6 +15,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/ctype.h>
 #include <linux/init.h>
 #include <linux/sysfs.h>
 #include <linux/kobject.h>
@@ -24,7 +25,7 @@
 
 static ssize_t (*memconsole_read_func)(char *, loff_t, size_t);
 
-static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
+static ssize_t memconsole_read_raw(struct file *filp, struct kobject *kobp,
 			       struct bin_attribute *bin_attr, char *buf,
 			       loff_t pos, size_t count)
 {
@@ -33,9 +34,28 @@ static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
 	return memconsole_read_func(buf, pos, count);
 }
 
-static struct bin_attribute memconsole_bin_attr = {
+static ssize_t memconsole_read_log(struct file *filp, struct kobject *kobp,
+			       struct bin_attribute *bin_attr, char *buf,
+			       loff_t pos, size_t count)
+{
+	ssize_t i;
+	ssize_t rv = memconsole_read_raw(filp, kobp, bin_attr, buf, pos, count);
+
+	if (rv > 0)
+		for (i = 0; i < rv; i++)
+			if (!isprint(buf[i]) && !isspace(buf[i]))
+				buf[i] = '?';
+	return rv;
+}
+
+/* Memconsoles may be much longer than 4K, so need to use binary attributes. */
+static struct bin_attribute memconsole_log_attr = {
 	.attr = {.name = "log", .mode = 0444},
-	.read = memconsole_read,
+	.read = memconsole_read_log,
+};
+static struct bin_attribute memconsole_raw_attr = {
+	.attr = {.name = "rawlog", .mode = 0444},
+	.read = memconsole_read_raw,
 };
 
 void memconsole_setup(ssize_t (*read_func)(char *, loff_t, size_t))
@@ -46,13 +66,15 @@ EXPORT_SYMBOL(memconsole_setup);
 
 int memconsole_sysfs_init(void)
 {
-	return sysfs_create_bin_file(firmware_kobj, &memconsole_bin_attr);
+	return sysfs_create_bin_file(firmware_kobj, &memconsole_log_attr) ||
+		sysfs_create_bin_file(firmware_kobj, &memconsole_raw_attr);
 }
 EXPORT_SYMBOL(memconsole_sysfs_init);
 
 void memconsole_exit(void)
 {
-	sysfs_remove_bin_file(firmware_kobj, &memconsole_bin_attr);
+	sysfs_remove_bin_file(firmware_kobj, &memconsole_log_attr);
+	sysfs_remove_bin_file(firmware_kobj, &memconsole_raw_attr);
 }
 EXPORT_SYMBOL(memconsole_exit);
 
-- 
2.12.2

Re: [PATCH v2 2/3] firmware: google: memconsole: Escape unprintable characters

[Greg Kroah-Hartman]

On Fri, Apr 28, 2017 at 01:42:24PM -0700, Julius Werner wrote:
> Recent improvements in coreboot's memory console allow it to contain
> logs from more than one boot as long as the information persists in
> memory. Since trying to persist a memory buffer across reboots often
> doesn't quite work perfectly it is now more likely for random bit flips
> to occur in the console. With the current implementation this can lead
> to stray control characters that cause weird effects for the most common
> use cases (such as just dumping the console with 'cat').
> 
> This patch changes the memconsole driver to replace unprintable
> characters with '?' by default. It also adds a new /sys/firmware/rawlog
> node next to the existing /sys/firmware/log for use cases where it's
> desired to read the raw characters.

Again, you are doing multiple things here.  Break it up.

And, can userspace handle this change?  You are now changing the format
of the existing file.

> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  drivers/firmware/google/memconsole.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/firmware/google/memconsole.c b/drivers/firmware/google/memconsole.c
> index 166f07c68c02..807fcd4f7f85 100644
> --- a/drivers/firmware/google/memconsole.c
> +++ b/drivers/firmware/google/memconsole.c
> @@ -15,6 +15,7 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <linux/ctype.h>
>  #include <linux/init.h>
>  #include <linux/sysfs.h>
>  #include <linux/kobject.h>
> @@ -24,7 +25,7 @@
>  
>  static ssize_t (*memconsole_read_func)(char *, loff_t, size_t);
>  
> -static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
> +static ssize_t memconsole_read_raw(struct file *filp, struct kobject *kobp,
>  			       struct bin_attribute *bin_attr, char *buf,
>  			       loff_t pos, size_t count)
>  {
> @@ -33,9 +34,28 @@ static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
>  	return memconsole_read_func(buf, pos, count);
>  }
>  
> -static struct bin_attribute memconsole_bin_attr = {
> +static ssize_t memconsole_read_log(struct file *filp, struct kobject *kobp,
> +			       struct bin_attribute *bin_attr, char *buf,
> +			       loff_t pos, size_t count)
> +{
> +	ssize_t i;
> +	ssize_t rv = memconsole_read_raw(filp, kobp, bin_attr, buf, pos, count);
> +
> +	if (rv > 0)
> +		for (i = 0; i < rv; i++)
> +			if (!isprint(buf[i]) && !isspace(buf[i]))
> +				buf[i] = '?';
> +	return rv;
> +}
> +
> +/* Memconsoles may be much longer than 4K, so need to use binary attributes. */
> +static struct bin_attribute memconsole_log_attr = {
>  	.attr = {.name = "log", .mode = 0444},
> -	.read = memconsole_read,
> +	.read = memconsole_read_log,
> +};
> +static struct bin_attribute memconsole_raw_attr = {
> +	.attr = {.name = "rawlog", .mode = 0444},
> +	.read = memconsole_read_raw,
>  };
>  
>  void memconsole_setup(ssize_t (*read_func)(char *, loff_t, size_t))
> @@ -46,13 +66,15 @@ EXPORT_SYMBOL(memconsole_setup);
>  
>  int memconsole_sysfs_init(void)
>  {
> -	return sysfs_create_bin_file(firmware_kobj, &memconsole_bin_attr);
> +	return sysfs_create_bin_file(firmware_kobj, &memconsole_log_attr) ||
> +		sysfs_create_bin_file(firmware_kobj, &memconsole_raw_attr);

While it is really rare, please do this properly and create one file,
and then the other, and provide proper error handling here (i.e. if the
second fails, remove the first).

thanks,

greg k-h

Re: [PATCH v2 2/3] firmware: google: memconsole: Escape unprintable characters

[Julius Werner]

On Fri, Apr 28, 2017 at 10:37 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Apr 28, 2017 at 01:42:24PM -0700, Julius Werner wrote:
>> Recent improvements in coreboot's memory console allow it to contain
>> logs from more than one boot as long as the information persists in
>> memory. Since trying to persist a memory buffer across reboots often
>> doesn't quite work perfectly it is now more likely for random bit flips
>> to occur in the console. With the current implementation this can lead
>> to stray control characters that cause weird effects for the most common
>> use cases (such as just dumping the console with 'cat').
>>
>> This patch changes the memconsole driver to replace unprintable
>> characters with '?' by default. It also adds a new /sys/firmware/rawlog
>> node next to the existing /sys/firmware/log for use cases where it's
>> desired to read the raw characters.
>
> Again, you are doing multiple things here.  Break it up.

Sorry, I'm not sure what else you want me to break up here? If I add
the escaping in one patch and then add the raw node in a different
patch, there's a gap between the two patches where we're losing
functionality. Isn't that undesirable?

> And, can userspace handle this change?  You are now changing the format
> of the existing file.

I'm escaping characters that previously hadn't been escaped, but
really, I'm just trying to make sure that people can continue to use
this node as before. The real change has already happened in coreboot,
outside of Linux. This node could previously be relied upon to only
contain printable characters, and with newer versions of coreboot
that's not always the case anymore. I chose to do it this way because
I thought it would be the least disruptive for most users.

It can be handled in userspace, yes. It depends on what you use to
read the file, mostly... people using less will be fine, but people
using cat may get weird behavior. You're right that it doesn't really
*need* to be escaped in the kernel, and honestly I don't care too
much... I can leave it out if you want. But I think this is the most
convenient way to do it for existing users.

>> -     return sysfs_create_bin_file(firmware_kobj, &memconsole_bin_attr);
>> +     return sysfs_create_bin_file(firmware_kobj, &memconsole_log_attr) ||
>> +             sysfs_create_bin_file(firmware_kobj, &memconsole_raw_attr);
>
> While it is really rare, please do this properly and create one file,
> and then the other, and provide proper error handling here (i.e. if the
> second fails, remove the first).

Okay, will send that with the next set once we agree on the other stuff.

Re: [PATCH v2 2/3] firmware: google: memconsole: Escape unprintable characters

[Greg Kroah-Hartman]

On Mon, May 01, 2017 at 04:44:15PM -0700, Julius Werner wrote:
> On Fri, Apr 28, 2017 at 10:37 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Fri, Apr 28, 2017 at 01:42:24PM -0700, Julius Werner wrote:
> >> Recent improvements in coreboot's memory console allow it to contain
> >> logs from more than one boot as long as the information persists in
> >> memory. Since trying to persist a memory buffer across reboots often
> >> doesn't quite work perfectly it is now more likely for random bit flips
> >> to occur in the console. With the current implementation this can lead
> >> to stray control characters that cause weird effects for the most common
> >> use cases (such as just dumping the console with 'cat').
> >>
> >> This patch changes the memconsole driver to replace unprintable
> >> characters with '?' by default. It also adds a new /sys/firmware/rawlog
> >> node next to the existing /sys/firmware/log for use cases where it's
> >> desired to read the raw characters.
> >
> > Again, you are doing multiple things here.  Break it up.
> 
> Sorry, I'm not sure what else you want me to break up here? If I add
> the escaping in one patch and then add the raw node in a different
> patch, there's a gap between the two patches where we're losing
> functionality. Isn't that undesirable?

Raw node first and then escape the old one?

> > And, can userspace handle this change?  You are now changing the format
> > of the existing file.
> 
> I'm escaping characters that previously hadn't been escaped, but
> really, I'm just trying to make sure that people can continue to use
> this node as before. The real change has already happened in coreboot,
> outside of Linux. This node could previously be relied upon to only
> contain printable characters, and with newer versions of coreboot
> that's not always the case anymore. I chose to do it this way because
> I thought it would be the least disruptive for most users.
> 
> It can be handled in userspace, yes. It depends on what you use to
> read the file, mostly... people using less will be fine, but people
> using cat may get weird behavior. You're right that it doesn't really
> *need* to be escaped in the kernel, and honestly I don't care too
> much... I can leave it out if you want. But I think this is the most
> convenient way to do it for existing users.

Binary sysfs files are supposed to be "pass through" only, the kernel
should not be touching the data at all, it's up to userspace to do what
it wants to do with things.  So don't escape anything at all, that's not
the kernel's job here.

thanks,

greg k-h

Re: [PATCH v2 2/3] firmware: google: memconsole: Escape unprintable characters

[Julius Werner]

> Binary sysfs files are supposed to be "pass through" only, the kernel
> should not be touching the data at all, it's up to userspace to do what
> it wants to do with things.  So don't escape anything at all, that's not
> the kernel's job here.

Okay, I'll drop this patch.

[PATCH v2 3/3] firmware: google: memconsole: Adapt to new coreboot ring buffer format

[Julius Werner]

The upstream coreboot implementation of memconsole was enhanced from a
single-boot console to a persistent ring buffer
(https://review.coreboot.org/#/c/18301). This patch changes the kernel
memconsole driver to be able to read the new format in all cases.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 drivers/firmware/google/memconsole-coreboot.c | 47 ++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/google/memconsole-coreboot.c
index b6234e61004d..23dc363b8519 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -26,19 +26,50 @@
 
 /* CBMEM firmware console log descriptor. */
 struct cbmem_cons {
-	u32 buffer_size;
-	u32 buffer_cursor;
-	u8  buffer_body[0];
+	u32 size;
+	u32 cursor;
+	u8  body[0];
 } __packed;
 
+#define CURSOR_MASK ((1 << 28) - 1)
+#define OVERFLOW (1 << 31)
+
 static struct cbmem_cons __iomem *cbmem_console;
 
+/*
+ * The cbmem_console structure is read again on every access because it may
+ * change at any time if runtime firmware logs new messages. This may rarely
+ * lead to race conditions where the firmware overwrites the beginning of the
+ * ring buffer with more lines after we have already read |cursor|. It should be
+ * rare and harmless enough that we don't spend extra effort working around it.
+ */
 static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count)
 {
-	return memory_read_from_buffer(buf, count, &pos,
-				       cbmem_console->buffer_body,
-				       min(cbmem_console->buffer_cursor,
-					   cbmem_console->buffer_size));
+	u32 cursor = cbmem_console->cursor & CURSOR_MASK;
+	u32 flags = cbmem_console->cursor & ~CURSOR_MASK;
+	u32 size = cbmem_console->size;
+	struct seg {	/* describes ring buffer segments in logical order */
+		u32 phys;	/* physical offset from start of mem buffer */
+		u32 len;	/* length of segment */
+	} seg[2] = { {0}, {0} };
+	size_t done = 0;
+	int i;
+
+	if (flags & OVERFLOW) {
+		if (cursor > size)	/* Shouldn't really happen, but... */
+			cursor = 0;
+		seg[0] = (struct seg){.phys = cursor, .len = size - cursor};
+		seg[1] = (struct seg){.phys = 0, .len = cursor};
+	} else {
+		seg[0] = (struct seg){.phys = 0, .len = min(cursor, size)};
+	}
+
+	for (i = 0; i < ARRAY_SIZE(seg) && count > done; i++) {
+		done += memory_read_from_buffer(buf + done, count - done, &pos,
+			cbmem_console->body + seg[i].phys, seg[i].len);
+		pos -= seg[i].len;
+	}
+	return done;
 }
 
 static int memconsole_coreboot_init(phys_addr_t physaddr)
@@ -51,7 +82,7 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
 		return -ENOMEM;
 
 	cbmem_console = memremap(physaddr,
-				 tmp_cbmc->buffer_size + sizeof(*cbmem_console),
+				 tmp_cbmc->size + sizeof(*cbmem_console),
 				 MEMREMAP_WB);
 	memunmap(tmp_cbmc);
 
-- 
2.12.2