2023-12-18 03:14:21

by Luming Yu

[permalink] [raw]
Subject: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure

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

Signed-off-by: Luming Yu <[email protected]>
---
arch/powerpc/kernel/interrupt.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index c4f6d3c69ba9..7fe704946e96 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -19,6 +19,7 @@
#include <asm/time.h>
#include <asm/tm.h>
#include <asm/unistd.h>
+#include <asm/entry-common.h>

#if defined(CONFIG_PPC_ADV_DEBUG_REGS) && defined(CONFIG_PPC32)
unsigned long global_dbcr0[NR_CPUS];
@@ -245,6 +246,8 @@ interrupt_exit_user_prepare_main(unsigned long ret, struct pt_regs *regs)
/* Restore user access locks last */
kuap_user_restore(regs);

+ arch_exit_to_user_mode_prepare(regs, ti_flags);
+
return ret;
}

--
2.42.0.windows.2



2023-12-18 09:25:11

by Aneesh Kumar K.V

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

Luming Yu <[email protected]> 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?

>
> Signed-off-by: Luming Yu <[email protected]>
> ---
> arch/powerpc/kernel/interrupt.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index c4f6d3c69ba9..7fe704946e96 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -19,6 +19,7 @@
> #include <asm/time.h>
> #include <asm/tm.h>
> #include <asm/unistd.h>
> +#include <asm/entry-common.h>
>
> #if defined(CONFIG_PPC_ADV_DEBUG_REGS) && defined(CONFIG_PPC32)
> unsigned long global_dbcr0[NR_CPUS];
> @@ -245,6 +246,8 @@ interrupt_exit_user_prepare_main(unsigned long ret, struct pt_regs *regs)
> /* Restore user access locks last */
> kuap_user_restore(regs);
>
> + arch_exit_to_user_mode_prepare(regs, ti_flags);
> +
>

That will run the notifier with user AMR/IAMR values.

> return ret;
> }
>
> --
> 2.42.0.windows.2

2023-12-19 06:34:00

by Michael Ellerman

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

Aneesh Kumar K.V <[email protected]> writes:
> Luming Yu <[email protected]> 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.

cheers

2024-02-20 08:57:45

by Christophe Leroy

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



Le 19/12/2023 à 07:33, Michael Ellerman a écrit :
> Aneesh Kumar K.V <[email protected]> writes:
>> Luming Yu <[email protected]> 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://github.com/KSPP/linux/issues/348

2024-02-20 09:03:29

by Christophe Leroy

[permalink] [raw]
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 <[email protected]> writes:
>>> Luming Yu <[email protected]> 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://github.com/KSPP/linux/issues/348

Correct one is https://github.com/linuxppc/issues/issues/477