Re: [RFC PATCH v1 08/16] unwind_user: Enable archs that save RA/FP in other registers

[Jens Remus <jremus@linux.ibm.com>]

On 17.07.2025 04:01, Josh Poimboeuf wrote:
> On Thu, Jul 10, 2025 at 06:35:14PM +0200, Jens Remus wrote:
>> +#ifndef unwind_user_get_reg
>> +
>> +/**
>> + * generic_unwind_user_get_reg - Get register value.
>> + * @val: Register value.
>> + * @regnum: DWARF register number to obtain the value from.
>> + *
>> + * Returns zero if successful. Otherwise -EINVAL.
>> + */
>> +static inline int generic_unwind_user_get_reg(unsigned long *val, int regnum)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +#define unwind_user_get_reg generic_unwind_user_get_reg
>> +
>> +#endif /* !unwind_user_get_reg */
> 
> I believe the traditional way to do this is to give the function the
> same name as the define:
> 
> #ifndef unwind_user_get_reg
> static inline int unwind_user_get_reg(unsigned long *val, int regnum)
> {
> 	return -EINVAL;
> }
> #define unwind_user_get_reg unwind_user_get_reg
> #endif

Thanks!  I will use use suggestion.

>> +/**
>> + * generic_sframe_set_frame_reginfo - Populate info to unwind FP/RA register
>> + * from SFrame offset.
>> + * @reginfo: Unwind info for FP/RA register.
>> + * @offset: SFrame offset value.
>> + *
>> + * A non-zero offset value denotes a stack offset from CFA and indicates
>> + * that the register is saved on the stack. A zero offset value indicates
>> + * that the register is not saved.
>> + */
>> +static inline void generic_sframe_set_frame_reginfo(
>> +	struct unwind_user_reginfo *reginfo,
>> +	s32 offset)
>> +{
>> +	if (offset) {
>> +		reginfo->loc = UNWIND_USER_LOC_STACK;
>> +		reginfo->frame_off = offset;
>> +	} else {
>> +		reginfo->loc = UNWIND_USER_LOC_NONE;
>> +	}
>> +}
> 
> This just inits the reginfo struct, can we call it sframe_init_reginfo()?
> 
> Also the function comment seems completely superfluous as the function
> is completely obvious.
> 
> Also the signature should match kernel style, something like:
> 
> static inline void
> sframe_init_reginfo(struct unwind_user_reginfo *reginfo, s32 offset)

Ditto.

>> @@ -98,26 +98,57 @@ static int unwind_user_next(struct unwind_user_state *state)
>>  
>>  
>>  	/* Get the Return Address (RA) */
>> -	if (frame->ra_off) {
>> +	switch (frame->ra.loc) {
>> +	case UNWIND_USER_LOC_NONE:
>> +		if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost)
>> +			goto done;
>> +		ra = user_return_address(task_pt_regs(current));
>> +		break;
>> +	case UNWIND_USER_LOC_STACK:
>> +		if (!frame->ra.frame_off)
>> +			goto done;
>>  		/* Make sure that the address is word aligned */
>>  		shift = sizeof(long) == 4 || compat_fp_state(state) ? 2 : 3;
>> -		if ((cfa + frame->ra_off) & ((1 << shift) - 1))
>> +		if ((cfa + frame->ra.frame_off) & ((1 << shift) - 1))
>>  			goto done;
>> -		if (unwind_get_user_long(ra, cfa + frame->ra_off, state))
>> +		if (unwind_get_user_long(ra, cfa + frame->ra.frame_off, state))
>>  			goto done;
>> -	} else {
>> -		if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost)
>> +		break;
>> +	case UNWIND_USER_LOC_REG:
>> +		if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_LOC_REG) || !topmost)
>>  			goto done;
>> -		ra = user_return_address(task_pt_regs(current));
>> +		if (unwind_user_get_reg(&ra, frame->ra.regnum))
>> +			goto done;
>> +		break;
>> +	default:
>> +		WARN_ON_ONCE(1);
>> +		goto done;
> 
> The default case will never happen, can we make it a BUG()?

Whatever Steve and you agree on.  I am new to Kernel development.

>>  	}
>>  
>>  	/* Get the Frame Pointer (FP) */
>> -	if (frame->fp_off && unwind_get_user_long(fp, cfa + frame->fp_off, state))
>> +	switch (frame->fp.loc) {
>> +	case UNWIND_USER_LOC_NONE:
>> +		break;
> 
> The UNWIND_USER_LOC_NONE behavior is different here compared to above.

See my comments below.

> Do we also need UNWIND_USER_LOC_PT_REGS?

Sorry, I cannot follow.  Do you suggest to rename UNWIND_USER_LOC_REG to
UNWIND_USER_LOC_PT_REGS?

>> +	case UNWIND_USER_LOC_STACK:
>> +		if (!frame->fp.frame_off)
>> +			goto done;
>> +		if (unwind_get_user_long(fp, cfa + frame->fp.frame_off, state))
>> +			goto done;
>> +		break;
>> +	case UNWIND_USER_LOC_REG:
>> +		if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_LOC_REG) || !topmost)
>> +			goto done;
> 
> The topmost checking is *really* getting cumbersome, I do hope we can
> get rid of that.

Restoring from arbitrary registers is only valid in the topmost frame,
as their values (i.e. task_pt_regs(current)) are only available there.
For other frames only SP, FP, and RA register values are available.

I think this test makes sense.  Is this test really that expensive?

>> +		if (unwind_user_get_reg(&fp, frame->fp.regnum))
>> +			goto done;
>> +		break;
>> +	default:
>> +		WARN_ON_ONCE(1);
>>  		goto done;
>> +	}
> 
> BUG(1) here as well.

Same as for other WARN_ON_ONCE() vs. BUG().

>>  	state->ip = ra;
>>  	state->sp = sp;
>> -	if (frame->fp_off)
>> +	if (frame->fp.loc != UNWIND_USER_LOC_NONE)
>>  		state->fp = fp;
> 
> Instead of the extra conditional here, can fp be initialized to zero?
> Either at the top of the function or in the RA switch statement?

No.  But fp could be initialized to state->fp, so that when it is not
saved and thus not restored it keeps it previous value.

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@de.ibm.com

IBM

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/