Re: [PATCH] x86: Reduce duplicated code in the x86_64 context switch path.

["H. Peter Anvin" <hpa@zytor.com>]

This is a net addition in code, and send to only make it harder to understand what is happening.  As such I don't think this is a good idea.

Joe Damato <ice799@gmail.com> wrote:

>Signed-off-by: Joe Damato <ice799@gmail.com>
>---
> arch/x86/include/asm/switch_to.h |   30 ++++++++++++++++++++++++++++++
> arch/x86/kernel/process_64.c     |   29 ++---------------------------
> 2 files changed, 32 insertions(+), 27 deletions(-)
>
>diff --git a/arch/x86/include/asm/switch_to.h
>b/arch/x86/include/asm/switch_to.h
>index 4ec45b3..a322cc6 100644
>--- a/arch/x86/include/asm/switch_to.h
>+++ b/arch/x86/include/asm/switch_to.h
>@@ -124,6 +124,36 @@ do {									\
> 	       __switch_canary_iparam					  \
> 	     : "memory", "cc" __EXTRA_CLOBBER)
> 
>+#define loadsegment_fs(fs, index)					  \
>+	loadsegment(fs, index)
>+
>+#define loadsegment_gs(gs, index)					  \
>+	load_gs_index(index)
>+
>+#define switch_segment(prev, next, index, seg, msr)			       \
>+	do {								       \
>+		/*							       \
>+		 * Segment register != 0 always requires a reload.  Also       \
>+		 * reload when it has changed.  When prev process used 64bit   \
>+		 * base always reload to avoid an information leak.	       \
>+		 */							       \
>+		if (unlikely(index | next->index | prev->seg)) {	       \
>+			loadsegment_##seg(seg, next->index);		       \
>+			/*						       \
>+			 * Check if the user used a selector != 0; if yes      \
>+			 *  clear 64bit base, since overloaded base is always  \
>+			 *  mapped to the Null selector			       \
>+			 */						       \
>+			if (index)					       \
>+			  prev->seg = 0;				       \
>+		}							       \
>+									       \
>+		/* when next process has a 64bit base use it */		       \
>+		if (next->seg)						       \
>+		  wrmsrl(msr, next->seg);				       \
>+		prev->index = index;					       \
>+	} while (0)
>+
> #endif /* CONFIG_X86_32 */
> 
> #endif /* _ASM_X86_SWITCH_TO_H */
>diff --git a/arch/x86/kernel/process_64.c
>b/arch/x86/kernel/process_64.c
>index 355ae06..f41d026 100644
>--- a/arch/x86/kernel/process_64.c
>+++ b/arch/x86/kernel/process_64.c
>@@ -318,34 +318,9 @@ __switch_to(struct task_struct *prev_p, struct
>task_struct *next_p)
> 
> 	/*
> 	 * Switch FS and GS.
>-	 *
>-	 * Segment register != 0 always requires a reload.  Also
>-	 * reload when it has changed.  When prev process used 64bit
>-	 * base always reload to avoid an information leak.
> 	 */
>-	if (unlikely(fsindex | next->fsindex | prev->fs)) {
>-		loadsegment(fs, next->fsindex);
>-		/*
>-		 * Check if the user used a selector != 0; if yes
>-		 *  clear 64bit base, since overloaded base is always
>-		 *  mapped to the Null selector
>-		 */
>-		if (fsindex)
>-			prev->fs = 0;
>-	}
>-	/* when next process has a 64bit base use it */
>-	if (next->fs)
>-		wrmsrl(MSR_FS_BASE, next->fs);
>-	prev->fsindex = fsindex;
>-
>-	if (unlikely(gsindex | next->gsindex | prev->gs)) {
>-		load_gs_index(next->gsindex);
>-		if (gsindex)
>-			prev->gs = 0;
>-	}
>-	if (next->gs)
>-		wrmsrl(MSR_KERNEL_GS_BASE, next->gs);
>-	prev->gsindex = gsindex;
>+	switch_segment(prev, next, fsindex, fs, MSR_FS_BASE);
>+	switch_segment(prev, next, gsindex, gs, MSR_KERNEL_GS_BASE);
> 
> 	switch_fpu_finish(next_p, fpu);
> 

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.