Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 24 Apr 2019 11:23:24 -0500
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     Raphael Gault <Raphael.Gault@arm.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "peterz@infradead.org" <peterz@infradead.org>,
        Catalin Marinas <Catalin.Marinas@arm.com>,
        Will Deacon <Will.Deacon@arm.com>,
        Julien Thierry <Julien.Thierry@arm.com>
Subject: Re: [RFC 2/6] objtool: arm64: Add required implementation for
 supporting the aarch64 architecture in objtool.
Message-ID: <20190424162324.36xn7aepuggyvbzi@treble>
References: <20190409135243.12424-1-raphael.gault@arm.com>
 <20190409135243.12424-3-raphael.gault@arm.com>
 <20190423201823.fnddnyxpu64jnlgp@treble>
 <fe8a45e3-dfb1-84fc-8912-9cff257e6f1f@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <fe8a45e3-dfb1-84fc-8912-9cff257e6f1f@arm.com>
User-Agent: NeoMutt/20180716
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Apr 24, 2019 at 04:16:26PM +0000, Raphael Gault wrote:
> On 4/23/19 9:18 PM, Josh Poimboeuf wrote:
> > On Tue, Apr 09, 2019 at 02:52:39PM +0100, Raphael Gault wrote:
> >> Provide implementation for the arch-dependent functions that are called by the main check
> >> function of objtool.
> >> The ORC unwinder is not yet supported by the arm64 architecture so we only provide a dummy
> >> interface for now.
> >> The decoding of the instruction is split into classes and subclasses as described into
> >> the Instruction Encoding in the ArmV8.5 Architecture Reference Manual.
> >
> > Where did the code for the decoder come from?  Was it written from
> > scratch?
> >
> 
> This decoder was indeed written from scratch based on the armv8 ARM. The
> automatic generation idea hasn't really been discussed yet.

Ok.  That's a lot of code.  Hopefully ARM folks can review it closely.

> >> +#ifndef __ASSEMBLY__
> >> +/*
> >> + * This struct is more or less a vastly simplified version of the DWARF Call
> >> + * Frame Information standard.  It contains only the necessary parts of DWARF
> >> + * CFI, simplified for ease of access by the in-kernel unwinder.  It tells the
> >> + * unwinder how to find the previous SP and BP (and sometimes entry regs) on
> >> + * the stack for a given code address.  Each instance of the struct corresponds
> >> + * to one or more code locations.
> >> + */
> >> +struct orc_entry {
> >> +s16sp_offset;
> >> +s16bp_offset;
> >> +unsignedsp_reg:4;
> >> +unsignedbp_reg:4;
> >> +unsignedtype:2;
> >> +unsignedend:1;
> >> +} __packed;
> >> +
> >> +/*
> >> + * This struct is used by asm and inline asm code to manually annotate the
> >> + * location of registers on the stack for the ORC unwinder.
> >> + *
> >> + * Type can be either ORC_TYPE_* or UNWIND_HINT_TYPE_*.
> >> + */
> >> +struct unwind_hint {
> >> +u32ip;
> >> +s16sp_offset;
> >> +u8sp_reg;
> >> +u8type;
> >> +u8end;
> >> +};
> >> +#endif /* __ASSEMBLY__ */
> >> +
> >> +#endif /* _ORC_TYPES_H */
> >
> > It seems odd to have the above header file in arm64 code, since it
> > doesn't implement ORC.  Is it really needed?
> >
> 
> The unwind_hint part can safely be removed. However the orc_entry seems
> to be needed since the struct instruction comports a struct orc_entry
> field. I have chosen to let it here as well but maybe a better approach
> is possible.

Ideally we can figure out a way to decouple 'struct instruction' from
'struct orc_entry'.  But yes, I think your approach is fine for now.

Though I think using an arch-independent header file would be better, to
avoid creating duplicated (dead) code.

-- 
Josh