diff mbox

[3/6] arm64: Add support for hooks to handle undefined instructions

Message ID CAKv+Gu8Nji0Jspk2cMCwVGKeODsiOb0NFUFWucm3rAbNdb_bTQ@mail.gmail.com
State New
Headers show

Commit Message

Ard Biesheuvel Aug. 26, 2014, 2:21 p.m. UTC 
On 26 August 2014 15:13, Will Deacon <will.deacon@arm.com> wrote:
> Hi Punit,
>
> On Tue, Aug 26, 2014 at 11:28:47AM +0100, Punit Agrawal wrote:
>> Add support to register hooks for undefined instructions. The handlers
>> will be called when the undefined instruction and the processor state
>> (as contained in pstate) match criteria used at registration.
>>
>> Note: The patch only deals with ARM instruction encodings and needs
>> fixing to handle thumb instructions as well.
>
> [...]
>
>> +static int call_undef_hook(struct pt_regs *regs)
>> +{
>> +     struct undef_hook *hook;
>> +     unsigned long flags;
>> +     u32 instr;
>> +     int (*fn)(struct pt_regs *regs, u32 instr) = NULL;
>> +     void __user *pc = (void __user *)instruction_pointer(regs);
>> +
>> +     /*
>> +      * Currently, undefined instruction patching is only supported
>> +      * for user mode. Also, as we're not emulating any thumb
>> +      * instructions lets not add thumb instruction decoding until
>> +      * it is needed.
>> +      */
>> +     if (!compat_user_mode(regs) || compat_thumb_mode(regs))
>> +             return 1;
>
> What do you mean by `undefined instruction patching'? I don't see anything
> in the mechanism that means this can't be reused for kernel code, then we
> just register the SWP emulation hook for userspace only using the mode (like
> we do for kgdb).
>

You need this patch in order to be able to return from an undef
exception taken in EL1:

Comments

Will Deacon Aug. 26, 2014, 2:30 p.m. UTC | #1 
On Tue, Aug 26, 2014 at 03:21:09PM +0100, Ard Biesheuvel wrote:
> On 26 August 2014 15:13, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Aug 26, 2014 at 11:28:47AM +0100, Punit Agrawal wrote:
> >> Add support to register hooks for undefined instructions. The handlers
> >> will be called when the undefined instruction and the processor state
> >> (as contained in pstate) match criteria used at registration.
> >>
> >> Note: The patch only deals with ARM instruction encodings and needs
> >> fixing to handle thumb instructions as well.
> >
> > [...]
> >
> >> +static int call_undef_hook(struct pt_regs *regs)
> >> +{
> >> +     struct undef_hook *hook;
> >> +     unsigned long flags;
> >> +     u32 instr;
> >> +     int (*fn)(struct pt_regs *regs, u32 instr) = NULL;
> >> +     void __user *pc = (void __user *)instruction_pointer(regs);
> >> +
> >> +     /*
> >> +      * Currently, undefined instruction patching is only supported
> >> +      * for user mode. Also, as we're not emulating any thumb
> >> +      * instructions lets not add thumb instruction decoding until
> >> +      * it is needed.
> >> +      */
> >> +     if (!compat_user_mode(regs) || compat_thumb_mode(regs))
> >> +             return 1;
> >
> > What do you mean by `undefined instruction patching'? I don't see anything
> > in the mechanism that means this can't be reused for kernel code, then we
> > just register the SWP emulation hook for userspace only using the mode (like
> > we do for kgdb).
> >
> 
> You need this patch in order to be able to return from an undef
> exception taken in EL1:
> 
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -287,7 +287,9 @@ el1_undef:
>          */
>         enable_dbg
>         mov     x0, sp
> -       b       do_undefinstr
> +       bl      do_undefinstr
> +
> +       kernel_exit 1
>  el1_dbg:
>         /*
>          * Debug exception handling

Hmm, I'm surprised we don't already need something like this for KGDB...

Will
Catalin Marinas Aug. 27, 2014, 4:47 p.m. UTC | #2 
On Tue, Aug 26, 2014 at 03:30:11PM +0100, Will Deacon wrote:
> On Tue, Aug 26, 2014 at 03:21:09PM +0100, Ard Biesheuvel wrote:
> > On 26 August 2014 15:13, Will Deacon <will.deacon@arm.com> wrote:
> > > On Tue, Aug 26, 2014 at 11:28:47AM +0100, Punit Agrawal wrote:
> > >> Add support to register hooks for undefined instructions. The handlers
> > >> will be called when the undefined instruction and the processor state
> > >> (as contained in pstate) match criteria used at registration.
> > >>
> > >> Note: The patch only deals with ARM instruction encodings and needs
> > >> fixing to handle thumb instructions as well.
> > >
> > > [...]
> > >
> > >> +static int call_undef_hook(struct pt_regs *regs)
> > >> +{
> > >> +     struct undef_hook *hook;
> > >> +     unsigned long flags;
> > >> +     u32 instr;
> > >> +     int (*fn)(struct pt_regs *regs, u32 instr) = NULL;
> > >> +     void __user *pc = (void __user *)instruction_pointer(regs);
> > >> +
> > >> +     /*
> > >> +      * Currently, undefined instruction patching is only supported
> > >> +      * for user mode. Also, as we're not emulating any thumb
> > >> +      * instructions lets not add thumb instruction decoding until
> > >> +      * it is needed.
> > >> +      */
> > >> +     if (!compat_user_mode(regs) || compat_thumb_mode(regs))
> > >> +             return 1;
> > >
> > > What do you mean by `undefined instruction patching'? I don't see anything
> > > in the mechanism that means this can't be reused for kernel code, then we
> > > just register the SWP emulation hook for userspace only using the mode (like
> > > we do for kgdb).
> > 
> > You need this patch in order to be able to return from an undef
> > exception taken in EL1:
> > 
> > --- a/arch/arm64/kernel/entry.S
> > +++ b/arch/arm64/kernel/entry.S
> > @@ -287,7 +287,9 @@ el1_undef:
> >          */
> >         enable_dbg
> >         mov     x0, sp
> > -       b       do_undefinstr
> > +       bl      do_undefinstr
> > +
> > +       kernel_exit 1
> >  el1_dbg:
> >         /*
> >          * Debug exception handling
> 
> Hmm, I'm surprised we don't already need something like this for KGDB...

We don't expect undef exceptions at EL1, so far they are fatal as we
don't have any hooks for them. Doesn't KGDB use dedicated breakpoint
instructions?
Will Deacon Aug. 27, 2014, 4:51 p.m. UTC | #3 
On Wed, Aug 27, 2014 at 05:47:14PM +0100, Catalin Marinas wrote:
> On Tue, Aug 26, 2014 at 03:30:11PM +0100, Will Deacon wrote:
> > On Tue, Aug 26, 2014 at 03:21:09PM +0100, Ard Biesheuvel wrote:
> > > You need this patch in order to be able to return from an undef
> > > exception taken in EL1:
> > > 
> > > --- a/arch/arm64/kernel/entry.S
> > > +++ b/arch/arm64/kernel/entry.S
> > > @@ -287,7 +287,9 @@ el1_undef:
> > >          */
> > >         enable_dbg
> > >         mov     x0, sp
> > > -       b       do_undefinstr
> > > +       bl      do_undefinstr
> > > +
> > > +       kernel_exit 1
> > >  el1_dbg:
> > >         /*
> > >          * Debug exception handling
> > 
> > Hmm, I'm surprised we don't already need something like this for KGDB...
> 
> We don't expect undef exceptions at EL1, so far they are fatal as we
> don't have any hooks for them. Doesn't KGDB use dedicated breakpoint
> instructions?

Ah yeah, we use magic immediates in the BRK instruction. I was getting
confused with arch/arm/, where we actually use an undefined encoding for
the same thing. That explains why things appear to be working!

Will
diff mbox

Patch

--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -287,7 +287,9 @@  el1_undef:
         */
        enable_dbg
        mov     x0, sp
-       b       do_undefinstr
+       bl      do_undefinstr
+
+       kernel_exit 1
 el1_dbg:
        /*
         * Debug exception handling