[PATCH 0/2] OPAL message log interface

[PATCH 0/2] OPAL message log interface

[Joel Stanley]

These two patches add support for the message log, and expose a new OPAL call
called opal_invalid that allow me to cause OPAL to inject messages into the
log.

The naming is a bit mixed, as our device tree node is opal-memcons and I
retained the naming of the header structure 'struct memcons', but all other
references are to the OPAL message log.

They have been tested on a POWER7+ machine running some recent firmware.

Joel Stanley (2):
  powerpc/powernv: Add OPAL message log interface
  powerpc/powernv: Add invalid OPAL call

 arch/powerpc/include/asm/opal.h                |  6 ++
 arch/powerpc/platforms/powernv/Makefile        |  1 +
 arch/powerpc/platforms/powernv/opal-messages.c | 97 ++++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/opal-wrappers.S |  1 +
 arch/powerpc/platforms/powernv/opal.c          |  6 +-
 5 files changed, 110 insertions(+), 1 deletion(-)
 create mode 100644 arch/powerpc/platforms/powernv/opal-messages.c

-- 
1.9.1

[PATCH 1/2] powerpc/powernv: Add OPAL message log interface

[Joel Stanley]

OPAL provides an in-memory circular buffer containing a message log
populated with various runtime messages produced by the firmware.

Provide a sysfs interface /sys/firmware/opal/messages for userspace to
view the messages.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 arch/powerpc/include/asm/opal.h                |  4 ++
 arch/powerpc/platforms/powernv/Makefile        |  1 +
 arch/powerpc/platforms/powernv/opal-messages.c | 97 ++++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/opal.c          |  4 +-
 4 files changed, 105 insertions(+), 1 deletion(-)
 create mode 100644 arch/powerpc/platforms/powernv/opal-messages.c

diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index ffafab0..6aa757e 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -729,6 +729,9 @@ typedef struct oppanel_line {
 /* /sys/firmware/opal */
 extern struct kobject *opal_kobj;
 
+/* /ibm,opal */
+extern struct device_node *opal_node;
+
 /* API functions */
 int64_t opal_console_write(int64_t term_number, __be64 *length,
 			   const uint8_t *buffer);
@@ -918,6 +921,7 @@ extern void opal_flash_init(void);
 extern int opal_elog_init(void);
 extern void opal_platform_dump_init(void);
 extern void opal_sys_param_init(void);
+extern void opal_messages_init(void);
 
 extern int opal_machine_check(struct pt_regs *regs);
 extern bool opal_mce_check_early_recovery(struct pt_regs *regs);
diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
index f324ea0..e2ba418 100644
--- a/arch/powerpc/platforms/powernv/Makefile
+++ b/arch/powerpc/platforms/powernv/Makefile
@@ -1,6 +1,7 @@
 obj-y			+= setup.o opal-takeover.o opal-wrappers.o opal.o opal-async.o
 obj-y			+= opal-rtc.o opal-nvram.o opal-lpc.o opal-flash.o
 obj-y			+= rng.o opal-elog.o opal-dump.o opal-sysparam.o opal-sensor.o
+obj-y			+= opal-messages.o
 
 obj-$(CONFIG_SMP)	+= smp.o
 obj-$(CONFIG_PCI)	+= pci.o pci-p5ioc2.o pci-ioda.o
diff --git a/arch/powerpc/platforms/powernv/opal-messages.c b/arch/powerpc/platforms/powernv/opal-messages.c
new file mode 100644
index 0000000..3a863e8
--- /dev/null
+++ b/arch/powerpc/platforms/powernv/opal-messages.c
@@ -0,0 +1,97 @@
+/*
+ * PowerNV OPAL in-memory console interface
+ *
+ * Copyright 2014 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <asm/io.h>
+#include <asm/opal.h>
+#include <linux/debugfs.h>
+#include <linux/of.h>
+#include <linux/types.h>
+
+/* OPAL in-memory console. Defined in OPAL source at core/console.c */
+struct memcons {
+	__be64 magic;
+#define MEMCONS_MAGIC	0x6630696567726173L
+	__be64 obuf_phys;
+	__be64 ibuf_phys;
+	__be32 obuf_size;
+	__be32 ibuf_size;
+	__be32 out_pos;
+#define MEMCONS_OUT_POS_WRAP	0x80000000u
+#define MEMCONS_OUT_POS_MASK	0x00ffffffu
+	__be32 in_prod;
+	__be32 in_cons;
+};
+
+static ssize_t opal_messages_read(struct file *file, struct kobject *kobj,
+	struct bin_attribute *bin_attr, char *to, loff_t pos, size_t count)
+{
+	struct memcons *mc = bin_attr->private;
+	const char *conbuf;
+	bool wrapped;
+	size_t num_read;
+	int out_pos;
+
+	if (!mc)
+		return -ENODEV;
+
+	conbuf = phys_to_virt(be64_to_cpu(mc->obuf_phys));
+	wrapped = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_WRAP;
+	out_pos = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_MASK;
+
+	if (!wrapped) {
+		num_read = memory_read_from_buffer(to, count, &pos, conbuf,
+				out_pos);
+	} else {
+		num_read = memory_read_from_buffer(to, count, &pos,
+				conbuf + out_pos,
+				be32_to_cpu(mc->obuf_size) - out_pos);
+
+		if (num_read < 0)
+			goto out;
+
+		num_read += memory_read_from_buffer(to + num_read,
+				count - num_read, &pos, conbuf, out_pos);
+	}
+out:
+	return num_read;
+}
+
+static struct bin_attribute messages_attr = {
+	.attr = {.name = "messages", .mode = 0444},
+	.read = opal_messages_read
+};
+
+void __init opal_messages_init(void)
+{
+	u64 mcaddr;
+	struct memcons *mc;
+
+	if (of_property_read_u64(opal_node, "ibm,opal-memcons", &mcaddr)) {
+		pr_warn("OPAL: Property ibm,opal-memcons not found, no message log\n");
+		return;
+	}
+
+	mc = phys_to_virt(mcaddr);
+	if (!mc) {
+		pr_warn("OPAL: memory console address is invalid\n");
+		return;
+	}
+
+	if (be64_to_cpu(mc->magic) != MEMCONS_MAGIC) {
+		pr_warn("OPAL: memory console version is invalid\n");
+		return;
+	}
+
+	messages_attr.private = mc;
+
+	if (sysfs_create_bin_file(opal_kobj, &messages_attr) != 0)
+		pr_warn("OPAL: sysfs file creation failed\n");
+}
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index e92f2f6..2bc032a 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -46,7 +46,7 @@ struct mcheck_recoverable_range {
 static struct mcheck_recoverable_range *mc_recoverable_range;
 static int mc_recoverable_range_len;
 
-static struct device_node *opal_node;
+struct device_node *opal_node;
 static DEFINE_SPINLOCK(opal_write_lock);
 extern u64 opal_mc_secondary_handler[];
 static unsigned int *opal_irqs;
@@ -574,6 +574,8 @@ static int __init opal_init(void)
 		opal_platform_dump_init();
 		/* Setup system parameters interface */
 		opal_sys_param_init();
+		/* Setup message log interface. */
+		opal_messages_init();
 	}
 
 	return 0;
-- 
1.9.1

[PATCH 2/2] powerpc/powernv: Add invalid OPAL call

[Joel Stanley]

This call will not be understood by OPAL, and cause it to add an error
to it's log. Among other things, this is useful for testing the
behaviour of the log as it fills up.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 arch/powerpc/include/asm/opal.h                | 2 ++
 arch/powerpc/platforms/powernv/opal-wrappers.S | 1 +
 arch/powerpc/platforms/powernv/opal.c          | 2 ++
 3 files changed, 5 insertions(+)

diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 6aa757e..7f480a4 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -87,6 +87,7 @@ extern int opal_enter_rtas(struct rtas_args *args,
 #define OPAL_ASYNC_COMPLETION	-15
 
 /* API Tokens (in r0) */
+#define OPAL_INVALID				-1
 #define OPAL_CONSOLE_WRITE			1
 #define OPAL_CONSOLE_READ			2
 #define OPAL_RTC_READ				3
@@ -853,6 +854,7 @@ int64_t opal_lpc_write(uint32_t chip_id, enum OpalLPCAddressType addr_type,
 int64_t opal_lpc_read(uint32_t chip_id, enum OpalLPCAddressType addr_type,
 		      uint32_t addr, __be32 *data, uint32_t sz);
 
+int64_t opal_invalid(void);
 int64_t opal_read_elog(uint64_t buffer, size_t size, uint64_t log_id);
 int64_t opal_get_elog_size(uint64_t *log_id, size_t *size, uint64_t *elog_type);
 int64_t opal_write_elog(uint64_t buffer, uint64_t size, uint64_t offset);
diff --git a/arch/powerpc/platforms/powernv/opal-wrappers.S b/arch/powerpc/platforms/powernv/opal-wrappers.S
index 75c89df..69657dc 100644
--- a/arch/powerpc/platforms/powernv/opal-wrappers.S
+++ b/arch/powerpc/platforms/powernv/opal-wrappers.S
@@ -143,3 +143,4 @@ OPAL_CALL(opal_sync_host_reboot,		OPAL_SYNC_HOST_REBOOT);
 OPAL_CALL(opal_sensor_read,			OPAL_SENSOR_READ);
 OPAL_CALL(opal_get_param,			OPAL_GET_PARAM);
 OPAL_CALL(opal_set_param,			OPAL_SET_PARAM);
+OPAL_CALL(opal_invalid,				OPAL_INVALID);
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 2bc032a..f58a3c7 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -43,6 +43,8 @@ struct mcheck_recoverable_range {
 	u64 recover_addr;
 };
 
+EXPORT_SYMBOL_GPL(opal_invalid);
+
 static struct mcheck_recoverable_range *mc_recoverable_range;
 static int mc_recoverable_range_len;
 
-- 
1.9.1

Re: [PATCH 1/2] powerpc/powernv: Add OPAL message log interface

[Stewart Smith]

Joel Stanley <joel@jms.id.au> writes:
> OPAL provides an in-memory circular buffer containing a message log
> populated with various runtime messages produced by the firmware.
>
> Provide a sysfs interface /sys/firmware/opal/messages for userspace to
> view the messages.

Acked-by: Stewart Smith <stewart@linux.vnet.ibm.com>

> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  arch/powerpc/include/asm/opal.h                |  4 ++
>  arch/powerpc/platforms/powernv/Makefile        |  1 +
>  arch/powerpc/platforms/powernv/opal-messages.c | 97 ++++++++++++++++++++++++++
>  arch/powerpc/platforms/powernv/opal.c          |  4 +-
>  4 files changed, 105 insertions(+), 1 deletion(-)
>  create mode 100644 arch/powerpc/platforms/powernv/opal-messages.c
>
> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> index ffafab0..6aa757e 100644
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -729,6 +729,9 @@ typedef struct oppanel_line {
>  /* /sys/firmware/opal */
>  extern struct kobject *opal_kobj;
>
> +/* /ibm,opal */
> +extern struct device_node *opal_node;
> +
>  /* API functions */
>  int64_t opal_console_write(int64_t term_number, __be64 *length,
>  			   const uint8_t *buffer);
> @@ -918,6 +921,7 @@ extern void opal_flash_init(void);
>  extern int opal_elog_init(void);
>  extern void opal_platform_dump_init(void);
>  extern void opal_sys_param_init(void);
> +extern void opal_messages_init(void);
>
>  extern int opal_machine_check(struct pt_regs *regs);
>  extern bool opal_mce_check_early_recovery(struct pt_regs *regs);
> diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
> index f324ea0..e2ba418 100644
> --- a/arch/powerpc/platforms/powernv/Makefile
> +++ b/arch/powerpc/platforms/powernv/Makefile
> @@ -1,6 +1,7 @@
>  obj-y			+= setup.o opal-takeover.o opal-wrappers.o opal.o opal-async.o
>  obj-y			+= opal-rtc.o opal-nvram.o opal-lpc.o opal-flash.o
>  obj-y			+= rng.o opal-elog.o opal-dump.o opal-sysparam.o opal-sensor.o
> +obj-y			+= opal-messages.o
>
>  obj-$(CONFIG_SMP)	+= smp.o
>  obj-$(CONFIG_PCI)	+= pci.o pci-p5ioc2.o pci-ioda.o
> diff --git a/arch/powerpc/platforms/powernv/opal-messages.c b/arch/powerpc/platforms/powernv/opal-messages.c
> new file mode 100644
> index 0000000..3a863e8
> --- /dev/null
> +++ b/arch/powerpc/platforms/powernv/opal-messages.c
> @@ -0,0 +1,97 @@
> +/*
> + * PowerNV OPAL in-memory console interface
> + *
> + * Copyright 2014 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <asm/io.h>
> +#include <asm/opal.h>
> +#include <linux/debugfs.h>
> +#include <linux/of.h>
> +#include <linux/types.h>
> +
> +/* OPAL in-memory console. Defined in OPAL source at core/console.c */
> +struct memcons {
> +	__be64 magic;
> +#define MEMCONS_MAGIC	0x6630696567726173L
> +	__be64 obuf_phys;
> +	__be64 ibuf_phys;
> +	__be32 obuf_size;
> +	__be32 ibuf_size;
> +	__be32 out_pos;
> +#define MEMCONS_OUT_POS_WRAP	0x80000000u
> +#define MEMCONS_OUT_POS_MASK	0x00ffffffu
> +	__be32 in_prod;
> +	__be32 in_cons;
> +};
> +
> +static ssize_t opal_messages_read(struct file *file, struct kobject *kobj,
> +	struct bin_attribute *bin_attr, char *to, loff_t pos, size_t count)
> +{
> +	struct memcons *mc = bin_attr->private;
> +	const char *conbuf;
> +	bool wrapped;
> +	size_t num_read;
> +	int out_pos;
> +
> +	if (!mc)
> +		return -ENODEV;
> +
> +	conbuf = phys_to_virt(be64_to_cpu(mc->obuf_phys));
> +	wrapped = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_WRAP;
> +	out_pos = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_MASK;
> +
> +	if (!wrapped) {
> +		num_read = memory_read_from_buffer(to, count, &pos, conbuf,
> +				out_pos);
> +	} else {
> +		num_read = memory_read_from_buffer(to, count, &pos,
> +				conbuf + out_pos,
> +				be32_to_cpu(mc->obuf_size) - out_pos);
> +
> +		if (num_read < 0)
> +			goto out;
> +
> +		num_read += memory_read_from_buffer(to + num_read,
> +				count - num_read, &pos, conbuf, out_pos);
> +	}
> +out:
> +	return num_read;
> +}
> +
> +static struct bin_attribute messages_attr = {
> +	.attr = {.name = "messages", .mode = 0444},
> +	.read = opal_messages_read
> +};
> +
> +void __init opal_messages_init(void)
> +{
> +	u64 mcaddr;
> +	struct memcons *mc;
> +
> +	if (of_property_read_u64(opal_node, "ibm,opal-memcons", &mcaddr)) {
> +		pr_warn("OPAL: Property ibm,opal-memcons not found, no message log\n");
> +		return;
> +	}
> +
> +	mc = phys_to_virt(mcaddr);
> +	if (!mc) {
> +		pr_warn("OPAL: memory console address is invalid\n");
> +		return;
> +	}
> +
> +	if (be64_to_cpu(mc->magic) != MEMCONS_MAGIC) {
> +		pr_warn("OPAL: memory console version is invalid\n");
> +		return;
> +	}
> +
> +	messages_attr.private = mc;
> +
> +	if (sysfs_create_bin_file(opal_kobj, &messages_attr) != 0)
> +		pr_warn("OPAL: sysfs file creation failed\n");
> +}
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index e92f2f6..2bc032a 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -46,7 +46,7 @@ struct mcheck_recoverable_range {
>  static struct mcheck_recoverable_range *mc_recoverable_range;
>  static int mc_recoverable_range_len;
>
> -static struct device_node *opal_node;
> +struct device_node *opal_node;
>  static DEFINE_SPINLOCK(opal_write_lock);
>  extern u64 opal_mc_secondary_handler[];
>  static unsigned int *opal_irqs;
> @@ -574,6 +574,8 @@ static int __init opal_init(void)
>  		opal_platform_dump_init();
>  		/* Setup system parameters interface */
>  		opal_sys_param_init();
> +		/* Setup message log interface. */
> +		opal_messages_init();
>  	}
>
>  	return 0;
> -- 
> 1.9.1

Re: [PATCH 1/2] powerpc/powernv: Add OPAL message log interface

[Michael Neuling]

Joel Stanley <joel@jms.id.au> wrote:

> OPAL provides an in-memory circular buffer containing a message log
> populated with various runtime messages produced by the firmware.
> 
> Provide a sysfs interface /sys/firmware/opal/messages for userspace to
> view the messages.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  arch/powerpc/include/asm/opal.h                |  4 ++
>  arch/powerpc/platforms/powernv/Makefile        |  1 +
>  arch/powerpc/platforms/powernv/opal-messages.c | 97 ++++++++++++++++++++++++++
>  arch/powerpc/platforms/powernv/opal.c          |  4 +-
>  4 files changed, 105 insertions(+), 1 deletion(-)
>  create mode 100644 arch/powerpc/platforms/powernv/opal-messages.c
> 
> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> index ffafab0..6aa757e 100644
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -729,6 +729,9 @@ typedef struct oppanel_line {
>  /* /sys/firmware/opal */
>  extern struct kobject *opal_kobj;
>  
> +/* /ibm,opal */
> +extern struct device_node *opal_node;
> +
>  /* API functions */
>  int64_t opal_console_write(int64_t term_number, __be64 *length,
>  			   const uint8_t *buffer);
> @@ -918,6 +921,7 @@ extern void opal_flash_init(void);
>  extern int opal_elog_init(void);
>  extern void opal_platform_dump_init(void);
>  extern void opal_sys_param_init(void);
> +extern void opal_messages_init(void);
>  
>  extern int opal_machine_check(struct pt_regs *regs);
>  extern bool opal_mce_check_early_recovery(struct pt_regs *regs);
> diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
> index f324ea0..e2ba418 100644
> --- a/arch/powerpc/platforms/powernv/Makefile
> +++ b/arch/powerpc/platforms/powernv/Makefile
> @@ -1,6 +1,7 @@
>  obj-y			+= setup.o opal-takeover.o opal-wrappers.o opal.o opal-async.o
>  obj-y			+= opal-rtc.o opal-nvram.o opal-lpc.o opal-flash.o
>  obj-y			+= rng.o opal-elog.o opal-dump.o opal-sysparam.o opal-sensor.o
> +obj-y			+= opal-messages.o
>  
>  obj-$(CONFIG_SMP)	+= smp.o
>  obj-$(CONFIG_PCI)	+= pci.o pci-p5ioc2.o pci-ioda.o
> diff --git a/arch/powerpc/platforms/powernv/opal-messages.c b/arch/powerpc/platforms/powernv/opal-messages.c
> new file mode 100644
> index 0000000..3a863e8
> --- /dev/null
> +++ b/arch/powerpc/platforms/powernv/opal-messages.c
> @@ -0,0 +1,97 @@
> +/*
> + * PowerNV OPAL in-memory console interface
> + *
> + * Copyright 2014 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <asm/io.h>
> +#include <asm/opal.h>
> +#include <linux/debugfs.h>
> +#include <linux/of.h>
> +#include <linux/types.h>
> +
> +/* OPAL in-memory console. Defined in OPAL source at core/console.c */
> +struct memcons {
> +	__be64 magic;
> +#define MEMCONS_MAGIC	0x6630696567726173L

0x6630696567726173 == f0iegras ... Ben!!! :-P

> +	__be64 obuf_phys;
> +	__be64 ibuf_phys;
> +	__be32 obuf_size;
> +	__be32 ibuf_size;
> +	__be32 out_pos;
> +#define MEMCONS_OUT_POS_WRAP	0x80000000u
> +#define MEMCONS_OUT_POS_MASK	0x00ffffffu
> +	__be32 in_prod;
> +	__be32 in_cons;
> +};
> +
> +static ssize_t opal_messages_read(struct file *file, struct kobject *kobj,
> +	struct bin_attribute *bin_attr, char *to, loff_t pos, size_t count)
> +{
> +	struct memcons *mc = bin_attr->private;
> +	const char *conbuf;
> +	bool wrapped;
> +	size_t num_read;
> +	int out_pos;
> +
> +	if (!mc)
> +		return -ENODEV;
> +
> +	conbuf = phys_to_virt(be64_to_cpu(mc->obuf_phys));
> +	wrapped = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_WRAP;
> +	out_pos = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_MASK;
> +

Are there ordering issues we need to think about here with reading
these?  Can the messages be written on another CPU at the same time as
these are being read?

What happens if in between reading wrapped and out_pos the buffer wraps?
You'd end up getting only a few bytes of console?  Maybe you need to
read wrapped before and after out_pos to make should it's not wrapped in
between.

> +	if (!wrapped) {

Why the negative case first?  Just make it:

   if (wrapped) {
      wrapped case
   } else {
      not wrapped case
   }

Also, no curlies needed for single statement.


> +		num_read = memory_read_from_buffer(to, count, &pos, conbuf,
> +				out_pos);

This is probably not necessary, but do we need to sanity check out_pos <
obuf_size?  I guess we don't generally sanity check numbers from OPAL as
it can screw us in many other ways anyway.

> +	} else {
> +		num_read = memory_read_from_buffer(to, count, &pos,
> +				conbuf + out_pos,
> +				be32_to_cpu(mc->obuf_size) - out_pos);
> +
> +		if (num_read < 0)
> +			goto out;
> +
> +		num_read += memory_read_from_buffer(to + num_read,
> +				count - num_read, &pos, conbuf,
>                out_pos);

What if this second read returns an error?  num_read += -ERRNO?  I think
you need to check this return independently.

Mikey

> +	}
> +out:
> +	return num_read;
> +}
> +
> +static struct bin_attribute messages_attr = {
> +	.attr = {.name = "messages", .mode = 0444},
> +	.read = opal_messages_read
> +};
> +
> +void __init opal_messages_init(void)
> +{
> +	u64 mcaddr;
> +	struct memcons *mc;
> +
> +	if (of_property_read_u64(opal_node, "ibm,opal-memcons", &mcaddr)) {
> +		pr_warn("OPAL: Property ibm,opal-memcons not found, no message log\n");
> +		return;
> +	}
> +
> +	mc = phys_to_virt(mcaddr);
> +	if (!mc) {
> +		pr_warn("OPAL: memory console address is invalid\n");
> +		return;
> +	}
> +
> +	if (be64_to_cpu(mc->magic) != MEMCONS_MAGIC) {
> +		pr_warn("OPAL: memory console version is invalid\n");
> +		return;
> +	}
> +
> +	messages_attr.private = mc;
> +
> +	if (sysfs_create_bin_file(opal_kobj, &messages_attr) != 0)
> +		pr_warn("OPAL: sysfs file creation failed\n");
> +}
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index e92f2f6..2bc032a 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -46,7 +46,7 @@ struct mcheck_recoverable_range {
>  static struct mcheck_recoverable_range *mc_recoverable_range;
>  static int mc_recoverable_range_len;
>  
> -static struct device_node *opal_node;
> +struct device_node *opal_node;
>  static DEFINE_SPINLOCK(opal_write_lock);
>  extern u64 opal_mc_secondary_handler[];
>  static unsigned int *opal_irqs;
> @@ -574,6 +574,8 @@ static int __init opal_init(void)
>  		opal_platform_dump_init();
>  		/* Setup system parameters interface */
>  		opal_sys_param_init();
> +		/* Setup message log interface. */
> +		opal_messages_init();
>  	}
>  
>  	return 0;
> -- 
> 1.9.1
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/2] powerpc/powernv: Add OPAL message log interface

[Michael Neuling]

Michael Neuling <mikey@neuling.org> wrote:

> Joel Stanley <joel@jms.id.au> wrote:
> 
> > OPAL provides an in-memory circular buffer containing a message log
> > populated with various runtime messages produced by the firmware.
> > 
> > Provide a sysfs interface /sys/firmware/opal/messages for userspace to
> > view the messages.
> > 
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > ---
> >  arch/powerpc/include/asm/opal.h                |  4 ++
> >  arch/powerpc/platforms/powernv/Makefile        |  1 +
> >  arch/powerpc/platforms/powernv/opal-messages.c | 97 ++++++++++++++++++++++++++
> >  arch/powerpc/platforms/powernv/opal.c          |  4 +-
> >  4 files changed, 105 insertions(+), 1 deletion(-)
> >  create mode 100644 arch/powerpc/platforms/powernv/opal-messages.c
> > 
> > diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> > index ffafab0..6aa757e 100644
> > --- a/arch/powerpc/include/asm/opal.h
> > +++ b/arch/powerpc/include/asm/opal.h
> > @@ -729,6 +729,9 @@ typedef struct oppanel_line {
> >  /* /sys/firmware/opal */
> >  extern struct kobject *opal_kobj;
> >  
> > +/* /ibm,opal */
> > +extern struct device_node *opal_node;
> > +
> >  /* API functions */
> >  int64_t opal_console_write(int64_t term_number, __be64 *length,
> >  			   const uint8_t *buffer);
> > @@ -918,6 +921,7 @@ extern void opal_flash_init(void);
> >  extern int opal_elog_init(void);
> >  extern void opal_platform_dump_init(void);
> >  extern void opal_sys_param_init(void);
> > +extern void opal_messages_init(void);
> >  
> >  extern int opal_machine_check(struct pt_regs *regs);
> >  extern bool opal_mce_check_early_recovery(struct pt_regs *regs);
> > diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
> > index f324ea0..e2ba418 100644
> > --- a/arch/powerpc/platforms/powernv/Makefile
> > +++ b/arch/powerpc/platforms/powernv/Makefile
> > @@ -1,6 +1,7 @@
> >  obj-y			+= setup.o opal-takeover.o opal-wrappers.o opal.o opal-async.o
> >  obj-y			+= opal-rtc.o opal-nvram.o opal-lpc.o opal-flash.o
> >  obj-y			+= rng.o opal-elog.o opal-dump.o opal-sysparam.o opal-sensor.o
> > +obj-y			+= opal-messages.o
> >  
> >  obj-$(CONFIG_SMP)	+= smp.o
> >  obj-$(CONFIG_PCI)	+= pci.o pci-p5ioc2.o pci-ioda.o
> > diff --git a/arch/powerpc/platforms/powernv/opal-messages.c b/arch/powerpc/platforms/powernv/opal-messages.c
> > new file mode 100644
> > index 0000000..3a863e8
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/powernv/opal-messages.c
> > @@ -0,0 +1,97 @@
> > +/*
> > + * PowerNV OPAL in-memory console interface
> > + *
> > + * Copyright 2014 IBM Corp.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#include <asm/io.h>
> > +#include <asm/opal.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/of.h>
> > +#include <linux/types.h>
> > +
> > +/* OPAL in-memory console. Defined in OPAL source at core/console.c */
> > +struct memcons {
> > +	__be64 magic;
> > +#define MEMCONS_MAGIC	0x6630696567726173L
> 
> 0x6630696567726173 == f0iegras ... Ben!!! :-P
> 
> > +	__be64 obuf_phys;
> > +	__be64 ibuf_phys;
> > +	__be32 obuf_size;
> > +	__be32 ibuf_size;
> > +	__be32 out_pos;
> > +#define MEMCONS_OUT_POS_WRAP	0x80000000u
> > +#define MEMCONS_OUT_POS_MASK	0x00ffffffu
> > +	__be32 in_prod;
> > +	__be32 in_cons;
> > +};
> > +
> > +static ssize_t opal_messages_read(struct file *file, struct kobject *kobj,
> > +	struct bin_attribute *bin_attr, char *to, loff_t pos, size_t count)
> > +{
> > +	struct memcons *mc = bin_attr->private;
> > +	const char *conbuf;
> > +	bool wrapped;
> > +	size_t num_read;
> > +	int out_pos;
> > +
> > +	if (!mc)
> > +		return -ENODEV;
> > +
> > +	conbuf = phys_to_virt(be64_to_cpu(mc->obuf_phys));
> > +	wrapped = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_WRAP;
> > +	out_pos = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_MASK;
> > +
> 
> Are there ordering issues we need to think about here with reading
> these?  Can the messages be written on another CPU at the same time as
> these are being read?
> 
> What happens if in between reading wrapped and out_pos the buffer wraps?
> You'd end up getting only a few bytes of console?  Maybe you need to
> read wrapped before and after out_pos to make should it's not wrapped in
> between.

wrapped = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_WRAP;
out_pos = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_MASK;

OK, I just realised this is reading from the same location.  So yeah,
don't do that.  Read it once and calculate wrapped and out_pos from that
single read.

> 
> > +	if (!wrapped) {
> 
> Why the negative case first?  Just make it:
> 
>    if (wrapped) {
>       wrapped case
>    } else {
>       not wrapped case
>    }
> 
> Also, no curlies needed for single statement.
> 
> 
> > +		num_read = memory_read_from_buffer(to, count, &pos, conbuf,
> > +				out_pos);
> 
> This is probably not necessary, but do we need to sanity check out_pos <
> obuf_size?  I guess we don't generally sanity check numbers from OPAL as
> it can screw us in many other ways anyway.
> 
> > +	} else {
> > +		num_read = memory_read_from_buffer(to, count, &pos,
> > +				conbuf + out_pos,
> > +				be32_to_cpu(mc->obuf_size) - out_pos);
> > +
> > +		if (num_read < 0)
> > +			goto out;
> > +
> > +		num_read += memory_read_from_buffer(to + num_read,
> > +				count - num_read, &pos, conbuf,
> >                out_pos);
> 
> What if this second read returns an error?  num_read += -ERRNO?  I think
> you need to check this return independently.
> 
> Mikey
> 
> > +	}
> > +out:
> > +	return num_read;
> > +}
> > +
> > +static struct bin_attribute messages_attr = {
> > +	.attr = {.name = "messages", .mode = 0444},
> > +	.read = opal_messages_read
> > +};
> > +
> > +void __init opal_messages_init(void)
> > +{
> > +	u64 mcaddr;
> > +	struct memcons *mc;
> > +
> > +	if (of_property_read_u64(opal_node, "ibm,opal-memcons", &mcaddr)) {
> > +		pr_warn("OPAL: Property ibm,opal-memcons not found, no message log\n");
> > +		return;
> > +	}
> > +
> > +	mc = phys_to_virt(mcaddr);
> > +	if (!mc) {
> > +		pr_warn("OPAL: memory console address is invalid\n");
> > +		return;
> > +	}
> > +
> > +	if (be64_to_cpu(mc->magic) != MEMCONS_MAGIC) {
> > +		pr_warn("OPAL: memory console version is invalid\n");
> > +		return;
> > +	}
> > +
> > +	messages_attr.private = mc;
> > +
> > +	if (sysfs_create_bin_file(opal_kobj, &messages_attr) != 0)
> > +		pr_warn("OPAL: sysfs file creation failed\n");
> > +}
> > diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> > index e92f2f6..2bc032a 100644
> > --- a/arch/powerpc/platforms/powernv/opal.c
> > +++ b/arch/powerpc/platforms/powernv/opal.c
> > @@ -46,7 +46,7 @@ struct mcheck_recoverable_range {
> >  static struct mcheck_recoverable_range *mc_recoverable_range;
> >  static int mc_recoverable_range_len;
> >  
> > -static struct device_node *opal_node;
> > +struct device_node *opal_node;
> >  static DEFINE_SPINLOCK(opal_write_lock);
> >  extern u64 opal_mc_secondary_handler[];
> >  static unsigned int *opal_irqs;
> > @@ -574,6 +574,8 @@ static int __init opal_init(void)
> >  		opal_platform_dump_init();
> >  		/* Setup system parameters interface */
> >  		opal_sys_param_init();
> > +		/* Setup message log interface. */
> > +		opal_messages_init();
> >  	}
> >  
> >  	return 0;
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/2] powerpc/powernv: Add OPAL message log interface

[Benjamin Herrenschmidt]

On Mon, 2014-03-31 at 15:08 +1100, Michael Neuling wrote:

> > +/* OPAL in-memory console. Defined in OPAL source at core/console.c */
> > +struct memcons {
> > +	__be64 magic;
> > +#define MEMCONS_MAGIC	0x6630696567726173L
> 
> 0x6630696567726173 == f0iegras ... Ben!!! :-P

Yummy ! :-)

> > +	__be64 obuf_phys;
> > +	__be64 ibuf_phys;
> > +	__be32 obuf_size;
> > +	__be32 ibuf_size;
> > +	__be32 out_pos;
> > +#define MEMCONS_OUT_POS_WRAP	0x80000000u
> > +#define MEMCONS_OUT_POS_MASK	0x00ffffffu
> > +	__be32 in_prod;
> > +	__be32 in_cons;
> > +};
> > +
> > +static ssize_t opal_messages_read(struct file *file, struct kobject *kobj,
> > +	struct bin_attribute *bin_attr, char *to, loff_t pos, size_t count)
> > +{
> > +	struct memcons *mc = bin_attr->private;
> > +	const char *conbuf;
> > +	bool wrapped;
> > +	size_t num_read;
> > +	int out_pos;
> > +
> > +	if (!mc)
> > +		return -ENODEV;
> > +
> > +	conbuf = phys_to_virt(be64_to_cpu(mc->obuf_phys));
> > +	wrapped = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_WRAP;
> > +	out_pos = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_MASK;
> > +
> 
> Are there ordering issues we need to think about here with reading
> these?  Can the messages be written on another CPU at the same time as
> these are being read?

Good point. out_pos should probably be read only once into a local
variable using the ACCESS_ONCE macro, and then only be broken up.

> What happens if in between reading wrapped and out_pos the buffer wraps?
> You'd end up getting only a few bytes of console?  Maybe you need to
> read wrapped before and after out_pos to make should it's not wrapped in
> between.

Unlikely but yes, it can happen.

> > +	if (!wrapped) {
> 
> Why the negative case first?  Just make it:
> 
>    if (wrapped) {
>       wrapped case
>    } else {
>       not wrapped case
>    }
> 
> Also, no curlies needed for single statement.
> 
> 
> > +		num_read = memory_read_from_buffer(to, count, &pos, conbuf,
> > +				out_pos);
> 
> This is probably not necessary, but do we need to sanity check out_pos <
> obuf_size?  I guess we don't generally sanity check numbers from OPAL as
> it can screw us in many other ways anyway.

On the other hand it doesn't cost much and if the FW goes bonkers it
will give us a better handle to debug from.

> > +	} else {
> > +		num_read = memory_read_from_buffer(to, count, &pos,
> > +				conbuf + out_pos,
> > +				be32_to_cpu(mc->obuf_size) - out_pos);
> > +
> > +		if (num_read < 0)
> > +			goto out;
> > +
> > +		num_read += memory_read_from_buffer(to + num_read,
> > +				count - num_read, &pos, conbuf,
> >                out_pos);
> 
> What if this second read returns an error?  num_read += -ERRNO?  I think
> you need to check this return independently.

Cheers,
Ben.

> Mikey
> 
> > +	}
> > +out:
> > +	return num_read;
> > +}
> > +
> > +static struct bin_attribute messages_attr = {
> > +	.attr = {.name = "messages", .mode = 0444},
> > +	.read = opal_messages_read
> > +};
> > +
> > +void __init opal_messages_init(void)
> > +{
> > +	u64 mcaddr;
> > +	struct memcons *mc;
> > +
> > +	if (of_property_read_u64(opal_node, "ibm,opal-memcons", &mcaddr)) {
> > +		pr_warn("OPAL: Property ibm,opal-memcons not found, no message log\n");
> > +		return;
> > +	}
> > +
> > +	mc = phys_to_virt(mcaddr);
> > +	if (!mc) {
> > +		pr_warn("OPAL: memory console address is invalid\n");
> > +		return;
> > +	}
> > +
> > +	if (be64_to_cpu(mc->magic) != MEMCONS_MAGIC) {
> > +		pr_warn("OPAL: memory console version is invalid\n");
> > +		return;
> > +	}
> > +
> > +	messages_attr.private = mc;
> > +
> > +	if (sysfs_create_bin_file(opal_kobj, &messages_attr) != 0)
> > +		pr_warn("OPAL: sysfs file creation failed\n");
> > +}
> > diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> > index e92f2f6..2bc032a 100644
> > --- a/arch/powerpc/platforms/powernv/opal.c
> > +++ b/arch/powerpc/platforms/powernv/opal.c
> > @@ -46,7 +46,7 @@ struct mcheck_recoverable_range {
> >  static struct mcheck_recoverable_range *mc_recoverable_range;
> >  static int mc_recoverable_range_len;
> >  
> > -static struct device_node *opal_node;
> > +struct device_node *opal_node;
> >  static DEFINE_SPINLOCK(opal_write_lock);
> >  extern u64 opal_mc_secondary_handler[];
> >  static unsigned int *opal_irqs;
> > @@ -574,6 +574,8 @@ static int __init opal_init(void)
> >  		opal_platform_dump_init();
> >  		/* Setup system parameters interface */
> >  		opal_sys_param_init();
> > +		/* Setup message log interface. */
> > +		opal_messages_init();
> >  	}
> >  
> >  	return 0;
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/2] powerpc/powernv: Add OPAL message log interface

[Joel Stanley]

On Mon, Mar 31, 2014 at 10:29 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>> > +   conbuf = phys_to_virt(be64_to_cpu(mc->obuf_phys));
>> > +   wrapped = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_WRAP;
>> > +   out_pos = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_MASK;
>> > +
>>
>> Are there ordering issues we need to think about here with reading
>> these?  Can the messages be written on another CPU at the same time as
>> these are being read?
>
> Good point. out_pos should probably be read only once into a local
> variable using the ACCESS_ONCE macro, and then only be broken up.

I've got a V2 that fixes this and the other issues Mikey pointed out.

I found that the log would get corrupted from Linux's point of view
once full. Dumping the memory suggests that the contents of the
circular buffer is fine, and it's just our pointer (out_pos) that is
incorrect. It's not clear why this is happening; I'll do some more
testing before sending out the updated patch.

Cheers,

Joel