linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] firmware: google: memconsole: Adapt to new coreboot ring buffer format
@ 2017-04-27 20:59 Julius Werner
  2017-04-28  7:14 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 3+ messages in thread
From: Julius Werner @ 2017-04-27 20:59 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, Thierry Escande, Dmitry Torokhov, Aaron Durbin,
	Julius Werner

This patch needs to be applied on top of the other memconsole patches
currently queued up in char-misc-next (ending in 049a59db34e).

-------------------------8<--------------------------------------------

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 addition, it is desirable for the driver to always return the current
state of the console, so that it may be used by runtime firmware to log
further messages after the kernel is already initialized. The existing
implementation would already re-read the buffer on every access, but
only measured the total size of the console once during initialization.
Thus, the generic console interface was redesigned to allow the
implementation full control over how to process an access, and the
coreboot implementation will re-read the console size every time.

Finally, add a new /sys/firmware/rawlog node and change the existing
/sys/firmware/log to sanitize non-printable characters from the output.
With the new persistent console it becomes more likely that bytes might
get slightly corrupted (due to DRAM degradation during a reboot), and
it's usually undesirable to get stray control characters in the console
dump because a bit in a letter flipped.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 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(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/google/memconsole-coreboot.c
index 21210144def7..6e9b07697f70 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -26,13 +26,52 @@
 
 /* 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)
+{
+	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)
 {
 	struct cbmem_cons __iomem *tmp_cbmc;
@@ -43,16 +82,14 @@ 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);
 
 	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..44b1fba8bf17 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>
@@ -22,39 +23,58 @@
 
 #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,
+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)
 {
-	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 = {
+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(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);
+	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);
 
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

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] firmware: google: memconsole: Adapt to new coreboot ring buffer format
  2017-04-27 20:59 [PATCH] firmware: google: memconsole: Adapt to new coreboot ring buffer format Julius Werner
@ 2017-04-28  7:14 ` Greg Kroah-Hartman
       [not found]   ` <CAODwPW9bJ=qX7L1uDBpnpV3RKHOxAurhF1=te2g54GGSJTAtBQ@mail.gmail.com>
  0 siblings, 1 reply; 3+ messages in thread
From: Greg Kroah-Hartman @ 2017-04-28  7:14 UTC (permalink / raw)
  To: Julius Werner
  Cc: linux-kernel, Thierry Escande, Dmitry Torokhov, Aaron Durbin

On Thu, Apr 27, 2017 at 01:59:17PM -0700, Julius Werner wrote:
> This patch needs to be applied on top of the other memconsole patches
> currently queued up in char-misc-next (ending in 049a59db34e).
> 
> -------------------------8<--------------------------------------------

That needs to go below the --- line, not above it.

> 
> 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 addition, it is desirable for the driver to always return the current
> state of the console, so that it may be used by runtime firmware to log
> further messages after the kernel is already initialized. The existing
> implementation would already re-read the buffer on every access, but
> only measured the total size of the console once during initialization.
> Thus, the generic console interface was redesigned to allow the
> implementation full control over how to process an access, and the
> coreboot implementation will re-read the console size every time.
> 
> Finally, add a new /sys/firmware/rawlog node and change the existing
> /sys/firmware/log to sanitize non-printable characters from the output.
> With the new persistent console it becomes more likely that bytes might
> get slightly corrupted (due to DRAM degradation during a reboot), and
> it's usually undesirable to get stray control characters in the console
> dump because a bit in a letter flipped.

When you say things like "in addition" and "finally", that implies that
the code needs to be broken up into multiple patches.  Rememeber, each
patch only should ever do 1 thing.

Please do that here and make a patch series.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] firmware: google: memconsole: Adapt to new coreboot ring buffer format
       [not found]   ` <CAODwPW9bJ=qX7L1uDBpnpV3RKHOxAurhF1=te2g54GGSJTAtBQ@mail.gmail.com>
@ 2017-04-29  5:33     ` Greg Kroah-Hartman
  0 siblings, 0 replies; 3+ messages in thread
From: Greg Kroah-Hartman @ 2017-04-29  5:33 UTC (permalink / raw)
  To: Julius Werner; +Cc: LKML, Thierry Escande, Dmitry Torokhov, Aaron Durbin

On Fri, Apr 28, 2017 at 01:44:45PM -0700, Julius Werner wrote:
>     > -------------------------8<--------------------------------------------
> 
>     That needs to go below the --- line, not above it.
> 
> 
> Really? git am --scissors seems to do the right thing for me this way...

Ah, never used that option before, it's not part of my normal workflow.

> Anyway, it was only an FYI, I'll just leave it out.
>  
> 
>     When you say things like "in addition" and "finally", that implies that
>     the code needs to be broken up into multiple patches.  Rememeber, each
>     patch only should ever do 1 thing.
> 
> 
> Well... all these things are required to make the new format work. But I can
> split them up further if you like. 

Please do, makes it easier to review, right?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2017-04-29  5:38 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-27 20:59 [PATCH] firmware: google: memconsole: Adapt to new coreboot ring buffer format Julius Werner
2017-04-28  7:14 ` Greg Kroah-Hartman
     [not found]   ` <CAODwPW9bJ=qX7L1uDBpnpV3RKHOxAurhF1=te2g54GGSJTAtBQ@mail.gmail.com>
2017-04-29  5:33     ` Greg Kroah-Hartman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).