Re: [PATCH RFC v8 05/24] arm64: Implement asm/kpkeys.h using POE

["David Hildenbrand (Arm)" <david@kernel.org>]

On 5/26/26 13:15, Kevin Brodsky wrote:
> Implement the kpkeys interface if CONFIG_ARM64_POE is enabled.
> The permissions for KPKEYS_PKEY_DEFAULT (pkey 0) are set to RWX as
> this pkey is also used for code mappings.
> 
> To allow <asm/kpkeys.h> to be included from assembly, also add
> appropriate #ifdef's to <asm/por.h>.
> 
> Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
> ---
>  arch/arm64/include/asm/kpkeys.h | 59 +++++++++++++++++++++++++++++++++++++++++
>  arch/arm64/include/asm/por.h    |  4 +++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kpkeys.h b/arch/arm64/include/asm/kpkeys.h
> new file mode 100644
> index 000000000000..4dbfeb3dfcfe
> --- /dev/null
> +++ b/arch/arm64/include/asm/kpkeys.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __ASM_KPKEYS_H
> +#define __ASM_KPKEYS_H
> +
> +#include <asm/barrier.h>
> +#include <asm/cpufeature.h>
> +#include <asm/por.h>
> +
> +#include <asm-generic/kpkeys.h>
> +
> +/*
> + * Equivalent to por_set_kpkeys_context(0, KPKEYS_CTX_DEFAULT), but can also be
> + * used in assembly.
> + */
> +#define POR_EL1_INIT	POR_ELx_PERM_PREP(KPKEYS_PKEY_DEFAULT, POE_RWX)

Okay, this matches

#define POR_EL0_INIT		POR_ELx_PERM_PREP(0, POE_RWX)

Is there a good reason we need KPKEYS_PKEY_DEFAULT at all (and not similarly
just hardcode it to 0)?

Just wondering, because apparently we didn't care about adding an indicator for
user space pkey 0.

> +
> +#ifndef __ASSEMBLY__
> +
> +static inline bool arch_supports_kpkeys(void)
> +{
> +	return system_supports_poe();
> +}
> +
> +#ifdef CONFIG_ARM64_POE
> +
> +static inline u64 por_set_kpkeys_context(u64 por, int ctx)
> +{
> +	por = por_elx_set_pkey_perms(por, KPKEYS_PKEY_DEFAULT, POE_RWX);
> +
> +	return por;

Why not

return por_elx_set_pkey_perms(por, KPKEYS_PKEY_DEFAULT, POE_RWX);

?

In light of API discussions, it would be nicer if arch_kpkeys_set_context()
would just return the old context. But that would mean that restoring the
context would require another read_sysreg_s(SYS_POR_EL1);

So instead of returning magic register values, that should be wrapped in some
arch state struct as mentioned as reply to a previous patch.

> +}
> +
> +static __always_inline void __kpkeys_set_pkey_reg_nosync(u64 pkey_reg)
> +{
> +	write_sysreg_s(pkey_reg, SYS_POR_EL1);
> +}
> +
> +static __always_inline u64 arch_kpkeys_set_context(int ctx)
> +{
> +	u64 prev_por = read_sysreg_s(SYS_POR_EL1);
> +	u64 new_por = por_set_kpkeys_context(prev_por, ctx);

Both can be const.

But maybe you just use a single "por" variable.

> +
> +	__kpkeys_set_pkey_reg_nosync(new_por);
> +	isb();
> +
> +	return prev_por;
> +}
> +
> +static __always_inline void arch_kpkeys_restore_pkey_reg(u64 pkey_reg)
> +{
> +	__kpkeys_set_pkey_reg_nosync(pkey_reg);
> +	isb();
Why is that isb() for both callers outside of the function? Do you expect
another user that doesn't need the isb?

-- 
Cheers,

David