Re: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure

From: Christophe Leroy
Date: Wed Aug 28 2024 - 01:47:04 EST


Hi,

Le 28/08/2024 à 05:17, 虞陆铭 a écrit :
Hi,

it appears the little feature might require a little bit more work to find its value of the patch.

Using the following debug module , some debugging shows the TIF_USER_RETURN_NOTIFY
bit is propagated in __switch_to among tasks , but USER_RETURN_NOTIFY call back seems to
be dropped somewhere on somone who carries the bit return to user space.
side notes:
there is an issue that the module symbols is not appended to /sys/kernel/debug/tracing/available_filter_functions
which should be sovled first to make it easier for further debuggig.

As far as I can see, user return notifier infrastructure was implemented in 2009 for KVM on x86, see https://lore.kernel.org/all/1253105134-8862-1-git-send-email-avi@xxxxxxxxxx/

Can you explain what is your usage of that infrastructure with your patch ? You are talking about debug, what's the added value, what is it used for ?

Thanks
Christophe


[root@localhost linux]# cat lib/user-return-test.c
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/container_of.h>
#include <linux/user-return-notifier.h>
#include <linux/delay.h>
#include <linux/kthread.h>
#include <linux/sched.h>

MODULE_LICENSE("GPL");


struct test_user_return {
struct user_return_notifier urn;
bool registered;
int urn_value_changed;
struct task_struct *worker;
};

static struct test_user_return __percpu *user_return_test;

static void test_user_return_cb(struct user_return_notifier *urn)
{
struct test_user_return *tur =
container_of(urn, struct test_user_return, urn);
unsigned long flags;

local_irq_save(flags);
tur->urn_value_changed++;
local_irq_restore(flags);
return;
}

static int test_user_return_worker(void *tur)
{
struct test_user_return *t;
t = (struct test_user_return *) tur;
preempt_disable();
user_return_notifier_register(&t->urn);
preempt_enable();
t->registered = true;
while (!kthread_should_stop()) {
static int err_rate = 0;

msleep (1000);
if (!test_thread_flag(TIF_USER_RETURN_NOTIFY) && (err_rate == 0)) {
pr_err("TIF_USER_RETURN_NOTIFY is lost");
err_rate++;
}
}
return 0;
}
static int init_test_user_return(void)
{
int r = 0;

user_return_test = alloc_percpu(struct test_user_return);
if (!user_return_test) {
pr_err("failed to allocate percpu test_user_return\n");
r = -ENOMEM;
goto exit;
}
{
unsigned int cpu;
struct task_struct *task;
struct test_user_return *tur;

for_each_online_cpu(cpu) {
tur = per_cpu_ptr(user_return_test, cpu);
if (!tur->registered) {
tur->urn.on_user_return = test_user_return_cb;
task = kthread_create(test_user_return_worker,
tur, "test_user_return");
if (IS_ERR(task))
pr_err("no test_user_return kthread created for cpu %d",cpu);
else {
tur->worker = task;
wake_up_process(task);
}
}
}
}

exit:
return r;
}
static void exit_test_user_return(void)
{
struct test_user_return *tur;
int i,ret=0;

for_each_online_cpu(i) {
tur = per_cpu_ptr(user_return_test, i);
if (tur->registered) {
pr_info("[cpu=%d, %d] ", i, tur->urn_value_changed);
user_return_notifier_unregister(&tur->urn);
tur->registered = false;
}
if (tur->worker) {
ret = kthread_stop(tur->worker);
if (ret)
pr_err("can't stop test_user_return kthread for cpu %d", i);
}
}
free_percpu(user_return_test);
return;
}

module_init(init_test_user_return);
module_exit(exit_test_user_return);

------------------ Original ------------------
From: "Christophe Leroy"<christophe.leroy@xxxxxxxxxx>;
Date: Tue, Feb 20, 2024 05:02 PM
To: "mpe"<mpe@xxxxxxxxxxxxxx>; "Aneesh Kumar K.V"<aneesh.kumar@xxxxxxxxxx>; "虞陆铭"<luming.yu@xxxxxxxxxxxx>; "linuxppc-dev"<linuxppc-dev@xxxxxxxxxxxxxxxx>; "linux-kernel"<linux-kernel@xxxxxxxxxxxxxxx>; "npiggin"<npiggin@xxxxxxxxx>;
Cc: "shenghui.qu@xxxxxxxxxxxx"<shenghui.qu@xxxxxxxxxxxx>; "dawei.li@xxxxxxxxxxxx"<dawei.li@xxxxxxxxxxxx>; "ke.zhao@xxxxxxxxxxxx"<ke.zhao@xxxxxxxxxxxx>; "luming.yu"<luming.yu@xxxxxxxxx>;
Subject: Re: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure




Le 20/02/2024 à 09:51, Christophe Leroy a écrit :


Le 19/12/2023 à 07:33, Michael Ellerman a écrit :
Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxx> writes:
Luming Yu <luming.yu@xxxxxxxxxxxx> writes:

Before we have powerpc to use the generic entry infrastructure,
the call to fire user return notifier is made temporarily in powerpc
entry code.


It is still not clear what will be registered as user return notifier.
Can you summarize that here?

fire_user_return_notifiers() is defined in kernel/user-return-notifier.c

That's built when CONFIG_USER_RETURN_NOTIFIER=y.

That is not user selectable, it's only enabled by:

arch/x86/kvm/Kconfig: select USER_RETURN_NOTIFIER

So it looks to me like (currently) it's always a nop and does nothing.

Which makes me wonder what the point of wiring this feature up is :)
Maybe it's needed for some other feature I don't know about?

Arguably we could just enable it because we can, and it currently does
nothing so it's unlikely to break anything. But that also makes it
impossible to test the implementation is correct, and runs the risk that
one day in the future when it does get enabled only then do we discover
it doesn't work.

Opened an "issue" for the day we need it:
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FKSPP%2Flinux%2Fissues%2F348&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Cd9e9b6315413430cfba108dcc7100633%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638604118862628419%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=iIYQTodb9zrGdfmzvnZrIVZ%2BKh2qZjMjT29ddkUpGIw%3D&reserved=0

Correct one is https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flinuxppc%2Fissues%2Fissues%2F477&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Cd9e9b6315413430cfba108dcc7100633%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638604118862637095%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=aJRVdRWu%2F7NQ13jQ6yFLBynXIIfPPPQ3nS4FxiXGNyw%3D&reserved=0