Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 9/9] objtool: Abstract unwind hint reading
To:     Josh Poimboeuf <jpoimboe@redhat.com>
Cc:     linux-kernel@vger.kernel.org, peterz@infradead.org, mbenes@suse.cz,
        raphael.gault@arm.com, benh@kernel.crashing.org
References: <20200730094652.28297-1-jthierry@redhat.com>
 <20200730094652.28297-10-jthierry@redhat.com>
 <20200730150341.udqnykbw7yfsjvin@treble>
From:   Julien Thierry <jthierry@redhat.com>
Message-ID: <1a078563-001d-c666-d2f5-9291f0efd35a@redhat.com>
Date:   Fri, 31 Jul 2020 08:00:58 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <20200730150341.udqnykbw7yfsjvin@treble>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 7/30/20 4:03 PM, Josh Poimboeuf wrote:
> On Thu, Jul 30, 2020 at 10:46:52AM +0100, Julien Thierry wrote:
>> The type of unwind hints and the semantics associated with them depend
>> on the architecture. Let arch specific code convert unwind hints into
>> objtool stack state descriptions.
>>
>> Signed-off-by: Julien Thierry <jthierry@redhat.com>
>> ---
>>   tools/objtool/arch.h            |  5 +--
>>   tools/objtool/arch/x86/decode.c | 54 ++++++++++++++++++++++++++++++
>>   tools/objtool/cfi.h             |  3 +-
>>   tools/objtool/check.c           | 58 +++++----------------------------
>>   tools/objtool/orc_gen.c         |  4 ++-
>>   5 files changed, 71 insertions(+), 53 deletions(-)
>>
>> diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
>> index 2e2ce089b0e9..44107e9aab71 100644
>> --- a/tools/objtool/arch.h
>> +++ b/tools/objtool/arch.h
>> @@ -7,12 +7,11 @@
>>   #define _ARCH_H
>>   
>>   #include <stdbool.h>
>> +#include <linux/frame.h>
>>   #include <linux/list.h>
>>   #include "objtool.h"
>>   #include "cfi.h"
>>   
>> -#include <asm/orc_types.h>
>> -
>>   enum insn_type {
>>   	INSN_JUMP_CONDITIONAL,
>>   	INSN_JUMP_UNCONDITIONAL,
>> @@ -86,4 +85,6 @@ unsigned long arch_dest_reloc_offset(int addend);
>>   
>>   const char *arch_nop_insn(int len);
>>   
>> +int arch_decode_insn_hint(struct instruction *insn, struct unwind_hint *hint);
>> +
>>   #endif /* _ARCH_H */
>> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
>> index 1967370440b3..2099809925af 100644
>> --- a/tools/objtool/arch/x86/decode.c
>> +++ b/tools/objtool/arch/x86/decode.c
>> @@ -6,6 +6,8 @@
>>   #include <stdio.h>
>>   #include <stdlib.h>
>>   
>> +#include <linux/frame.h>
>> +
>>   #define unlikely(cond) (cond)
>>   #include <asm/insn.h>
>>   #include "../../../arch/x86/lib/inat.c"
>> @@ -15,6 +17,7 @@
>>   #include "../../elf.h"
>>   #include "../../arch.h"
>>   #include "../../warn.h"
>> +#include <asm/orc_types.h>
>>   
>>   static unsigned char op_to_cfi_reg[][2] = {
>>   	{CFI_AX, CFI_R8},
>> @@ -583,3 +586,54 @@ const char *arch_nop_insn(int len)
>>   
>>   	return nops[len-1];
>>   }
>> +
>> +int arch_decode_insn_hint(struct instruction *insn, struct unwind_hint *hint)
>> +{
>> +	struct cfi_reg *cfa = &insn->cfi.cfa;
>> +
>> +	if (hint->type == UNWIND_HINT_TYPE_RET_OFFSET) {
>> +		insn->ret_offset = hint->sp_offset;
>> +		return 0;
>> +	}
>> +
>> +	insn->hint = true;
>> +
>> +	switch (hint->sp_reg) {
>> +	case ORC_REG_UNDEFINED:
>> +		cfa->base = CFI_UNDEFINED;
>> +		break;
>> +	case ORC_REG_SP:
>> +		cfa->base = CFI_SP;
>> +		break;
>> +	case ORC_REG_BP:
>> +		cfa->base = CFI_BP;
>> +		break;
>> +	case ORC_REG_SP_INDIRECT:
>> +		cfa->base = CFI_SP_INDIRECT;
>> +		break;
>> +	case ORC_REG_R10:
>> +		cfa->base = CFI_R10;
>> +		break;
>> +	case ORC_REG_R13:
>> +		cfa->base = CFI_R13;
>> +		break;
>> +	case ORC_REG_DI:
>> +		cfa->base = CFI_DI;
>> +		break;
>> +	case ORC_REG_DX:
>> +		cfa->base = CFI_DX;
>> +		break;
>> +	default:
>> +		WARN_FUNC("unsupported unwind_hint sp base reg %d",
>> +			  insn->sec, insn->offset, hint->sp_reg);
>> +		return -1;
>> +	}
>> +
>> +	cfa->offset = hint->sp_offset;
>> +	insn->cfi.hint_type = hint->type;
>> +	insn->cfi.end = hint->end;
>> +
>> +	insn->cfi.sp_only = hint->type == ORC_TYPE_REGS || hint->type == ORC_TYPE_REGS_IRET;
> 
> What does "sp" mean here in sp_only?
>

Stack pointer, like in CFI_SP. When objtool encounters one of these 
hints, it starts to only track the stack frame with the stack pointer 
(no BP, no drap register, no move to temporary registers). Just trying 
to make some sense of this corner case.

>> +		if (arch_decode_insn_hint(insn, hint)) {
>> +			WARN_FUNC("Bad unwind hint", insn->sec, insn->offset);
> 
> No need for a warning here, since the arch-specific function already
> prints one.
> 

Right.

Thanks,

-- 
Julien Thierry