Re: [PATCH 2/4] x86: get rid of KERNEL_STACK_OFFSET

[Andy Lutomirski]

On Thu, Feb 26, 2015 at 7:21 AM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
> On Thu, Feb 26, 2015 at 3:42 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>>
>> * Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
>>
>>> >> I added that in and applied this patch.
>>> >
>>> > So this is not just slightly buggy, it's fundamentally
>>> > wrong as well as it removes the possibility of an RSP
>>> > value optimization from the 64-bit path, see my
>>> > previous mail.
>>>
>>> This is just trying to check that the function is
>>> executing on the per-thread stack.  It was correct (and
>>> fairly heavily tested by Tony) wither KERNEL_STACK_OFFSET
>>> being nonzero, but we're checking the wrong page if
>>> KERNEL_STACK_OFFSET becomes zero.
>>>
>>> I don't think I understand your objection to this bit.
>>
>> I object to the KERNEL_STACK_OFFSET removal patch you fixed
>> here, not to the add-on fix in particular (which is correct
>> in the context of that patch).
>>
>
> Aha. I misunderstood.
>
> I would object, too, except that I think that a better improvement
> would be to build the entire frame using pushes, including the "top of
> stack" part, even on fast-path syscalls.  That would have
> KERNEL_STACK_OFFSET=0.
>
> Actually, I want an even bigger change.  kernel_stack seems entirely
> pointless to me, since we could just as easily be using tss.sp0 (I
> think), as long as there's no useful offset.  So maybe the right way
> to handle this patch would be to first clean up the ia32entry code and
> all the non-asm users to use tss.sp0, and then to figure out what to
> do about the syscall64 path.
>
> I'd be happy to defer this patch until the FIXUP_TOP_OF_STACK cleanups
> later on, assuming it can be easily extricated, which I haven't
> checked yet.  Thoughts?
>

Damnit, there are too many sets of patches that I've confused myself.
I haven't applied this one yet.  My bad for letting the set of things
flying around get out of control.

I think I'll NAK this patch as is.  Let's get the existing stuff
stabilized first.

For a resubmission of this, or something like it, let's do it in nice
bite-sized pieces:

a) Make sure that asm can read sp0.  (It's at a fixed offset from a
percpu variable, so it should be fine, but asm-offsets might need
updating.)

b) Remove the C inline helpers and fix anything that needs fixing,
such as the BUG_ON in traps.c

c) In appropriately split-up pieces, change over the asm code to use
sp0 instead of kernel_stack

d) For the 64-bit syscall path, either switch to sp0 or keep using
kernel_stack but *ename it to avoid to avoid confusion.

One thing that was problematic with the big layout change patch is
that it unnecessarily made too many changes at once.  IMO it really
ought to have introduced new macros (including
ALLOC_PARTIAL_WHATEVER_ON_STACK), then switch users of the macros to
new macros piece by piece without layout changes (to improve
bisectability and to fix the real underlying problem with the old
code, namely that it was totally incomprehensible), and *then* to
change the layout with a patch in which both the old and new code was
readable.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/