On Mon Jun 5, 2023 at 9:04 PM AEST, Christophe Leroy wrote:
> All but book3s/64 use a static branch key for disabling kuap.
> book3s/64 uses a memory feature.
>
> Refactor all targets except book3s/64.
>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> arch/powerpc/include/asm/book3s/32/kup.h | 7 -------
> arch/powerpc/include/asm/book3s/64/kup.h | 1 +
> arch/powerpc/include/asm/kup.h | 15 +++++++++++++++
> arch/powerpc/include/asm/nohash/32/kup-8xx.h | 7 -------
> arch/powerpc/include/asm/nohash/kup-booke.h | 7 -------
> arch/powerpc/mm/book3s32/kuap.c | 3 ---
> arch/powerpc/mm/init-common.c | 3 +++
> arch/powerpc/mm/nohash/kup.c | 3 ---
> 8 files changed, 19 insertions(+), 27 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
> index 466a19cfb4df..8da9997a67ba 100644
> --- a/arch/powerpc/include/asm/book3s/32/kup.h
> +++ b/arch/powerpc/include/asm/book3s/32/kup.h
> @@ -11,8 +11,6 @@
>
> #include <linux/jump_label.h>
>
> -extern struct static_key_false disable_kuap_key;
> -
> static __always_inline bool kuep_is_disabled(void)
> {
> return !IS_ENABLED(CONFIG_PPC_KUEP);
> @@ -25,11 +23,6 @@ static __always_inline bool kuep_is_disabled(void)
> #define KUAP_NONE (~0UL)
> #define KUAP_ALL (~1UL)
>
> -static __always_inline bool kuap_is_disabled(void)
> -{
> - return static_branch_unlikely(&disable_kuap_key);
> -}
> -
> static inline void kuap_lock_one(unsigned long addr)
> {
> mtsr(mfsr(addr) | SR_KS, addr);
> diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
> index 1b0215ff3710..f8b8e93c488c 100644
> --- a/arch/powerpc/include/asm/book3s/64/kup.h
> +++ b/arch/powerpc/include/asm/book3s/64/kup.h
> @@ -233,6 +233,7 @@ static __always_inline bool kuap_is_disabled(void)
> {
> return !mmu_has_feature(MMU_FTR_BOOK3S_KUAP);
> }
> +#define kuap_is_disabled kuap_is_disabled
Is there any point to doing this pattern since the code is in places
that have ifdef PPC6 S etc?
> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
> index 119ef491f797..74e140b1efef 100644
> --- a/arch/powerpc/mm/init-common.c
> +++ b/arch/powerpc/mm/init-common.c
> @@ -32,6 +32,9 @@ EXPORT_SYMBOL_GPL(kernstart_virt_addr);
> bool disable_kuep = !IS_ENABLED(CONFIG_PPC_KUEP);
> bool disable_kuap = !IS_ENABLED(CONFIG_PPC_KUAP);
>
> +struct static_key_false disable_kuap_key;
> +EXPORT_SYMBOL(disable_kuap_key);
> +
That's going to define it on 64s?
Nice refactoring though.
Thanks,
Nick
Le 06/06/2023 à 11:16, Nicholas Piggin a écrit :
> On Mon Jun 5, 2023 at 9:04 PM AEST, Christophe Leroy wrote:
>> All but book3s/64 use a static branch key for disabling kuap.
>> book3s/64 uses a memory feature.
>>
>> Refactor all targets except book3s/64.
>>
>> Signed-off-by: Christophe Leroy <[email protected]>
>> ---
>> arch/powerpc/include/asm/book3s/32/kup.h | 7 -------
>> arch/powerpc/include/asm/book3s/64/kup.h | 1 +
>> arch/powerpc/include/asm/kup.h | 15 +++++++++++++++
>> arch/powerpc/include/asm/nohash/32/kup-8xx.h | 7 -------
>> arch/powerpc/include/asm/nohash/kup-booke.h | 7 -------
>> arch/powerpc/mm/book3s32/kuap.c | 3 ---
>> arch/powerpc/mm/init-common.c | 3 +++
>> arch/powerpc/mm/nohash/kup.c | 3 ---
>> 8 files changed, 19 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
>> index 466a19cfb4df..8da9997a67ba 100644
>> --- a/arch/powerpc/include/asm/book3s/32/kup.h
>> +++ b/arch/powerpc/include/asm/book3s/32/kup.h
>> @@ -11,8 +11,6 @@
>>
>> #include <linux/jump_label.h>
>>
>> -extern struct static_key_false disable_kuap_key;
>> -
>> static __always_inline bool kuep_is_disabled(void)
>> {
>> return !IS_ENABLED(CONFIG_PPC_KUEP);
>> @@ -25,11 +23,6 @@ static __always_inline bool kuep_is_disabled(void)
>> #define KUAP_NONE (~0UL)
>> #define KUAP_ALL (~1UL)
>>
>> -static __always_inline bool kuap_is_disabled(void)
>> -{
>> - return static_branch_unlikely(&disable_kuap_key);
>> -}
>> -
>> static inline void kuap_lock_one(unsigned long addr)
>> {
>> mtsr(mfsr(addr) | SR_KS, addr);
>> diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
>> index 1b0215ff3710..f8b8e93c488c 100644
>> --- a/arch/powerpc/include/asm/book3s/64/kup.h
>> +++ b/arch/powerpc/include/asm/book3s/64/kup.h
>> @@ -233,6 +233,7 @@ static __always_inline bool kuap_is_disabled(void)
>> {
>> return !mmu_has_feature(MMU_FTR_BOOK3S_KUAP);
>> }
>> +#define kuap_is_disabled kuap_is_disabled
>
> Is there any point to doing this pattern since the code is in places
> that have ifdef PPC6 S etc?
I'm not sure what you have in mind.
There is a default kuap_is_disabled() in arch/powerpc/include/asm/kup.h
For 64s we want the version from
arch/powerpc/include/asm/book3s/64/kup.h instead of the default one.
And kuap_is_disabled() is used in some common code in
arch/powerpc/include/asm/kup.h, not only in the code encloded by #ifndef 64s
>
>> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
>> index 119ef491f797..74e140b1efef 100644
>> --- a/arch/powerpc/mm/init-common.c
>> +++ b/arch/powerpc/mm/init-common.c
>> @@ -32,6 +32,9 @@ EXPORT_SYMBOL_GPL(kernstart_virt_addr);
>> bool disable_kuep = !IS_ENABLED(CONFIG_PPC_KUEP);
>> bool disable_kuap = !IS_ENABLED(CONFIG_PPC_KUAP);
>>
>> +struct static_key_false disable_kuap_key;
>> +EXPORT_SYMBOL(disable_kuap_key);
>> +
>
> That's going to define it on 64s?
Is that a problem at all ?
By the way I was thinking about also using that key on 64s, will look at
it as a second step.
Thanks for the review
Christophe
>
> Nice refactoring though.
>
> Thanks,
> Nick