| Message ID | 1531308586-29340-4-git-send-email-joro@8bytes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> On Jul 11, 2018, at 4:29 AM, Joerg Roedel <joro@8bytes.org> wrote: > > From: Joerg Roedel <jroedel@suse.de> > We want x86_tss.sp0 point to the entry stack later to use > it as a trampoline stack for other kernel entry points > besides SYSENTER. Makes sense: sp0 will be the entry stack. But: > > > /* Offset from the sysenter stack to tss.sp0 */ > - DEFINE(TSS_entry_stack, offsetof(struct cpu_entry_area, tss.x86_tss.sp0) - > + DEFINE(TSS_entry_stack, offsetof(struct cpu_entry_area, tss.x86_tss.sp1) - > offsetofend(struct cpu_entry_area, entry_stack_page.stack)); > The code reads differently. Did you perhaps mean TSS_task_stack? Also, the “top of task stack” is a bit weird on 32-bit due to vm86. Can you document *exactly* what goes in sp1?
On Thu, Jul 12, 2018 at 01:49:13PM -0700, Andy Lutomirski wrote: > > On Jul 11, 2018, at 4:29 AM, Joerg Roedel <joro@8bytes.org> wrote: > > /* Offset from the sysenter stack to tss.sp0 */ > > - DEFINE(TSS_entry_stack, offsetof(struct cpu_entry_area, tss.x86_tss.sp0) - > > + DEFINE(TSS_entry_stack, offsetof(struct cpu_entry_area, tss.x86_tss.sp1) - > > offsetofend(struct cpu_entry_area, entry_stack_page.stack)); > > > > The code reads differently. Did you perhaps mean TSS_task_stack? Well, the offset name came from TSS_sysenter_sp0, which was the offset from the sysenter_sp0 (==sysenter-stack) to the task stack in TSS, now sysenter_sp0 became entry_stack, because its used for all entry points and not only sysenter. So with the old convention the naming makes still sense, no? > Also, the “top of task stack” is a bit weird on 32-bit due to vm86. > Can you document *exactly* what goes in sp1? Will do, thanks for your feedback! Joerg
On Fri, Jul 13, 2018 at 2:48 AM, Joerg Roedel <joro@8bytes.org> wrote: > On Thu, Jul 12, 2018 at 01:49:13PM -0700, Andy Lutomirski wrote: >> > On Jul 11, 2018, at 4:29 AM, Joerg Roedel <joro@8bytes.org> wrote: >> > /* Offset from the sysenter stack to tss.sp0 */ >> > - DEFINE(TSS_entry_stack, offsetof(struct cpu_entry_area, tss.x86_tss.sp0) - >> > + DEFINE(TSS_entry_stack, offsetof(struct cpu_entry_area, tss.x86_tss.sp1) - >> > offsetofend(struct cpu_entry_area, entry_stack_page.stack)); >> > >> >> The code reads differently. Did you perhaps mean TSS_task_stack? > > Well, the offset name came from TSS_sysenter_sp0, which was the offset > from the sysenter_sp0 (==sysenter-stack) to the task stack in TSS, now > sysenter_sp0 became entry_stack, because its used for all entry points > and not only sysenter. So with the old convention the naming makes still > sense, no? > Trying to parse it certainly makes my brain hurt a bit. This is the offset from the entry stack to sp1, where sp1 is the location of the pointer to the task stack. Maybe all the arithmetic could go in entry_32.S and the asm-offset name could just be TSS_sp1, just like on 64-bit? --Andy
On Fri, Jul 13, 2018 at 10:19 AM, Andy Lutomirski <luto@kernel.org> wrote: > On Fri, Jul 13, 2018 at 2:48 AM, Joerg Roedel <joro@8bytes.org> wrote: >> On Thu, Jul 12, 2018 at 01:49:13PM -0700, Andy Lutomirski wrote: >>> > On Jul 11, 2018, at 4:29 AM, Joerg Roedel <joro@8bytes.org> wrote: >>> > /* Offset from the sysenter stack to tss.sp0 */ >>> > - DEFINE(TSS_entry_stack, offsetof(struct cpu_entry_area, tss.x86_tss.sp0) - >>> > + DEFINE(TSS_entry_stack, offsetof(struct cpu_entry_area, tss.x86_tss.sp1) - >>> > offsetofend(struct cpu_entry_area, entry_stack_page.stack)); >>> > >>> >>> The code reads differently. Did you perhaps mean TSS_task_stack? >> >> Well, the offset name came from TSS_sysenter_sp0, which was the offset >> from the sysenter_sp0 (==sysenter-stack) to the task stack in TSS, now >> sysenter_sp0 became entry_stack, because its used for all entry points >> and not only sysenter. So with the old convention the naming makes still >> sense, no? >> > > Trying to parse it certainly makes my brain hurt a bit. This is the > offset from the entry stack to sp1, where sp1 is the location of the > pointer to the task stack. > > Maybe all the arithmetic could go in entry_32.S and the asm-offset > name could just be TSS_sp1, just like on 64-bit? > I re-read it again. How about keeping TSS_entry_stack but making it be the offset from the TSS to the entry stack. Then do the arithmetic in asm.
On Fri, Jul 13, 2018 at 04:17:40PM -0700, Andy Lutomirski wrote: > I re-read it again. How about keeping TSS_entry_stack but making it > be the offset from the TSS to the entry stack. Then do the arithmetic > in asm. Hmm, I think its better to keep the arithmetic in the C file for better readability. How about renaming it to TSS_entry2task_stack? Regards, Joerg
On Tue, Jul 17, 2018 at 12:05 AM, Joerg Roedel <jroedel@suse.de> wrote: > On Fri, Jul 13, 2018 at 04:17:40PM -0700, Andy Lutomirski wrote: >> I re-read it again. How about keeping TSS_entry_stack but making it >> be the offset from the TSS to the entry stack. Then do the arithmetic >> in asm. > > Hmm, I think its better to keep the arithmetic in the C file for better > readability. How about renaming it to TSS_entry2task_stack? That's okay with me. > > > Regards, > > Joerg
diff --git a/arch/x86/kernel/asm-offsets_32.c b/arch/x86/kernel/asm-offsets_32.c index ab2d949..36d77d3 100644 --- a/arch/x86/kernel/asm-offsets_32.c +++ b/arch/x86/kernel/asm-offsets_32.c @@ -47,7 +47,7 @@ void foo(void) BLANK(); /* Offset from the sysenter stack to tss.sp0 */ - DEFINE(TSS_entry_stack, offsetof(struct cpu_entry_area, tss.x86_tss.sp0) - + DEFINE(TSS_entry_stack, offsetof(struct cpu_entry_area, tss.x86_tss.sp1) - offsetofend(struct cpu_entry_area, entry_stack_page.stack)); #ifdef CONFIG_STACKPROTECTOR diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c index 0ae659d..ec62cc7 100644 --- a/arch/x86/kernel/process_32.c +++ b/arch/x86/kernel/process_32.c @@ -290,6 +290,8 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p) this_cpu_write(cpu_current_top_of_stack, (unsigned long)task_stack_page(next_p) + THREAD_SIZE); + /* SYSENTER reads the task-stack from tss.sp1 */ + this_cpu_write(cpu_tss_rw.x86_tss.sp1, next_p->thread.sp0); /* * Restore %gs if needed (which is common)