From: Matt Brown <hidden> Date: 2017-02-24 06:21:15
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 <redacted>
---
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
@@ -0,0 +1,65 @@+/*+*PowerNVOPALHDATinterface+*+*Author:MattBrown<matthew.brown.dev@gmail.com>+*+*Copyright2017IBMCorp.+*+*Thisprogramisfreesoftware;youcanredistributeitand/or+*modifyitunderthetermsoftheGNUGeneralPublicLicense+*aspublishedbytheFreeSoftwareFoundation;eitherversion+*2oftheLicense,or(atyouroption)anylaterversion.+*/++#include<asm/io.h>+#include<asm/opal.h>+#include<linux/of.h>+#include<linux/types.h>++structhdat_info{+char*base;+u64size;+};++staticstructhdat_infohdat_inf;++/* Read function for HDAT attribute in sysfs */+staticssize_thdat_read(structfile*file,structkobject*kobj,+structbin_attribute*bin_attr,char*to,+loff_tpos,size_tcount)+{+if(!hdat_inf.base)+return-ENODEV;++returnmemory_read_from_buffer(to,count,&pos,hdat_inf.base,+hdat_inf.size);+}+++/* HDAT attribute for sysfs */+staticstructbin_attributehdat_attr={+.attr={.name="hdat",.mode=0444},+.read=hdat_read+};++void__initopal_hdat_sysfs_init(void)+{+u64hdat_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");++}
From: Andrew Donnellan <hidden> Date: 2017-02-27 02:00:16
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 <redacted>
Changes look good, thanks for addressing the comments! Still a couple of
minor points below, otherwise:
Reviewed-by: Andrew Donnellan <redacted>
Stewart: this might need your ACK?
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
On Mon, Feb 27, 2017 at 12:59 PM, Andrew Donnellan
[off-list ref] wrote:
On 24/02/17 17:20, Matt Brown wrote:
quoted
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 <redacted>
Changes look good, thanks for addressing the comments! Still a couple of
minor points below, otherwise:
Reviewed-by: Andrew Donnellan <redacted>
Stewart: this might need your ACK?
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
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-02-27 10:52:56
Andrew Donnellan [off-list ref] writes:
On 24/02/17 17:20, Matt Brown wrote:
quoted
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 <redacted>
Changes look good, thanks for addressing the comments! Still a couple of
minor points below, otherwise:
Reviewed-by: Andrew Donnellan <redacted>
Stewart: this might need your ACK?
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.
quoted
+ * 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.
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
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
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