On Fri, Dec 01, 2017 at 11:58:36AM +0000, Mark Rutland wrote:
> On Thu, Nov 30, 2017 at 04:39:40PM +0000, Will Deacon wrote:
> > We will need to treat exceptions from EL0 differently in kernel_ventry,
> > so rework the macro to take the exception level as an argument and
> > construct the branch target using that.
> >
> > Signed-off-by: Will Deacon <[email protected]>
> > ---
> > arch/arm64/kernel/entry.S | 46
> > +++++++++++++++++++++++-----------------------
> > 1 file changed, 23 insertions(+), 23 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > index dea196f287a0..688e52f65a8d 100644
> > --- a/arch/arm64/kernel/entry.S
> > +++ b/arch/arm64/kernel/entry.S
> > @@ -71,7 +71,7 @@
> > #define BAD_FIQ 2
> > #define BAD_ERROR 3
> >
> > - .macro kernel_ventry label
> > + .macro kernel_ventry, el, label, regsize = 64
> > .align 7
> > sub sp, sp, #S_FRAME_SIZE
> > #ifdef CONFIG_VMAP_STACK
> > @@ -84,7 +84,7 @@
> > tbnz x0, #THREAD_SHIFT, 0f
> > sub x0, sp, x0 // x0'' = sp' - x0' = (sp + x0)
> > - sp = x0
> > sub sp, sp, x0 // sp'' = sp' - x0 = (sp + x0)
> > - x0 = sp
> > - b \label
> > + b el\()\el\()_\label
> >
> > 0:
> > /*
> > @@ -116,7 +116,7 @@
> > sub sp, sp, x0
> > mrs x0, tpidrro_el0
> > #endif
> > - b \label
> > + b el\()\el\()_\label
> > .endm
> >
> > .macro kernel_entry, el, regsize = 64
> > @@ -369,31 +369,31 @@ tsk .req x28 // current thread_info
> >
> > .align 11
> > ENTRY(vectors)
> > - kernel_ventry el1_sync_invalid // Synchronous EL1t
> > - kernel_ventry el1_irq_invalid // IRQ EL1t
> > - kernel_ventry el1_fiq_invalid // FIQ EL1t
> > - kernel_ventry el1_error_invalid // Error EL1t
> > + kernel_ventry 1, sync_invalid // Synchronous EL1t
> > + kernel_ventry 1, irq_invalid // IRQ EL1t
> > + kernel_ventry 1, fiq_invalid // FIQ EL1t
> > + kernel_ventry 1, error_invalid // Error EL1t
>
> Using the el paramter to build the branch name has the unfortunate
> property of obscuring the branch name. For example, that makes it
> difficult to jump around the entry asm with ctags, which is somewhat
> painful.
>
> Could we leave the full branch name in place, e.g.
>
> kernel_ventry 1, el1_sync_invalid // Synchronous EL1t
> kernel_ventry 1, el1_irq_invalid // IRQ EL1t
> kernel_ventry 1, el1_fiq_invalid // FIQ EL1t
> kernel_ventry 1, el1_error_invalid // Error EL1t
>
> ... or have separate kernel_ventry and user_ventry macros that
> implicitly encoded the source EL, also leaving the label name as-is.
The downside of doing that is that it makes it possible to say things like:
kernel_ventry 0, el1_sync
which I don't want to be expressible.
Given that ctags already chokes on lots of entry.S (for example, any macro
that is defined outside of the file) *and* that you can easily search for
things like el1_sync_invalid within the file, I'm inclined to leave this
patch as-is, but I'll note your objection and buy you a pint.
Will
