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

[Kevin Brodsky <kevin.brodsky@arm.com>]

On 22/06/2026 15:35, David Hildenbrand (Arm) wrote:
> 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)?

I did wonder about that. As per the discussion on patch 1 I don't see
that value ever changing, so I'm fine with hardcoding 0 if that feels
more consistent/intuitive.

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

It's never too late to add a constant for userspace as well, but of
course since it's been ABI for so long the value isn't going to change.

>> +
>> +#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);
>
> ?

Because of patch 17, I wanted to minimise the diff. Happy to change it
if that feels less surprising.

> 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);

Correct, that's what I wanted to avoid.

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

Agreed.

>> +}
>> +
>> +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.

Sure.

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

Not sure how? prev_por is returned and new_por is based on its value.

>> +
>> +	__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?

Outside of __kpkeys_set_pkey_reg_nosync() that is? That function is also
used by patch 22, which doesn't want the ISB because we already have an
ISB somewhere else. The naming follows that of __set_pte_nosync().

- Kevin