[PATCH v3] powerpc/powernv: add hdat attribute to sysfs

[PATCH v3] powerpc/powernv: add hdat attribute to sysfs

[Matt Brown]

The HDAT data area is consumed by skiboot and turned into a device-tree.
In some cases we would like to look directly at the HDAT, so this patch
adds a sysfs node to allow it to be viewed.  This is not possible through
/dev/mem as it is reserved memory which is stopped by the /dev/mem filter.

Signed-off-by: Matt Brown <matthew.brown.dev@gmail.com>
---

Changes between v2 to v3:
	- fixed header comments
	- simplified if statement

---
 arch/powerpc/include/asm/opal.h            |  1 +
 arch/powerpc/platforms/powernv/Makefile    |  1 +
 arch/powerpc/platforms/powernv/opal-hdat.c | 65 ++++++++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/opal.c      |  2 +
 4 files changed, 69 insertions(+)
 create mode 100644 arch/powerpc/platforms/powernv/opal-hdat.c

diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 5c7db0f..b26944e 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -277,6 +277,7 @@ extern int opal_async_comp_init(void);
 extern int opal_sensor_init(void);
 extern int opal_hmi_handler_init(void);
 extern int opal_event_init(void);
+extern void opal_hdat_sysfs_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 b5d98cb..9a0c9d6 100644
--- a/arch/powerpc/platforms/powernv/Makefile
+++ b/arch/powerpc/platforms/powernv/Makefile
@@ -3,6 +3,7 @@ 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-msglog.o opal-hmi.o opal-power.o opal-irqchip.o
 obj-y			+= opal-kmsg.o
+obj-y			+= opal-hdat.o
 
 obj-$(CONFIG_SMP)	+= smp.o subcore.o subcore-asm.o
 obj-$(CONFIG_PCI)	+= pci.o pci-ioda.o npu-dma.o
diff --git a/arch/powerpc/platforms/powernv/opal-hdat.c b/arch/powerpc/platforms/powernv/opal-hdat.c
new file mode 100644
index 0000000..3315dd3
--- /dev/null
+++ b/arch/powerpc/platforms/powernv/opal-hdat.c
@@ -0,0 +1,65 @@
+/*
+ * PowerNV OPAL HDAT interface
+ *
+ * Author: Matt Brown <matthew.brown.dev@gmail.com>
+ *
+ * Copyright 2017 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/of.h>
+#include <linux/types.h>
+
+struct hdat_info {
+	char *base;
+	u64 size;
+};
+
+static struct hdat_info hdat_inf;
+
+/* Read function for HDAT attribute in sysfs */
+static ssize_t hdat_read(struct file *file, struct kobject *kobj,
+			 struct bin_attribute *bin_attr, char *to,
+			 loff_t pos, size_t count)
+{
+	if (!hdat_inf.base)
+		return -ENODEV;
+
+	return memory_read_from_buffer(to, count, &pos, hdat_inf.base,
+					hdat_inf.size);
+}
+
+
+/* HDAT attribute for sysfs */
+static struct bin_attribute hdat_attr = {
+	.attr = {.name = "hdat", .mode = 0444},
+	.read = hdat_read
+};
+
+void __init opal_hdat_sysfs_init(void)
+{
+	u64 hdat_addr[2];
+
+	/* Check for the hdat-map prop in device-tree */
+	if (of_property_read_u64_array(opal_node, "hdat-map", hdat_addr, 2)) {
+		pr_debug("OPAL: Property hdat-map not found.\n");
+		return;
+	}
+
+	/* Print out hdat-map values. [0]: base, [1]: size */
+	pr_debug("OPAL: HDAT Base address: %#llx\n", hdat_addr[0]);
+	pr_debug("OPAL: HDAT Size: %#llx\n", hdat_addr[1]);
+
+	hdat_inf.base = phys_to_virt(hdat_addr[0]);
+	hdat_inf.size = hdat_addr[1];
+
+	if (sysfs_create_bin_file(opal_kobj, &hdat_attr))
+		pr_debug("OPAL: sysfs file creation for HDAT failed");
+
+}
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 2822935..cae3745 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -740,6 +740,8 @@ static int __init opal_init(void)
 		opal_sys_param_init();
 		/* Setup message log sysfs interface. */
 		opal_msglog_sysfs_init();
+		/* Create hdat object under sys/firmware/opal */
+		opal_hdat_sysfs_init();
 	}
 
 	/* Initialize platform devices: IPMI backend, PRD & flash interface */
-- 
2.9.3

Re: [PATCH v3] powerpc/powernv: add hdat attribute to sysfs

[Andrew Donnellan]

On 24/02/17 17:20, Matt Brown wrote:
> The HDAT data area is consumed by skiboot and turned into a device-tree.
> In some cases we would like to look directly at the HDAT, so this patch
> adds a sysfs node to allow it to be viewed.  This is not possible through
> /dev/mem as it is reserved memory which is stopped by the /dev/mem filter.
>
> Signed-off-by: Matt Brown <matthew.brown.dev@gmail.com>

Changes look good, thanks for addressing the comments! Still a couple of 
minor points below, otherwise:

Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

Stewart: this might need your ACK?

> ---
>
> Changes between v2 to v3:
> 	- fixed header comments
> 	- simplified if statement
>
> ---
>  arch/powerpc/include/asm/opal.h            |  1 +
>  arch/powerpc/platforms/powernv/Makefile    |  1 +
>  arch/powerpc/platforms/powernv/opal-hdat.c | 65 ++++++++++++++++++++++++++++++
>  arch/powerpc/platforms/powernv/opal.c      |  2 +
>  4 files changed, 69 insertions(+)
>  create mode 100644 arch/powerpc/platforms/powernv/opal-hdat.c
>
> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> index 5c7db0f..b26944e 100644
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -277,6 +277,7 @@ extern int opal_async_comp_init(void);
>  extern int opal_sensor_init(void);
>  extern int opal_hmi_handler_init(void);
>  extern int opal_event_init(void);
> +extern void opal_hdat_sysfs_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 b5d98cb..9a0c9d6 100644
> --- a/arch/powerpc/platforms/powernv/Makefile
> +++ b/arch/powerpc/platforms/powernv/Makefile
> @@ -3,6 +3,7 @@ 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-msglog.o opal-hmi.o opal-power.o opal-irqchip.o
>  obj-y			+= opal-kmsg.o
> +obj-y			+= opal-hdat.o

Normally we keep putting new object files on the same line until it gets 
long enough that we have to break it. This is very minor though :)

>
>  obj-$(CONFIG_SMP)	+= smp.o subcore.o subcore-asm.o
>  obj-$(CONFIG_PCI)	+= pci.o pci-ioda.o npu-dma.o
> diff --git a/arch/powerpc/platforms/powernv/opal-hdat.c b/arch/powerpc/platforms/powernv/opal-hdat.c
> new file mode 100644
> index 0000000..3315dd3
> --- /dev/null
> +++ b/arch/powerpc/platforms/powernv/opal-hdat.c
> @@ -0,0 +1,65 @@
> +/*
> + * PowerNV OPAL HDAT interface
> + *
> + * Author: Matt Brown <matthew.brown.dev@gmail.com>
> + *
> + * Copyright 2017 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/of.h>
> +#include <linux/types.h>
> +
> +struct hdat_info {
> +	char *base;
> +	u64 size;
> +};
> +
> +static struct hdat_info hdat_inf;

As Oliver pointed out, we could do with a better name than hdat_inf - 
it's only one character away from the name of the struct type. Hmm, 
perhaps "hdat_location", or maybe Oliver has a better suggestion.


-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCH v3] powerpc/powernv: add hdat attribute to sysfs

[Oliver O'Halloran]

On Mon, Feb 27, 2017 at 12:59 PM, Andrew Donnellan
<andrew.donnellan@au1.ibm.com> wrote:
> On 24/02/17 17:20, Matt Brown wrote:
>>
>> The HDAT data area is consumed by skiboot and turned into a device-tree.
>> In some cases we would like to look directly at the HDAT, so this patch
>> adds a sysfs node to allow it to be viewed.  This is not possible through
>> /dev/mem as it is reserved memory which is stopped by the /dev/mem filter.
>>
>> Signed-off-by: Matt Brown <matthew.brown.dev@gmail.com>
>
>
> Changes look good, thanks for addressing the comments! Still a couple of
> minor points below, otherwise:
>
> Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
>
> Stewart: this might need your ACK?
>
>
>> ---
>>
>> Changes between v2 to v3:
>>         - fixed header comments
>>         - simplified if statement
>>
>> ---
>>  arch/powerpc/include/asm/opal.h            |  1 +
>>  arch/powerpc/platforms/powernv/Makefile    |  1 +
>>  arch/powerpc/platforms/powernv/opal-hdat.c | 65
>> ++++++++++++++++++++++++++++++
>>  arch/powerpc/platforms/powernv/opal.c      |  2 +
>>  4 files changed, 69 insertions(+)
>>  create mode 100644 arch/powerpc/platforms/powernv/opal-hdat.c
>>
>> diff --git a/arch/powerpc/include/asm/opal.h
>> b/arch/powerpc/include/asm/opal.h
>> index 5c7db0f..b26944e 100644
>> --- a/arch/powerpc/include/asm/opal.h
>> +++ b/arch/powerpc/include/asm/opal.h
>> @@ -277,6 +277,7 @@ extern int opal_async_comp_init(void);
>>  extern int opal_sensor_init(void);
>>  extern int opal_hmi_handler_init(void);
>>  extern int opal_event_init(void);
>> +extern void opal_hdat_sysfs_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 b5d98cb..9a0c9d6 100644
>> --- a/arch/powerpc/platforms/powernv/Makefile
>> +++ b/arch/powerpc/platforms/powernv/Makefile
>> @@ -3,6 +3,7 @@ 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-msglog.o opal-hmi.o opal-power.o
>> opal-irqchip.o
>>  obj-y                  += opal-kmsg.o
>> +obj-y                  += opal-hdat.o
>
>
> Normally we keep putting new object files on the same line until it gets
> long enough that we have to break it. This is very minor though :)
>
>
>>
>>  obj-$(CONFIG_SMP)      += smp.o subcore.o subcore-asm.o
>>  obj-$(CONFIG_PCI)      += pci.o pci-ioda.o npu-dma.o
>> diff --git a/arch/powerpc/platforms/powernv/opal-hdat.c
>> b/arch/powerpc/platforms/powernv/opal-hdat.c
>> new file mode 100644
>> index 0000000..3315dd3
>> --- /dev/null
>> +++ b/arch/powerpc/platforms/powernv/opal-hdat.c
>> @@ -0,0 +1,65 @@
>> +/*
>> + * PowerNV OPAL HDAT interface
>> + *
>> + * Author: Matt Brown <matthew.brown.dev@gmail.com>
>> + *
>> + * Copyright 2017 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/of.h>
>> +#include <linux/types.h>
>> +
>> +struct hdat_info {
>> +       char *base;
>> +       u64 size;
>> +};
>> +
>> +static struct hdat_info hdat_inf;
>
>
> As Oliver pointed out, we could do with a better name than hdat_inf - it's
> only one character away from the name of the struct type. Hmm, perhaps
> "hdat_location", or maybe Oliver has a better suggestion.

I'm not that bothered by it.

Reviewed-by: Oliver O'Halloran <oohall@gmail.com>

>
>
> --
> Andrew Donnellan              OzLabs, ADL Canberra
> andrew.donnellan@au1.ibm.com  IBM Australia Limited
>

Re: [PATCH v3] powerpc/powernv: add hdat attribute to sysfs

[Michael Ellerman]

Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:

> On 24/02/17 17:20, Matt Brown wrote:
>> The HDAT data area is consumed by skiboot and turned into a device-tree.
>> In some cases we would like to look directly at the HDAT, so this patch
>> adds a sysfs node to allow it to be viewed.  This is not possible through
>> /dev/mem as it is reserved memory which is stopped by the /dev/mem filter.
>>
>> Signed-off-by: Matt Brown <matthew.brown.dev@gmail.com>
>
> Changes look good, thanks for addressing the comments! Still a couple of 
> minor points below, otherwise:
>
> Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
>
> Stewart: this might need your ACK?

I don't see it in skiboot yet ?

>> diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
>> index b5d98cb..9a0c9d6 100644
>> --- a/arch/powerpc/platforms/powernv/Makefile
>> +++ b/arch/powerpc/platforms/powernv/Makefile
>> @@ -3,6 +3,7 @@ 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-msglog.o opal-hmi.o opal-power.o opal-irqchip.o
>>  obj-y			+= opal-kmsg.o
>> +obj-y			+= opal-hdat.o
>
> Normally we keep putting new object files on the same line until it gets 
> long enough that we have to break it. This is very minor though :)

Yeah, please put it on the same line as opal-kmsg.o

>> diff --git a/arch/powerpc/platforms/powernv/opal-hdat.c b/arch/powerpc/platforms/powernv/opal-hdat.c
>> new file mode 100644
>> index 0000000..3315dd3
>> --- /dev/null
>> +++ b/arch/powerpc/platforms/powernv/opal-hdat.c
>> @@ -0,0 +1,65 @@
>> +/*
>> + * PowerNV OPAL HDAT interface
>> + *
>> + * Author: Matt Brown <matthew.brown.dev@gmail.com>
>> + *
>> + * Copyright 2017 IBM Corp.

I dislike email addresses in source files, they just end up being wrong
and needing to be updated. We have your email in the commit log anyway.

So I prefer:

 * Copyright 2017, Matt Brown, IBM Corp.

Which I believe is also blessed by the lawyers.

>> + * 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/of.h>
>> +#include <linux/types.h>

Preferred style is to put the linux includes first, before the asm ones.

>> +struct hdat_info {
>> +	char *base;
>> +	u64 size;
>> +};
>> +
>> +static struct hdat_info hdat_inf;
>
> As Oliver pointed out, we could do with a better name than hdat_inf - 
> it's only one character away from the name of the struct type. Hmm, 
> perhaps "hdat_location", or maybe Oliver has a better suggestion.

Why not hdat_info ?

In fact for bonus points, you can just do:

static struct {
	char *base;
	u64 size;
} hdat_info;

cheers

Re: [PATCH v3] powerpc/powernv: add hdat attribute to sysfs

[Michael Ellerman]

Matt Brown <matthew.brown.dev@gmail.com> writes:
> diff --git a/arch/powerpc/platforms/powernv/opal-hdat.c b/arch/powerpc/platforms/powernv/opal-hdat.c
> new file mode 100644
> index 0000000..3315dd3
> --- /dev/null
> +++ b/arch/powerpc/platforms/powernv/opal-hdat.c
> @@ -0,0 +1,65 @@
...
> +
> +
> +/* HDAT attribute for sysfs */
> +static struct bin_attribute hdat_attr = {
> +	.attr = {.name = "hdat", .mode = 0444},
                                         ^^^^
ajd and oohal report to my office.

cheers

Re: [PATCH v3] powerpc/powernv: add hdat attribute to sysfs

[Oliver O'Halloran]

On Mon, Feb 27, 2017 at 9:56 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Matt Brown <matthew.brown.dev@gmail.com> writes:
>> diff --git a/arch/powerpc/platforms/powernv/opal-hdat.c b/arch/powerpc/platforms/powernv/opal-hdat.c
>> new file mode 100644
>> index 0000000..3315dd3
>> --- /dev/null
>> +++ b/arch/powerpc/platforms/powernv/opal-hdat.c
>> @@ -0,0 +1,65 @@
> ...
>> +
>> +
>> +/* HDAT attribute for sysfs */
>> +static struct bin_attribute hdat_attr = {
>> +     .attr = {.name = "hdat", .mode = 0444},
>                                          ^^^^
> ajd and oohal report to my office.

I don't think there's anything in the HDAT that's sensitive. That
said, this might not be true in the future so making it only readable
by root might be a good idea.

Oliver

Re: [PATCH v3] powerpc/powernv: add hdat attribute to sysfs

[Michael Ellerman]

Oliver O'Halloran <oohall@gmail.com> writes:

> On Mon, Feb 27, 2017 at 9:56 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Matt Brown <matthew.brown.dev@gmail.com> writes:
>>> diff --git a/arch/powerpc/platforms/powernv/opal-hdat.c b/arch/powerpc/platforms/powernv/opal-hdat.c
>>> new file mode 100644
>>> index 0000000..3315dd3
>>> --- /dev/null
>>> +++ b/arch/powerpc/platforms/powernv/opal-hdat.c
>>> @@ -0,0 +1,65 @@
>> ...
>>> +
>>> +
>>> +/* HDAT attribute for sysfs */
>>> +static struct bin_attribute hdat_attr = {
>>> +     .attr = {.name = "hdat", .mode = 0444},
>>                                          ^^^^
>> ajd and oohal report to my office.
>
> I don't think there's anything in the HDAT that's sensitive. That
> said, this might not be true in the future so making it only readable
> by root might be a good idea.

Right. What's sensitive can also change over time, in addition to the
actual content changing without our knowledge.

This is also firmly a debugging thing, so unless there's a compelling
reason why it should be world readable then it shouldn't be.

So 0400 please.

cheers

Re: [PATCH v3] powerpc/powernv: add hdat attribute to sysfs

[Andrew Donnellan]

On 27/02/17 21:56, Michael Ellerman wrote:
>> +/* HDAT attribute for sysfs */
>> +static struct bin_attribute hdat_attr = {
>> +	.attr = {.name = "hdat", .mode = 0444},
>                                          ^^^^
> ajd and oohal report to my office.

...ACK... :/

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited