lkml.org 
[lkml]   [2017]   [Dec]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: arm64: unhandled level 0 translation fault
On Fri, Dec 15, 2017 at 02:30:00PM +0100, Geert Uytterhoeven wrote:
> Hi Dave,
>
> On Fri, Dec 15, 2017 at 12:23 PM, Dave Martin <Dave.Martin@arm.com> wrote:
> > On Thu, Dec 14, 2017 at 07:08:27PM +0100, Geert Uytterhoeven wrote:
> >> On Thu, Dec 14, 2017 at 4:24 PM, Dave P Martin <Dave.Martin@arm.com> wrote:

[...]

> >> > Good work on the bisect -- I'll need to have a think about this...
> >> >
> >> > That patch fixes a genuine problem so we can't just revert it.
> >> >
> >> > What if you revert _just this function_ back to what it was in v4.14?
> >>
> >> With fpsimd_update_current_state() reverted to v4.14, and
> >>
> >> - __this_cpu_write(fpsimd_last_state, st);
> >> + __this_cpu_write(fpsimd_last_state.st, st);
> >>
> >> to make it build, the problem seems to be fixed, too.
>
> > Interesting if I apply that to v4.14 and then flatten the new code for CONFIG_ARM64_SVE=n, I get:
> >
> > Working:
> >
> > void fpsimd_update_current_state(struct fpsimd_state *state)
> > {
> > local_bh_disable();
> >
> > fpsimd_load_state(state);
> > if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
> > struct fpsimd_state *st = &current->thread.fpsimd_state;
> >
> > __this_cpu_write(fpsimd_last_state.st, st);
> > st->cpu = smp_processor_id();
> > }
> >
> > local_bh_enable();
> > }
> >
> > Broken:
> >
> > void fpsimd_update_current_state(struct fpsimd_state *state)
> > {
> > struct fpsimd_last_state_struct *last;
> > struct fpsimd_state *st;
> >
> > local_bh_disable();
> >
> > current->thread.fpsimd_state = *state;
> > fpsimd_load_state(&current->thread.fpsimd_state);
> >
> > if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
> > last = this_cpu_ptr(&fpsimd_last_state);
> > st = &current->thread.fpsimd_state;
> >
> > last->st = st;
> > last->sve_in_use = test_thread_flag(TIF_SVE);
> > st->cpu = smp_processor_id();
> > }
> >
> > local_bh_enable();
> > }
> >
> > Can you try my flattened "broken" version by itself and see if that does
> > reproduce the bug? If not, my flattening may be making bad assumptions...
> >
> > Assuming the "broken" version reproduces the bug, I can't yet see exactly
> > where the breakage comes from.
>
> Correct, above "Working" is working, and "Broken" is broken.
>
> > The two important differences here seem to be
> >
> > 1) Staging the state via current->thread.fpsimd_state instead of loading
> > directly:
> >
> > - fpsimd_load_state(state);
> > + current->thread.fpsimd_state = *state;
> > + fpsimd_load_state(&current->thread.fpsimd_state);
>
> The change above introduces the breakage.
>
> > 2) Using this_cpu_ptr() + assignment instead of __this_cpu_write() when
> > reassociating the task's fpsimd context with the cpu:
> >
> > {
> > + struct fpsimd_last_state_struct *last;
> > + struct fpsimd_state *st;
> >
> > [...]
> >
> > if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
> > - struct fpsimd_state *st = &current->thread.fpsimd_state;
> > -
> > - __this_cpu_write(fpsimd_last_state.st, st);
> > - st->cpu = smp_processor_id();
> > + last = this_cpu_ptr(&fpsimd_last_state);
> > + st = &current->thread.fpsimd_state;
> > +
> > + last->st = st;
> > + last->sve_in_use = test_thread_flag(TIF_SVE);
> > + st->cpu = smp_processor_id();
> > }
>
> The change above is fine.

Thanks for this.

Will came up with a convincing hypothesis for how the dodgy change broke
things here -- see the diff in his separate reply.

I'll cook up a more complete fix, but the diff Will provided should at
least get things working.

Cheers
---Dave

\
 
 \ /
  Last update: 2017-12-15 18:12    [W:0.067 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site