| Message ID | 1512059986-21325-13-git-send-email-will.deacon@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <will.deacon@arm.com> > --- > 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. Otherwise, this looks fine to me. Thanks, Mark.
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 <will.deacon@arm.com> > > --- > > 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
On Fri, Dec 01, 2017 at 05:51:44PM +0000, Will Deacon wrote: > 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: > > > + .macro kernel_ventry, el, label, regsize = 64 > > > + b el\()\el\()_\label > > > - kernel_ventry el1_sync_invalid // Synchronous EL1t > > > + kernel_ventry 1, sync_invalid // Synchronous 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. I guess I'll live with it, then. ;) Assuming I can't twist your arm, feel free to take my Reviewed-by here too. Thanks, Mark.
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 - kernel_ventry el1_sync // Synchronous EL1h - kernel_ventry el1_irq // IRQ EL1h - kernel_ventry el1_fiq_invalid // FIQ EL1h - kernel_ventry el1_error // Error EL1h + kernel_ventry 1, sync // Synchronous EL1h + kernel_ventry 1, irq // IRQ EL1h + kernel_ventry 1, fiq_invalid // FIQ EL1h + kernel_ventry 1, error // Error EL1h - kernel_ventry el0_sync // Synchronous 64-bit EL0 - kernel_ventry el0_irq // IRQ 64-bit EL0 - kernel_ventry el0_fiq_invalid // FIQ 64-bit EL0 - kernel_ventry el0_error // Error 64-bit EL0 + kernel_ventry 0, sync // Synchronous 64-bit EL0 + kernel_ventry 0, irq // IRQ 64-bit EL0 + kernel_ventry 0, fiq_invalid // FIQ 64-bit EL0 + kernel_ventry 0, error // Error 64-bit EL0 #ifdef CONFIG_COMPAT - kernel_ventry el0_sync_compat // Synchronous 32-bit EL0 - kernel_ventry el0_irq_compat // IRQ 32-bit EL0 - kernel_ventry el0_fiq_invalid_compat // FIQ 32-bit EL0 - kernel_ventry el0_error_compat // Error 32-bit EL0 + kernel_ventry 0, sync_compat, 32 // Synchronous 32-bit EL0 + kernel_ventry 0, irq_compat, 32 // IRQ 32-bit EL0 + kernel_ventry 0, fiq_invalid_compat, 32 // FIQ 32-bit EL0 + kernel_ventry 0, error_compat, 32 // Error 32-bit EL0 #else - kernel_ventry el0_sync_invalid // Synchronous 32-bit EL0 - kernel_ventry el0_irq_invalid // IRQ 32-bit EL0 - kernel_ventry el0_fiq_invalid // FIQ 32-bit EL0 - kernel_ventry el0_error_invalid // Error 32-bit EL0 + kernel_ventry 0, sync_invalid, 32 // Synchronous 32-bit EL0 + kernel_ventry 0, irq_invalid, 32 // IRQ 32-bit EL0 + kernel_ventry 0, fiq_invalid, 32 // FIQ 32-bit EL0 + kernel_ventry 0, error_invalid, 32 // Error 32-bit EL0 #endif END(vectors)
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 <will.deacon@arm.com> --- arch/arm64/kernel/entry.S | 46 +++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 23 deletions(-)