Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Julien Thierry <jthierry@redhat.com>]

On 4/2/20 9:17 AM, Peter Zijlstra wrote:
> On Thu, Apr 02, 2020 at 09:50:36AM +0200, Peter Zijlstra wrote:
>> On Thu, Apr 02, 2020 at 07:41:46AM +0100, Julien Thierry wrote:
> 
>>> Also, instead of adding a special "arch_exception_frame_size", I could
>>> suggest:
>>> - Picking this patch [1] from a completely arbitrary source
>>> - Getting rid of INSN_STACK type, any instruction could then include stack
>>> ops on top of their existing semantics, they can just have an empty list if
>>> they don't touch SP/BP
>>> - x86 decoder adds a stack_op to the iret to modify the stack pointer by the
>>> right amount
>>
>> That's not the worst idea, lemme try that.
> 
> Something like so then?
> 

Yes, you could even remove INSN_STACK from insn_type and just always 
call handle_insn_ops() before the switch statement on insn->type. If the 
list is empty it does nothing.
This way you wouldn't need to call it for the INSN_EXCEPTION_RETURN 
case, and any type of instructions could use stack_ops.


And the other suggestion is my other email was that you don't even need 
to add INSN_EXCEPTION_RETURN. You can keep IRET as INSN_CONTEXT_SWITCH 
by default and x86 decoder lookups the symbol conaining an iret. If it's 
a function symbol, it can just set the type to INSN_OTHER so that it 
caries on to the next instruction after having handled the stack_op.

And everything fits under tools/objtool/arch/x86 :) .

Or is it too far-fetch'd?


> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -738,7 +738,6 @@ static inline void sync_core(void)
>   	unsigned int tmp;
>   
>   	asm volatile (
> -		UNWIND_HINT_SAVE
>   		"mov %%ss, %0\n\t"
>   		"pushq %q0\n\t"
>   		"pushq %%rsp\n\t"
> @@ -748,7 +747,6 @@ static inline void sync_core(void)
>   		"pushq %q0\n\t"
>   		"pushq $1f\n\t"
>   		"iretq\n\t"
> -		UNWIND_HINT_RESTORE
>   		"1:"
>   		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
>   #endif
> --- a/tools/objtool/arch.h
> +++ b/tools/objtool/arch.h
> @@ -19,6 +19,7 @@ enum insn_type {
>   	INSN_CALL,
>   	INSN_CALL_DYNAMIC,
>   	INSN_RETURN,
> +	INSN_EXCEPTION_RETURN,
>   	INSN_CONTEXT_SWITCH,
>   	INSN_STACK,
>   	INSN_BUG,
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -435,9 +435,19 @@ int arch_decode_instruction(struct elf *
>   		*type = INSN_RETURN;
>   		break;
>   
> +	case 0xcf: /* iret */
> +		*type = INSN_EXCEPTION_RETURN;
> +
> +		/* add $40, %rsp */
> +		op->src.type = OP_SRC_ADD;
> +		op->src.reg = CFI_SP;
> +		op->src.offset = 5*8;
> +		op->dest.type = OP_DEST_REG;
> +		op->dest.reg = CFI_SP;
> +		break;
> +
>   	case 0xca: /* retf */
>   	case 0xcb: /* retf */
> -	case 0xcf: /* iret */
>   		*type = INSN_CONTEXT_SWITCH;
>   		break;
>   
> @@ -483,7 +493,7 @@ int arch_decode_instruction(struct elf *
>   
>   	*immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
>   
> -	if (*type == INSN_STACK)
> +	if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN)
>   		list_add_tail(&op->list, ops_list);
>   	else
>   		free(op);
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2224,6 +2224,20 @@ static int validate_branch(struct objtoo
>   
>   			break;
>   
> +		case INSN_EXCEPTION_RETURN:
> +			if (handle_insn_ops(insn, &state))
> +				return 1;
> +
> +			/*
> +			 * This handles x86's sync_core() case, where we use an
> +			 * IRET to self. All 'normal' IRET instructions are in
> +			 * STT_NOTYPE entry symbols.
> +			 */
> +			if (func)
> +				break;
> +
> +			return 0;
> +
>   		case INSN_CONTEXT_SWITCH:
>   			if (func && (!next_insn || !next_insn->hint)) {
>   				WARN_FUNC("unsupported instruction in callable function",
> 

Cheers,

-- 
Julien Thierry