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
