[PATCH RFC v8 01/24] mm: Introduce kpkeys

[Kevin Brodsky]

On 26/05/2026 15:17, Linus Walleij wrote:
> Hi Kevin,
>
> here is just a few drive-by review comments pertaining to the
> API which will be used for all architectures wanting to use
> pkeys.

Appreciated, thanks!

> [...]
>
>> Because each architecture implementing pkeys uses a different
>> representation for the pkey register, and may reserve certain pkeys
>> for specific uses, support for kpkeys must be explicitly indicated
>> by selecting ARCH_HAS_KPKEYS and defining the following functions in
>> <asm/kpkeys.h>, in addition to the macros provided in
>> <asm-generic/kpkeys.h>:
>>
>> - arch_kpkeys_set_context()
>> - arch_kpkeys_restore_pkey_reg()
>> - arch_supports_kpkeys()
>>
>> Signed-off-by: Kevin Brodsky <kevin.brodsky at arm.com>
> I'm following so far.
>
> Is there no need for something like
>
> ctx = arch_kpkeys_request_context(KEY_TYPE) ?
>
> I'm just thinking of the scenario that e.g. two architectures want
> to request the same type of key for some generic resource, but
> the underlying implementation is vastly different for the two
> architectures, so relying on hard-coded arch-specific constants
> may not work. I'm also thinking about archs supporting different
> versions/types of kpkeys in a multi-platform executable kernel
> that may execute on several different silicon with the same ISA.

I'm not sure I understand what potential limitations this is trying to
address. The only thing that is imposed by the framework is the value of
KPKEYS_CTX_*, which is purely symbolic. Everything else is up to the
architecture, including the value of each key (which could be determined
at runtime if needed) and the permissions associated to each context.

Conceptually a context is a set of permission overlays for all existing
keys, it doesn't have to be tied to just one key.

> Maybe this is too much upfront design, I understand it needs to
> be kept simple.
>
>> +#ifndef KPKEYS_PKEY_DEFAULT
>> +#define KPKEYS_PKEY_DEFAULT    0
>> +#endif
> (...)
>> +#define KPKEYS_CTX_DEFAULT     0
>> +
>> +#define KPKEYS_CTX_MIN         KPKEYS_CTX_DEFAULT
>> +#define KPKEYS_CTX_MAX         KPKEYS_CTX_DEFAULT
> I was thinking an enum:
>
> enum kpkey_type {
>     KPKEY_DEFAULT = 0,
>     KPKEY_MMU_TABLE = 1,
>    ....
> };
>
> since this is what is used later.
>
>> +/**
>> + * kpkeys_set_context() - switch kpkeys context
>> + * @ctx: the context to switch to
>> + *
>> + * Switches to specified kpkeys context. @ctx must be a compile-time
>> + * constant. The arch-specific pkey register will be updated accordingly, and
>> + * the original value returned.
>> + *
>> + * Return: the original pkey register value if the register was written to, or
>> + *         KPKEYS_PKEY_REG_INVAL otherwise (no write to the register was
>> + *         required).
>> + */
>> +static __always_inline u64 kpkeys_set_context(int ctx)
> Should ctx be unsigned here? I'm nor sure what a negativ context
> would mean.
> kpkeys_set_context(unsigned int ctx)

That's a good point, now that we say "context" and not "level" an enum
would be a better representation. I would directly use:

    u64 kpkeys_set_context(enum kpkeys_ctx ctx);

... unless we really need another layer of abstraction.

- Kevin

> And then I was thinking:
>
> unsigned int kpkeys_request_context(enum kpkey_type type) {}...
>
> + arch_ -prefixed version of the same.
>
> unsigned int ctx;
> ctx = kpkeys_request_context(KPKEY_MMU_TABLE);
> (...)
>
> Constant sinking in the compiler will optimize the arch
> function to a constant in the object code so it will still
> be fast just a bit more talkative in source form.
>
> Yours,
> Linus Walleij