* Re: [PATCH v3 22/27] powerpc/powernv/pmem: Implement the heartbeat command
From: Andrew Donnellan @ 2020-02-28 6:20 UTC (permalink / raw)
To: Alastair D'Silva, alastair
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Oliver O'Halloran, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Krzysztof Kozlowski, Anju T Sudhakar,
Mahesh Salgaonkar, Arnd Bergmann, Greg Kurz, Nicholas Piggin,
Cédric Le Goater, Dan Williams, Hari Bathini, linux-mm,
Greg Kroah-Hartman, linux-kernel, Vishal Verma, Frederic Barrat,
Paul Mackerras, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20200221032720.33893-23-alastair@au1.ibm.com>
On 21/2/20 2:27 pm, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> The heartbeat admin command is a simple admin command that exercises
> the communication mechanisms within the controller.
>
> This patch issues a heartbeat command to the card during init to ensure
> we can communicate with the card's controller.
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
Looks okay.
Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply
* Re: [PATCH v3 21/27] powerpc/powernv/pmem: Add an IOCTL to request controller health & perf data
From: Andrew Donnellan @ 2020-02-28 6:12 UTC (permalink / raw)
To: Alastair D'Silva, alastair
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Oliver O'Halloran, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Krzysztof Kozlowski, Anju T Sudhakar,
Mahesh Salgaonkar, Arnd Bergmann, Greg Kurz, Nicholas Piggin,
Cédric Le Goater, Dan Williams, Hari Bathini, linux-mm,
Greg Kroah-Hartman, linux-kernel, Vishal Verma, Frederic Barrat,
Paul Mackerras, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20200221032720.33893-22-alastair@au1.ibm.com>
On 21/2/20 2:27 pm, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> When health & performance data is requested from the controller,
> it responds with an error log containing the requested information.
>
> This patch allows the request to me issued via an IOCTL.
A better explanation would be good - this IOCTL triggers a request to
the controller to collect controller health/perf data, and the
controller will later respond with an error log that can be picked up
via the error log IOCTL that you've defined earlier.
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply
* [PATCH 1/1] powerpc/kernel: Enables memory hot-remove after reboot on pseries guests
From: Leonardo Bras @ 2020-02-28 6:04 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Greg Kroah-Hartman, Hari Bathini, Leonardo Bras, Christophe Leroy,
Thomas Gleixner, Claudio Carvalho, mdroth
Cc: linuxppc-dev, linux-kernel
While providing guests, it's desirable to resize it's memory on demand.
By now, it's possible to do so by creating a guest with a small base
memory, hot-plugging all the rest, and using 'movable_node' kernel
command-line parameter, which puts all hot-plugged memory in
ZONE_MOVABLE, allowing it to be removed whenever needed.
But there is an issue regarding guest reboot:
If memory is hot-plugged, and then the guest is rebooted, all hot-plugged
memory goes to ZONE_NORMAL, which offers no guaranteed hot-removal.
It usually prevents this memory to be hot-removed from the guest.
It's possible to use device-tree information to fix that behavior, as
it stores flags for LMB ranges on ibm,dynamic-memory-vN.
It involves marking each memblock with the correct flags as hotpluggable
memory, which mm/memblock.c puts in ZONE_MOVABLE during boot if
'movable_node' is passed.
For base memory, qemu assigns these flags for it's LMBs:
(DRCONF_MEM_AI_INVALID | DRCONF_MEM_RESERVED)
For hot-plugged memory, it assigns (DRCONF_MEM_ASSIGNED).
While guest kernel reads the device-tree, early_init_drmem_lmb() is
called for every added LMBs, doing nothing for base memory, and adding
memblocks for hot-plugged memory. Skipping base memory happens here:
if ((lmb->flags & DRCONF_MEM_RESERVED) ||
!(lmb->flags & DRCONF_MEM_ASSIGNED))
return;
Marking memblocks added by this function as hotplugable memory
is enough to get the desirable behavior, and should cause no change
if 'movable_node' parameter is not passed to kernel.
Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com>
---
arch/powerpc/kernel/prom.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 6620f37abe73..f4d14c67bf53 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -518,6 +518,8 @@ static void __init early_init_drmem_lmb(struct drmem_lmb *lmb,
DBG("Adding: %llx -> %llx\n", base, size);
if (validate_mem_limit(base, &size))
memblock_add(base, size);
+
+ early_init_dt_mark_hotplug_memory_arch(base, size);
} while (--rngs);
}
#endif /* CONFIG_PPC_PSERIES */
--
2.24.1
^ permalink raw reply related
* Re: [PATCH 1/3] powerpc/of: split out new_property() for reusing
From: Andrew Donnellan @ 2020-02-28 6:03 UTC (permalink / raw)
To: Pingfan Liu, linuxppc-dev
Cc: Aneesh Kumar K . V, kexec, Paul Mackerras, Oliver O'Halloran,
Dan Williams, Hari Bathini
In-Reply-To: <1582869192-9284-1-git-send-email-kernelfans@gmail.com>
On 28/2/20 4:53 pm, Pingfan Liu wrote:
> Since new_property() is used in several calling sites, splitting it out for
> reusing.
>
> To ease the review, although the split out part has coding style issue,
> keeping it untouched and fixed in next patch.
>
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> To: linuxppc-dev@lists.ozlabs.org
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Hari Bathini <hbathini@linux.ibm.com>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Cc: Oliver O'Halloran <oohall@gmail.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: kexec@lists.infradead.org
Which tree does this apply to? I don't see a new_property() in mm/drmem.c...
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply
* [PATCH 3/3] pseries/scm: buffer pmem's bound addr in dt for kexec kernel
From: Pingfan Liu @ 2020-02-28 5:53 UTC (permalink / raw)
To: linuxppc-dev
Cc: kexec, Pingfan Liu, Paul Mackerras, Aneesh Kumar K . V,
Oliver O'Halloran, Dan Williams, Hari Bathini
In-Reply-To: <1582869192-9284-1-git-send-email-kernelfans@gmail.com>
At present, plpar_hcall(H_SCM_BIND_MEM, ...) takes a very long time, so
if dumping to fsdax, it will take a very long time.
Take a closer look, during the papr_scm initialization, the only
configuration is through drc_pmem_bind()-> plpar_hcall(H_SCM_BIND_MEM,
...), which helps to set up the bound address.
On pseries, for kexec -l/-p kernel, there is no reset of hardware, and this
step can be stepped around to save times. So the pmem bound address can be
passed to the 2nd kernel through a dynamic added property "bound-addr" in
dt node 'ibm,pmemory'.
Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
To: linuxppc-dev@lists.ozlabs.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Hari Bathini <hbathini@linux.ibm.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: kexec@lists.infradead.org
---
note: I can not find such a pseries machine, and not finish it yet.
---
arch/powerpc/platforms/pseries/papr_scm.c | 32 +++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index c2ef320..555e746 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -382,7 +382,7 @@ static int papr_scm_probe(struct platform_device *pdev)
{
struct device_node *dn = pdev->dev.of_node;
u32 drc_index, metadata_size;
- u64 blocks, block_size;
+ u64 blocks, block_size, bound_addr = 0;
struct papr_scm_priv *p;
const char *uuid_str;
u64 uuid[2];
@@ -439,17 +439,29 @@ static int papr_scm_probe(struct platform_device *pdev)
p->metadata_size = metadata_size;
p->pdev = pdev;
- /* request the hypervisor to bind this region to somewhere in memory */
- rc = drc_pmem_bind(p);
+ of_property_read_u64(dn, "bound-addr", &bound_addr);
+ if (bound_addr)
+ p->bound_addr = bound_addr;
+ else {
+ struct property *property;
+ u64 big;
- /* If phyp says drc memory still bound then force unbound and retry */
- if (rc == H_OVERLAP)
- rc = drc_pmem_query_n_bind(p);
+ /* request the hypervisor to bind this region to somewhere in memory */
+ rc = drc_pmem_bind(p);
- if (rc != H_SUCCESS) {
- dev_err(&p->pdev->dev, "bind err: %d\n", rc);
- rc = -ENXIO;
- goto err;
+ /* If phyp says drc memory still bound then force unbound and retry */
+ if (rc == H_OVERLAP)
+ rc = drc_pmem_query_n_bind(p);
+
+ if (rc != H_SUCCESS) {
+ dev_err(&p->pdev->dev, "bind err: %d\n", rc);
+ rc = -ENXIO;
+ goto err;
+ }
+ big = cpu_to_be64(p->bound_addr);
+ property = new_property("bound-addr", sizeof(u64), &big,
+ NULL);
+ of_add_property(dn, property);
}
/* setup the resource for the newly bound range */
--
2.7.5
^ permalink raw reply related
* [PATCH 2/3] powerpc/of: coding style cleanup
From: Pingfan Liu @ 2020-02-28 5:53 UTC (permalink / raw)
To: linuxppc-dev
Cc: kexec, Pingfan Liu, Paul Mackerras, Aneesh Kumar K . V,
Oliver O'Halloran, Dan Williams, Hari Bathini
In-Reply-To: <1582869192-9284-1-git-send-email-kernelfans@gmail.com>
Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
To: linuxppc-dev@lists.ozlabs.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Hari Bathini <hbathini@linux.ibm.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: kexec@lists.infradead.org
---
arch/powerpc/kernel/of_property.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/of_property.c b/arch/powerpc/kernel/of_property.c
index e56c832..c6abf7e 100644
--- a/arch/powerpc/kernel/of_property.c
+++ b/arch/powerpc/kernel/of_property.c
@@ -5,16 +5,18 @@
#include <linux/slab.h>
struct property *new_property(const char *name, const int length,
- const unsigned char *value, struct property *last)
+ const unsigned char *value, struct property *last)
{
struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
if (!new)
return NULL;
- if (!(new->name = kstrdup(name, GFP_KERNEL)))
+ new->name = kstrdup(name, GFP_KERNEL);
+ if (!new->name)
goto cleanup;
- if (!(new->value = kmalloc(length + 1, GFP_KERNEL)))
+ new->value = kmalloc(length + 1, GFP_KERNEL);
+ if (!new->value)
goto cleanup;
memcpy(new->value, value, length);
--
2.7.5
^ permalink raw reply related
* [PATCH 1/3] powerpc/of: split out new_property() for reusing
From: Pingfan Liu @ 2020-02-28 5:53 UTC (permalink / raw)
To: linuxppc-dev
Cc: kexec, Pingfan Liu, Paul Mackerras, Aneesh Kumar K . V,
Oliver O'Halloran, Dan Williams, Hari Bathini
Since new_property() is used in several calling sites, splitting it out for
reusing.
To ease the review, although the split out part has coding style issue,
keeping it untouched and fixed in next patch.
Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
To: linuxppc-dev@lists.ozlabs.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Hari Bathini <hbathini@linux.ibm.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: kexec@lists.infradead.org
---
arch/powerpc/include/asm/prom.h | 2 ++
arch/powerpc/kernel/Makefile | 2 +-
arch/powerpc/kernel/of_property.c | 32 +++++++++++++++++++++++++++++++
arch/powerpc/mm/drmem.c | 26 -------------------------
arch/powerpc/platforms/pseries/reconfig.c | 26 -------------------------
5 files changed, 35 insertions(+), 53 deletions(-)
create mode 100644 arch/powerpc/kernel/of_property.c
diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
index 94e3fd5..02f7b1b 100644
--- a/arch/powerpc/include/asm/prom.h
+++ b/arch/powerpc/include/asm/prom.h
@@ -90,6 +90,8 @@ struct of_drc_info {
extern int of_read_drc_info_cell(struct property **prop,
const __be32 **curval, struct of_drc_info *data);
+extern struct property *new_property(const char *name, const int length,
+ const unsigned char *value, struct property *last);
/*
* There are two methods for telling firmware what our capabilities are.
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 157b014..23375fd 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -47,7 +47,7 @@ obj-y := cputable.o ptrace.o syscalls.o \
signal.o sysfs.o cacheinfo.o time.o \
prom.o traps.o setup-common.o \
udbg.o misc.o io.o misc_$(BITS).o \
- of_platform.o prom_parse.o
+ of_platform.o prom_parse.o of_property.o
obj-$(CONFIG_PPC64) += setup_64.o sys_ppc32.o \
signal_64.o ptrace32.o \
paca.o nvram_64.o firmware.o note.o
diff --git a/arch/powerpc/kernel/of_property.c b/arch/powerpc/kernel/of_property.c
new file mode 100644
index 0000000..e56c832
--- /dev/null
+++ b/arch/powerpc/kernel/of_property.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/of.h>
+#include <linux/string.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+
+struct property *new_property(const char *name, const int length,
+ const unsigned char *value, struct property *last)
+{
+ struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
+
+ if (!new)
+ return NULL;
+
+ if (!(new->name = kstrdup(name, GFP_KERNEL)))
+ goto cleanup;
+ if (!(new->value = kmalloc(length + 1, GFP_KERNEL)))
+ goto cleanup;
+
+ memcpy(new->value, value, length);
+ *(((char *)new->value) + length) = 0;
+ new->length = length;
+ new->next = last;
+ return new;
+
+cleanup:
+ kfree(new->name);
+ kfree(new->value);
+ kfree(new);
+ return NULL;
+}
+
diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
index 85b088a..888227e 100644
--- a/arch/powerpc/mm/drmem.c
+++ b/arch/powerpc/mm/drmem.c
@@ -99,32 +99,6 @@ static void init_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
extern int test_hotplug;
-static struct property *new_property(const char *name, const int length,
- const unsigned char *value, struct property *last)
-{
- struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
-
- if (!new)
- return NULL;
-
- if (!(new->name = kstrdup(name, GFP_KERNEL)))
- goto cleanup;
- if (!(new->value = kmalloc(length + 1, GFP_KERNEL)))
- goto cleanup;
-
- memcpy(new->value, value, length);
- *(((char *)new->value) + length) = 0;
- new->length = length;
- new->next = last;
- return new;
-
-cleanup:
- kfree(new->name);
- kfree(new->value);
- kfree(new);
- return NULL;
-}
-
static int drmem_update_dt_v2(struct device_node *memory,
struct property *prop)
{
diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 7f7369f..8e5a2ba 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -165,32 +165,6 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
return tmp;
}
-static struct property *new_property(const char *name, const int length,
- const unsigned char *value, struct property *last)
-{
- struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
-
- if (!new)
- return NULL;
-
- if (!(new->name = kstrdup(name, GFP_KERNEL)))
- goto cleanup;
- if (!(new->value = kmalloc(length + 1, GFP_KERNEL)))
- goto cleanup;
-
- memcpy(new->value, value, length);
- *(((char *)new->value) + length) = 0;
- new->length = length;
- new->next = last;
- return new;
-
-cleanup:
- kfree(new->name);
- kfree(new->value);
- kfree(new);
- return NULL;
-}
-
static int do_add_node(char *buf, size_t bufsize)
{
char *path, *end, *name;
--
2.7.5
^ permalink raw reply related
* Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
From: Nicholas Piggin @ 2020-02-28 4:53 UTC (permalink / raw)
To: Jordan Niethe; +Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
In-Reply-To: <CACzsE9oyVoukqHZHkSbEoWBmotksqORKJNOuFz_4J4ExMUeijw@mail.gmail.com>
Jordan Niethe's on February 28, 2020 1:23 pm:
> On Fri, Feb 28, 2020 at 12:48 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> Jordan Niethe's on February 27, 2020 10:58 am:
>> > On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>> >>
>> >> Jordan Niethe's on February 26, 2020 2:07 pm:
>> >> > @@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
>> >> > }
>> >> >
>> >> > if (!ret) {
>> >> > - patch_instruction(p->ainsn.insn, *p->addr);
>> >> > + patch_instruction(&p->ainsn.insn[0], p->addr[0]);
>> >> > + if (IS_PREFIX(insn))
>> >> > + patch_instruction(&p->ainsn.insn[1], p->addr[1]);
>> >> > p->opcode = *p->addr;
>> >>
>> >> Not to single out this hunk or this patch even, but what do you reckon
>> >> about adding an instruction data type, and then use that in all these
>> >> call sites rather than adding the extra arg or doing the extra copy
>> >> manually in each place depending on prefix?
>> >>
>> >> instrs_are_equal, get_user_instr, analyse_instr, patch_instruction,
>> >> etc., would all take this new instr. Places that open code a memory
>> >> access like your MCE change need some accessor
>> >>
>> >> instr = *(unsigned int *)(instr_addr);
>> >> - if (!analyse_instr(&op, &tmp, instr, PPC_NO_SUFFIX)) {
>> >> + if (IS_PREFIX(instr))
>> >> + suffix = *(unsigned int *)(instr_addr + 4);
>> >>
>> >> Becomes
>> >> read_instr(instr_addr, &instr);
>> >> if (!analyse_instr(&op, &tmp, instr)) ...
>> >>
>> >> etc.
>> > Daniel Axtens also talked about this and my reasons not to do so were
>> > pretty unconvincing, so I started trying something like this. One
>>
>> Okay.
>>
>> > thing I have been wondering is how pervasive should the new type be.
>>
>> I wouldn't mind it being quite pervasive. We have to be careful not
>> to copy it directly to/from memory now, but if we have accessors to
>> do all that with, I think it should be okay.
>>
>> > Below is data type I have started using, which I think works
>> > reasonably for replacing unsigned ints everywhere (like within
>> > code-patching.c). In a few architecture independent places such as
>> > uprobes which want to do ==, etc the union type does not work so well.
>>
>> There will be some places you have to make the boundary. I would start
>> by just making it a powerpc thing, but possibly there is or could be
>> some generic helpers. How does something like x86 cope with this?
>
> One of the places I was thinking of was is_swbp_insn() in
> kernel/events/uprobes.c. The problem was I wanted to typedef
> uprobe_opcode_t as ppc_insn type which was probably the wrong thing to
> do. x86 typedef's it as u8 (size of the trap they use). So we probably
> can do the same thing and just keep it as a u32.
Sounds good.
>> > I will have the next revision of the series start using a type.
>>
>> Thanks for doing that.
>>
>> >
>> > diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
>> > new file mode 100644
>> > index 000000000000..50adb3dbdeb4
>> > --- /dev/null
>> > +++ b/arch/powerpc/include/asm/inst.h
>> > @@ -0,0 +1,87 @@
>> > +
>> > +#ifndef _ASM_INST_H
>> > +#define _ASM_INST_H
>> > +
>> > +#ifdef __powerpc64__
>> > +
>> > +/* 64 bit Instruction */
>> > +
>> > +typedef struct {
>> > + unsigned int prefix;
>> > + unsigned int suffix;
>>
>> u32?
> Sure.
>>
>> > +} __packed ppc_prefixed_inst;
>> > +
>> > +typedef union ppc_inst {
>> > + unsigned int w;
>> > + ppc_prefixed_inst p;
>> > +} ppc_inst;
>>
>> I'd make it a struct and use nameless structs/unions inside it (with
>> appropriate packed annotation):
> Yeah that will be nicer.
>>
>> struct ppc_inst {
>> union {
>> struct {
>> u32 word;
>> u32 pad;
>> };
>> struct {
>> u32 prefix;
>> u32 suffix;
>> };
>> };
>> };
>>
>> > +
>> > +#define PPC_INST_IS_PREFIXED(inst) (((inst).w >> 26) == 1)
>> > +#define PPC_INST_LEN(inst) (PPC_INST_IS_PREFIXED((inst)) ?
>> > sizeof((inst).p) : sizeof((inst).w))
>>
>> Good accessors, I'd make them all C inline functions and lower case.
> Will change.
>>
>> > +
>> > +#define PPC_INST_NEW_WORD(x) ((ppc_inst) { .w = (x) })
>> > +#define PPC_INST_NEW_WORD_PAD(x) ((ppc_inst) { .p.prefix = (x),
>> > .p.suffix = (0x60000000) })
>> > +#define PPC_INST_NEW_PREFIXED(x, y) ((ppc_inst) { .p.prefix = (x),
>> > .p.suffix = (y) })
>>
>> If these are widely used, I'd make them a bit shorter
>>
>> #define PPC_INST(x)
>> #define PPC_INST_PREFIXED(x)
> Good idea, they do end up used quite widely.
>>
>> I'd also set padding to something invalid 0 or 0xffffffff for the first
>> case, and have your accessors check that rather than carrying around
>> another type of ppc_inst (prefixed, padded, non-padded).
>>
>> > +
>> > +#define PPC_INST_WORD(x) ((x).w)
>> > +#define PPC_INST_PREFIX(x) (x.p.prefix)
>> > +#define PPC_INST_SUFFIX(x) (x.p.suffix)
>> > +#define PPC_INST_EMPTY(x) (PPC_INST_WORD(x) == 0)
>>
>> I'd avoid simple accessors like this completely. If they have any use
>> it would be to ensure you don't try to use prefix/suffix on a
>> non-prefixed instruction for example. If you want to do that I'd use
>> inline functions for it.
> What I was thinking with these macros was that they would let us keep
> the instruction type as a simple u32 on 32 bit ppc. Is it worth trying
> to do that?
Hmm, it might be. On the other hand if ppc32 can use the same
struct ppc_insn struct it might help share code without too many
macros.
I guess it's up to Christophe and Michael and you I won't complain
any more (so long as they're lower case and inlines where possible :))
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v3 01/14] powerpc: Enable Prefixed Instructions
From: Nicholas Piggin @ 2020-02-28 4:48 UTC (permalink / raw)
To: Jordan Niethe; +Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
In-Reply-To: <CACzsE9oQSnEsbD=ftCOd51cPRrDyvv4EHY-1pn+DUUO=AVyTGg@mail.gmail.com>
Jordan Niethe's on February 28, 2020 12:52 pm:
> On Wed, Feb 26, 2020 at 5:50 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> Jordan Niethe's on February 26, 2020 2:07 pm:
>> > From: Alistair Popple <alistair@popple.id.au>
>> >
>> > Prefix instructions have their own FSCR bit which needs to enabled via
>> > a CPU feature. The kernel will save the FSCR for problem state but it
>> > needs to be enabled initially.
>> >
>> > Signed-off-by: Alistair Popple <alistair@popple.id.au>
>> > ---
>> > arch/powerpc/include/asm/reg.h | 3 +++
>> > arch/powerpc/kernel/dt_cpu_ftrs.c | 23 +++++++++++++++++++++++
>> > 2 files changed, 26 insertions(+)
>> >
>> > diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
>> > index 1aa46dff0957..c7758c2ccc5f 100644
>> > --- a/arch/powerpc/include/asm/reg.h
>> > +++ b/arch/powerpc/include/asm/reg.h
>> > @@ -397,6 +397,7 @@
>> > #define SPRN_RWMR 0x375 /* Region-Weighting Mode Register */
>> >
>> > /* HFSCR and FSCR bit numbers are the same */
>> > +#define FSCR_PREFIX_LG 13 /* Enable Prefix Instructions */
>> > #define FSCR_SCV_LG 12 /* Enable System Call Vectored */
>> > #define FSCR_MSGP_LG 10 /* Enable MSGP */
>> > #define FSCR_TAR_LG 8 /* Enable Target Address Register */
>> > @@ -408,11 +409,13 @@
>> > #define FSCR_VECVSX_LG 1 /* Enable VMX/VSX */
>> > #define FSCR_FP_LG 0 /* Enable Floating Point */
>> > #define SPRN_FSCR 0x099 /* Facility Status & Control Register */
>> > +#define FSCR_PREFIX __MASK(FSCR_PREFIX_LG)
>>
>> When you add a new FSCR, there's a couple more things to do, check
>> out traps.c.
>>
>> > #define FSCR_SCV __MASK(FSCR_SCV_LG)
>> > #define FSCR_TAR __MASK(FSCR_TAR_LG)
>> > #define FSCR_EBB __MASK(FSCR_EBB_LG)
>> > #define FSCR_DSCR __MASK(FSCR_DSCR_LG)
>> > #define SPRN_HFSCR 0xbe /* HV=1 Facility Status & Control Register */
>> > +#define HFSCR_PREFIX __MASK(FSCR_PREFIX_LG)
>> > #define HFSCR_MSGP __MASK(FSCR_MSGP_LG)
>> > #define HFSCR_TAR __MASK(FSCR_TAR_LG)
>> > #define HFSCR_EBB __MASK(FSCR_EBB_LG)
>> > diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> > index 182b4047c1ef..396f2c6c588e 100644
>> > --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
>> > +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> > @@ -553,6 +553,28 @@ static int __init feat_enable_large_ci(struct dt_cpu_feature *f)
>> > return 1;
>> > }
>> >
>> > +static int __init feat_enable_prefix(struct dt_cpu_feature *f)
>> > +{
>> > + u64 fscr, hfscr;
>> > +
>> > + if (f->usable_privilege & USABLE_HV) {
>> > + hfscr = mfspr(SPRN_HFSCR);
>> > + hfscr |= HFSCR_PREFIX;
>> > + mtspr(SPRN_HFSCR, hfscr);
>> > + }
>> > +
>> > + if (f->usable_privilege & USABLE_OS) {
>> > + fscr = mfspr(SPRN_FSCR);
>> > + fscr |= FSCR_PREFIX;
>> > + mtspr(SPRN_FSCR, fscr);
>> > +
>> > + if (f->usable_privilege & USABLE_PR)
>> > + current->thread.fscr |= FSCR_PREFIX;
>> > + }
>> > +
>> > + return 1;
>> > +}
>>
>> It would be good to be able to just use the default feature matching
>> for this, if possible? Do we not do the right thing with
>> init_thread.fscr?
> The default feature matching do you mean feat_enable()?
> I just tested using that again, within feat_enable() I can print and
> see that the [h]fscr gets the bits set for enabling prefixed
> instructions. However once I get to the shell and start xmon, the fscr
> bit for prefixed instructions can be seen to be unset. What we are
> doing in feat_enable_prefix() avoids that problem. So it seems maybe
> something is not quite right with the init_thread.fscr. I will look
> into further.
Okay it probably gets overwritten somewhere.
But hmm, the dt_cpu_ftrs parsing should be picking those up and setting
them by default I would think (or maybe not because you don't expect
FSCR bit if OS support is not required).
All the other FSCR bits are done similarly to this AFAIKS:
https://patchwork.ozlabs.org/patch/1244476/
I would do that for now rather than digging into it too far, but we
should look at cleaning that up and doing something more like what
you have here.
>> > struct dt_cpu_feature_match {
>> > const char *name;
>> > int (*enable)(struct dt_cpu_feature *f);
>> > @@ -626,6 +648,7 @@ static struct dt_cpu_feature_match __initdata
>> > {"vector-binary128", feat_enable, 0},
>> > {"vector-binary16", feat_enable, 0},
>> > {"wait-v3", feat_enable, 0},
>> > + {"prefix-instructions", feat_enable_prefix, 0},
>>
>> That's reasonable to make that a feature, will it specify a minimum
>> base set of prefix instructions or just that prefix instructions
>> with the prefix/suffix arrangement exist?
> This was just going to be that they exist.
>>
>> You may not need "-instructions" on the end, none of the other
>> instructions do.
> Good point.
>>
>> I would maybe just hold off upstreaming the dt_cpu_ftrs changes for
>> a bit. We have to do a pass over new CPU feature device tree, and
>> some compatibility questions have come up recently.
>>
>> If you wouldn't mind just adding the new [H]FSCR bits and faults
>> upstream for now, that would be good.
> No problem.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v4 06/13] powerpc/ptrace: split out ALTIVEC related functions.
From: kbuild test robot @ 2020-02-28 4:08 UTC (permalink / raw)
To: Christophe Leroy
Cc: mikey, kbuild-all, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <80bba4b3f280b0f10c7ccbfae3edb7fdc9b9c4e1.1582803998.git.christophe.leroy@c-s.fr>
[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]
Hi Christophe,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on v5.6-rc3 next-20200227]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/Reduce-ifdef-mess-in-ptrace/20200228-094814
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> arch/powerpc/kernel/ptrace/ptrace-altivec.c:8:10: fatal error: ptrace_decl.h: No such file or directory
#include "ptrace_decl.h"
^~~~~~~~~~~~~~~
compilation terminated.
vim +8 arch/powerpc/kernel/ptrace/ptrace-altivec.c
7
> 8 #include "ptrace_decl.h"
9
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25741 bytes --]
^ permalink raw reply
* Re: [PATCH v4 13/13] powerpc/ptrace: move ptrace_triggered() into hw_breakpoint.c
From: Russell Currey @ 2020-02-28 4:02 UTC (permalink / raw)
To: Andrew Donnellan, Michael Ellerman, Christophe Leroy,
Benjamin Herrenschmidt, Paul Mackerras, mikey
Cc: linuxppc-dev, Linux Kernel Mailing List
In-Reply-To: <b2b97a86-2f29-af14-52c2-a69ef6202b75@linux.ibm.com>
This was my fault, I should really test changes like these before they go live. Apologies for the confusion caused
--
Russell Currey
ruscur@russell.cc
On Fri, Feb 28, 2020, at 2:59 PM, Andrew Donnellan wrote:
> On 28/2/20 9:16 am, Michael Ellerman wrote:
> > Christophe Leroy <christophe.leroy@c-s.fr> writes:
> >> Russel,
> >>
> >> Le 27/02/2020 à 12:49, Christophe Leroy a écrit :
> >>> ptrace_triggered() is declared in asm/hw_breakpoint.h and
> >>> only needed when CONFIG_HW_BREAKPOINT is set, so move it
> >>> into hw_breakpoint.c
> >>
> >> My series v4 is definitely buggy (I included ptrace_decl.h instead
> >> instead of ptrace-decl.h), how can Snowpatch say build succeeded
> >> (https://patchwork.ozlabs.org/patch/1245807/) ?
> >
> > Which links to:
> > https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/15895//artifact/linux/report.txt
> >
> > The actual build log of which is:
> > https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/15895/artifact/linux/build_new.log
> >
> > Which contains:
> > scripts/Makefile.build:267: recipe for target 'arch/powerpc/kernel/ptrace/ptrace-altivec.o' failed
> > make[3]: *** [arch/powerpc/kernel/ptrace/ptrace-altivec.o] Error 1
> > make[3]: *** Waiting for unfinished jobs....
> > scripts/Makefile.build:505: recipe for target 'arch/powerpc/kernel/ptrace' failed
> > make[2]: *** [arch/powerpc/kernel/ptrace] Error 2
> > make[2]: *** Waiting for unfinished jobs....
> > scripts/Makefile.build:505: recipe for target 'arch/powerpc/kernel' failed
> > make[1]: *** [arch/powerpc/kernel] Error 2
> > make[1]: *** Waiting for unfinished jobs....
> > Makefile:1681: recipe for target 'arch/powerpc' failed
> > make: *** [arch/powerpc] Error 2
> > make: *** Waiting for unfinished jobs....
> >
> > Same for ppc64le:
> > https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/15896/artifact/linux/build_new.log
> >
> >
> > So it seems like snowpatch always reports the build as succeeded even
> > when it fails.
>
> Turns out there was an issue in a recent change in our build script
> which caused build failures to return the wrong exit code and put the
> wrong text in the reports, because of some confusion with bash
> subshells. I've fixed it (I think).
>
> --
> Andrew Donnellan OzLabs, ADL Canberra
> ajd@linux.ibm.com IBM Australia Limited
>
>
^ permalink raw reply
* Re: [PATCH v4 13/13] powerpc/ptrace: move ptrace_triggered() into hw_breakpoint.c
From: Andrew Donnellan @ 2020-02-28 3:59 UTC (permalink / raw)
To: Michael Ellerman, Christophe Leroy, Benjamin Herrenschmidt,
Paul Mackerras, mikey, Russell Currey
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <87pndz1xsf.fsf@mpe.ellerman.id.au>
On 28/2/20 9:16 am, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> Russel,
>>
>> Le 27/02/2020 à 12:49, Christophe Leroy a écrit :
>>> ptrace_triggered() is declared in asm/hw_breakpoint.h and
>>> only needed when CONFIG_HW_BREAKPOINT is set, so move it
>>> into hw_breakpoint.c
>>
>> My series v4 is definitely buggy (I included ptrace_decl.h instead
>> instead of ptrace-decl.h), how can Snowpatch say build succeeded
>> (https://patchwork.ozlabs.org/patch/1245807/) ?
>
> Which links to:
> https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/15895//artifact/linux/report.txt
>
> The actual build log of which is:
> https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/15895/artifact/linux/build_new.log
>
> Which contains:
> scripts/Makefile.build:267: recipe for target 'arch/powerpc/kernel/ptrace/ptrace-altivec.o' failed
> make[3]: *** [arch/powerpc/kernel/ptrace/ptrace-altivec.o] Error 1
> make[3]: *** Waiting for unfinished jobs....
> scripts/Makefile.build:505: recipe for target 'arch/powerpc/kernel/ptrace' failed
> make[2]: *** [arch/powerpc/kernel/ptrace] Error 2
> make[2]: *** Waiting for unfinished jobs....
> scripts/Makefile.build:505: recipe for target 'arch/powerpc/kernel' failed
> make[1]: *** [arch/powerpc/kernel] Error 2
> make[1]: *** Waiting for unfinished jobs....
> Makefile:1681: recipe for target 'arch/powerpc' failed
> make: *** [arch/powerpc] Error 2
> make: *** Waiting for unfinished jobs....
>
> Same for ppc64le:
> https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/15896/artifact/linux/build_new.log
>
>
> So it seems like snowpatch always reports the build as succeeded even
> when it fails.
Turns out there was an issue in a recent change in our build script
which caused build failures to return the wrong exit code and put the
wrong text in the reports, because of some confusion with bash
subshells. I've fixed it (I think).
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply
* Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
From: Jordan Niethe @ 2020-02-28 3:23 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
In-Reply-To: <1582853017.uien0qdstw.astroid@bobo.none>
On Fri, Feb 28, 2020 at 12:48 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Jordan Niethe's on February 27, 2020 10:58 am:
> > On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npiggin@gmail.com> wrote:
> >>
> >> Jordan Niethe's on February 26, 2020 2:07 pm:
> >> > @@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
> >> > }
> >> >
> >> > if (!ret) {
> >> > - patch_instruction(p->ainsn.insn, *p->addr);
> >> > + patch_instruction(&p->ainsn.insn[0], p->addr[0]);
> >> > + if (IS_PREFIX(insn))
> >> > + patch_instruction(&p->ainsn.insn[1], p->addr[1]);
> >> > p->opcode = *p->addr;
> >>
> >> Not to single out this hunk or this patch even, but what do you reckon
> >> about adding an instruction data type, and then use that in all these
> >> call sites rather than adding the extra arg or doing the extra copy
> >> manually in each place depending on prefix?
> >>
> >> instrs_are_equal, get_user_instr, analyse_instr, patch_instruction,
> >> etc., would all take this new instr. Places that open code a memory
> >> access like your MCE change need some accessor
> >>
> >> instr = *(unsigned int *)(instr_addr);
> >> - if (!analyse_instr(&op, &tmp, instr, PPC_NO_SUFFIX)) {
> >> + if (IS_PREFIX(instr))
> >> + suffix = *(unsigned int *)(instr_addr + 4);
> >>
> >> Becomes
> >> read_instr(instr_addr, &instr);
> >> if (!analyse_instr(&op, &tmp, instr)) ...
> >>
> >> etc.
> > Daniel Axtens also talked about this and my reasons not to do so were
> > pretty unconvincing, so I started trying something like this. One
>
> Okay.
>
> > thing I have been wondering is how pervasive should the new type be.
>
> I wouldn't mind it being quite pervasive. We have to be careful not
> to copy it directly to/from memory now, but if we have accessors to
> do all that with, I think it should be okay.
>
> > Below is data type I have started using, which I think works
> > reasonably for replacing unsigned ints everywhere (like within
> > code-patching.c). In a few architecture independent places such as
> > uprobes which want to do ==, etc the union type does not work so well.
>
> There will be some places you have to make the boundary. I would start
> by just making it a powerpc thing, but possibly there is or could be
> some generic helpers. How does something like x86 cope with this?
One of the places I was thinking of was is_swbp_insn() in
kernel/events/uprobes.c. The problem was I wanted to typedef
uprobe_opcode_t as ppc_insn type which was probably the wrong thing to
do. x86 typedef's it as u8 (size of the trap they use). So we probably
can do the same thing and just keep it as a u32.
>
> > I will have the next revision of the series start using a type.
>
> Thanks for doing that.
>
> >
> > diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
> > new file mode 100644
> > index 000000000000..50adb3dbdeb4
> > --- /dev/null
> > +++ b/arch/powerpc/include/asm/inst.h
> > @@ -0,0 +1,87 @@
> > +
> > +#ifndef _ASM_INST_H
> > +#define _ASM_INST_H
> > +
> > +#ifdef __powerpc64__
> > +
> > +/* 64 bit Instruction */
> > +
> > +typedef struct {
> > + unsigned int prefix;
> > + unsigned int suffix;
>
> u32?
Sure.
>
> > +} __packed ppc_prefixed_inst;
> > +
> > +typedef union ppc_inst {
> > + unsigned int w;
> > + ppc_prefixed_inst p;
> > +} ppc_inst;
>
> I'd make it a struct and use nameless structs/unions inside it (with
> appropriate packed annotation):
Yeah that will be nicer.
>
> struct ppc_inst {
> union {
> struct {
> u32 word;
> u32 pad;
> };
> struct {
> u32 prefix;
> u32 suffix;
> };
> };
> };
>
> > +
> > +#define PPC_INST_IS_PREFIXED(inst) (((inst).w >> 26) == 1)
> > +#define PPC_INST_LEN(inst) (PPC_INST_IS_PREFIXED((inst)) ?
> > sizeof((inst).p) : sizeof((inst).w))
>
> Good accessors, I'd make them all C inline functions and lower case.
Will change.
>
> > +
> > +#define PPC_INST_NEW_WORD(x) ((ppc_inst) { .w = (x) })
> > +#define PPC_INST_NEW_WORD_PAD(x) ((ppc_inst) { .p.prefix = (x),
> > .p.suffix = (0x60000000) })
> > +#define PPC_INST_NEW_PREFIXED(x, y) ((ppc_inst) { .p.prefix = (x),
> > .p.suffix = (y) })
>
> If these are widely used, I'd make them a bit shorter
>
> #define PPC_INST(x)
> #define PPC_INST_PREFIXED(x)
Good idea, they do end up used quite widely.
>
> I'd also set padding to something invalid 0 or 0xffffffff for the first
> case, and have your accessors check that rather than carrying around
> another type of ppc_inst (prefixed, padded, non-padded).
>
> > +
> > +#define PPC_INST_WORD(x) ((x).w)
> > +#define PPC_INST_PREFIX(x) (x.p.prefix)
> > +#define PPC_INST_SUFFIX(x) (x.p.suffix)
> > +#define PPC_INST_EMPTY(x) (PPC_INST_WORD(x) == 0)
>
> I'd avoid simple accessors like this completely. If they have any use
> it would be to ensure you don't try to use prefix/suffix on a
> non-prefixed instruction for example. If you want to do that I'd use
> inline functions for it.
What I was thinking with these macros was that they would let us keep
the instruction type as a simple u32 on 32 bit ppc. Is it worth trying
to do that?
>
> > +
> > +#define DEREF_PPC_INST_PTR(ptr) \
> > +({ \
> > + ppc_inst __inst; \
> > + __inst.w = *(unsigned int *)(ptr); \
> > + if (PPC_INST_IS_PREFIXED(__inst)) \
> > + __inst.p = *(ppc_prefixed_inst *)(ptr); \
> > + __inst; \
> > +})
>
> I'd go an inline with shorter lowercase names. ppc_inst_read(&inst);
Will do.
>
> > +#define PPC_INST_NEXT(ptr) ((ptr) += PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr))))
> > +#define PPC_INST_PREV(ptr) ((ptr) -= PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr))))
>
> Wouldn't bother with this accessor, caller could ptr += ppc_inst_len(inst)
Good point.
>
>
> > +#define PPC_INST_EQ(x, y) \
> > +({ \
> > + long pic_ret = 0; \
> > + pic_ret = (PPC_INST_PREFIX(x) == PPC_INST_PREFIX(y)); \
> > + if (pic_ret) { \
> > + if (PPC_INST_IS_PREFIXED(x) && PPC_INST_IS_PREFIXED(y)) { \
> > + pic_ret = (PPC_INST_SUFFIX(x) == PPC_INST_SUFFIX(y)); \
> > + } else { \
> > + pic_ret = 0; \
> > + } \
> > + } \
> > + pic_ret; \
> > +})
>
> static inline bool ppc_inst_eq(struct ppc_inst x, struct ppc_inst y)
> {
> return !memcmp(&x, &y, sizeof(struct ppc_inst));
> }
Well that is definitely cleaner!
Thank you for the feedback.
>
> If all accessors and initalisers are such that the padding gets set,
> that'll work.
>
> Thanks,
> Nick
^ permalink raw reply
* [PATCH 2/2] powerpc/powernv: Wire up OPAL address lookups
From: Nicholas Piggin @ 2020-02-28 3:10 UTC (permalink / raw)
To: linuxppc-dev; +Cc: linux-arch, skiboot, linux-kernel, Nicholas Piggin
In-Reply-To: <20200228031027.271510-1-npiggin@gmail.com>
Use ARCH_HAS_ADDRESS_LOOKUP to look up the opal symbol table. This
allows crashes and xmon debugging to print firmware symbols.
Oops: System Reset, sig: 6 [#1]
LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA PowerNV
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.6.0-rc2-dirty #903
NIP: 0000000030020434 LR: 000000003000378c CTR: 0000000030020414
REGS: c0000000fffc3d70 TRAP: 0100 Not tainted (5.6.0-rc2-dirty)
MSR: 9000000002101002 <SF,HV,VEC,ME,RI> CR: 28022284 XER: 20040000
CFAR: 0000000030003788 IRQMASK: 3
GPR00: 000000003000378c 0000000031c13c90 0000000030136200 c0000000012cfa10
GPR04: c0000000012cfa10 0000000000000010 0000000000000000 0000000031c10060
GPR08: c0000000012cfaaf 0000000030003640 0000000000000000 0000000000000001
GPR12: 00000000300e0000 c000000001490000 0000000000000000 c00000000139c588
GPR16: 0000000031c10000 c00000000125a900 0000000000000000 c0000000012076a8
GPR20: c0000000012a3950 0000000000000001 0000000031c10060 c0000000012cfaaf
GPR24: 0000000000000019 0000000030003640 0000000000000000 0000000000000000
GPR28: 0000000000000010 c0000000012cfa10 0000000000000000 0000000000000000
NIP [0000000030020434] .dummy_console_write_buffer_space+0x20/0x64 [OPAL]
LR [000000003000378c] opal_entry+0x14c/0x17c [OPAL]
This won't unwind the firmware stack (or its Linux caller) properly if
firmware and kernel endians don't match, but that problem could be solved
in powerpc's unwinder.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/opal-api.h | 6 +++-
arch/powerpc/include/asm/opal.h | 3 ++
arch/powerpc/platforms/powernv/opal-call.c | 2 ++
arch/powerpc/platforms/powernv/opal.c | 40 ++++++++++++++++++++++
5 files changed, 51 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 497b7d0b2d7e..4d32b02d35e8 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -115,6 +115,7 @@ config PPC
# Please keep this list sorted alphabetically.
#
select ARCH_32BIT_OFF_T if PPC32
+ select ARCH_HAS_ADDRESS_LOOKUP if PPC_POWERNV
select ARCH_HAS_DEBUG_VIRTUAL
select ARCH_HAS_DEVMEM_IS_ALLOWED
select ARCH_HAS_ELF_RANDOMIZE
diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h
index c1f25a760eb1..c3a2a797177a 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -214,7 +214,11 @@
#define OPAL_SECVAR_GET 176
#define OPAL_SECVAR_GET_NEXT 177
#define OPAL_SECVAR_ENQUEUE_UPDATE 178
-#define OPAL_LAST 178
+#define OPAL_PHB_SET_OPTION 179
+#define OPAL_PHB_GET_OPTION 180
+#define OPAL_GET_SYMBOL 181
+#define OPAL_LOOKUP_SYMBOL 182
+#define OPAL_LAST 182
#define QUIESCE_HOLD 1 /* Spin all calls at entry */
#define QUIESCE_REJECT 2 /* Fail all calls with OPAL_BUSY */
diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 9986ac34b8e2..ef2d9273f06f 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -312,6 +312,9 @@ s64 opal_mpipl_query_tag(enum opal_mpipl_tags tag, u64 *addr);
s64 opal_signal_system_reset(s32 cpu);
s64 opal_quiesce(u64 shutdown_type, s32 cpu);
+int64_t opal_get_symbol(uint64_t addr, __be64 *symaddr, __be64 *symsize, char *namebuf, uint64_t buflen);
+int64_t opal_lookup_symbol(const char *name, __be64 *symaddr, __be64 *symsize);
+
/* Internal functions */
extern int early_init_dt_scan_opal(unsigned long node, const char *uname,
int depth, void *data);
diff --git a/arch/powerpc/platforms/powernv/opal-call.c b/arch/powerpc/platforms/powernv/opal-call.c
index 5cd0f52d258f..ba11112d94df 100644
--- a/arch/powerpc/platforms/powernv/opal-call.c
+++ b/arch/powerpc/platforms/powernv/opal-call.c
@@ -293,3 +293,5 @@ OPAL_CALL(opal_mpipl_query_tag, OPAL_MPIPL_QUERY_TAG);
OPAL_CALL(opal_secvar_get, OPAL_SECVAR_GET);
OPAL_CALL(opal_secvar_get_next, OPAL_SECVAR_GET_NEXT);
OPAL_CALL(opal_secvar_enqueue_update, OPAL_SECVAR_ENQUEUE_UPDATE);
+OPAL_CALL(opal_get_symbol, OPAL_GET_SYMBOL);
+OPAL_CALL(opal_lookup_symbol, OPAL_LOOKUP_SYMBOL);
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 2b3dfd0b6cdd..fdf6c4e6f7f9 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -107,6 +107,46 @@ void opal_configure_cores(void)
cur_cpu_spec->cpu_restore();
}
+const char *arch_address_lookup(unsigned long addr,
+ unsigned long *symbolsize,
+ unsigned long *offset,
+ char **modname, char *namebuf)
+{
+ __be64 symaddr;
+ __be64 symsize;
+
+ if (!firmware_has_feature(FW_FEATURE_OPAL))
+ return NULL;
+
+ if (opal_get_symbol(addr, &symaddr, &symsize, namebuf,
+ cpu_to_be64(KSYM_NAME_LEN)) != OPAL_SUCCESS)
+ return NULL;
+
+ *symbolsize = be64_to_cpu(symsize);
+ *offset = addr - be64_to_cpu(symaddr);
+ *modname = "OPAL";
+
+ return namebuf;
+}
+
+unsigned long arch_address_lookup_name(const char *name)
+{
+ __be64 addr;
+ __be64 size;
+
+ if (!firmware_has_feature(FW_FEATURE_OPAL))
+ return 0;
+
+ /* opal: prefix allows lookup of symbols that clash with kernel */
+ if (!strncasecmp(name, "opal:", strlen("opal:")))
+ name += strlen("opal:");
+
+ if (opal_lookup_symbol(name, &addr, &size) != OPAL_SUCCESS)
+ return 0;
+
+ return be64_to_cpu(addr);
+}
+
int __init early_init_dt_scan_opal(unsigned long node,
const char *uname, int depth, void *data)
{
--
2.23.0
^ permalink raw reply related
* [PATCH 1/2] kallsyms: architecture specific symbol lookups
From: Nicholas Piggin @ 2020-02-28 3:10 UTC (permalink / raw)
To: linuxppc-dev; +Cc: linux-arch, skiboot, linux-kernel, Nicholas Piggin
Provide CONFIG_ARCH_HAS_ADDRESS_LOOKUP which allows architectures to
do their own symbol/address lookup if kernel and module lookups miss.
powerpc will use this to deal with firmware symbols.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
include/linux/kallsyms.h | 20 ++++++++++++++++++++
kernel/kallsyms.c | 13 ++++++++++++-
lib/Kconfig | 3 +++
3 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index 657a83b943f0..8fdd44873373 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -83,6 +83,26 @@ extern int kallsyms_lookup_size_offset(unsigned long addr,
unsigned long *symbolsize,
unsigned long *offset);
+#ifdef CONFIG_ARCH_HAS_ADDRESS_LOOKUP
+const char *arch_address_lookup(unsigned long addr,
+ unsigned long *symbolsize,
+ unsigned long *offset,
+ char **modname, char *namebuf);
+unsigned long arch_address_lookup_name(const char *name);
+#else
+static inline const char *arch_address_lookup(unsigned long addr,
+ unsigned long *symbolsize,
+ unsigned long *offset,
+ char **modname, char *namebuf)
+{
+ return NULL;
+}
+static inline unsigned long arch_address_lookup_name(const char *name)
+{
+ return 0;
+}
+#endif
+
/* Lookup an address. modname is set to NULL if it's in the kernel. */
const char *kallsyms_lookup(unsigned long addr,
unsigned long *symbolsize,
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index a9b3f660dee7..580c762fadd8 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -164,6 +164,7 @@ static unsigned long kallsyms_sym_address(int idx)
unsigned long kallsyms_lookup_name(const char *name)
{
char namebuf[KSYM_NAME_LEN];
+ unsigned long ret;
unsigned long i;
unsigned int off;
@@ -173,7 +174,12 @@ unsigned long kallsyms_lookup_name(const char *name)
if (strcmp(namebuf, name) == 0)
return kallsyms_sym_address(i);
}
- return module_kallsyms_lookup_name(name);
+
+ ret = module_kallsyms_lookup_name(name);
+ if (ret)
+ return ret;
+
+ return arch_address_lookup_name(name);
}
EXPORT_SYMBOL_GPL(kallsyms_lookup_name);
@@ -311,6 +317,11 @@ const char *kallsyms_lookup(unsigned long addr,
if (!ret)
ret = ftrace_mod_address_lookup(addr, symbolsize,
offset, modname, namebuf);
+
+ if (!ret)
+ ret = arch_address_lookup(addr, symbolsize,
+ offset, modname, namebuf);
+
return ret;
}
diff --git a/lib/Kconfig b/lib/Kconfig
index bc7e56370129..16d3b8dbcadf 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -80,6 +80,9 @@ config ARCH_USE_CMPXCHG_LOCKREF
config ARCH_HAS_FAST_MULTIPLIER
bool
+config ARCH_HAS_ADDRESS_LOOKUP
+ bool
+
config INDIRECT_PIO
bool "Access I/O in non-MMIO mode"
depends on ARM64
--
2.23.0
^ permalink raw reply related
* [PATCH] Add OPAL_GET_SYMBOL / OPAL_LOOKUP_SYMBOL
From: Nicholas Piggin @ 2020-02-28 3:07 UTC (permalink / raw)
To: skiboot; +Cc: linuxppc-dev, Nicholas Piggin
These calls can be used by Linux to annotate BUG addresses with symbols,
look up symbol addresses in xmon, etc.
This is preferable over having Linux parse the OPAL symbol map itself,
because OPAL's parsing code already exists for its own symbol printing,
and it can support other code regions than the skiboot symbols, e.g.,
the wake-up code in the HOMER (where CPUs have been seen to get stuck).
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
core/opal.c | 2 +
core/utils.c | 92 +++++++++++++++++++++++--
doc/opal-api/opal-get-symbol-181.rst | 42 +++++++++++
doc/opal-api/opal-lookup-symbol-182.rst | 35 ++++++++++
include/opal-api.h | 4 +-
5 files changed, 168 insertions(+), 7 deletions(-)
create mode 100644 doc/opal-api/opal-get-symbol-181.rst
create mode 100644 doc/opal-api/opal-lookup-symbol-182.rst
diff --git a/core/opal.c b/core/opal.c
index d6ff6027b..d9fc4fe05 100644
--- a/core/opal.c
+++ b/core/opal.c
@@ -142,6 +142,8 @@ int64_t opal_entry_check(struct stack_frame *eframe)
case OPAL_CEC_REBOOT:
case OPAL_CEC_REBOOT2:
case OPAL_SIGNAL_SYSTEM_RESET:
+ case OPAL_GET_SYMBOL:
+ case OPAL_LOOKUP_SYMBOL:
break;
default:
printf("CPU ATTEMPT TO RE-ENTER FIRMWARE! PIR=%04lx cpu @%p -> pir=%04x token=%llu\n",
diff --git a/core/utils.c b/core/utils.c
index 8fd63fcb7..5f0d5130b 100644
--- a/core/utils.c
+++ b/core/utils.c
@@ -48,40 +48,120 @@ char __attrconst tohex(uint8_t nibble)
return __tohex[nibble];
}
-static unsigned long get_symbol(unsigned long addr, char **sym, char **sym_end)
+static unsigned long get_symbol(unsigned long addr, char **sym, char **sym_end, unsigned long *size)
{
unsigned long prev = 0, next;
char *psym = NULL, *p = __sym_map_start;
*sym = *sym_end = NULL;
- while(p < __sym_map_end) {
+ while (p < __sym_map_end) {
next = strtoul(p, &p, 16) | SKIBOOT_BASE;
if (next > addr && prev <= addr) {
- p = psym + 3;;
+ p = psym + 3;
if (p >= __sym_map_end)
return 0;
*sym = p;
- while(p < __sym_map_end && *p != 10)
+ while (p < __sym_map_end && *p != '\n')
p++;
*sym_end = p;
+ *size = next - prev;
return prev;
}
prev = next;
psym = p;
- while(p < __sym_map_end && *p != 10)
+ while (p < __sym_map_end && *p != '\n')
p++;
p++;
}
return 0;
}
+static unsigned long lookup_symbol(const char *name, unsigned long *size)
+{
+ size_t len = strlen(name);
+ unsigned long addr = 0;
+ char *sym;
+ char *p = __sym_map_start;
+
+ while (p < __sym_map_end) {
+ addr = strtoul(p, &p, 16) | SKIBOOT_BASE;
+ p += 3;
+ if (p >= __sym_map_end)
+ return 0;
+
+ if (*(p + len) == '\n' && !strncmp(name, p, len)) {
+ char *sym_end;
+
+ if (get_symbol(addr, &sym, &sym_end, size) == 0) {
+ assert(!strcmp(name, "_end"));
+ *size = 0;
+ }
+
+ /*
+ * May be more than one symbol at this address but
+ * symbol length calculation should still work in
+ * that case.
+ */
+
+ return addr;
+ }
+
+ while(p < __sym_map_end && *p != '\n')
+ p++;
+ p++;
+ }
+ return 0;
+}
+
+static int64_t opal_get_symbol(uint64_t addr, __be64 *symaddr, __be64 *symsize, char *namebuf, uint64_t buflen)
+{
+ unsigned long saddr;
+ unsigned long ssize;
+ char *sym, *sym_end;
+ size_t l;
+
+ saddr = get_symbol(addr, &sym, &sym_end, &ssize);
+ if (!saddr)
+ return OPAL_RESOURCE;
+
+ if (buflen > sym_end - sym)
+ l = sym_end - sym;
+ else
+ l = buflen - 1;
+ memcpy(namebuf, sym, l);
+ namebuf[l] = '\0';
+
+ *symaddr = cpu_to_be64(saddr);
+ *symsize = cpu_to_be64(ssize);
+
+ return OPAL_SUCCESS;
+}
+opal_call(OPAL_GET_SYMBOL, opal_get_symbol, 5);
+
+static int64_t opal_lookup_symbol(const char *name, __be64 *symaddr, __be64 *symsize)
+{
+ unsigned long saddr;
+ unsigned long ssize;
+
+ saddr = lookup_symbol(name, &ssize);
+ if (!saddr)
+ return OPAL_RESOURCE;
+
+ *symaddr = cpu_to_be64(saddr);
+ *symsize = cpu_to_be64(ssize);
+
+ return OPAL_SUCCESS;
+}
+opal_call(OPAL_LOOKUP_SYMBOL, opal_lookup_symbol, 3);
+
size_t snprintf_symbol(char *buf, size_t len, uint64_t addr)
{
unsigned long saddr;
+ unsigned long ssize;
char *sym, *sym_end;
size_t l;
- saddr = get_symbol(addr, &sym, &sym_end);
+ saddr = get_symbol(addr, &sym, &sym_end, &ssize);
if (!saddr)
return 0;
diff --git a/doc/opal-api/opal-get-symbol-181.rst b/doc/opal-api/opal-get-symbol-181.rst
new file mode 100644
index 000000000..a57de6c1c
--- /dev/null
+++ b/doc/opal-api/opal-get-symbol-181.rst
@@ -0,0 +1,42 @@
+.. _OPAL_GET_SYMBOL:
+
+OPAL_GET_SYMBOL
+================
+
+.. code-block:: c
+
+ #define OPAL_GET_SYMBOL 181
+
+ static int64_t opal_get_symbol(uint64_t addr, __be64 *symaddr, __be64 *symsize, char *namebuf, uint64_t buflen);
+
+This OPAL call looks up a firmware code address for symbol information.
+
+Arguments
+---------
+
+ ``addr``
+ Contains address to be looked up.
+
+ ``symaddr``
+ Returns the start address of the symbol for the object which
+ contains addr or immediately precedes addr.
+
+ ``symsize``
+ Returns the size of the object, or the number of bytes until the
+ next symbol.
+
+ ``namebuf``
+ Contains a buffer for the symbol name to be copied into, as a NUL
+ terminated string.
+
+ ``buflen``
+ Contains the length of the bufer that may be used.
+
+
+Returns
+-------
+
+:ref:`OPAL_SUCCESS`
+ Found a symbol.
+:ref:`OPAL_RESOURCE`
+ Did not find a symbol.
diff --git a/doc/opal-api/opal-lookup-symbol-182.rst b/doc/opal-api/opal-lookup-symbol-182.rst
new file mode 100644
index 000000000..e9f6eba33
--- /dev/null
+++ b/doc/opal-api/opal-lookup-symbol-182.rst
@@ -0,0 +1,35 @@
+.. _OPAL_LOOKUP_SYMBOL:
+
+OPAL_LOOKUP_SYMBOL
+================
+
+.. code-block:: c
+
+ #define OPAL_LOOKUP_SYMBOL 182
+
+ static int64_t opal_lookup_symbol(const char *name, __be64 *symaddr, __be64 *symsize);
+
+This OPAL call looks up a firmware symbol name for its address.
+
+Arguments
+---------
+
+ ``name``
+ Contains a pointer to NUL terminated symbol name to be looked up.
+
+ ``symaddr``
+ Returns the start address of the symbol for the object which
+ contains addr or immediately precedes addr.
+
+ ``symsize``
+ Returns the size of the object, or the number of bytes until the
+ next symbol.
+
+
+Returns
+-------
+
+:ref:`OPAL_SUCCESS`
+ Found a symbol.
+:ref:`OPAL_RESOURCE`
+ Did not find a symbol.
diff --git a/include/opal-api.h b/include/opal-api.h
index 5b07dea13..4474631a6 100644
--- a/include/opal-api.h
+++ b/include/opal-api.h
@@ -227,7 +227,9 @@
#define OPAL_SECVAR_ENQUEUE_UPDATE 178
#define OPAL_PHB_SET_OPTION 179
#define OPAL_PHB_GET_OPTION 180
-#define OPAL_LAST 180
+#define OPAL_GET_SYMBOL 181
+#define OPAL_LOOKUP_SYMBOL 182
+#define OPAL_LAST 182
#define QUIESCE_HOLD 1 /* Spin all calls at entry */
#define QUIESCE_REJECT 2 /* Fail all calls with OPAL_BUSY */
--
2.23.0
^ permalink raw reply related
* Re: [PATCH v3 1/4] ASoC: fsl_asrc: Change asrc_width to asrc_format
From: Shengjiu Wang @ 2020-02-28 2:54 UTC (permalink / raw)
To: Nicolin Chen
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux-ALSA, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, Liam Girdwood, Rob Herring, Mark Brown,
linuxppc-dev, linux-kernel
In-Reply-To: <20200227174540.GA17040@Asurada-Nvidia.nvidia.com>
Hi
On Fri, Feb 28, 2020 at 1:45 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> On Thu, Feb 27, 2020 at 01:10:19PM +0800, Shengjiu Wang wrote:
> > On Thu, Feb 27, 2020 at 11:43 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> > >
> > > On Thu, Feb 27, 2020 at 10:41:55AM +0800, Shengjiu Wang wrote:
> > > > asrc_format is more inteligent variable, which is align
> > > > with the alsa definition snd_pcm_format_t.
> > > >
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > > ---
> > > > sound/soc/fsl/fsl_asrc.c | 23 +++++++++++------------
> > > > sound/soc/fsl/fsl_asrc.h | 4 ++--
> > > > sound/soc/fsl/fsl_asrc_dma.c | 2 +-
> > > > 3 files changed, 14 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> > > > index 0dcebc24c312..2b6a1643573c 100644
> > > > --- a/sound/soc/fsl/fsl_asrc.c
> > > > +++ b/sound/soc/fsl/fsl_asrc.c
> > >
> > > > @@ -600,11 +599,6 @@ static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream,
> > > >
> > > > pair->config = &config;
> > > >
> > > > - if (asrc_priv->asrc_width == 16)
> > > > - format = SNDRV_PCM_FORMAT_S16_LE;
> > > > - else
> > > > - format = SNDRV_PCM_FORMAT_S24_LE;
> > >
> > > It feels better to me that we have format settings in hw_params().
> > >
> > > Why not let fsl_easrc align with this? Any reason that I'm missing?
> >
> > because the asrc_width is not formal, in the future we can direct
>
> Hmm..that's our DT binding. And I don't feel it is a problem
> to be ASoC irrelative.
>
> > input the format from the dts. format involve the info about width.
>
> Is there such any formal ASoC binding? I don't see those PCM
> formats under include/dt-bindings/ folder. How are we going
> to involve those formats in DT?
There is no formal binding of this case.
I think it is not good to convert width to format, because, for example
width = 24, there is two option, we can select format S24_LE, or
format S24_3LE, width is ambiguous for selecting.
In EASRC, it support other two 24bit format U24_LE, U24_3LE .
if we use the format in DT, then it is clear for usage in driver.
best regards
wang shengjiu
^ permalink raw reply
* Re: [PATCH v3 01/14] powerpc: Enable Prefixed Instructions
From: Jordan Niethe @ 2020-02-28 2:52 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
In-Reply-To: <1582698829.pxzksoow7n.astroid@bobo.none>
On Wed, Feb 26, 2020 at 5:50 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Jordan Niethe's on February 26, 2020 2:07 pm:
> > From: Alistair Popple <alistair@popple.id.au>
> >
> > Prefix instructions have their own FSCR bit which needs to enabled via
> > a CPU feature. The kernel will save the FSCR for problem state but it
> > needs to be enabled initially.
> >
> > Signed-off-by: Alistair Popple <alistair@popple.id.au>
> > ---
> > arch/powerpc/include/asm/reg.h | 3 +++
> > arch/powerpc/kernel/dt_cpu_ftrs.c | 23 +++++++++++++++++++++++
> > 2 files changed, 26 insertions(+)
> >
> > diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> > index 1aa46dff0957..c7758c2ccc5f 100644
> > --- a/arch/powerpc/include/asm/reg.h
> > +++ b/arch/powerpc/include/asm/reg.h
> > @@ -397,6 +397,7 @@
> > #define SPRN_RWMR 0x375 /* Region-Weighting Mode Register */
> >
> > /* HFSCR and FSCR bit numbers are the same */
> > +#define FSCR_PREFIX_LG 13 /* Enable Prefix Instructions */
> > #define FSCR_SCV_LG 12 /* Enable System Call Vectored */
> > #define FSCR_MSGP_LG 10 /* Enable MSGP */
> > #define FSCR_TAR_LG 8 /* Enable Target Address Register */
> > @@ -408,11 +409,13 @@
> > #define FSCR_VECVSX_LG 1 /* Enable VMX/VSX */
> > #define FSCR_FP_LG 0 /* Enable Floating Point */
> > #define SPRN_FSCR 0x099 /* Facility Status & Control Register */
> > +#define FSCR_PREFIX __MASK(FSCR_PREFIX_LG)
>
> When you add a new FSCR, there's a couple more things to do, check
> out traps.c.
>
> > #define FSCR_SCV __MASK(FSCR_SCV_LG)
> > #define FSCR_TAR __MASK(FSCR_TAR_LG)
> > #define FSCR_EBB __MASK(FSCR_EBB_LG)
> > #define FSCR_DSCR __MASK(FSCR_DSCR_LG)
> > #define SPRN_HFSCR 0xbe /* HV=1 Facility Status & Control Register */
> > +#define HFSCR_PREFIX __MASK(FSCR_PREFIX_LG)
> > #define HFSCR_MSGP __MASK(FSCR_MSGP_LG)
> > #define HFSCR_TAR __MASK(FSCR_TAR_LG)
> > #define HFSCR_EBB __MASK(FSCR_EBB_LG)
> > diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> > index 182b4047c1ef..396f2c6c588e 100644
> > --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> > +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> > @@ -553,6 +553,28 @@ static int __init feat_enable_large_ci(struct dt_cpu_feature *f)
> > return 1;
> > }
> >
> > +static int __init feat_enable_prefix(struct dt_cpu_feature *f)
> > +{
> > + u64 fscr, hfscr;
> > +
> > + if (f->usable_privilege & USABLE_HV) {
> > + hfscr = mfspr(SPRN_HFSCR);
> > + hfscr |= HFSCR_PREFIX;
> > + mtspr(SPRN_HFSCR, hfscr);
> > + }
> > +
> > + if (f->usable_privilege & USABLE_OS) {
> > + fscr = mfspr(SPRN_FSCR);
> > + fscr |= FSCR_PREFIX;
> > + mtspr(SPRN_FSCR, fscr);
> > +
> > + if (f->usable_privilege & USABLE_PR)
> > + current->thread.fscr |= FSCR_PREFIX;
> > + }
> > +
> > + return 1;
> > +}
>
> It would be good to be able to just use the default feature matching
> for this, if possible? Do we not do the right thing with
> init_thread.fscr?
The default feature matching do you mean feat_enable()?
I just tested using that again, within feat_enable() I can print and
see that the [h]fscr gets the bits set for enabling prefixed
instructions. However once I get to the shell and start xmon, the fscr
bit for prefixed instructions can be seen to be unset. What we are
doing in feat_enable_prefix() avoids that problem. So it seems maybe
something is not quite right with the init_thread.fscr. I will look
into further.
>
>
> > +
> > struct dt_cpu_feature_match {
> > const char *name;
> > int (*enable)(struct dt_cpu_feature *f);
> > @@ -626,6 +648,7 @@ static struct dt_cpu_feature_match __initdata
> > {"vector-binary128", feat_enable, 0},
> > {"vector-binary16", feat_enable, 0},
> > {"wait-v3", feat_enable, 0},
> > + {"prefix-instructions", feat_enable_prefix, 0},
>
> That's reasonable to make that a feature, will it specify a minimum
> base set of prefix instructions or just that prefix instructions
> with the prefix/suffix arrangement exist?
This was just going to be that they exist.
>
> You may not need "-instructions" on the end, none of the other
> instructions do.
Good point.
>
> I would maybe just hold off upstreaming the dt_cpu_ftrs changes for
> a bit. We have to do a pass over new CPU feature device tree, and
> some compatibility questions have come up recently.
>
> If you wouldn't mind just adding the new [H]FSCR bits and faults
> upstream for now, that would be good.
No problem.
>
> Thanks,
> Nick
^ permalink raw reply
* Re: [PATCH v3 07/14] powerpc/traps: Check for prefixed instructions in facility_unavailable_exception()
From: Nicholas Piggin @ 2020-02-28 1:57 UTC (permalink / raw)
To: Jordan Niethe; +Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
In-Reply-To: <CACzsE9qmF+mpPQzL7HN5jOdjCtTJbqWvQQfnOxXkpJMs-fQxRg@mail.gmail.com>
Jordan Niethe's on February 27, 2020 9:52 am:
> On Wed, Feb 26, 2020 at 5:53 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> Jordan Niethe's on February 26, 2020 2:07 pm:
>> > If prefixed instructions are made unavailable by the [H]FSCR, attempting
>> > to use them will cause a facility unavailable exception. Add "PREFIX" to
>> > the facility_strings[].
>> >
>> > Currently there are no prefixed instructions that are actually emulated
>> > by emulate_instruction() within facility_unavailable_exception().
>> > However, when caused by a prefixed instructions the SRR1 PREFIXED bit is
>> > set. Prepare for dealing with emulated prefixed instructions by checking
>> > for this bit.
>> >
>> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
>>
>> Oh you've got it here, I would just squash this together with the first
>> patch.
> Sure, I'll put them together. When you mentioned a couple more things
> to do in traps.c, was it just this? Or is there still more to be done
> adding an FSCR?
Hmm... maybe it's just that one.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
From: Nicholas Piggin @ 2020-02-28 1:56 UTC (permalink / raw)
To: Jordan Niethe; +Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
In-Reply-To: <1582853017.uien0qdstw.astroid@bobo.none>
Nicholas Piggin's on February 28, 2020 11:47 am:
> Jordan Niethe's on February 27, 2020 10:58 am:
>> On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>> +
>> +#define DEREF_PPC_INST_PTR(ptr) \
>> +({ \
>> + ppc_inst __inst; \
>> + __inst.w = *(unsigned int *)(ptr); \
>> + if (PPC_INST_IS_PREFIXED(__inst)) \
>> + __inst.p = *(ppc_prefixed_inst *)(ptr); \
>> + __inst; \
>> +})
>
> I'd go an inline with shorter lowercase names. ppc_inst_read(&inst);
Probably inst = ppc_inst_read(mem), rather.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
From: Nicholas Piggin @ 2020-02-28 1:47 UTC (permalink / raw)
To: Jordan Niethe; +Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
In-Reply-To: <CACzsE9r+dKAgmTPsWtzPTnjmVskG_BO5G3=0V_Qm_6pu-_ZRFg@mail.gmail.com>
Jordan Niethe's on February 27, 2020 10:58 am:
> On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> Jordan Niethe's on February 26, 2020 2:07 pm:
>> > @@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
>> > }
>> >
>> > if (!ret) {
>> > - patch_instruction(p->ainsn.insn, *p->addr);
>> > + patch_instruction(&p->ainsn.insn[0], p->addr[0]);
>> > + if (IS_PREFIX(insn))
>> > + patch_instruction(&p->ainsn.insn[1], p->addr[1]);
>> > p->opcode = *p->addr;
>>
>> Not to single out this hunk or this patch even, but what do you reckon
>> about adding an instruction data type, and then use that in all these
>> call sites rather than adding the extra arg or doing the extra copy
>> manually in each place depending on prefix?
>>
>> instrs_are_equal, get_user_instr, analyse_instr, patch_instruction,
>> etc., would all take this new instr. Places that open code a memory
>> access like your MCE change need some accessor
>>
>> instr = *(unsigned int *)(instr_addr);
>> - if (!analyse_instr(&op, &tmp, instr, PPC_NO_SUFFIX)) {
>> + if (IS_PREFIX(instr))
>> + suffix = *(unsigned int *)(instr_addr + 4);
>>
>> Becomes
>> read_instr(instr_addr, &instr);
>> if (!analyse_instr(&op, &tmp, instr)) ...
>>
>> etc.
> Daniel Axtens also talked about this and my reasons not to do so were
> pretty unconvincing, so I started trying something like this. One
Okay.
> thing I have been wondering is how pervasive should the new type be.
I wouldn't mind it being quite pervasive. We have to be careful not
to copy it directly to/from memory now, but if we have accessors to
do all that with, I think it should be okay.
> Below is data type I have started using, which I think works
> reasonably for replacing unsigned ints everywhere (like within
> code-patching.c). In a few architecture independent places such as
> uprobes which want to do ==, etc the union type does not work so well.
There will be some places you have to make the boundary. I would start
by just making it a powerpc thing, but possibly there is or could be
some generic helpers. How does something like x86 cope with this?
> I will have the next revision of the series start using a type.
Thanks for doing that.
>
> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
> new file mode 100644
> index 000000000000..50adb3dbdeb4
> --- /dev/null
> +++ b/arch/powerpc/include/asm/inst.h
> @@ -0,0 +1,87 @@
> +
> +#ifndef _ASM_INST_H
> +#define _ASM_INST_H
> +
> +#ifdef __powerpc64__
> +
> +/* 64 bit Instruction */
> +
> +typedef struct {
> + unsigned int prefix;
> + unsigned int suffix;
u32?
> +} __packed ppc_prefixed_inst;
> +
> +typedef union ppc_inst {
> + unsigned int w;
> + ppc_prefixed_inst p;
> +} ppc_inst;
I'd make it a struct and use nameless structs/unions inside it (with
appropriate packed annotation):
struct ppc_inst {
union {
struct {
u32 word;
u32 pad;
};
struct {
u32 prefix;
u32 suffix;
};
};
};
> +
> +#define PPC_INST_IS_PREFIXED(inst) (((inst).w >> 26) == 1)
> +#define PPC_INST_LEN(inst) (PPC_INST_IS_PREFIXED((inst)) ?
> sizeof((inst).p) : sizeof((inst).w))
Good accessors, I'd make them all C inline functions and lower case.
> +
> +#define PPC_INST_NEW_WORD(x) ((ppc_inst) { .w = (x) })
> +#define PPC_INST_NEW_WORD_PAD(x) ((ppc_inst) { .p.prefix = (x),
> .p.suffix = (0x60000000) })
> +#define PPC_INST_NEW_PREFIXED(x, y) ((ppc_inst) { .p.prefix = (x),
> .p.suffix = (y) })
If these are widely used, I'd make them a bit shorter
#define PPC_INST(x)
#define PPC_INST_PREFIXED(x)
I'd also set padding to something invalid 0 or 0xffffffff for the first
case, and have your accessors check that rather than carrying around
another type of ppc_inst (prefixed, padded, non-padded).
> +
> +#define PPC_INST_WORD(x) ((x).w)
> +#define PPC_INST_PREFIX(x) (x.p.prefix)
> +#define PPC_INST_SUFFIX(x) (x.p.suffix)
> +#define PPC_INST_EMPTY(x) (PPC_INST_WORD(x) == 0)
I'd avoid simple accessors like this completely. If they have any use
it would be to ensure you don't try to use prefix/suffix on a
non-prefixed instruction for example. If you want to do that I'd use
inline functions for it.
> +
> +#define DEREF_PPC_INST_PTR(ptr) \
> +({ \
> + ppc_inst __inst; \
> + __inst.w = *(unsigned int *)(ptr); \
> + if (PPC_INST_IS_PREFIXED(__inst)) \
> + __inst.p = *(ppc_prefixed_inst *)(ptr); \
> + __inst; \
> +})
I'd go an inline with shorter lowercase names. ppc_inst_read(&inst);
> +#define PPC_INST_NEXT(ptr) ((ptr) += PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr))))
> +#define PPC_INST_PREV(ptr) ((ptr) -= PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr))))
Wouldn't bother with this accessor, caller could ptr += ppc_inst_len(inst)
> +#define PPC_INST_EQ(x, y) \
> +({ \
> + long pic_ret = 0; \
> + pic_ret = (PPC_INST_PREFIX(x) == PPC_INST_PREFIX(y)); \
> + if (pic_ret) { \
> + if (PPC_INST_IS_PREFIXED(x) && PPC_INST_IS_PREFIXED(y)) { \
> + pic_ret = (PPC_INST_SUFFIX(x) == PPC_INST_SUFFIX(y)); \
> + } else { \
> + pic_ret = 0; \
> + } \
> + } \
> + pic_ret; \
> +})
static inline bool ppc_inst_eq(struct ppc_inst x, struct ppc_inst y)
{
return !memcmp(&x, &y, sizeof(struct ppc_inst));
}
If all accessors and initalisers are such that the padding gets set,
that'll work.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v3 09/14] powerpc/xmon: Add initial support for prefixed instructions
From: Nicholas Piggin @ 2020-02-28 1:23 UTC (permalink / raw)
To: Christophe Leroy, Jordan Niethe
Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
In-Reply-To: <CACzsE9r6JGOuTBwaLxDhWb2W4E=Ag0M-QHrxJUCcjP4XNYZxkQ@mail.gmail.com>
Jordan Niethe's on February 28, 2020 10:37 am:
> On Thu, Feb 27, 2020 at 6:14 PM Christophe Leroy
> <christophe.leroy@c-s.fr> wrote:
>>
>>
>>
>> Le 27/02/2020 à 01:11, Jordan Niethe a écrit :
>> > On Wed, Feb 26, 2020 at 6:10 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>> >>
>> >> Jordan Niethe's on February 26, 2020 2:07 pm:
>> >>> A prefixed instruction is composed of a word prefix and a word suffix.
>> >>> It does not make sense to be able to have a breakpoint on the suffix of
>> >>> a prefixed instruction, so make this impossible.
>> >>>
>> >>> When leaving xmon_core() we check to see if we are currently at a
>> >>> breakpoint. If this is the case, the breakpoint needs to be proceeded
>> >>> from. Initially emulate_step() is tried, but if this fails then we need
>> >>> to execute the saved instruction out of line. The NIP is set to the
>> >>> address of bpt::instr[] for the current breakpoint. bpt::instr[]
>> >>> contains the instruction replaced by the breakpoint, followed by a trap
>> >>> instruction. After bpt::instr[0] is executed and we hit the trap we
>> >>> enter back into xmon_bpt(). We know that if we got here and the offset
>> >>> indicates we are at bpt::instr[1] then we have just executed out of line
>> >>> so we can put the NIP back to the instruction after the breakpoint
>> >>> location and continue on.
>> >>>
>> >>> Adding prefixed instructions complicates this as the bpt::instr[1] needs
>> >>> to be used to hold the suffix. To deal with this make bpt::instr[] big
>> >>> enough for three word instructions. bpt::instr[2] contains the trap,
>> >>> and in the case of word instructions pad bpt::instr[1] with a noop.
>> >>>
>> >>> No support for disassembling prefixed instructions.
>> >>>
>> >>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
>> >>> ---
>> >>> v2: Rename sufx to suffix
>> >>> v3: - Just directly use PPC_INST_NOP
>> >>> - Typo: plac -> place
>> >>> - Rename read_inst() to mread_inst(). Do not have it call mread().
>> >>> ---
>> >>> arch/powerpc/xmon/xmon.c | 90 ++++++++++++++++++++++++++++++++++------
>> >>> 1 file changed, 78 insertions(+), 12 deletions(-)
>> >>>
>> >>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>> >>> index a673cf55641c..a73a35aa4a75 100644
>> >>> --- a/arch/powerpc/xmon/xmon.c
>> >>> +++ b/arch/powerpc/xmon/xmon.c
>> >>> @@ -97,7 +97,8 @@ static long *xmon_fault_jmp[NR_CPUS];
>> >>> /* Breakpoint stuff */
>> >>> struct bpt {
>> >>> unsigned long address;
>> >>> - unsigned int instr[2];
>> >>> + /* Prefixed instructions can not cross 64-byte boundaries */
>> >>> + unsigned int instr[3] __aligned(64);
>> >>
>> >> This is pretty wild, I didn't realize xmon executes breakpoints out
>> >> of line like this.
>>
>> Neither did I. That's problematic. Kernel data is mapped NX on some
>> platforms.
>>
>> >>
>> >> IMO the break point entries here should correspond with a range of
>> >> reserved bytes in .text so we patch instructions into normal executable
>> >> pages rather than .data.
>> > Would it make sense to use vmalloc_exec() and use that like we are
>> > going to do in kprobes()?
>>
>> As we are (already) doing in kprobes() you mean ?
> Sorry for the confusion, I was mainly thinking of the patch that you
> pointed out before:
> https://patchwork.ozlabs.org/patch/1232619/
>>
>> In fact kprobes uses module_alloc(), and it works because kprobe depends
>> on module. On some platforms (i.e. book3s/32) vmalloc space is marked NX
>> in segment registers when CONFIG_MODULES is not set, see
>> mmu_mark_initmem_nx(). On other ones the Instruction TLB miss exception
>> does not manage misses at kernel addresses when CONFIG_MODULES is not
>> selected.
>>
>> So if we want XMON to work at all time, we need to use some (linear)
>> text address and use patch_instruction() to change it.
> Thank you for the detailed clarification, I will do it like that.
Yeah I would just make a little array in .text.xmon_bpts or something,
which should do the trick.
That wouldn't depend on this series, but if you want to do it as a fix
up patch before it, that would be good.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v3 10/27] powerpc: Add driver for OpenCAPI Persistent Memory
From: Alastair D'Silva @ 2020-02-28 0:54 UTC (permalink / raw)
To: Frederic Barrat
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Oliver O'Halloran, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Krzysztof Kozlowski, Anju T Sudhakar,
Mahesh Salgaonkar, Andrew Donnellan, Arnd Bergmann, Greg Kurz,
Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Vishal Verma, Paul Mackerras, Andrew Morton, linuxppc-dev,
David S. Miller
In-Reply-To: <da02a596-ce4e-f004-5072-cb3dff0a7484@linux.ibm.com>
On Thu, 2020-02-27 at 21:44 +0100, Frederic Barrat wrote:
>
> Le 21/02/2020 à 04:27, Alastair D'Silva a écrit :
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > This driver exposes LPC memory on OpenCAPI pmem cards
> > as an NVDIMM, allowing the existing nvram infrastructure
> > to be used.
> >
> > Namespace metadata is stored on the media itself, so
> > scm_reserve_metadata() maps 1 section's worth of PMEM storage
> > at the start to hold this. The rest of the PMEM range is registered
> > with libnvdimm as an nvdimm. scm_ndctl_config_read/write/size()
> > provide
> > callbacks to libnvdimm to access the metadata.
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> > arch/powerpc/platforms/powernv/Kconfig | 3 +
> > arch/powerpc/platforms/powernv/Makefile | 1 +
> > arch/powerpc/platforms/powernv/pmem/Kconfig | 15 +
> > arch/powerpc/platforms/powernv/pmem/Makefile | 7 +
> > arch/powerpc/platforms/powernv/pmem/ocxl.c | 473
> > ++++++++++++++++++
> > .../platforms/powernv/pmem/ocxl_internal.h | 28 ++
> > 6 files changed, 527 insertions(+)
> > create mode 100644 arch/powerpc/platforms/powernv/pmem/Kconfig
> > create mode 100644 arch/powerpc/platforms/powernv/pmem/Makefile
> > create mode 100644 arch/powerpc/platforms/powernv/pmem/ocxl.c
> > create mode 100644
> > arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> >
> > diff --git a/arch/powerpc/platforms/powernv/Kconfig
> > b/arch/powerpc/platforms/powernv/Kconfig
> > index 938803eab0ad..fc8976af0e52 100644
> > --- a/arch/powerpc/platforms/powernv/Kconfig
> > +++ b/arch/powerpc/platforms/powernv/Kconfig
> > @@ -50,3 +50,6 @@ config PPC_VAS
> > config SCOM_DEBUGFS
> > bool "Expose SCOM controllers via debugfs"
> > depends on DEBUG_FS
> > +
> > +source "arch/powerpc/platforms/powernv/pmem/Kconfig"
> > +
> > diff --git a/arch/powerpc/platforms/powernv/Makefile
> > b/arch/powerpc/platforms/powernv/Makefile
> > index c0f8120045c3..0bbd72988b6f 100644
> > --- a/arch/powerpc/platforms/powernv/Makefile
> > +++ b/arch/powerpc/platforms/powernv/Makefile
> > @@ -21,3 +21,4 @@ obj-$(CONFIG_PPC_VAS) += vas.o vas-window.o
> > vas-debug.o
> > obj-$(CONFIG_OCXL_BASE) += ocxl.o
> > obj-$(CONFIG_SCOM_DEBUGFS) += opal-xscom.o
> > obj-$(CONFIG_PPC_SECURE_BOOT) += opal-secvar.o
> > +obj-$(CONFIG_LIBNVDIMM) += pmem/
> > diff --git a/arch/powerpc/platforms/powernv/pmem/Kconfig
> > b/arch/powerpc/platforms/powernv/pmem/Kconfig
> > new file mode 100644
> > index 000000000000..c5d927520920
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/powernv/pmem/Kconfig
> > @@ -0,0 +1,15 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +if LIBNVDIMM
> > +
> > +config OCXL_PMEM
> > + tristate "OpenCAPI Persistent Memory"
> > + depends on LIBNVDIMM && PPC_POWERNV && PCI && EEH &&
> > ZONE_DEVICE && OCXL
> > + help
> > + Exposes devices that implement the OpenCAPI Storage Class
> > Memory
> > + specification as persistent memory regions. You may also want
> > + DEV_DAX, DEV_DAX_PMEM & FS_DAX if you plan on using DAX
> > devices
> > + stacked on top of this driver.
> > +
> > + Select N if unsure.
> > +
> > +endif
> > diff --git a/arch/powerpc/platforms/powernv/pmem/Makefile
> > b/arch/powerpc/platforms/powernv/pmem/Makefile
> > new file mode 100644
> > index 000000000000..1c55c4193175
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/powernv/pmem/Makefile
> > @@ -0,0 +1,7 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +ccflags-$(CONFIG_PPC_WERROR) += -Werror
> > +
> > +obj-$(CONFIG_OCXL_PMEM) += ocxlpmem.o
> > +
> > +ocxlpmem-y := ocxl.o
> > diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > new file mode 100644
> > index 000000000000..3c4eeb5dcc0f
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> > @@ -0,0 +1,473 @@
> > +// SPDX-License-Id
> > +// Copyright 2019 IBM Corp.
> > +
> > +/*
> > + * A driver for OpenCAPI devices that implement the Storage Class
> > + * Memory specification.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <misc/ocxl.h>
> > +#include <linux/ndctl.h>
> > +#include <linux/mm_types.h>
> > +#include <linux/memory_hotplug.h>
> > +#include "ocxl_internal.h"
> > +
> > +
> > +static const struct pci_device_id ocxlpmem_pci_tbl[] = {
> > + { PCI_DEVICE(PCI_VENDOR_ID_IBM, 0x0625), },
> > + { }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(pci, ocxlpmem_pci_tbl);
> > +
> > +#define NUM_MINORS 256 // Total to reserve
> > +
> > +static dev_t ocxlpmem_dev;
> > +static struct class *ocxlpmem_class;
> > +static struct mutex minors_idr_lock;
> > +static struct idr minors_idr;
> > +
> > +/**
> > + * ndctl_config_write() - Handle a ND_CMD_SET_CONFIG_DATA command
> > from ndctl
> > + * @ocxlpmem: the device metadata
> > + * @command: the incoming data to write
> > + * Return: 0 on success, negative on failure
> > + */
> > +static int ndctl_config_write(struct ocxlpmem *ocxlpmem,
> > + struct nd_cmd_set_config_hdr *command)
> > +{
> > + if (command->in_offset + command->in_length > LABEL_AREA_SIZE)
> > + return -EINVAL;
> > +
> > + memcpy_flushcache(ocxlpmem->metadata_addr + command->in_offset,
> > command->in_buf,
> > + command->in_length);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * ndctl_config_read() - Handle a ND_CMD_GET_CONFIG_DATA command
> > from ndctl
> > + * @ocxlpmem: the device metadata
> > + * @command: the read request
> > + * Return: 0 on success, negative on failure
> > + */
> > +static int ndctl_config_read(struct ocxlpmem *ocxlpmem,
> > + struct nd_cmd_get_config_data_hdr
> > *command)
> > +{
> > + if (command->in_offset + command->in_length > LABEL_AREA_SIZE)
> > + return -EINVAL;
> > +
> > + memcpy_mcsafe(command->out_buf, ocxlpmem->metadata_addr +
> > command->in_offset,
> > + command->in_length);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * ndctl_config_size() - Handle a ND_CMD_GET_CONFIG_SIZE command
> > from ndctl
> > + * @command: the read request
> > + * Return: 0 on success, negative on failure
> > + */
> > +static int ndctl_config_size(struct nd_cmd_get_config_size
> > *command)
> > +{
> > + command->status = 0;
> > + command->config_size = LABEL_AREA_SIZE;
> > + command->max_xfer = PAGE_SIZE;
> > +
> > + return 0;
> > +}
> > +
> > +static int ndctl(struct nvdimm_bus_descriptor *nd_desc,
> > + struct nvdimm *nvdimm,
> > + unsigned int cmd, void *buf, unsigned int buf_len, int
> > *cmd_rc)
> > +{
> > + struct ocxlpmem *ocxlpmem = container_of(nd_desc, struct
> > ocxlpmem, bus_desc);
> > +
> > + switch (cmd) {
> > + case ND_CMD_GET_CONFIG_SIZE:
> > + *cmd_rc = ndctl_config_size(buf);
> > + return 0;
> > +
> > + case ND_CMD_GET_CONFIG_DATA:
> > + *cmd_rc = ndctl_config_read(ocxlpmem, buf);
> > + return 0;
> > +
> > + case ND_CMD_SET_CONFIG_DATA:
> > + *cmd_rc = ndctl_config_write(ocxlpmem, buf);
> > + return 0;
> > +
> > + default:
> > + return -ENOTTY;
> > + }
> > +}
> > +
> > +/**
> > + * reserve_metadata() - Reserve space for nvdimm metadata
> > + * @ocxlpmem: the device metadata
> > + * @lpc_mem: The resource representing the LPC memory of the
> > OpenCAPI device
> > + */
> > +static int reserve_metadata(struct ocxlpmem *ocxlpmem,
> > + struct resource *lpc_mem)
> > +{
> > + ocxlpmem->metadata_addr = devm_memremap(&ocxlpmem->dev,
> > lpc_mem->start,
> > + LABEL_AREA_SIZE,
> > MEMREMAP_WB);
> > + if (IS_ERR(ocxlpmem->metadata_addr))
> > + return PTR_ERR(ocxlpmem->metadata_addr);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * register_lpc_mem() - Discover persistent memory on a device and
> > register it with the NVDIMM subsystem
> > + * @ocxlpmem: the device metadata
> > + * Return: 0 on success
> > + */
> > +static int register_lpc_mem(struct ocxlpmem *ocxlpmem)
> > +{
> > + struct nd_region_desc region_desc;
> > + struct nd_mapping_desc nd_mapping_desc;
> > + struct resource *lpc_mem;
> > + const struct ocxl_afu_config *config;
> > + const struct ocxl_fn_config *fn_config;
> > + int rc;
> > + unsigned long nvdimm_cmd_mask = 0;
> > + unsigned long nvdimm_flags = 0;
> > + int target_node;
> > + char serial[16+1];
> > +
> > + // Set up the reserved metadata area
> > + rc = ocxl_afu_map_lpc_mem(ocxlpmem->ocxl_afu);
> > + if (rc < 0)
> > + return rc;
> > +
> > + lpc_mem = ocxl_afu_lpc_mem(ocxlpmem->ocxl_afu);
> > + if (lpc_mem == NULL || lpc_mem->start == 0)
> > + return -EINVAL;
> > +
> > + config = ocxl_afu_config(ocxlpmem->ocxl_afu);
> > + fn_config = ocxl_function_config(ocxlpmem->ocxl_fn);
> > +
> > + rc = reserve_metadata(ocxlpmem, lpc_mem);
> > + if (rc)
> > + return rc;
> > +
> > + ocxlpmem->bus_desc.provider_name = "ocxl-pmem";
> > + ocxlpmem->bus_desc.ndctl = ndctl;
> > + ocxlpmem->bus_desc.module = THIS_MODULE;
> > +
> > + ocxlpmem->nvdimm_bus = nvdimm_bus_register(&ocxlpmem->dev,
> > + &ocxlpmem-
> > >bus_desc);
> > + if (!ocxlpmem->nvdimm_bus)
> > + return -EINVAL;
> > +
> > + ocxlpmem->pmem_res.start = (u64)lpc_mem->start +
> > LABEL_AREA_SIZE;
> > + ocxlpmem->pmem_res.end = (u64)lpc_mem->start + config-
> > >lpc_mem_size - 1;
> > + ocxlpmem->pmem_res.name = "OpenCAPI persistent memory";
> > +
> > + set_bit(ND_CMD_GET_CONFIG_SIZE, &nvdimm_cmd_mask);
> > + set_bit(ND_CMD_GET_CONFIG_DATA, &nvdimm_cmd_mask);
> > + set_bit(ND_CMD_SET_CONFIG_DATA, &nvdimm_cmd_mask);
> > +
> > + set_bit(NDD_ALIASING, &nvdimm_flags);
> > +
> > + snprintf(serial, sizeof(serial), "%llx", fn_config->serial);
> > + nd_mapping_desc.nvdimm = nvdimm_create(ocxlpmem->nvdimm_bus,
> > ocxlpmem,
> > + NULL, nvdimm_flags, nvdimm_cmd_mask,
> > + 0, NULL);
> > + if (!nd_mapping_desc.nvdimm)
> > + return -ENOMEM;
> > +
> > + if (nvdimm_bus_check_dimm_count(ocxlpmem->nvdimm_bus, 1))
> > + return -EINVAL;
> > +
> > + nd_mapping_desc.start = ocxlpmem->pmem_res.start;
> > + nd_mapping_desc.size = resource_size(&ocxlpmem->pmem_res);
> > + nd_mapping_desc.position = 0;
> > +
> > + ocxlpmem->nd_set.cookie1 = fn_config->serial + 1; // allow for
> > empty serial
> > + ocxlpmem->nd_set.cookie2 = fn_config->serial + 1;
> > +
> > + target_node = of_node_to_nid(ocxlpmem->pdev->dev.of_node);
> > +
> > + memset(®ion_desc, 0, sizeof(region_desc));
> > + region_desc.res = &ocxlpmem->pmem_res;
> > + region_desc.numa_node = NUMA_NO_NODE;
> > + region_desc.target_node = target_node;
> > + region_desc.num_mappings = 1;
> > + region_desc.mapping = &nd_mapping_desc;
> > + region_desc.nd_set = &ocxlpmem->nd_set;
> > +
> > + set_bit(ND_REGION_PAGEMAP, ®ion_desc.flags);
> > + /*
> > + * NB: libnvdimm copies the data from ndr_desc into it's own
> > + * structures so passing a stack pointer is fine.
> > + */
> > + ocxlpmem->nd_region = nvdimm_pmem_region_create(ocxlpmem-
> > >nvdimm_bus,
> > + ®ion_desc);
> > + if (!ocxlpmem->nd_region)
> > + return -EINVAL;
> > +
> > + dev_info(&ocxlpmem->dev,
> > + "Onlining %lluMB of persistent memory\n",
> > + nd_mapping_desc.size / SZ_1M);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * allocate_minor() - Allocate a minor number to use for an
> > OpenCAPI pmem device
> > + * @ocxlpmem: the device metadata
> > + * Return: the allocated minor number
> > + */
> > +static int allocate_minor(struct ocxlpmem *ocxlpmem)
> > +{
> > + int minor;
> > +
> > + mutex_lock(&minors_idr_lock);
> > + minor = idr_alloc(&minors_idr, ocxlpmem, 0, NUM_MINORS,
> > GFP_KERNEL);
> > + mutex_unlock(&minors_idr_lock);
> > + return minor;
> > +}
> > +
> > +static void free_minor(struct ocxlpmem *ocxlpmem)
> > +{
> > + mutex_lock(&minors_idr_lock);
> > + idr_remove(&minors_idr, MINOR(ocxlpmem->dev.devt));
> > + mutex_unlock(&minors_idr_lock);
> > +}
> > +
> > +/**
> > + * free_ocxlpmem() - Free all members of an ocxlpmem struct
> > + * @ocxlpmem: the device struct to clear
> > + */
> > +static void free_ocxlpmem(struct ocxlpmem *ocxlpmem)
> > +{
> > + int rc;
> > +
> > + if (ocxlpmem->nvdimm_bus)
> > + nvdimm_bus_unregister(ocxlpmem->nvdimm_bus);
> > +
> > + free_minor(ocxlpmem);
> > +
> > + if (ocxlpmem->metadata_addr)
> > + devm_memunmap(&ocxlpmem->dev, ocxlpmem->metadata_addr);
> > +
> > + if (ocxlpmem->ocxl_context) {
> > + rc = ocxl_context_detach(ocxlpmem->ocxl_context);
> > + if (rc == -EBUSY)
> > + dev_warn(&ocxlpmem->dev, "Timeout detaching
> > ocxl context\n");
> > + else
> > + ocxl_context_free(ocxlpmem->ocxl_context);
> > +
> > + }
> > +
> > + if (ocxlpmem->ocxl_afu)
> > + ocxl_afu_put(ocxlpmem->ocxl_afu);
> > +
> > + if (ocxlpmem->ocxl_fn)
> > + ocxl_function_close(ocxlpmem->ocxl_fn);
> > +
> > + kfree(ocxlpmem);
> > +}
> > +
> > +/**
> > + * free_ocxlpmem_dev() - Free an OpenCAPI persistent memory device
> > + * @dev: The device struct
> > + */
> > +static void free_ocxlpmem_dev(struct device *dev)
> > +{
> > + struct ocxlpmem *ocxlpmem = container_of(dev, struct ocxlpmem,
> > dev);
> > +
> > + free_ocxlpmem(ocxlpmem);
> > +}
> > +
> > +/**
> > + * ocxlpmem_register() - Register an OpenCAPI pmem device with the
> > kernel
> > + * @ocxlpmem: the device metadata
> > + * Return: 0 on success, negative on failure
> > + */
> > +static int ocxlpmem_register(struct ocxlpmem *ocxlpmem)
> > +{
> > + int rc;
> > + int minor = allocate_minor(ocxlpmem);
> > +
> > + if (minor < 0)
> > + return minor;
> > +
> > + ocxlpmem->dev.release = free_ocxlpmem_dev;
> > + rc = dev_set_name(&ocxlpmem->dev, "ocxlpmem%d", minor);
> > + if (rc < 0)
> > + return rc;
> > +
> > + ocxlpmem->dev.devt = MKDEV(MAJOR(ocxlpmem_dev), minor);
> > + ocxlpmem->dev.class = ocxlpmem_class;
>
>
> This function, as well as allocate_minor() and free_minor() above
> reference resources (the IDR, the file class, ...) which are not
> initialized yet. The function file_init() is coming in a later patch.
>
Thanks, I caught this at runtime when I booted a kernel with (just)
this patch :) Fixed in v4.
>
>
> > + ocxlpmem->dev.parent = &ocxlpmem->pdev->dev;
> > +
> > + return device_register(&ocxlpmem->dev);
> > +}
> > +
> > +/**
> > + * ocxlpmem_remove() - Free an OpenCAPI persistent memory device
> > + * @pdev: the PCI device information struct
> > + */
> > +static void ocxlpmem_remove(struct pci_dev *pdev)
> > +{
> > + if (PCI_FUNC(pdev->devfn) == 0) {
> > + struct ocxlpmem_function0 *func0 =
> > pci_get_drvdata(pdev);
> > +
> > + if (func0) {
> > + ocxl_function_close(func0->ocxl_fn);
> > + func0->ocxl_fn = NULL;
> > + }
>
>
> The struct ocxlpmem_function0 allocated on probe() should be freed.
>
I've dropped struct as per the thread from Andrew Donellan.
>
>
> > + } else {
> > + struct ocxlpmem *ocxlpmem = pci_get_drvdata(pdev);
> > +
> > + if (ocxlpmem)
> > + device_unregister(&ocxlpmem->dev);
> > + }
> > +}
> > +
> > +/**
> > + * probe_function0() - Set up function 0 for an OpenCAPI
> > persistent memory device
> > + * This is important as it enables templates higher than 0 across
> > all other functions,
> > + * which in turn enables higher bandwidth accesses
> > + * @pdev: the PCI device information struct
> > + * Return: 0 on success, negative on failure
> > + */
> > +static int probe_function0(struct pci_dev *pdev)
> > +{
> > + struct ocxlpmem_function0 *func0 = NULL;
> > + struct ocxl_fn *fn;
> > +
> > + func0 = kzalloc(sizeof(*func0), GFP_KERNEL);
> > + if (!func0)
> > + return -ENOMEM;
> > +
> > + func0->pdev = pdev;
>
>
> Storing the struct pci_dev for function 0 appears to be useless.
>
Yup
>
>
> > + fn = ocxl_function_open(pdev);
> > + if (IS_ERR(fn)) {
> > + kfree(func0);
> > + dev_err(&pdev->dev, "failed to open OCXL function\n");
> > + return PTR_ERR(fn);
> > + }
> > + func0->ocxl_fn = fn;
> > +
> > + pci_set_drvdata(pdev, func0);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * probe() - Init an OpenCAPI persistent memory device
> > + * @pdev: the PCI device information struct
> > + * @ent: The entry from ocxlpmem_pci_tbl
> > + * Return: 0 on success, negative on failure
> > + */
> > +static int probe(struct pci_dev *pdev, const struct pci_device_id
> > *ent)
> > +{
> > + struct ocxlpmem *ocxlpmem;
> > + int rc;
> > +
> > + if (PCI_FUNC(pdev->devfn) == 0)
> > + return probe_function0(pdev);
> > + else if (PCI_FUNC(pdev->devfn) != 1)
> > + return 0;
> > +
> > + ocxlpmem = kzalloc(sizeof(*ocxlpmem), GFP_KERNEL);
> > + if (!ocxlpmem) {
> > + dev_err(&pdev->dev, "Could not allocate OpenCAPI
> > persistent memory metadata\n");
> > + rc = -ENOMEM;
> > + goto err;
> > + }
> > + ocxlpmem->pdev = pdev;
>
> We should probably call pci_dev_get() here if we store the struct
> pci_dev pointer. We could debate how useful it really is,
> considering
> we're registering a device, which will also take a reference, but it
> looks like the safe thing to do considering all those resources
> don't
> have exactly the same life cycle and it is standard practice to
> guarantee that we won't have a dangling pointer.
>
Ok
>
>
> > +
> > + pci_set_drvdata(pdev, ocxlpmem);
> > +
> > + ocxlpmem->ocxl_fn = ocxl_function_open(pdev);
> > + if (IS_ERR(ocxlpmem->ocxl_fn)) {
> > + kfree(ocxlpmem);
>
> ocxlpmem is freed...
>
>
> > + pci_set_drvdata(pdev, NULL);
> > + dev_err(&pdev->dev, "failed to open OCXL function\n");
> > + rc = PTR_ERR(ocxlpmem->ocxl_fn);
>
> ... and then referenced.
>
Ok
>
>
> > + goto err;
> > + }
> > +
> > + ocxlpmem->ocxl_afu = ocxl_function_fetch_afu(ocxlpmem->ocxl_fn,
> > 0);
> > + if (ocxlpmem->ocxl_afu == NULL) {
> > + dev_err(&pdev->dev, "Could not get OCXL AFU from
> > function\n");
>
> The error path here should match the above, to free struct ocxlpmem.
>
Yup, I've factored out err_unregiseterd to unify the error paths.
>
> > + rc = -ENXIO;
> > + goto err;
> > + }
> > +
> > + ocxl_afu_get(ocxlpmem->ocxl_afu);
> > +
> > + // Resources allocated below here are cleaned up in the release
> > handler
> > +
> > + rc = ocxlpmem_register(ocxlpmem);
> > + if (rc) {
> > + dev_err(&pdev->dev, "Could not register OpenCAPI
> > persistent memory device with the kernel\n");
> > + goto err;
> > + }
> > +
> > + rc = ocxl_context_alloc(&ocxlpmem->ocxl_context, ocxlpmem-
> > >ocxl_afu, NULL);
> > + if (rc) {
> > + dev_err(&pdev->dev, "Could not allocate OCXL
> > context\n");
> > + goto err;
> > + }
> > +
> > + rc = ocxl_context_attach(ocxlpmem->ocxl_context, 0, NULL);
> > + if (rc) {
> > + dev_err(&pdev->dev, "Could not attach ocxl context\n");
> > + goto err;
> > + }
> > +
> > + rc = register_lpc_mem(ocxlpmem);
> > + if (rc) {
> > + dev_err(&pdev->dev, "Could not register OpenCAPI
> > persistent memory with libnvdimm\n");
> > + goto err;
> > + }
> > +
> > + return 0;
> > +
> > +err:
> > + /*
> > + * Further cleanup is done in the release handler via
> > free_ocxlpmem()
> > + * This allows us to keep the character device live to handle
> > IOCTLs to
> > + * investigate issues if the card has an error
> > + */
>
> If we fail probe, we don't call device_unregister() and the data
> structures will never be freed. The comment seems to indicate it's
> done
> on purpose but that looks suprising and wrong. If we fail probe, the
> kernel thinks the driver it _not_ handling the device, so we need to
> exit probe() cleanly. We're not supposed to be able to make some
> debug
> ioctl calls. Once we fail probe, the kernel is free to do whatever
> it
> wants with the pci device. If you manage to extract some debug info
> during development, then fine, but it's not something we can rely on
> and
> upstream.
> If the card enters an error state after probe(), then we don't need
> that
> anyway. We have all the time in the world to call ioctl's, as long as
> we
> don't call the remove callback of the driver.
>
>
Ok
>
>
> > +
> > + dev_err(&pdev->dev,
> > + "Error detected, will not register OpenCAPI persistent
> > memory\n");
> > + return rc;
> > +}
> > +
> > +static struct pci_driver pci_driver = {
> > + .name = "ocxl-pmem",
> > + .id_table = ocxlpmem_pci_tbl,
> > + .probe = probe,
> > + .remove = ocxlpmem_remove,
> > + .shutdown = ocxlpmem_remove,
>
>
> nitpick: why doesn't the probe callback follow the same naming
> convention? It's all static and doesn't really matter, but...
I had dropped the prefix when I renamed from scm as it doesn't really
value-add, but clearly, I missed some :) I'll fix it.
>
> Fred
>
>
>
> > +};
> > +
> > +static int __init ocxlpmem_init(void)
> > +{
> > + int rc = 0;
> > +
> > + rc = pci_register_driver(&pci_driver);
> > + if (rc)
> > + return rc;
> > +
> > + return 0;
> > +}
> > +
> > +static void ocxlpmem_exit(void)
> > +{
> > + pci_unregister_driver(&pci_driver);
> > +}
> > +
> > +module_init(ocxlpmem_init);
> > +module_exit(ocxlpmem_exit);
> > +
> > +MODULE_DESCRIPTION("OpenCAPI Persistent Memory");
> > +MODULE_LICENSE("GPL");
> > diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> > b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> > new file mode 100644
> > index 000000000000..0faf3740e9b8
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> > @@ -0,0 +1,28 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +// Copyright 2019 IBM Corp.
> > +
> > +#include <linux/pci.h>
> > +#include <misc/ocxl.h>
> > +#include <linux/libnvdimm.h>
> > +#include <linux/mm.h>
> > +
> > +#define LABEL_AREA_SIZE (1UL << PA_SECTION_SHIFT)
> > +
> > +struct ocxlpmem_function0 {
> > + struct pci_dev *pdev;
> > + struct ocxl_fn *ocxl_fn;
> > +};
> > +
> > +struct ocxlpmem {
> > + struct device dev;
> > + struct pci_dev *pdev;
> > + struct ocxl_fn *ocxl_fn;
> > + struct nd_interleave_set nd_set;
> > + struct nvdimm_bus_descriptor bus_desc;
> > + struct nvdimm_bus *nvdimm_bus;
> > + struct ocxl_afu *ocxl_afu;
> > + struct ocxl_context *ocxl_context;
> > + void *metadata_addr;
> > + struct resource pmem_res;
> > + struct nd_region *nd_region;
> > +};
> >
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply
* Re: [PATCH v3 09/14] powerpc/xmon: Add initial support for prefixed instructions
From: Jordan Niethe @ 2020-02-28 0:37 UTC (permalink / raw)
To: Christophe Leroy
Cc: Alistair Popple, Balamuruhan S, linuxppc-dev, Nicholas Piggin,
Daniel Axtens
In-Reply-To: <f6db0844-bcd0-50af-5699-a89f10538ed0@c-s.fr>
On Thu, Feb 27, 2020 at 6:14 PM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
>
>
> Le 27/02/2020 à 01:11, Jordan Niethe a écrit :
> > On Wed, Feb 26, 2020 at 6:10 PM Nicholas Piggin <npiggin@gmail.com> wrote:
> >>
> >> Jordan Niethe's on February 26, 2020 2:07 pm:
> >>> A prefixed instruction is composed of a word prefix and a word suffix.
> >>> It does not make sense to be able to have a breakpoint on the suffix of
> >>> a prefixed instruction, so make this impossible.
> >>>
> >>> When leaving xmon_core() we check to see if we are currently at a
> >>> breakpoint. If this is the case, the breakpoint needs to be proceeded
> >>> from. Initially emulate_step() is tried, but if this fails then we need
> >>> to execute the saved instruction out of line. The NIP is set to the
> >>> address of bpt::instr[] for the current breakpoint. bpt::instr[]
> >>> contains the instruction replaced by the breakpoint, followed by a trap
> >>> instruction. After bpt::instr[0] is executed and we hit the trap we
> >>> enter back into xmon_bpt(). We know that if we got here and the offset
> >>> indicates we are at bpt::instr[1] then we have just executed out of line
> >>> so we can put the NIP back to the instruction after the breakpoint
> >>> location and continue on.
> >>>
> >>> Adding prefixed instructions complicates this as the bpt::instr[1] needs
> >>> to be used to hold the suffix. To deal with this make bpt::instr[] big
> >>> enough for three word instructions. bpt::instr[2] contains the trap,
> >>> and in the case of word instructions pad bpt::instr[1] with a noop.
> >>>
> >>> No support for disassembling prefixed instructions.
> >>>
> >>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> >>> ---
> >>> v2: Rename sufx to suffix
> >>> v3: - Just directly use PPC_INST_NOP
> >>> - Typo: plac -> place
> >>> - Rename read_inst() to mread_inst(). Do not have it call mread().
> >>> ---
> >>> arch/powerpc/xmon/xmon.c | 90 ++++++++++++++++++++++++++++++++++------
> >>> 1 file changed, 78 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> >>> index a673cf55641c..a73a35aa4a75 100644
> >>> --- a/arch/powerpc/xmon/xmon.c
> >>> +++ b/arch/powerpc/xmon/xmon.c
> >>> @@ -97,7 +97,8 @@ static long *xmon_fault_jmp[NR_CPUS];
> >>> /* Breakpoint stuff */
> >>> struct bpt {
> >>> unsigned long address;
> >>> - unsigned int instr[2];
> >>> + /* Prefixed instructions can not cross 64-byte boundaries */
> >>> + unsigned int instr[3] __aligned(64);
> >>
> >> This is pretty wild, I didn't realize xmon executes breakpoints out
> >> of line like this.
>
> Neither did I. That's problematic. Kernel data is mapped NX on some
> platforms.
>
> >>
> >> IMO the break point entries here should correspond with a range of
> >> reserved bytes in .text so we patch instructions into normal executable
> >> pages rather than .data.
> > Would it make sense to use vmalloc_exec() and use that like we are
> > going to do in kprobes()?
>
> As we are (already) doing in kprobes() you mean ?
Sorry for the confusion, I was mainly thinking of the patch that you
pointed out before:
https://patchwork.ozlabs.org/patch/1232619/
>
> In fact kprobes uses module_alloc(), and it works because kprobe depends
> on module. On some platforms (i.e. book3s/32) vmalloc space is marked NX
> in segment registers when CONFIG_MODULES is not set, see
> mmu_mark_initmem_nx(). On other ones the Instruction TLB miss exception
> does not manage misses at kernel addresses when CONFIG_MODULES is not
> selected.
>
> So if we want XMON to work at all time, we need to use some (linear)
> text address and use patch_instruction() to change it.
Thank you for the detailed clarification, I will do it like that.
>
> Christophe
>
> >>
> >> Anyway that's for patch.
> >>
> >> Thanks,
> >> Nick
^ permalink raw reply
* [PATCH v5 11/13] powerpc/ptrace: create ptrace_get_debugreg()
From: Christophe Leroy @ 2020-02-28 0:14 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, mikey
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1582848567.git.christophe.leroy@c-s.fr>
Create ptrace_get_debugreg() to handle PTRACE_GET_DEBUGREG and
reduce ifdef mess
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/kernel/ptrace/ptrace-adv.c | 9 +++++++++
arch/powerpc/kernel/ptrace/ptrace-decl.h | 2 ++
arch/powerpc/kernel/ptrace/ptrace-noadv.c | 13 +++++++++++++
arch/powerpc/kernel/ptrace/ptrace.c | 18 ++----------------
4 files changed, 26 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/kernel/ptrace/ptrace-adv.c b/arch/powerpc/kernel/ptrace/ptrace-adv.c
index eebcd41edc3d..c71bedd54a8b 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-adv.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-adv.c
@@ -56,6 +56,15 @@ void user_disable_single_step(struct task_struct *task)
clear_tsk_thread_flag(task, TIF_SINGLESTEP);
}
+int ptrace_get_debugreg(struct task_struct *child, unsigned long addr,
+ unsigned long __user *datalp)
+{
+ /* We only support one DABR and no IABRS at the moment */
+ if (addr > 0)
+ return -EINVAL;
+ return put_user(child->thread.debug.dac1, datalp);
+}
+
int ptrace_set_debugreg(struct task_struct *task, unsigned long addr, unsigned long data)
{
#ifdef CONFIG_HAVE_HW_BREAKPOINT
diff --git a/arch/powerpc/kernel/ptrace/ptrace-decl.h b/arch/powerpc/kernel/ptrace/ptrace-decl.h
index bdba09a87aea..4b4b6a1d508a 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-decl.h
+++ b/arch/powerpc/kernel/ptrace/ptrace-decl.h
@@ -176,6 +176,8 @@ int tm_cgpr32_set(struct task_struct *target, const struct user_regset *regset,
extern const struct user_regset_view user_ppc_native_view;
/* ptrace-(no)adv */
+int ptrace_get_debugreg(struct task_struct *child, unsigned long addr,
+ unsigned long __user *datalp);
int ptrace_set_debugreg(struct task_struct *task, unsigned long addr, unsigned long data);
long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_info);
long ppc_del_hwdebug(struct task_struct *child, long data);
diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
index cf05fadba0d5..9c1ef9c2ffd4 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
@@ -39,6 +39,19 @@ void user_disable_single_step(struct task_struct *task)
clear_tsk_thread_flag(task, TIF_SINGLESTEP);
}
+int ptrace_get_debugreg(struct task_struct *child, unsigned long addr,
+ unsigned long __user *datalp)
+{
+ unsigned long dabr_fake;
+
+ /* We only support one DABR and no IABRS at the moment */
+ if (addr > 0)
+ return -EINVAL;
+ dabr_fake = ((child->thread.hw_brk.address & (~HW_BRK_TYPE_DABR)) |
+ (child->thread.hw_brk.type & HW_BRK_TYPE_DABR));
+ return put_user(dabr_fake, datalp);
+}
+
int ptrace_set_debugreg(struct task_struct *task, unsigned long addr, unsigned long data)
{
#ifdef CONFIG_HAVE_HW_BREAKPOINT
diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
index 48e095e88a2f..d6e1a301d20e 100644
--- a/arch/powerpc/kernel/ptrace/ptrace.c
+++ b/arch/powerpc/kernel/ptrace/ptrace.c
@@ -191,23 +191,9 @@ long arch_ptrace(struct task_struct *child, long request,
break;
}
- case PTRACE_GET_DEBUGREG: {
-#ifndef CONFIG_PPC_ADV_DEBUG_REGS
- unsigned long dabr_fake;
-#endif
- ret = -EINVAL;
- /* We only support one DABR and no IABRS at the moment */
- if (addr > 0)
- break;
-#ifdef CONFIG_PPC_ADV_DEBUG_REGS
- ret = put_user(child->thread.debug.dac1, datalp);
-#else
- dabr_fake = ((child->thread.hw_brk.address & (~HW_BRK_TYPE_DABR)) |
- (child->thread.hw_brk.type & HW_BRK_TYPE_DABR));
- ret = put_user(dabr_fake, datalp);
-#endif
+ case PTRACE_GET_DEBUGREG:
+ ret = ptrace_get_debugreg(child, addr, datalp);
break;
- }
case PTRACE_SET_DEBUGREG:
ret = ptrace_set_debugreg(child, addr, data);
--
2.25.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox