Re: [PATCH RFC v9 01/25] mm: Introduce kpkeys

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

On 27/08/2026 20:00, David Hildenbrand (Arm) wrote:
>> [...]
>>
>> +/**
>> + * kpkeys_enter_context() - enter a kpkeys context
>> + * @ctx: the context to switch to
>> + *
>> + * Enters the specified kpkeys context. @ctx must be a compile-time constant.
>> + *
>> + * Return: state to be passed to kpkeys_leave_context().
>> + */
>> +static __always_inline
>> +struct kpkeys_state kpkeys_enter_context(enum kpkeys_ctx ctx)
>> +{
>> +	BUILD_BUG_ON_MSG(!__builtin_constant_p(ctx),
>> +			 "kpkeys_enter_context() only takes constant values");
>> +	BUILD_BUG_ON_MSG(ctx < 0 || ctx >= KPKEYS_CTX_COUNT,
>> +			 "Invalid value passed to kpkeys_enter_context()");
>> +
>> +	return arch_kpkeys_enter_context(ctx);
>> +}
>> +
>> +/**
>> + * kpkeys_leave_context() - leave a kpkeys context
>> + * @state: state returned by kpkeys_enter_context()
>> + *
>> + * Restores the state saved when entering a kpkeys context. If no context was
>> + * entered, this function does nothing.
>> + */
>> +static __always_inline
>> +void kpkeys_leave_context(const struct kpkeys_state *state)
>> +{
>> +	if (state->entered_context)
>> +		arch_kpkeys_leave_context(state);
> state->entered_context is a common code variable, but it's not set by
> commoncode. Is there a reason?

Not a good one, agreed the asymmetry isn't great.

> IOW, should arch_kpkeys_enter_context() only return the arch parts, and
> entered_context would be set in common code?
>
> Also, should entering bail out if the context was already entered.
>
> Last but not least, when would we expect to call kpkeys_leave_context() but the
> context was not entered?

It may be worth clarifying that this is not the same situation as lazy
MMU mode, where the state is thread-global. Here the state is supposed
to be on the stack and you should never have nesting or unmatched
enter/leave calls for a given kpkeys_state. (I could certainly add some
VM_WARN_ON_ONCE() to check these invariants, like in the lazy MMU API.)

The other difference is that only arch code can decide whether we need
to enter that state, as this is based on the value of the
(arch-specific) pkeys register. 

> If this is really arch-specific stuff, probably it should go entirely into arch
> doe. If this is common code stuff, likely it should be maintained entirely in
> common code.

The decision is made by the arch, but I think all architectures would
want this behaviour. So we could have:

    bool arch_kpkeys_enter_context(enum kpkeys_ctx ctx, struct
arch_kpkeys_state *arch_state);
    void arch_kpkeys_leave_context(const struct arch_kpkeys_state
*arch_state);

It's a little less elegant because the state to be set now needs to be
passed as an extra argument, but maybe that's better encapsulation. It
also has the advantage of removing the dependency on struct kpkeys_state
in <asm/kpkeys.h>, so we could get potentially get rid of the separate
<linux/kpkeys_types.h>.

The alternative is to make the entire struct kpkeys_state arch-specific
and move all the handling to the arch helpers. Currently the difference
is academic, but when other architectures implement the interface this
could lead to undesirable discrepancies.

> Overall this looks much cleaner to me compared to what I reviewed the last time
> (was that v8? I don't remember :D )

It was indeed RFC v8, and thanks :D

- Kevin