powerpc: Add user-return-notifier support

PATCH In review Ananth N Mavinakayanahalli 3 participants 9 messages Aug 25, 2015 – Sep 8, 2015

Subsystems (3)

Documentation PowerPC KVM PowerPC

lore

Share & tools

lore

Ananth N Mavinakayanahalli

author

2015-08-25 05:41 UTC #0 lore raw

PATCH 1/1 powerpc: Add user-return-notifier support

Add user return notifier support for powerpc. Similar to x86, this feature keys off of the KVM Kconfig.

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

---
 Documentation/features/debug/user-ret-profiler/arch-support.txt |    2 +-
 arch/powerpc/Kconfig                                            |    1 +
 arch/powerpc/include/asm/thread_info.h                          |    2 ++
 arch/powerpc/kernel/process.c                                   |    4 ++++
 arch/powerpc/kernel/signal.c                                    |    4 ++++
 arch/powerpc/kvm/Kconfig                                        |    1 +
 6 files changed, 13 insertions(+), 1 deletion(-)

Documentation/features/debug/user-ret-profiler/arch-support.txt +1 -1

Index: linux-4.2-rc6/Documentation/features/debug/user-ret-profiler/arch-support.txt
===================================================================
@@ -27,7 +27,7 @@
     |       nios2: | TODO |
     |    openrisc: | TODO |
     |      parisc: | TODO |
-    |     powerpc: | TODO |
+    |     powerpc: |  ok  |
     |        s390: | TODO |
     |       score: | TODO |
     |          sh: | TODO |

arch/powerpc/Kconfig +1 -0

Index: linux-4.2-rc6/arch/powerpc/Kconfig
===================================================================
@@ -106,6 +106,7 @@ config PPC
 	select HAVE_ARCH_KGDB
 	select HAVE_KRETPROBES
 	select HAVE_ARCH_TRACEHOOK
+	select HAVE_USER_RETURN_NOTIFIER
 	select HAVE_MEMBLOCK
 	select HAVE_MEMBLOCK_NODE_MAP
 	select HAVE_DMA_ATTRS

arch/powerpc/include/asm/thread_info.h +2 -0

Index: linux-4.2-rc6/arch/powerpc/include/asm/thread_info.h
===================================================================
@@ -86,6 +86,7 @@ static inline struct thread_info *curren
 					   TIF_NEED_RESCHED */
 #define TIF_32BIT		4	/* 32 bit binary */
 #define TIF_RESTORE_TM		5	/* need to restore TM FP/VEC/VSX */
+#define TIF_USER_RETURN_NOTIFY	6	/* notify kernel of userspace return */
 #define TIF_SYSCALL_AUDIT	7	/* syscall auditing active */
 #define TIF_SINGLESTEP		8	/* singlestepping active */
 #define TIF_NOHZ		9	/* in adaptive nohz mode */

@@ -109,6 +110,7 @@ static inline struct thread_info *curren
 #define _TIF_POLLING_NRFLAG	(1<<TIF_POLLING_NRFLAG)
 #define _TIF_32BIT		(1<<TIF_32BIT)
 #define _TIF_RESTORE_TM		(1<<TIF_RESTORE_TM)
+#define _TIF_USER_RETURN_NOTIFY	(1<<TIF_USER_RETURN_NOTIFY)
 #define _TIF_SYSCALL_AUDIT	(1<<TIF_SYSCALL_AUDIT)
 #define _TIF_SINGLESTEP		(1<<TIF_SINGLESTEP)
 #define _TIF_SECCOMP		(1<<TIF_SECCOMP)

arch/powerpc/kernel/process.c +4 -0

Index: linux-4.2-rc6/arch/powerpc/kernel/process.c
===================================================================
@@ -38,6 +38,7 @@
 #include <linux/random.h>
 #include <linux/hw_breakpoint.h>
 #include <linux/uaccess.h>
+#include <linux/user-return-notifier.h>
 
 #include <asm/pgtable.h>
 #include <asm/io.h>

@@ -894,6 +895,9 @@ struct task_struct *__switch_to(struct t
 	}
 #endif /* CONFIG_PPC_BOOK3S_64 */
 
+	if (unlikely(task_thread_info(prev)->flags & _TIF_USER_RETURN_NOTIFY))
+		propagate_user_return_notify(prev, new);
+
 	return last;
 }

arch/powerpc/kernel/signal.c +4 -0

Index: linux-4.2-rc6/arch/powerpc/kernel/signal.c
===================================================================
@@ -14,6 +14,7 @@
 #include <linux/uprobes.h>
 #include <linux/key.h>
 #include <linux/context_tracking.h>
+#include <linux/user-return-notifier.h>
 #include <asm/hw_breakpoint.h>
 #include <asm/uaccess.h>
 #include <asm/unistd.h>

@@ -159,6 +160,9 @@ void do_notify_resume(struct pt_regs *re
 		tracehook_notify_resume(regs);
 	}
 
+	if (thread_info_flags & _TIF_USER_RETURN_NOTIFY)
+		fire_user_return_notifiers();
+
 	user_enter();
 }

arch/powerpc/kvm/Kconfig +1 -0

Index: linux-4.2-rc6/arch/powerpc/kvm/Kconfig
===================================================================
@@ -22,6 +22,7 @@ config KVM
 	select ANON_INODES
 	select HAVE_KVM_EVENTFD
 	select SRCU
+	select USER_RETURN_NOTIFIER
 
 config KVM_BOOK3S_HANDLER
 	bool

Michael Ellerman

reviewer

2015-08-31 10:35 UTC #1 lore raw

Quoted 2 lines · Ananth N Mavinakayanahalli

Add user return notifier support for powerpc. Similar to x86, this feature keys off of the KVM Kconfig.

Please flesh this out.

What is it, why do we want it, why is your implementation correct.

cheers

Ananth N Mavinakayanahalli

author

2015-09-01 06:42 UTC #2 lore raw

Quoted 7 lines · Michael Ellerman

Quoted 2 lines · Ananth N Mavinakayanahalli
Add user return notifier support for powerpc. Similar to x86, this feature keys off of the KVM Kconfig.
Please flesh this out.
What is it, why do we want it, why is your implementation correct.

The only current in-kernel user of the infrastructure is KVM on x86. It is useful for optimizations when MSR values can continue to be used between the host and guest. Commit log for 18863bdd60f8 upstream has a more complete explanation.

I do not know the inner details of the KVM implementation on Power, perhaps Paul/Gautham can comment on if a similar optimization will benefit Power systems too?

We could certainly make this a generic config option.. but I am yet to see a real usecase outside of the KVM thingy. We do use TIF_UPROBE for something very similar, though that needs to fire much before the outstanding signals get handled. The user-return notifier is the last thing executed before return to userspace -- the two cannot be merged.

Ananth

PS: I am not sure having an 'ok' against the Documentation/features/ for powerpc is a valid enough argument for this :-)

Scott Wood

2015-09-02 01:37 UTC #3 lore raw

Quoted 10 lines · Ananth N Mavinakayanahalli

Quoted 8 lines · Michael Ellerman
Quoted 3 lines · Ananth N Mavinakayanahalli
Add user return notifier support for powerpc. Similar to x86, this feature
keys off of the KVM Kconfig.
Please flesh this out.
What is it, why do we want it, why is your implementation correct.

Quoted 8 lines

The only current in-kernel user of the infrastructure is KVM on x86. It is useful for optimizations when MSR values can continue to be used between the host and guest. Commit log for 18863bdd60f8 upstream has a more complete explanation.
I do not know the inner details of the KVM implementation on Power, perhaps Paul/Gautham can comment on if a similar optimization will benefit Power systems too?

"MSR" is x86-specific terminology and is pretty vague. What specifically is the functionality being optimized, in terms of things that actually exist on PPC?

In any case, that commit log doesn't explain what user-return-notifier is or how it works, just that it's being used.

Quoted 10 lines

We could certainly make this a generic config option.. but I am yet to see a real usecase outside of the KVM thingy. We do use TIF_UPROBE for something very similar, though that needs to fire much before the outstanding signals get handled. The user-return notifier is the last thing executed before return to userspace -- the two cannot be merged.
Ananth
PS: I am not sure having an 'ok' against the Documentation/features/ for powerpc is a valid enough argument for this :-)

So how did you test this? What platforms did you test it on? What hardware support does it need?

-Scott

Ananth N Mavinakayanahalli

author

2015-09-02 02:38 UTC #4 lore raw

Quoted 20 lines · Scott Wood

Quoted 10 lines · Ananth N Mavinakayanahalli
Quoted 8 lines · Michael Ellerman
Quoted 3 lines · Ananth N Mavinakayanahalli
Add user return notifier support for powerpc. Similar to x86, this feature
keys off of the KVM Kconfig.
Please flesh this out.
What is it, why do we want it, why is your implementation correct.
Quoted 8 lines
The only current in-kernel user of the infrastructure is KVM on x86. It is useful for optimizations when MSR values can continue to be used between the host and guest. Commit log for 18863bdd60f8 upstream has a more complete explanation.
I do not know the inner details of the KVM implementation on Power, perhaps Paul/Gautham can comment on if a similar optimization will benefit Power systems too?

Quoted 6 lines

"MSR" is x86-specific terminology and is pretty vague. What specifically is the functionality being optimized, in terms of things that actually exist on PPC?
In any case, that commit log doesn't explain what user-return-notifier is or how it works, just that it's being used.

User return notifier is a simple mechanism to fire a one-shot custom handler in the context of the thread its registered against, just before it returns to userspace. It works by setting a TIF flag to indicate that a handler needs to run, and on the userspace return path (do_notify_resume()), this flag is checked to call any registered handlers.

Quoted 6 lines

Quoted 6 lines
We could certainly make this a generic config option.. but I am yet to see a real usecase outside of the KVM thingy. We do use TIF_UPROBE for something very similar, though that needs to fire much before the outstanding signals get handled. The user-return notifier is the last thing executed before return to userspace -- the two cannot be merged.

Quoted 7 lines

Quoted 4 lines
Ananth
PS: I am not sure having an 'ok' against the Documentation/features/ for powerpc is a valid enough argument for this :-)
So how did you test this? What platforms did you test it on? What hardware support does it need?

Simple kernel module below... Its fairly evident from the patch that the feature is a software only construct and no specific hardware support is needed. I've tested this on a P7.

Ananth

---
/* uret test module */

#include <linux/init.h>
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/user-return-notifier.h>

struct user_return_notifier urn;
static int count;

static void test_on_user_return(struct user_return_notifier *urn)

{
	count++;
}

static int init_urn(void)

{
	urn.on_user_return = test_on_user_return;
	user_return_notifier_register(&urn);
	printk("urn registered\n");
	return 0;
}

static void cleanup_urn(void)

{
	user_return_notifier_unregister(&urn);
	printk("urn unregistered; count = %d\n", count);
}

module_init(init_urn);
module_exit(cleanup_urn);
MODULE_LICENSE("GPL");

Scott Wood

2015-09-02 03:29 UTC #5 lore raw

Quoted 31 lines · Ananth N Mavinakayanahalli

Quoted 26 lines · Scott Wood
Quoted 10 lines · Ananth N Mavinakayanahalli
Quoted 8 lines · Michael Ellerman
Quoted 3 lines · Ananth N Mavinakayanahalli
Add user return notifier support for powerpc. Similar to x86, this feature
keys off of the KVM Kconfig.
Please flesh this out.
What is it, why do we want it, why is your implementation correct.
Quoted 8 lines
The only current in-kernel user of the infrastructure is KVM on x86. It is useful for optimizations when MSR values can continue to be used between the host and guest. Commit log for 18863bdd60f8 upstream has a more complete explanation.
I do not know the inner details of the KVM implementation on Power, perhaps Paul/Gautham can comment on if a similar optimization will benefit Power systems too?
"MSR" is x86-specific terminology and is pretty vague. What specifically is
the functionality being optimized, in terms of things that actually exist on
PPC?
Quoted 3 lines
In any case, that commit log doesn't explain what user-return-notifier is or
how it works, just that it's being used.

Quoted 8 lines

User return notifier is a simple mechanism to fire a one-shot custom handler in
the context of the thread its registered against, just before it returns to userspace. It works by setting a TIF flag to indicate that a handler needs to
run, and on the userspace return path (do_notify_resume()), this flag is checked
to call any registered handlers.

Thanks.

Quoted 11 lines

Quoted 11 lines
Quoted 10 lines
We could certainly make this a generic config option.. but I am yet to see a real usecase outside of the KVM thingy. We do use TIF_UPROBE for something very similar, though that needs to fire much before the outstanding signals get handled. The user-return notifier is the last thing executed before return to userspace -- the two cannot be merged.
Ananth
PS: I am not sure having an 'ok' against the Documentation/features/ for powerpc is a valid enough argument for this :-)

Quoted 7 lines

Quoted 3 lines
So how did you test this? What platforms did you test it on? What hardware
support does it need?
Simple kernel module below... Its fairly evident from the patch that the feature is a software only construct and no specific hardware support is needed.

It's evident now that I'm aware of what the purpose is... It wasn't 100%

clear before, at least from a quick glance, given that the explanation was

"look at this hardware-specific KVM patch", whether there was an assumption of something else going on -- and even things that are implemented in software often work differently on book3s versus book3e, ppc32 versus ppc64, etc.

Why is this selected by KVM on PPC if KVM on PPC doesn't use it? What is the user you're trying to enable?

Where is the "profiler" that Documentation/features/debug/user-ret- profiler/arch-support.txt hints at?

-Scott

Ananth N Mavinakayanahalli

author

2015-09-02 05:10 UTC #6 lore raw

Quoted 5 lines · Scott Wood

Quoted 4 lines · Ananth N Mavinakayanahalli
Quoted 3 lines · Scott Wood
Quoted 2 lines · Ananth N Mavinakayanahalli
Quoted 1 line · Michael Ellerman
On Tue, 2015-25-08 at 05:41:10 UTC, Ananth N Mavinakayanahalli wrote:

...

Quoted 19 lines

Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
We could certainly make this a generic config option.. but I am yet to see a real usecase outside of the KVM thingy. We do use TIF_UPROBE for something very similar, though that needs to fire much before the outstanding signals get handled. The user-return notifier is the last thing executed before return to userspace -- the two cannot be merged.
Ananth
PS: I am not sure having an 'ok' against the Documentation/features/ for powerpc is a valid enough argument for this :-)
Quoted 7 lines
Quoted 3 lines
So how did you test this? What platforms did you test it on? What hardware
support does it need?
Simple kernel module below... Its fairly evident from the patch that the feature is a software only construct and no specific hardware support is needed.

Quoted 8 lines

It's evident now that I'm aware of what the purpose is... It wasn't 100%
clear before, at least from a quick glance, given that the explanation was
"look at this hardware-specific KVM patch", whether there was an assumption of something else going on -- and even things that are implemented in software often work differently on book3s versus book3e, ppc32 versus ppc64, etc.

I meant the infrastructure is agnostic of the underlying Power platform. Sorry if I wasn't clear.

Quoted 2 lines

Why is this selected by KVM on PPC if KVM on PPC doesn't use it? What is the user you're trying to enable?

I copied Paul and Gautham to get their thoughts on whether it is something they could use on Power. At this time, apart from enabling the feature, I do not have a specific usecase for it. We can park this patch pending a real user on powerpc.

Quoted 2 lines

Where is the "profiler" that Documentation/features/debug/user-ret- profiler/arch-support.txt hints at?

I guess profiler is a misnomer. Not sure why it was named that and not a notifier.

Ananth

Michael Ellerman

reviewer

2015-09-08 09:24 UTC #7 lore raw

Quoted 9 lines · Ananth N Mavinakayanahalli

Quoted 2 lines · Scott Wood
Why is this selected by KVM on PPC if KVM on PPC doesn't use it? What is the user you're trying to enable?
I copied Paul and Gautham to get their thoughts on whether it is something they could use on Power. At this time, apart from enabling the feature, I do not have a specific usecase for it. We can park this patch pending a real user on powerpc.

OK, thanks for hashing that out guys. I think parking it is the best solution for now, we could merge it and disable it somehow, but that would be ugly and prone to breaking.

Do we want to do something like this?

cheers

Documentation/features/arch-support.txt +1 -0

index d22a1095e661..3e2cf6161cf4 100644
@@ -8,4 +8,5 @@ The meaning of entries in the tables is:
     | ok |  # feature supported by the architecture
     |TODO|  # feature not yet supported by the architecture
     | .. |  # feature cannot be supported by the hardware
+    |n/a |  # feature is not needed/wanted/meaningful for the architecture

Documentation/features/debug/user-ret-profiler/arch-support.txt +1 -1

index 44cc1ff3f603..669813180dc5 100644
@@ -27,7 +27,7 @@
     |       nios2: | TODO |
     |    openrisc: | TODO |
     |      parisc: | TODO |
-    |     powerpc: | TODO |
+    |     powerpc: | n/a  |    # http://lkml.kernel.org/r/[email protected]
     |        s390: | TODO |
     |       score: | TODO |
     |          sh: | TODO |

Ananth N Mavinakayanahalli

author

2015-09-08 10:51 UTC #8 lore raw

Quoted 11 lines · Michael Ellerman

Quoted 9 lines · Ananth N Mavinakayanahalli
Quoted 2 lines · Scott Wood
Why is this selected by KVM on PPC if KVM on PPC doesn't use it? What is the user you're trying to enable?
I copied Paul and Gautham to get their thoughts on whether it is something they could use on Power. At this time, apart from enabling the feature, I do not have a specific usecase for it. We can park this patch pending a real user on powerpc.

Quoted 5 lines

OK, thanks for hashing that out guys. I think parking it is the best solution for now, we could merge it and disable it somehow, but that would be ugly and prone to breaking.
Do we want to do something like this?

Yes. This looks fine -- folks who want it down the line know where to look.

Thanks,
Ananth

Quoted 28 lines

cheers

Documentation/features/arch-support.txt +1 -0

index d22a1095e661..3e2cf6161cf4 100644
@@ -8,4 +8,5 @@ The meaning of entries in the tables is:
     | ok |  # feature supported by the architecture
     |TODO|  # feature not yet supported by the architecture
     | .. |  # feature cannot be supported by the hardware
+    |n/a |  # feature is not needed/wanted/meaningful for the architecture

Documentation/features/debug/user-ret-profiler/arch-support.txt +1 -1

index 44cc1ff3f603..669813180dc5 100644
@@ -27,7 +27,7 @@
     |       nios2: | TODO |
     |    openrisc: | TODO |
     |      parisc: | TODO |
-    |     powerpc: | TODO |
+    |     powerpc: | n/a  |    # http://lkml.kernel.org/r/[email protected]
     |        s390: | TODO |
     |       score: | TODO |
     |          sh: | TODO |