Re: [PATCH 1/4] arm64: signal: Preserve POR_EL0 if poe_context is missing

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

On 23/04/2026 14:41, Will Deacon wrote:
> On Wed, Apr 22, 2026 at 04:55:05PM +0200, Kevin Brodsky wrote:
>> On 22/04/2026 14:19, Will Deacon wrote:
>>>> @@ -74,8 +76,12 @@ struct rt_sigframe_user_layout {
>>>>   * This state needs to be carefully managed to ensure that it doesn't cause
>>>>   * uaccess to fail when setting up the signal frame, and the signal handler
>>>>   * itself also expects a well-defined state when entered.
>>>> + *
>>>> + * The valid_fields member is a bitfield (see UA_STATE_HAS_*), specifying which
>>>> + * of the remaining fields is valid (has been set to a value).
>>>>   */
>>>>  struct user_access_state {
>>>> +	unsigned int valid_fields;
>>>>  	u64 por_el0;
>>>>  };
>>> Do you think it would be worth adding some accessors to make it easier
>>> to keep the flags in sync? For example:
>>>
>>> /* Stores por_el0 into uas->por_el0 and sets UA_STATE_HAS_POR_EL0 */
>>> void set_ua_state_por_el0(struct user_access_state *uas, u64 por_el0);
>>>
>>> /*
>>>  * If UA_STATE_HAS_POR_EL0, *por_el0 = uas->por_el0 and return 0.
>>>  * Otherwise, return -ENOENT.
>>>  */
>>> int get_ua_state_por_el0(struct user_access_state *uas, u64 *por_el0);
>>>
>>> WDYT?
>> I did get a feeling having helpers would be a good idea. I wonder if
>> getters/setters aren't a bit overkill though, as they make accesses to
>> the struct more cumbersome and we'd need a pair for every member (unless
>> we use some macro magic).
> We only have one struct member, so it's probably fine for now, and we
> could group related members together in sub-structures to help in future.
> But it's up to you -- I don't feel strongly about it, but requiring the
> caller to update the flag manually is going to be a bug magnet.
>
>> Maybe it would be sufficient to have say
>> ua_state_has_field(POR_EL0) to check if the bit is set, and
>> ua_state_set_field_valid(POR_EL0) to set the bit?
> I don't think that really helps with my concern. I'd like to avoid callers
> having to remember to deal with the flags when they update the data.

Got it. I can't think of a better way to do this so I'll just go ahead
with your suggestion, and prefix all members of the struct with __ to
make it clear they shouldn't be accessed directly. The macro magic can
wait until we have more than one struct member ;)

I've also reconsidered setup_sigframe() and realised using valid_fields
there is not a great idea, because the record is allocated in
setup_sigframe_layout() based on system_supports_poe(), and leaving a
record uninitialised would be bad™. I'll remove that change and I'll
have preserve_poe_context() fail with a WARN_ON() if
get_ua_state_por_el0() somehow returns an error.

- Kevin