LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [RFC 0/3] Baby steps toward cleaning up KERNEL_STACK_OFFSET
@ 2015-02-26 23:52 Andy Lutomirski
  2015-02-27  0:07 ` [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu Andy Lutomirski
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Andy Lutomirski @ 2015-02-26 23:52 UTC (permalink / raw)
  To: Denys Vlasenko, linux-kernel; +Cc: x86, Borislav Petkov, Andy Lutomirski

Denys is right that KERNEL_STACK_OFFSET is a mess.  Let's start fixing
it.

This removes all C code that *reads* kernel_stack.  It also fixes the
KERNEL_STACK_OFFSET abomination in ia32_sysenter_target.

It does not fix the KERNEL_STACK_OFFSET abomination in GET_THREAD_INFO
and THREAD_INFO.  I think that should be its own patch.

It also doesn't change the two syscall targets.  To fix them, we should
make a decision.  Either we should make KERNEL_STACK_OFFSET have the
correct nonzero value to save an instruction or we should get rid of
kernel_stack entirely.

Andy Lutomirski (3):
  x86: Add this_cpu_sp0() to read sp0 for the current cpu
  x86: Switch all C consumers of kernel_stack to this_cpu_sp0
  x86, asm: Change the 32-bit sysenter code to use sp0

 arch/x86/ia32/ia32entry.S          | 3 +--
 arch/x86/include/asm/processor.h   | 5 +++++
 arch/x86/include/asm/thread_info.h | 3 +--
 arch/x86/kernel/asm-offsets_64.c   | 1 +
 arch/x86/kernel/traps.c            | 2 +-
 arch/x86/lguest/boot.c             | 1 +
 arch/x86/xen/enlighten.c           | 1 +
 7 files changed, 11 insertions(+), 5 deletions(-)

-- 
2.1.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu
  2015-02-26 23:52 [RFC 0/3] Baby steps toward cleaning up KERNEL_STACK_OFFSET Andy Lutomirski
@ 2015-02-27  0:07 ` Andy Lutomirski
  2015-03-04  8:02   ` Ingo Molnar
  2015-02-27  0:07 ` [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0 Andy Lutomirski
  2015-02-27  0:07 ` [RFC 3/3] x86, asm: Change the 32-bit sysenter code to use sp0 Andy Lutomirski
  2 siblings, 1 reply; 17+ messages in thread
From: Andy Lutomirski @ 2015-02-27  0:07 UTC (permalink / raw)
  To: x86, linux-kernel
  Cc: Borislav Petkov, Oleg Nesterov, Denys Vlasenko, Andy Lutomirski

We currently store references to the top of the kernel stack in
multiple places: kernel_stack (with an offset) and
init_tss.x86_tss.sp0 (no offset).  The latter is defined by hardware
and is a clean canonical way to find the top of the stack.  Add an
accessor so we can start using it.

This needs minor paravirt tweaks to ensure that On native, sp0
defines the top of the kernel stack.  On Xen and lguest, the
hypervisor tracks it, but we want to start reading sp0 in the
kernel.  Fixing this is simple: just update our local copy of sp0 as
well as the hypervisor's copy on task switches.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 arch/x86/include/asm/processor.h | 5 +++++
 arch/x86/lguest/boot.c           | 1 +
 arch/x86/xen/enlighten.c         | 1 +
 3 files changed, 7 insertions(+)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index a092a0cce0b7..477b1ff4a6c9 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -564,6 +564,11 @@ static inline void native_swapgs(void)
 #endif
 }
 
+static inline unsigned long this_cpu_sp0(void)
+{
+	return this_cpu_read_stable(init_tss.x86_tss.sp0);
+}
+
 #ifdef CONFIG_PARAVIRT
 #include <asm/paravirt.h>
 #else
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
index c1c1544b8485..77a609d1898d 100644
--- a/arch/x86/lguest/boot.c
+++ b/arch/x86/lguest/boot.c
@@ -1053,6 +1053,7 @@ static void lguest_load_sp0(struct tss_struct *tss,
 {
 	lazy_hcall3(LHCALL_SET_STACK, __KERNEL_DS | 0x1, thread->sp0,
 		   THREAD_SIZE / PAGE_SIZE);
+	tss->x86_tss.sp0 = sp0;
 }
 
 /* Let's just say, I wouldn't do debugging under a Guest. */
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 78a881b7fc41..d8195ca15346 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -912,6 +912,7 @@ static void xen_load_sp0(struct tss_struct *tss,
 	mcs = xen_mc_entry(0);
 	MULTI_stack_switch(mcs.mc, __KERNEL_DS, thread->sp0);
 	xen_mc_issue(PARAVIRT_LAZY_CPU);
+	tss->x86_tss.sp0 = thread->sp0;
 }
 
 static void xen_set_iopl_mask(unsigned mask)
-- 
2.1.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0
  2015-02-26 23:52 [RFC 0/3] Baby steps toward cleaning up KERNEL_STACK_OFFSET Andy Lutomirski
  2015-02-27  0:07 ` [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu Andy Lutomirski
@ 2015-02-27  0:07 ` Andy Lutomirski
  2015-02-27 16:13   ` Denys Vlasenko
  2015-02-27 16:52   ` Denys Vlasenko
  2015-02-27  0:07 ` [RFC 3/3] x86, asm: Change the 32-bit sysenter code to use sp0 Andy Lutomirski
  2 siblings, 2 replies; 17+ messages in thread
From: Andy Lutomirski @ 2015-02-27  0:07 UTC (permalink / raw)
  To: x86, linux-kernel
  Cc: Borislav Petkov, Oleg Nesterov, Denys Vlasenko, Andy Lutomirski,
	Konrad Rzeszutek Wilk, Boris Ostrovsky, Rusty Russell

This will make modifying the semantics of kernel_stack easier.

Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 arch/x86/include/asm/thread_info.h | 3 +--
 arch/x86/kernel/traps.c            | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index e82e95abc92b..92549053d86d 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
 static inline struct thread_info *current_thread_info(void)
 {
 	struct thread_info *ti;
-	ti = (void *)(this_cpu_read_stable(kernel_stack) +
-		      KERNEL_STACK_OFFSET - THREAD_SIZE);
+	ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
 	return ti;
 }
 
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index c74f2f5652da..d287ea779728 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -174,7 +174,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
 	 * will catch asm bugs and any attempt to use ist_preempt_enable
 	 * from double_fault.
 	 */
-	BUG_ON(((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
+	BUG_ON(((current_stack_pointer() ^ (this_cpu_sp0() - 1))
 		& ~(THREAD_SIZE - 1)) != 0);
 
 	preempt_count_sub(HARDIRQ_OFFSET);
-- 
2.1.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC 3/3] x86, asm: Change the 32-bit sysenter code to use sp0
  2015-02-26 23:52 [RFC 0/3] Baby steps toward cleaning up KERNEL_STACK_OFFSET Andy Lutomirski
  2015-02-27  0:07 ` [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu Andy Lutomirski
  2015-02-27  0:07 ` [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0 Andy Lutomirski
@ 2015-02-27  0:07 ` Andy Lutomirski
  2 siblings, 0 replies; 17+ messages in thread
From: Andy Lutomirski @ 2015-02-27  0:07 UTC (permalink / raw)
  To: x86, linux-kernel
  Cc: Borislav Petkov, Oleg Nesterov, Denys Vlasenko, Andy Lutomirski

The ia32 sysenter code loaded the top of the kernel stack into rsp
by loading kernel_stack and then adjusting it.  It can be simplified
to just read sp0 directly.

This requires the addition of a new asm-offsets entry for sp0.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 arch/x86/ia32/ia32entry.S        | 3 +--
 arch/x86/kernel/asm-offsets_64.c | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
index ed9746340363..719db63b35c4 100644
--- a/arch/x86/ia32/ia32entry.S
+++ b/arch/x86/ia32/ia32entry.S
@@ -113,8 +113,7 @@ ENTRY(ia32_sysenter_target)
 	CFI_DEF_CFA	rsp,0
 	CFI_REGISTER	rsp,rbp
 	SWAPGS_UNSAFE_STACK
-	movq	PER_CPU_VAR(kernel_stack), %rsp
-	addq	$(KERNEL_STACK_OFFSET),%rsp
+	movq	PER_CPU_VAR(init_tss + TSS_sp0), %rsp
 	/*
 	 * No need to follow this irqs on/off section: the syscall
 	 * disabled irqs, here we enable it straight after entry:
diff --git a/arch/x86/kernel/asm-offsets_64.c b/arch/x86/kernel/asm-offsets_64.c
index fdcbb4d27c9f..5ce6f2da8763 100644
--- a/arch/x86/kernel/asm-offsets_64.c
+++ b/arch/x86/kernel/asm-offsets_64.c
@@ -81,6 +81,7 @@ int main(void)
 #undef ENTRY
 
 	OFFSET(TSS_ist, tss_struct, x86_tss.ist);
+	OFFSET(TSS_sp0, tss_struct, x86_tss.sp0);
 	BLANK();
 
 	DEFINE(__NR_syscall_max, sizeof(syscalls_64) - 1);
-- 
2.1.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0
  2015-02-27  0:07 ` [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0 Andy Lutomirski
@ 2015-02-27 16:13   ` Denys Vlasenko
  2015-02-27 19:56     ` Andy Lutomirski
  2015-02-27 16:52   ` Denys Vlasenko
  1 sibling, 1 reply; 17+ messages in thread
From: Denys Vlasenko @ 2015-02-27 16:13 UTC (permalink / raw)
  To: Andy Lutomirski, x86, linux-kernel
  Cc: Borislav Petkov, Oleg Nesterov, Konrad Rzeszutek Wilk,
	Boris Ostrovsky, Rusty Russell

On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
> This will make modifying the semantics of kernel_stack easier.
> 
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  arch/x86/include/asm/thread_info.h | 3 +--
>  arch/x86/kernel/traps.c            | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index e82e95abc92b..92549053d86d 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
>  static inline struct thread_info *current_thread_info(void)
>  {
>  	struct thread_info *ti;
> -	ti = (void *)(this_cpu_read_stable(kernel_stack) +
> -		      KERNEL_STACK_OFFSET - THREAD_SIZE);
> +	ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
>  	return ti;
>  }
>  
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index c74f2f5652da..d287ea779728 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -174,7 +174,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
>  	 * will catch asm bugs and any attempt to use ist_preempt_enable
>  	 * from double_fault.
>  	 */
> -	BUG_ON(((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
> +	BUG_ON(((current_stack_pointer() ^ (this_cpu_sp0() - 1))
>  		& ~(THREAD_SIZE - 1)) != 0);

While we are at it, I propose a more readable version of this check:

BUG_ON(this_cpu_sp0() - current_stack_pointer() >= THREAD_SIZE);

Yes, I am aware that it is not equivalent to the existing condition
- it uses the fact that this_cpu_sp0(), previous check
wasn't making that assumption. But that assumption is true,
so shouldn't be a problem.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0
  2015-02-27  0:07 ` [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0 Andy Lutomirski
  2015-02-27 16:13   ` Denys Vlasenko
@ 2015-02-27 16:52   ` Denys Vlasenko
  2015-02-27 20:02     ` Andy Lutomirski
  1 sibling, 1 reply; 17+ messages in thread
From: Denys Vlasenko @ 2015-02-27 16:52 UTC (permalink / raw)
  To: Andy Lutomirski, x86, linux-kernel
  Cc: Borislav Petkov, Oleg Nesterov, Konrad Rzeszutek Wilk,
	Boris Ostrovsky, Rusty Russell

On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
> This will make modifying the semantics of kernel_stack easier.
> 
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  arch/x86/include/asm/thread_info.h | 3 +--
>  arch/x86/kernel/traps.c            | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index e82e95abc92b..92549053d86d 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
>  static inline struct thread_info *current_thread_info(void)
>  {
>  	struct thread_info *ti;
> -	ti = (void *)(this_cpu_read_stable(kernel_stack) +
> -		      KERNEL_STACK_OFFSET - THREAD_SIZE);
> +	ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
>  	return ti;
>  }

This makes modpost unhappy, it says these modules need init_tss symbol
to be visible to them:

  Building modules, stage 2.
  MODPOST 2556 modules
ERROR: "init_tss" [sound/usb/usx2y/snd-usb-usx2y.ko] undefined!
ERROR: "init_tss" [sound/pci/hda/snd-hda-codec.ko] undefined!
ERROR: "init_tss" [sound/pci/emu10k1/snd-emu10k1.ko] undefined!
ERROR: "init_tss" [sound/core/snd-pcm.ko] undefined!
ERROR: "init_tss" [sound/core/snd-hwdep.ko] undefined!
ERROR: "init_tss" [sound/core/seq/snd-seq.ko] undefined!
ERROR: "init_tss" [sound/core/oss/snd-pcm-oss.ko] undefined!
ERROR: "init_tss" [net/sunrpc/sunrpc.ko] undefined!
ERROR: "init_tss" [net/sctp/sctp.ko] undefined!
ERROR: "init_tss" [net/rds/rds_tcp.ko] undefined!
ERROR: "init_tss" [net/core/pktgen.ko] undefined!
ERROR: "init_tss" [net/batman-adv/batman-adv.ko] undefined!
ERROR: "init_tss" [net/9p/9pnet.ko] undefined!
ERROR: "init_tss" [fs/reiserfs/reiserfs.ko] undefined!
ERROR: "init_tss" [fs/ocfs2/dlmfs/ocfs2_dlmfs.ko] undefined!
ERROR: "init_tss" [fs/ocfs2/cluster/ocfs2_nodemanager.ko] undefined!
ERROR: "init_tss" [fs/nfsd/nfsd.ko] undefined!
ERROR: "init_tss" [fs/fscache/fscache.ko] undefined!
ERROR: "init_tss" [fs/fat/fat.ko] undefined!
ERROR: "init_tss" [fs/ecryptfs/ecryptfs.ko] undefined!
ERROR: "init_tss" [fs/cachefiles/cachefiles.ko] undefined!
ERROR: "init_tss" [fs/btrfs/btrfs.ko] undefined!
ERROR: "init_tss" [fs/9p/9p.ko] undefined!
ERROR: "init_tss" [drivers/xen/xen-privcmd.ko] undefined!
ERROR: "init_tss" [drivers/vhost/vhost.ko] undefined!
ERROR: "init_tss" [drivers/tty/n_hdlc.ko] undefined!
ERROR: "init_tss" [drivers/target/target_core_file.ko] undefined!
ERROR: "init_tss" [drivers/scsi/pmcraid.ko] undefined!
ERROR: "init_tss" [drivers/scsi/lpfc/lpfc.ko] undefined!
ERROR: "init_tss" [drivers/parport/parport.ko] undefined!
ERROR: "init_tss" [drivers/net/ethernet/broadcom/tg3.ko] undefined!
ERROR: "init_tss" [drivers/net/bonding/bonding.ko] undefined!
ERROR: "init_tss" [drivers/mtd/mtd.ko] undefined!
ERROR: "init_tss" [drivers/misc/vmw_vmci/vmw_vmci.ko] undefined!
ERROR: "init_tss" [drivers/media/v4l2-core/videodev.ko] undefined!
ERROR: "init_tss" [drivers/media/usb/uvc/uvcvideo.ko] undefined!
ERROR: "init_tss" [drivers/media/pci/saa7134/saa7134.ko] undefined!
ERROR: "init_tss" [drivers/media/pci/ivtv/ivtvfb.ko] undefined!
ERROR: "init_tss" [drivers/media/i2c/vpx3220.ko] undefined!
ERROR: "init_tss" [drivers/md/bcache/bcache.ko] undefined!
ERROR: "init_tss" [drivers/isdn/i4l/isdn.ko] undefined!
ERROR: "init_tss" [drivers/input/misc/uinput.ko] undefined!
ERROR: "init_tss" [drivers/infiniband/hw/qib/ib_qib.ko] undefined!
ERROR: "init_tss" [drivers/infiniband/hw/ipath/ib_ipath.ko] undefined!
ERROR: "init_tss" [drivers/infiniband/core/ib_uverbs.ko] undefined!
ERROR: "init_tss" [drivers/hid/uhid.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/radeon/radeon.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/qxl/qxl.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/i915/i915.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/drm.ko] undefined!
ERROR: "init_tss" [drivers/firewire/firewire-core.ko] undefined!
ERROR: "init_tss" [drivers/char/tpm/tpm_tis.ko] undefined!
ERROR: "init_tss" [drivers/char/tpm/tpm.ko] undefined!
ERROR: "init_tss" [drivers/char/pcmcia/cm4000_cs.ko] undefined!
ERROR: "init_tss" [drivers/char/lp.ko] undefined!
ERROR: "init_tss" [drivers/char/ipmi/ipmi_devintf.ko] undefined!
ERROR: "init_tss" [drivers/block/drbd/drbd.ko] undefined!
ERROR: "init_tss" [drivers/acpi/acpi_pad.ko] undefined!
ERROR: "init_tss" [arch/x86/oprofile/oprofile.ko] undefined!
ERROR: "init_tss" [arch/x86/kvm/kvm.ko] undefined!
ERROR: "init_tss" [arch/x86/kvm/kvm-intel.ko] undefined!
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

I took one and investigated how it references init_tss.

drivers/hid/uhid.c:

static int uhid_event_from_user(const char __user *buffer, size_t len,
                                struct uhid_event *event)
{
        if (is_compat_task()) {   <=========== HERE
                u32 type;

                if (get_user(type, buffer))
                        return -EFAULT;

                if (type == UHID_CREATE) {


-- 
vda


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0
  2015-02-27 16:13   ` Denys Vlasenko
@ 2015-02-27 19:56     ` Andy Lutomirski
  2015-02-27 21:11       ` Denys Vlasenko
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Lutomirski @ 2015-02-27 19:56 UTC (permalink / raw)
  To: Denys Vlasenko
  Cc: Borislav Petkov, Rusty Russell, Konrad Rzeszutek Wilk, X86 ML,
	Boris Ostrovsky, Oleg Nesterov, linux-kernel

On Feb 27, 2015 8:13 AM, "Denys Vlasenko" <dvlasenk@redhat.com> wrote:
>
> On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
> > This will make modifying the semantics of kernel_stack easier.
> >
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> > ---
> >  arch/x86/include/asm/thread_info.h | 3 +--
> >  arch/x86/kernel/traps.c            | 2 +-
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> > index e82e95abc92b..92549053d86d 100644
> > --- a/arch/x86/include/asm/thread_info.h
> > +++ b/arch/x86/include/asm/thread_info.h
> > @@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
> >  static inline struct thread_info *current_thread_info(void)
> >  {
> >       struct thread_info *ti;
> > -     ti = (void *)(this_cpu_read_stable(kernel_stack) +
> > -                   KERNEL_STACK_OFFSET - THREAD_SIZE);
> > +     ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
> >       return ti;
> >  }
> >
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index c74f2f5652da..d287ea779728 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -174,7 +174,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
> >        * will catch asm bugs and any attempt to use ist_preempt_enable
> >        * from double_fault.
> >        */
> > -     BUG_ON(((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
> > +     BUG_ON(((current_stack_pointer() ^ (this_cpu_sp0() - 1))
> >               & ~(THREAD_SIZE - 1)) != 0);
>
> While we are at it, I propose a more readable version of this check:
>
> BUG_ON(this_cpu_sp0() - current_stack_pointer() >= THREAD_SIZE);
>
> Yes, I am aware that it is not equivalent to the existing condition
> - it uses the fact that this_cpu_sp0(), previous check
> wasn't making that assumption. But that assumption is true,
> so shouldn't be a problem.

You're missing an absolute value in here, though.  This isn't a check
for overflow; it's a check that we aren't on an IST or other per cpu
stack.

--Andy

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0
  2015-02-27 16:52   ` Denys Vlasenko
@ 2015-02-27 20:02     ` Andy Lutomirski
  0 siblings, 0 replies; 17+ messages in thread
From: Andy Lutomirski @ 2015-02-27 20:02 UTC (permalink / raw)
  To: Denys Vlasenko
  Cc: X86 ML, linux-kernel, Borislav Petkov, Oleg Nesterov,
	Konrad Rzeszutek Wilk, Boris Ostrovsky, Rusty Russell

On Fri, Feb 27, 2015 at 8:52 AM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
> On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
>> This will make modifying the semantics of kernel_stack easier.
>>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> ---
>>  arch/x86/include/asm/thread_info.h | 3 +--
>>  arch/x86/kernel/traps.c            | 2 +-
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
>> index e82e95abc92b..92549053d86d 100644
>> --- a/arch/x86/include/asm/thread_info.h
>> +++ b/arch/x86/include/asm/thread_info.h
>> @@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
>>  static inline struct thread_info *current_thread_info(void)
>>  {
>>       struct thread_info *ti;
>> -     ti = (void *)(this_cpu_read_stable(kernel_stack) +
>> -                   KERNEL_STACK_OFFSET - THREAD_SIZE);
>> +     ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
>>       return ti;
>>  }
>
> This makes modpost unhappy, it says these modules need init_tss symbol
> to be visible to them:
>
>   Building modules, stage 2.
>   MODPOST 2556 modules
> ERROR: "init_tss" [sound/usb/usx2y/snd-usb-usx2y.ko] undefined!
> ERROR: "init_tss" [sound/pci/hda/snd-hda-codec.ko] undefined!
> ERROR: "init_tss" [sound/pci/emu10k1/snd-emu10k1.ko] undefined!
> ERROR: "init_tss" [sound/core/snd-pcm.ko] undefined!
> ERROR: "init_tss" [sound/core/snd-hwdep.ko] undefined!
> ERROR: "init_tss" [sound/core/seq/snd-seq.ko] undefined!
> ERROR: "init_tss" [sound/core/oss/snd-pcm-oss.ko] undefined!
> ERROR: "init_tss" [net/sunrpc/sunrpc.ko] undefined!
> ERROR: "init_tss" [net/sctp/sctp.ko] undefined!
> ERROR: "init_tss" [net/rds/rds_tcp.ko] undefined!
> ERROR: "init_tss" [net/core/pktgen.ko] undefined!
> ERROR: "init_tss" [net/batman-adv/batman-adv.ko] undefined!
> ERROR: "init_tss" [net/9p/9pnet.ko] undefined!
> ERROR: "init_tss" [fs/reiserfs/reiserfs.ko] undefined!
> ERROR: "init_tss" [fs/ocfs2/dlmfs/ocfs2_dlmfs.ko] undefined!
> ERROR: "init_tss" [fs/ocfs2/cluster/ocfs2_nodemanager.ko] undefined!
> ERROR: "init_tss" [fs/nfsd/nfsd.ko] undefined!
> ERROR: "init_tss" [fs/fscache/fscache.ko] undefined!
> ERROR: "init_tss" [fs/fat/fat.ko] undefined!
> ERROR: "init_tss" [fs/ecryptfs/ecryptfs.ko] undefined!
> ERROR: "init_tss" [fs/cachefiles/cachefiles.ko] undefined!
> ERROR: "init_tss" [fs/btrfs/btrfs.ko] undefined!
> ERROR: "init_tss" [fs/9p/9p.ko] undefined!
> ERROR: "init_tss" [drivers/xen/xen-privcmd.ko] undefined!
> ERROR: "init_tss" [drivers/vhost/vhost.ko] undefined!
> ERROR: "init_tss" [drivers/tty/n_hdlc.ko] undefined!
> ERROR: "init_tss" [drivers/target/target_core_file.ko] undefined!
> ERROR: "init_tss" [drivers/scsi/pmcraid.ko] undefined!
> ERROR: "init_tss" [drivers/scsi/lpfc/lpfc.ko] undefined!
> ERROR: "init_tss" [drivers/parport/parport.ko] undefined!
> ERROR: "init_tss" [drivers/net/ethernet/broadcom/tg3.ko] undefined!
> ERROR: "init_tss" [drivers/net/bonding/bonding.ko] undefined!
> ERROR: "init_tss" [drivers/mtd/mtd.ko] undefined!
> ERROR: "init_tss" [drivers/misc/vmw_vmci/vmw_vmci.ko] undefined!
> ERROR: "init_tss" [drivers/media/v4l2-core/videodev.ko] undefined!
> ERROR: "init_tss" [drivers/media/usb/uvc/uvcvideo.ko] undefined!
> ERROR: "init_tss" [drivers/media/pci/saa7134/saa7134.ko] undefined!
> ERROR: "init_tss" [drivers/media/pci/ivtv/ivtvfb.ko] undefined!
> ERROR: "init_tss" [drivers/media/i2c/vpx3220.ko] undefined!
> ERROR: "init_tss" [drivers/md/bcache/bcache.ko] undefined!
> ERROR: "init_tss" [drivers/isdn/i4l/isdn.ko] undefined!
> ERROR: "init_tss" [drivers/input/misc/uinput.ko] undefined!
> ERROR: "init_tss" [drivers/infiniband/hw/qib/ib_qib.ko] undefined!
> ERROR: "init_tss" [drivers/infiniband/hw/ipath/ib_ipath.ko] undefined!
> ERROR: "init_tss" [drivers/infiniband/core/ib_uverbs.ko] undefined!
> ERROR: "init_tss" [drivers/hid/uhid.ko] undefined!
> ERROR: "init_tss" [drivers/gpu/drm/radeon/radeon.ko] undefined!
> ERROR: "init_tss" [drivers/gpu/drm/qxl/qxl.ko] undefined!
> ERROR: "init_tss" [drivers/gpu/drm/i915/i915.ko] undefined!
> ERROR: "init_tss" [drivers/gpu/drm/drm.ko] undefined!
> ERROR: "init_tss" [drivers/firewire/firewire-core.ko] undefined!
> ERROR: "init_tss" [drivers/char/tpm/tpm_tis.ko] undefined!
> ERROR: "init_tss" [drivers/char/tpm/tpm.ko] undefined!
> ERROR: "init_tss" [drivers/char/pcmcia/cm4000_cs.ko] undefined!
> ERROR: "init_tss" [drivers/char/lp.ko] undefined!
> ERROR: "init_tss" [drivers/char/ipmi/ipmi_devintf.ko] undefined!
> ERROR: "init_tss" [drivers/block/drbd/drbd.ko] undefined!
> ERROR: "init_tss" [drivers/acpi/acpi_pad.ko] undefined!
> ERROR: "init_tss" [arch/x86/oprofile/oprofile.ko] undefined!
> ERROR: "init_tss" [arch/x86/kvm/kvm.ko] undefined!
> ERROR: "init_tss" [arch/x86/kvm/kvm-intel.ko] undefined!
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
>
> I took one and investigated how it references init_tss.
>
> drivers/hid/uhid.c:
>
> static int uhid_event_from_user(const char __user *buffer, size_t len,
>                                 struct uhid_event *event)
> {
>         if (is_compat_task()) {   <=========== HERE
>                 u32 type;
>
>                 if (get_user(type, buffer))
>                         return -EFAULT;
>
>                 if (type == UHID_CREATE) {
>
>

Whoops!  I fixed it locally with an EXPORT_SYMBOL_GPL.

--Andy

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0
  2015-02-27 19:56     ` Andy Lutomirski
@ 2015-02-27 21:11       ` Denys Vlasenko
  2015-02-27 22:39         ` Andy Lutomirski
  0 siblings, 1 reply; 17+ messages in thread
From: Denys Vlasenko @ 2015-02-27 21:11 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Borislav Petkov, Rusty Russell, Konrad Rzeszutek Wilk, X86 ML,
	Boris Ostrovsky, Oleg Nesterov, linux-kernel

On 02/27/2015 08:56 PM, Andy Lutomirski wrote:
> On Feb 27, 2015 8:13 AM, "Denys Vlasenko" <dvlasenk@redhat.com> wrote:
>>
>> On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
>>> This will make modifying the semantics of kernel_stack easier.
>>>
>>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>>> ---
>>>  arch/x86/include/asm/thread_info.h | 3 +--
>>>  arch/x86/kernel/traps.c            | 2 +-
>>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
>>> index e82e95abc92b..92549053d86d 100644
>>> --- a/arch/x86/include/asm/thread_info.h
>>> +++ b/arch/x86/include/asm/thread_info.h
>>> @@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
>>>  static inline struct thread_info *current_thread_info(void)
>>>  {
>>>       struct thread_info *ti;
>>> -     ti = (void *)(this_cpu_read_stable(kernel_stack) +
>>> -                   KERNEL_STACK_OFFSET - THREAD_SIZE);
>>> +     ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
>>>       return ti;
>>>  }
>>>
>>> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
>>> index c74f2f5652da..d287ea779728 100644
>>> --- a/arch/x86/kernel/traps.c
>>> +++ b/arch/x86/kernel/traps.c
>>> @@ -174,7 +174,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
>>>        * will catch asm bugs and any attempt to use ist_preempt_enable
>>>        * from double_fault.
>>>        */
>>> -     BUG_ON(((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
>>> +     BUG_ON(((current_stack_pointer() ^ (this_cpu_sp0() - 1))
>>>               & ~(THREAD_SIZE - 1)) != 0);
>>
>> While we are at it, I propose a more readable version of this check:
>>
>> BUG_ON(this_cpu_sp0() - current_stack_pointer() >= THREAD_SIZE);
>>
>> Yes, I am aware that it is not equivalent to the existing condition
>> - it uses the fact that this_cpu_sp0(), previous check
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Oops...
I meant to say "...the fact that this_cpu_sp0() points to the very top
of the stack, previous check ..."

>> wasn't making that assumption. But that assumption is true,
>> so shouldn't be a problem.
> 
> You're missing an absolute value in here, though.  This isn't a check
> for overflow; it's a check that we aren't on an IST or other per cpu
> stack.

Yes, that's exactly what the condition checks for. It reads

"is current stack pointer below task's kernel stack by no more
than THREAD_SIZE?"

which is only possible if we are on task's kernel stack:

If current_stack_pointer() is elsewhere, it is either
(a) much smaller than this_cpu_sp0(), and BUG_ON condition
    obviously triggers; or
(b) it is somewhere above this_cpu_sp0(), in which case subtraction
    overflows and condition triggers too.

How would you write this condition so that it's easily readable?
Evidently, my version isn't as readable sa I hoped...

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0
  2015-02-27 21:11       ` Denys Vlasenko
@ 2015-02-27 22:39         ` Andy Lutomirski
  0 siblings, 0 replies; 17+ messages in thread
From: Andy Lutomirski @ 2015-02-27 22:39 UTC (permalink / raw)
  To: Denys Vlasenko
  Cc: linux-kernel, Borislav Petkov, X86 ML, Konrad Rzeszutek Wilk,
	Rusty Russell, Boris Ostrovsky, Oleg Nesterov

On Feb 27, 2015 1:12 PM, "Denys Vlasenko" <dvlasenk@redhat.com> wrote:
>
> On 02/27/2015 08:56 PM, Andy Lutomirski wrote:
> > On Feb 27, 2015 8:13 AM, "Denys Vlasenko" <dvlasenk@redhat.com> wrote:
> >>
> >> On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
> >>> This will make modifying the semantics of kernel_stack easier.
> >>>
> >>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >>> Cc: Rusty Russell <rusty@rustcorp.com.au>
> >>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> >>> ---
> >>>  arch/x86/include/asm/thread_info.h | 3 +--
> >>>  arch/x86/kernel/traps.c            | 2 +-
> >>>  2 files changed, 2 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> >>> index e82e95abc92b..92549053d86d 100644
> >>> --- a/arch/x86/include/asm/thread_info.h
> >>> +++ b/arch/x86/include/asm/thread_info.h
> >>> @@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
> >>>  static inline struct thread_info *current_thread_info(void)
> >>>  {
> >>>       struct thread_info *ti;
> >>> -     ti = (void *)(this_cpu_read_stable(kernel_stack) +
> >>> -                   KERNEL_STACK_OFFSET - THREAD_SIZE);
> >>> +     ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
> >>>       return ti;
> >>>  }
> >>>
> >>> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> >>> index c74f2f5652da..d287ea779728 100644
> >>> --- a/arch/x86/kernel/traps.c
> >>> +++ b/arch/x86/kernel/traps.c
> >>> @@ -174,7 +174,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
> >>>        * will catch asm bugs and any attempt to use ist_preempt_enable
> >>>        * from double_fault.
> >>>        */
> >>> -     BUG_ON(((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
> >>> +     BUG_ON(((current_stack_pointer() ^ (this_cpu_sp0() - 1))
> >>>               & ~(THREAD_SIZE - 1)) != 0);
> >>
> >> While we are at it, I propose a more readable version of this check:
> >>
> >> BUG_ON(this_cpu_sp0() - current_stack_pointer() >= THREAD_SIZE);
> >>
> >> Yes, I am aware that it is not equivalent to the existing condition
> >> - it uses the fact that this_cpu_sp0(), previous check
>              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Oops...
> I meant to say "...the fact that this_cpu_sp0() points to the very top
> of the stack, previous check ..."
>
> >> wasn't making that assumption. But that assumption is true,
> >> so shouldn't be a problem.
> >
> > You're missing an absolute value in here, though.  This isn't a check
> > for overflow; it's a check that we aren't on an IST or other per cpu
> > stack.
>
> Yes, that's exactly what the condition checks for. It reads
>
> "is current stack pointer below task's kernel stack by no more
> than THREAD_SIZE?"
>
> which is only possible if we are on task's kernel stack:
>
> If current_stack_pointer() is elsewhere, it is either
> (a) much smaller than this_cpu_sp0(), and BUG_ON condition
>     obviously triggers; or
> (b) it is somewhere above this_cpu_sp0(), in which case subtraction
>     overflows and condition triggers too.
>

Right, it's unsigned.  I like yours better if I add a comment.  It
also avoids relying on the stack being aligned to its full size.

--Andy

> How would you write this condition so that it's easily readable?
> Evidently, my version isn't as readable sa I hoped...

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu
  2015-02-27  0:07 ` [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu Andy Lutomirski
@ 2015-03-04  8:02   ` Ingo Molnar
  2015-03-04 18:51     ` Andy Lutomirski
  0 siblings, 1 reply; 17+ messages in thread
From: Ingo Molnar @ 2015-03-04  8:02 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: x86, linux-kernel, Borislav Petkov, Oleg Nesterov,
	Denys Vlasenko, Linus Torvalds


* Andy Lutomirski <luto@amacapital.net> wrote:

> We currently store references to the top of the kernel stack in 
> multiple places: kernel_stack (with an offset) and 
> init_tss.x86_tss.sp0 (no offset).  The latter is defined by hardware 
> and is a clean canonical way to find the top of the stack.  Add an

Btw., 'per_cpu(init_tss)' is a somewhat misleading name these days, as 
there's nothing 'init' about it anymore - we load it during CPU init 
and then manually maintain its contents. A better name would be 
'current_tss' - referring to both the current CPU and the current 
task?

> This needs minor paravirt tweaks to ensure that On native, sp0

nit: s/On/on/

> defines the top of the kernel stack.  On Xen and lguest, the 
> hypervisor tracks it, but we want to start reading sp0 in the 
> kernel.  Fixing this is simple: just update our local copy of sp0 as 
> well as the hypervisor's copy on task switches.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu
  2015-03-04  8:02   ` Ingo Molnar
@ 2015-03-04 18:51     ` Andy Lutomirski
  2015-03-04 20:39       ` Ingo Molnar
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Lutomirski @ 2015-03-04 18:51 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: X86 ML, linux-kernel, Borislav Petkov, Oleg Nesterov,
	Denys Vlasenko, Linus Torvalds

On Wed, Mar 4, 2015 at 12:02 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@amacapital.net> wrote:
>
>> We currently store references to the top of the kernel stack in
>> multiple places: kernel_stack (with an offset) and
>> init_tss.x86_tss.sp0 (no offset).  The latter is defined by hardware
>> and is a clean canonical way to find the top of the stack.  Add an
>
> Btw., 'per_cpu(init_tss)' is a somewhat misleading name these days, as
> there's nothing 'init' about it anymore - we load it during CPU init
> and then manually maintain its contents. A better name would be
> 'current_tss' - referring to both the current CPU and the current
> task?

Hmm.  That seems a little odd to me, since we never change the TSS
pointer.  It's certainly better than init_tss, though.  I'll add a
followup to rename it.

>
>> This needs minor paravirt tweaks to ensure that On native, sp0
>
> nit: s/On/on/
>
>> defines the top of the kernel stack.  On Xen and lguest, the
>> hypervisor tracks it, but we want to start reading sp0 in the
>> kernel.  Fixing this is simple: just update our local copy of sp0 as
>> well as the hypervisor's copy on task switches.

Thanks,
Andy

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu
  2015-03-04 18:51     ` Andy Lutomirski
@ 2015-03-04 20:39       ` Ingo Molnar
  2015-03-04 20:41         ` Andy Lutomirski
  0 siblings, 1 reply; 17+ messages in thread
From: Ingo Molnar @ 2015-03-04 20:39 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: X86 ML, linux-kernel, Borislav Petkov, Oleg Nesterov,
	Denys Vlasenko, Linus Torvalds


* Andy Lutomirski <luto@amacapital.net> wrote:

> On Wed, Mar 4, 2015 at 12:02 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Andy Lutomirski <luto@amacapital.net> wrote:
> >
> >> We currently store references to the top of the kernel stack in
> >> multiple places: kernel_stack (with an offset) and
> >> init_tss.x86_tss.sp0 (no offset).  The latter is defined by hardware
> >> and is a clean canonical way to find the top of the stack.  Add an
> >
> > Btw., 'per_cpu(init_tss)' is a somewhat misleading name these days, as
> > there's nothing 'init' about it anymore - we load it during CPU init
> > and then manually maintain its contents. A better name would be
> > 'current_tss' - referring to both the current CPU and the current
> > task?
> 
> Hmm.  That seems a little odd to me, since we never change the TSS 
> pointer.  It's certainly better than init_tss, though.  I'll add a 
> followup to rename it.

Alternatively we could use 'cpu_tss': the CPU's current TSS.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu
  2015-03-04 20:39       ` Ingo Molnar
@ 2015-03-04 20:41         ` Andy Lutomirski
  2015-03-04 21:05           ` Borislav Petkov
  2015-03-04 22:12           ` Ingo Molnar
  0 siblings, 2 replies; 17+ messages in thread
From: Andy Lutomirski @ 2015-03-04 20:41 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: X86 ML, linux-kernel, Borislav Petkov, Oleg Nesterov,
	Denys Vlasenko, Linus Torvalds

On Wed, Mar 4, 2015 at 12:39 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@amacapital.net> wrote:
>
>> On Wed, Mar 4, 2015 at 12:02 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Andy Lutomirski <luto@amacapital.net> wrote:
>> >
>> >> We currently store references to the top of the kernel stack in
>> >> multiple places: kernel_stack (with an offset) and
>> >> init_tss.x86_tss.sp0 (no offset).  The latter is defined by hardware
>> >> and is a clean canonical way to find the top of the stack.  Add an
>> >
>> > Btw., 'per_cpu(init_tss)' is a somewhat misleading name these days, as
>> > there's nothing 'init' about it anymore - we load it during CPU init
>> > and then manually maintain its contents. A better name would be
>> > 'current_tss' - referring to both the current CPU and the current
>> > task?
>>
>> Hmm.  That seems a little odd to me, since we never change the TSS
>> pointer.  It's certainly better than init_tss, though.  I'll add a
>> followup to rename it.
>
> Alternatively we could use 'cpu_tss': the CPU's current TSS.

I painted my bikeshed "singleton_tss", since cpu_tss seemed redundant
for something that's already per cpu.  If you prefer your bikeshed
color, let me know.

--Andy

>
> Thanks,
>
>         Ingo



-- 
Andy Lutomirski
AMA Capital Management, LLC

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu
  2015-03-04 20:41         ` Andy Lutomirski
@ 2015-03-04 21:05           ` Borislav Petkov
  2015-03-04 22:12           ` Ingo Molnar
  1 sibling, 0 replies; 17+ messages in thread
From: Borislav Petkov @ 2015-03-04 21:05 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Ingo Molnar, X86 ML, linux-kernel, Oleg Nesterov, Denys Vlasenko,
	Linus Torvalds

On Wed, Mar 04, 2015 at 12:41:35PM -0800, Andy Lutomirski wrote:
> I painted my bikeshed "singleton_tss", since cpu_tss seemed redundant
> for something that's already per cpu.  If you prefer your bikeshed
> color, let me know.

I want "pink_pony_tss".

:-P

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu
  2015-03-04 20:41         ` Andy Lutomirski
  2015-03-04 21:05           ` Borislav Petkov
@ 2015-03-04 22:12           ` Ingo Molnar
  2015-03-04 22:20             ` Andy Lutomirski
  1 sibling, 1 reply; 17+ messages in thread
From: Ingo Molnar @ 2015-03-04 22:12 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: X86 ML, linux-kernel, Borislav Petkov, Oleg Nesterov,
	Denys Vlasenko, Linus Torvalds


* Andy Lutomirski <luto@amacapital.net> wrote:

> On Wed, Mar 4, 2015 at 12:39 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Andy Lutomirski <luto@amacapital.net> wrote:
> >
> >> On Wed, Mar 4, 2015 at 12:02 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >> >
> >> > * Andy Lutomirski <luto@amacapital.net> wrote:
> >> >
> >> >> We currently store references to the top of the kernel stack in
> >> >> multiple places: kernel_stack (with an offset) and
> >> >> init_tss.x86_tss.sp0 (no offset).  The latter is defined by hardware
> >> >> and is a clean canonical way to find the top of the stack.  Add an
> >> >
> >> > Btw., 'per_cpu(init_tss)' is a somewhat misleading name these days, as
> >> > there's nothing 'init' about it anymore - we load it during CPU init
> >> > and then manually maintain its contents. A better name would be
> >> > 'current_tss' - referring to both the current CPU and the current
> >> > task?
> >>
> >> Hmm.  That seems a little odd to me, since we never change the TSS
> >> pointer.  It's certainly better than init_tss, though.  I'll add a
> >> followup to rename it.
> >
> > Alternatively we could use 'cpu_tss': the CPU's current TSS.
> 
> I painted my bikeshed "singleton_tss", since cpu_tss seemed 
> redundant for something that's already per cpu.  If you prefer your 
> bikeshed color, let me know.

Yeah, name it *anything* but 'singleton'!!

/me gets sick of design patterns ;-)

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu
  2015-03-04 22:12           ` Ingo Molnar
@ 2015-03-04 22:20             ` Andy Lutomirski
  0 siblings, 0 replies; 17+ messages in thread
From: Andy Lutomirski @ 2015-03-04 22:20 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: X86 ML, linux-kernel, Borislav Petkov, Oleg Nesterov,
	Denys Vlasenko, Linus Torvalds

On Wed, Mar 4, 2015 at 2:12 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@amacapital.net> wrote:
>
>> On Wed, Mar 4, 2015 at 12:39 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Andy Lutomirski <luto@amacapital.net> wrote:
>> >
>> >> On Wed, Mar 4, 2015 at 12:02 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >> >
>> >> > * Andy Lutomirski <luto@amacapital.net> wrote:
>> >> >
>> >> >> We currently store references to the top of the kernel stack in
>> >> >> multiple places: kernel_stack (with an offset) and
>> >> >> init_tss.x86_tss.sp0 (no offset).  The latter is defined by hardware
>> >> >> and is a clean canonical way to find the top of the stack.  Add an
>> >> >
>> >> > Btw., 'per_cpu(init_tss)' is a somewhat misleading name these days, as
>> >> > there's nothing 'init' about it anymore - we load it during CPU init
>> >> > and then manually maintain its contents. A better name would be
>> >> > 'current_tss' - referring to both the current CPU and the current
>> >> > task?
>> >>
>> >> Hmm.  That seems a little odd to me, since we never change the TSS
>> >> pointer.  It's certainly better than init_tss, though.  I'll add a
>> >> followup to rename it.
>> >
>> > Alternatively we could use 'cpu_tss': the CPU's current TSS.
>>
>> I painted my bikeshed "singleton_tss", since cpu_tss seemed
>> redundant for something that's already per cpu.  If you prefer your
>> bikeshed color, let me know.
>
> Yeah, name it *anything* but 'singleton'!!
>
> /me gets sick of design patterns ;-)

My tss factory laughs at your silly ideas :)

cpu_tss it is.

--Andy

>
> Thanks,
>
>         Ingo



-- 
Andy Lutomirski
AMA Capital Management, LLC

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2015-03-04 22:20 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-26 23:52 [RFC 0/3] Baby steps toward cleaning up KERNEL_STACK_OFFSET Andy Lutomirski
2015-02-27  0:07 ` [RFC 1/3] x86: Add this_cpu_sp0() to read sp0 for the current cpu Andy Lutomirski
2015-03-04  8:02   ` Ingo Molnar
2015-03-04 18:51     ` Andy Lutomirski
2015-03-04 20:39       ` Ingo Molnar
2015-03-04 20:41         ` Andy Lutomirski
2015-03-04 21:05           ` Borislav Petkov
2015-03-04 22:12           ` Ingo Molnar
2015-03-04 22:20             ` Andy Lutomirski
2015-02-27  0:07 ` [RFC 2/3] x86: Switch all C consumers of kernel_stack to this_cpu_sp0 Andy Lutomirski
2015-02-27 16:13   ` Denys Vlasenko
2015-02-27 19:56     ` Andy Lutomirski
2015-02-27 21:11       ` Denys Vlasenko
2015-02-27 22:39         ` Andy Lutomirski
2015-02-27 16:52   ` Denys Vlasenko
2015-02-27 20:02     ` Andy Lutomirski
2015-02-27  0:07 ` [RFC 3/3] x86, asm: Change the 32-bit sysenter code to use sp0 Andy Lutomirski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).