Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1665761 > unrolled thread

[PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the path

Started byBrian Gerst <brgerst@gmail.com>
First post2017-06-14 14:50 +0200
Last post2017-06-15 03:40 +0200
Articles 8 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the path Brian Gerst <brgerst@gmail.com> - 2017-06-14 14:50 +0200
    Re: [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the  path Juergen Gross <jgross@suse.com> - 2017-06-14 15:20 +0200
      Re: [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the path Brian Gerst <brgerst@gmail.com> - 2017-06-14 19:40 +0200
        Re: [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the  path Juergen Gross <jgross@suse.com> - 2017-06-14 19:40 +0200
    Re: [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the path Andy Lutomirski <luto@kernel.org> - 2017-06-14 19:50 +0200
      Re: [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the  path Andrew Cooper <andrew.cooper3@citrix.com> - 2017-06-14 20:10 +0200
        Re: [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the path Brian Gerst <brgerst@gmail.com> - 2017-06-14 20:30 +0200
          Re: [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the path Andy Lutomirski <luto@kernel.org> - 2017-06-15 03:40 +0200

#1665761 — [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the path

FromBrian Gerst <brgerst@gmail.com>
Date2017-06-14 14:50 +0200
Subject[PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the path
Message-ID<tSfUd-5dU-7@gated-at.bofh.it>
Since tasks using IOPL are very rare, move the switching code to the slow
path for lower impact on normal tasks.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
---
 arch/x86/include/asm/paravirt.h       |  6 ++++++
 arch/x86/include/asm/processor.h      |  1 +
 arch/x86/include/asm/thread_info.h    |  5 ++++-
 arch/x86/include/asm/xen/hypervisor.h |  2 --
 arch/x86/kernel/ioport.c              |  6 ++++++
 arch/x86/kernel/process.c             |  8 ++++++++
 arch/x86/kernel/process_32.c          |  9 ---------
 arch/x86/kernel/process_64.c          | 11 -----------
 arch/x86/xen/enlighten_pv.c           |  2 +-
 9 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 9a15739..2145dbd 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -265,6 +265,12 @@ static inline void write_idt_entry(gate_desc *dt, int entry, const gate_desc *g)
 {
 	PVOP_VCALL3(pv_cpu_ops.write_idt_entry, dt, entry, g);
 }
+
+static inline bool paravirt_iopl(void)
+{
+	return pv_cpu_ops.set_iopl_mask != paravirt_nop;
+}
+
 static inline void set_iopl_mask(unsigned mask)
 {
 	PVOP_VCALL1(pv_cpu_ops.set_iopl_mask, mask);
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 06c4795..4411d67 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -523,6 +523,7 @@ static inline void load_sp0(struct tss_struct *tss,
 	native_load_sp0(tss, thread);
 }
 
+static inline bool paravirt_iopl(void) { return false; }
 static inline void set_iopl_mask(unsigned mask) { }
 #endif /* CONFIG_PARAVIRT */
 
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index e00e1bd..350f3d3 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -79,6 +79,7 @@ struct thread_info {
 #define TIF_SIGPENDING		2	/* signal pending */
 #define TIF_NEED_RESCHED	3	/* rescheduling necessary */
 #define TIF_SINGLESTEP		4	/* reenable singlestep on user return*/
+#define TIF_PV_IOPL		5	/* call hypervisor to change IOPL */
 #define TIF_SYSCALL_EMU		6	/* syscall emulation active */
 #define TIF_SYSCALL_AUDIT	7	/* syscall auditing active */
 #define TIF_SECCOMP		8	/* secure computing */
@@ -104,6 +105,7 @@ struct thread_info {
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
 #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
 #define _TIF_SINGLESTEP		(1 << TIF_SINGLESTEP)
+#define _TIF_PV_IOPL		(1 << TIF_PV_IOPL)
 #define _TIF_SYSCALL_EMU	(1 << TIF_SYSCALL_EMU)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
@@ -141,7 +143,8 @@ struct thread_info {
 
 /* flags to check in __switch_to() */
 #define _TIF_WORK_CTXSW							\
-	(_TIF_IO_BITMAP|_TIF_NOCPUID|_TIF_NOTSC|_TIF_BLOCKSTEP)
+	(_TIF_IO_BITMAP | _TIF_NOCPUID | _TIF_NOTSC | _TIF_BLOCKSTEP |	\
+	 _TIF_PV_IOPL)
 
 #define _TIF_WORK_CTXSW_PREV (_TIF_WORK_CTXSW|_TIF_USER_RETURN_NOTIFY)
 #define _TIF_WORK_CTXSW_NEXT (_TIF_WORK_CTXSW)
diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
index 39171b3..8b2d4be 100644
--- a/arch/x86/include/asm/xen/hypervisor.h
+++ b/arch/x86/include/asm/xen/hypervisor.h
@@ -62,6 +62,4 @@ void xen_arch_register_cpu(int num);
 void xen_arch_unregister_cpu(int num);
 #endif
 
-extern void xen_set_iopl_mask(unsigned mask);
-
 #endif /* _ASM_X86_XEN_HYPERVISOR_H */
diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
index 9c3cf09..2051e7d 100644
--- a/arch/x86/kernel/ioport.c
+++ b/arch/x86/kernel/ioport.c
@@ -126,6 +126,12 @@ SYSCALL_DEFINE1(iopl, unsigned int, level)
 	regs->flags = (regs->flags & ~X86_EFLAGS_IOPL) |
 		(level << X86_EFLAGS_IOPL_BIT);
 	t->iopl = level << X86_EFLAGS_IOPL_BIT;
+	if (paravirt_iopl()) {
+		if (level > 0)
+			set_thread_flag(TIF_PV_IOPL);
+		else
+			clear_thread_flag(TIF_PV_IOPL);
+	}
 	set_iopl_mask(t->iopl);
 
 	return 0;
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 0bb88428c..78d1ab2 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -296,6 +296,14 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 
 	if ((tifp ^ tifn) & _TIF_NOCPUID)
 		set_cpuid_faulting(!!(tifn & _TIF_NOCPUID));
+
+	/*
+	 * On Xen PV, IOPL bits in pt_regs->flags have no effect, and
+	 * current_pt_regs()->flags may not match the current task's
+	 * intended IOPL.  We need to switch it manually.
+	 */
+	if (prev->iopl != next->iopl)
+		set_iopl_mask(next->iopl);
 }
 
 /*
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index b2d1f7c..19527b4 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -258,15 +258,6 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	load_TLS(next, cpu);
 
 	/*
-	 * Restore IOPL if needed.  In normal use, the flags restore
-	 * in the switch assembly will handle this.  But if the kernel
-	 * is running virtualized at a non-zero CPL, the popf will
-	 * not restore flags, so it must be done in a separate step.
-	 */
-	if (unlikely(prev->iopl != next->iopl))
-		set_iopl_mask(next->iopl);
-
-	/*
 	 * Now maybe handle debug registers and/or IO bitmaps
 	 */
 	if (unlikely(task_thread_info(prev_p)->flags & _TIF_WORK_CTXSW_PREV ||
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 9c39ab8..9525e10 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -446,17 +446,6 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 		     task_thread_info(prev_p)->flags & _TIF_WORK_CTXSW_PREV))
 		__switch_to_xtra(prev_p, next_p, tss);
 
-#ifdef CONFIG_XEN_PV
-	/*
-	 * On Xen PV, IOPL bits in pt_regs->flags have no effect, and
-	 * current_pt_regs()->flags may not match the current task's
-	 * intended IOPL.  We need to switch it manually.
-	 */
-	if (unlikely(static_cpu_has(X86_FEATURE_XENPV) &&
-		     prev->iopl != next->iopl))
-		xen_set_iopl_mask(next->iopl);
-#endif
-
 	if (static_cpu_has_bug(X86_BUG_SYSRET_SS_ATTRS)) {
 		/*
 		 * AMD CPUs have a misfeature: SYSRET sets the SS selector but
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 05257c0..ea0d269 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -813,7 +813,7 @@ static void xen_load_sp0(struct tss_struct *tss,
 	tss->x86_tss.sp0 = thread->sp0;
 }
 
-void xen_set_iopl_mask(unsigned mask)
+static void xen_set_iopl_mask(unsigned mask)
 {
 	struct physdev_set_iopl set_iopl;
 
-- 
2.9.4

[toc] | [next] | [standalone]

#1665815 — Re: [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the path

FromJuergen Gross <jgross@suse.com>
Date2017-06-14 15:20 +0200
SubjectRe: [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the path
Message-ID<tSgng-5F5-9@gated-at.bofh.it>
In reply to#1665761
On 14/06/17 14:40, Brian Gerst wrote:
> Since tasks using IOPL are very rare, move the switching code to the slow
> path for lower impact on normal tasks.
> 
> Signed-off-by: Brian Gerst <brgerst@gmail.com>
> ---
>  arch/x86/include/asm/paravirt.h       |  6 ++++++
>  arch/x86/include/asm/processor.h      |  1 +
>  arch/x86/include/asm/thread_info.h    |  5 ++++-
>  arch/x86/include/asm/xen/hypervisor.h |  2 --
>  arch/x86/kernel/ioport.c              |  6 ++++++
>  arch/x86/kernel/process.c             |  8 ++++++++
>  arch/x86/kernel/process_32.c          |  9 ---------
>  arch/x86/kernel/process_64.c          | 11 -----------
>  arch/x86/xen/enlighten_pv.c           |  2 +-
>  9 files changed, 26 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
> index 9a15739..2145dbd 100644
> --- a/arch/x86/include/asm/paravirt.h
> +++ b/arch/x86/include/asm/paravirt.h
> @@ -265,6 +265,12 @@ static inline void write_idt_entry(gate_desc *dt, int entry, const gate_desc *g)
>  {
>  	PVOP_VCALL3(pv_cpu_ops.write_idt_entry, dt, entry, g);
>  }
> +
> +static inline bool paravirt_iopl(void)
> +{
> +	return pv_cpu_ops.set_iopl_mask != paravirt_nop;
> +}
> +
>  static inline void set_iopl_mask(unsigned mask)
>  {
>  	PVOP_VCALL1(pv_cpu_ops.set_iopl_mask, mask);
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 06c4795..4411d67 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -523,6 +523,7 @@ static inline void load_sp0(struct tss_struct *tss,
>  	native_load_sp0(tss, thread);
>  }
>  
> +static inline bool paravirt_iopl(void) { return false; }
>  static inline void set_iopl_mask(unsigned mask) { }
>  #endif /* CONFIG_PARAVIRT */
>  
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index e00e1bd..350f3d3 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -79,6 +79,7 @@ struct thread_info {
>  #define TIF_SIGPENDING		2	/* signal pending */
>  #define TIF_NEED_RESCHED	3	/* rescheduling necessary */
>  #define TIF_SINGLESTEP		4	/* reenable singlestep on user return*/
> +#define TIF_PV_IOPL		5	/* call hypervisor to change IOPL */
>  #define TIF_SYSCALL_EMU		6	/* syscall emulation active */
>  #define TIF_SYSCALL_AUDIT	7	/* syscall auditing active */
>  #define TIF_SECCOMP		8	/* secure computing */
> @@ -104,6 +105,7 @@ struct thread_info {
>  #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
>  #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
>  #define _TIF_SINGLESTEP		(1 << TIF_SINGLESTEP)
> +#define _TIF_PV_IOPL		(1 << TIF_PV_IOPL)
>  #define _TIF_SYSCALL_EMU	(1 << TIF_SYSCALL_EMU)
>  #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
>  #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
> @@ -141,7 +143,8 @@ struct thread_info {
>  
>  /* flags to check in __switch_to() */
>  #define _TIF_WORK_CTXSW							\
> -	(_TIF_IO_BITMAP|_TIF_NOCPUID|_TIF_NOTSC|_TIF_BLOCKSTEP)
> +	(_TIF_IO_BITMAP | _TIF_NOCPUID | _TIF_NOTSC | _TIF_BLOCKSTEP |	\
> +	 _TIF_PV_IOPL)
>  
>  #define _TIF_WORK_CTXSW_PREV (_TIF_WORK_CTXSW|_TIF_USER_RETURN_NOTIFY)
>  #define _TIF_WORK_CTXSW_NEXT (_TIF_WORK_CTXSW)
> diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
> index 39171b3..8b2d4be 100644
> --- a/arch/x86/include/asm/xen/hypervisor.h
> +++ b/arch/x86/include/asm/xen/hypervisor.h
> @@ -62,6 +62,4 @@ void xen_arch_register_cpu(int num);
>  void xen_arch_unregister_cpu(int num);
>  #endif
>  
> -extern void xen_set_iopl_mask(unsigned mask);
> -
>  #endif /* _ASM_X86_XEN_HYPERVISOR_H */
> diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
> index 9c3cf09..2051e7d 100644
> --- a/arch/x86/kernel/ioport.c
> +++ b/arch/x86/kernel/ioport.c
> @@ -126,6 +126,12 @@ SYSCALL_DEFINE1(iopl, unsigned int, level)
>  	regs->flags = (regs->flags & ~X86_EFLAGS_IOPL) |
>  		(level << X86_EFLAGS_IOPL_BIT);
>  	t->iopl = level << X86_EFLAGS_IOPL_BIT;
> +	if (paravirt_iopl()) {
> +		if (level > 0)
> +			set_thread_flag(TIF_PV_IOPL);
> +		else
> +			clear_thread_flag(TIF_PV_IOPL);
> +	}

You could get rid of paravirt_iopl() by adding a "setflag" boolean
parameter to set_iopl_mask(). Only the Xen variant would need above
thread flag manipulation.


Juergen

>  	set_iopl_mask(t->iopl);
>  
>  	return 0;
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 0bb88428c..78d1ab2 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -296,6 +296,14 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
>  
>  	if ((tifp ^ tifn) & _TIF_NOCPUID)
>  		set_cpuid_faulting(!!(tifn & _TIF_NOCPUID));
> +
> +	/*
> +	 * On Xen PV, IOPL bits in pt_regs->flags have no effect, and
> +	 * current_pt_regs()->flags may not match the current task's
> +	 * intended IOPL.  We need to switch it manually.
> +	 */
> +	if (prev->iopl != next->iopl)
> +		set_iopl_mask(next->iopl);
>  }
>  
>  /*
> diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
> index b2d1f7c..19527b4 100644
> --- a/arch/x86/kernel/process_32.c
> +++ b/arch/x86/kernel/process_32.c
> @@ -258,15 +258,6 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
>  	load_TLS(next, cpu);
>  
>  	/*
> -	 * Restore IOPL if needed.  In normal use, the flags restore
> -	 * in the switch assembly will handle this.  But if the kernel
> -	 * is running virtualized at a non-zero CPL, the popf will
> -	 * not restore flags, so it must be done in a separate step.
> -	 */
> -	if (unlikely(prev->iopl != next->iopl))
> -		set_iopl_mask(next->iopl);
> -
> -	/*
>  	 * Now maybe handle debug registers and/or IO bitmaps
>  	 */
>  	if (unlikely(task_thread_info(prev_p)->flags & _TIF_WORK_CTXSW_PREV ||
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 9c39ab8..9525e10 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -446,17 +446,6 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
>  		     task_thread_info(prev_p)->flags & _TIF_WORK_CTXSW_PREV))
>  		__switch_to_xtra(prev_p, next_p, tss);
>  
> -#ifdef CONFIG_XEN_PV
> -	/*
> -	 * On Xen PV, IOPL bits in pt_regs->flags have no effect, and
> -	 * current_pt_regs()->flags may not match the current task's
> -	 * intended IOPL.  We need to switch it manually.
> -	 */
> -	if (unlikely(static_cpu_has(X86_FEATURE_XENPV) &&
> -		     prev->iopl != next->iopl))
> -		xen_set_iopl_mask(next->iopl);
> -#endif
> -
>  	if (static_cpu_has_bug(X86_BUG_SYSRET_SS_ATTRS)) {
>  		/*
>  		 * AMD CPUs have a misfeature: SYSRET sets the SS selector but
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index 05257c0..ea0d269 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -813,7 +813,7 @@ static void xen_load_sp0(struct tss_struct *tss,
>  	tss->x86_tss.sp0 = thread->sp0;
>  }
>  
> -void xen_set_iopl_mask(unsigned mask)
> +static void xen_set_iopl_mask(unsigned mask)
>  {
>  	struct physdev_set_iopl set_iopl;
>  
>

[toc] | [prev] | [next] | [standalone]

#1666101

FromBrian Gerst <brgerst@gmail.com>
Date2017-06-14 19:40 +0200
Message-ID<tSkqR-88X-1@gated-at.bofh.it>
In reply to#1665815
On Wed, Jun 14, 2017 at 9:14 AM, Juergen Gross <jgross@suse.com> wrote:
> On 14/06/17 14:40, Brian Gerst wrote:
>> Since tasks using IOPL are very rare, move the switching code to the slow
>> path for lower impact on normal tasks.
>>
>> Signed-off-by: Brian Gerst <brgerst@gmail.com>
>> ---
>>  arch/x86/include/asm/paravirt.h       |  6 ++++++
>>  arch/x86/include/asm/processor.h      |  1 +
>>  arch/x86/include/asm/thread_info.h    |  5 ++++-
>>  arch/x86/include/asm/xen/hypervisor.h |  2 --
>>  arch/x86/kernel/ioport.c              |  6 ++++++
>>  arch/x86/kernel/process.c             |  8 ++++++++
>>  arch/x86/kernel/process_32.c          |  9 ---------
>>  arch/x86/kernel/process_64.c          | 11 -----------
>>  arch/x86/xen/enlighten_pv.c           |  2 +-
>>  9 files changed, 26 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
>> index 9a15739..2145dbd 100644
>> --- a/arch/x86/include/asm/paravirt.h
>> +++ b/arch/x86/include/asm/paravirt.h
>> @@ -265,6 +265,12 @@ static inline void write_idt_entry(gate_desc *dt, int entry, const gate_desc *g)
>>  {
>>       PVOP_VCALL3(pv_cpu_ops.write_idt_entry, dt, entry, g);
>>  }
>> +
>> +static inline bool paravirt_iopl(void)
>> +{
>> +     return pv_cpu_ops.set_iopl_mask != paravirt_nop;
>> +}
>> +
>>  static inline void set_iopl_mask(unsigned mask)
>>  {
>>       PVOP_VCALL1(pv_cpu_ops.set_iopl_mask, mask);
>> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
>> index 06c4795..4411d67 100644
>> --- a/arch/x86/include/asm/processor.h
>> +++ b/arch/x86/include/asm/processor.h
>> @@ -523,6 +523,7 @@ static inline void load_sp0(struct tss_struct *tss,
>>       native_load_sp0(tss, thread);
>>  }
>>
>> +static inline bool paravirt_iopl(void) { return false; }
>>  static inline void set_iopl_mask(unsigned mask) { }
>>  #endif /* CONFIG_PARAVIRT */
>>
>> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
>> index e00e1bd..350f3d3 100644
>> --- a/arch/x86/include/asm/thread_info.h
>> +++ b/arch/x86/include/asm/thread_info.h
>> @@ -79,6 +79,7 @@ struct thread_info {
>>  #define TIF_SIGPENDING               2       /* signal pending */
>>  #define TIF_NEED_RESCHED     3       /* rescheduling necessary */
>>  #define TIF_SINGLESTEP               4       /* reenable singlestep on user return*/
>> +#define TIF_PV_IOPL          5       /* call hypervisor to change IOPL */
>>  #define TIF_SYSCALL_EMU              6       /* syscall emulation active */
>>  #define TIF_SYSCALL_AUDIT    7       /* syscall auditing active */
>>  #define TIF_SECCOMP          8       /* secure computing */
>> @@ -104,6 +105,7 @@ struct thread_info {
>>  #define _TIF_SIGPENDING              (1 << TIF_SIGPENDING)
>>  #define _TIF_NEED_RESCHED    (1 << TIF_NEED_RESCHED)
>>  #define _TIF_SINGLESTEP              (1 << TIF_SINGLESTEP)
>> +#define _TIF_PV_IOPL         (1 << TIF_PV_IOPL)
>>  #define _TIF_SYSCALL_EMU     (1 << TIF_SYSCALL_EMU)
>>  #define _TIF_SYSCALL_AUDIT   (1 << TIF_SYSCALL_AUDIT)
>>  #define _TIF_SECCOMP         (1 << TIF_SECCOMP)
>> @@ -141,7 +143,8 @@ struct thread_info {
>>
>>  /* flags to check in __switch_to() */
>>  #define _TIF_WORK_CTXSW                                                      \
>> -     (_TIF_IO_BITMAP|_TIF_NOCPUID|_TIF_NOTSC|_TIF_BLOCKSTEP)
>> +     (_TIF_IO_BITMAP | _TIF_NOCPUID | _TIF_NOTSC | _TIF_BLOCKSTEP |  \
>> +      _TIF_PV_IOPL)
>>
>>  #define _TIF_WORK_CTXSW_PREV (_TIF_WORK_CTXSW|_TIF_USER_RETURN_NOTIFY)
>>  #define _TIF_WORK_CTXSW_NEXT (_TIF_WORK_CTXSW)
>> diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
>> index 39171b3..8b2d4be 100644
>> --- a/arch/x86/include/asm/xen/hypervisor.h
>> +++ b/arch/x86/include/asm/xen/hypervisor.h
>> @@ -62,6 +62,4 @@ void xen_arch_register_cpu(int num);
>>  void xen_arch_unregister_cpu(int num);
>>  #endif
>>
>> -extern void xen_set_iopl_mask(unsigned mask);
>> -
>>  #endif /* _ASM_X86_XEN_HYPERVISOR_H */
>> diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
>> index 9c3cf09..2051e7d 100644
>> --- a/arch/x86/kernel/ioport.c
>> +++ b/arch/x86/kernel/ioport.c
>> @@ -126,6 +126,12 @@ SYSCALL_DEFINE1(iopl, unsigned int, level)
>>       regs->flags = (regs->flags & ~X86_EFLAGS_IOPL) |
>>               (level << X86_EFLAGS_IOPL_BIT);
>>       t->iopl = level << X86_EFLAGS_IOPL_BIT;
>> +     if (paravirt_iopl()) {
>> +             if (level > 0)
>> +                     set_thread_flag(TIF_PV_IOPL);
>> +             else
>> +                     clear_thread_flag(TIF_PV_IOPL);
>> +     }
>
> You could get rid of paravirt_iopl() by adding a "setflag" boolean
> parameter to set_iopl_mask(). Only the Xen variant would need above
> thread flag manipulation.

After the first two patches, the hook is a no-op except in the Xen
case, so the thread flag change gets skipped except on Xen.  Moving
the code to Xen would work, but it would mean that any other
hypervisor using that hook would also need to duplicate the thread
flag changes.  Granted, it looks like Xen is doing something unique
here (running the guest kernel at CPL 1), so that probably won't be an
issue.

--
Brian Gerst

[toc] | [prev] | [next] | [standalone]

#1666102 — Re: [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the path

FromJuergen Gross <jgross@suse.com>
Date2017-06-14 19:40 +0200
SubjectRe: [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the path
Message-ID<tSkqR-88X-3@gated-at.bofh.it>
In reply to#1666101
On 14/06/17 19:29, Brian Gerst wrote:
> On Wed, Jun 14, 2017 at 9:14 AM, Juergen Gross <jgross@suse.com> wrote:
>> On 14/06/17 14:40, Brian Gerst wrote:
>>> Since tasks using IOPL are very rare, move the switching code to the slow
>>> path for lower impact on normal tasks.
>>>
>>> Signed-off-by: Brian Gerst <brgerst@gmail.com>
>>> ---
>>>  arch/x86/include/asm/paravirt.h       |  6 ++++++
>>>  arch/x86/include/asm/processor.h      |  1 +
>>>  arch/x86/include/asm/thread_info.h    |  5 ++++-
>>>  arch/x86/include/asm/xen/hypervisor.h |  2 --
>>>  arch/x86/kernel/ioport.c              |  6 ++++++
>>>  arch/x86/kernel/process.c             |  8 ++++++++
>>>  arch/x86/kernel/process_32.c          |  9 ---------
>>>  arch/x86/kernel/process_64.c          | 11 -----------
>>>  arch/x86/xen/enlighten_pv.c           |  2 +-
>>>  9 files changed, 26 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
>>> index 9a15739..2145dbd 100644
>>> --- a/arch/x86/include/asm/paravirt.h
>>> +++ b/arch/x86/include/asm/paravirt.h
>>> @@ -265,6 +265,12 @@ static inline void write_idt_entry(gate_desc *dt, int entry, const gate_desc *g)
>>>  {
>>>       PVOP_VCALL3(pv_cpu_ops.write_idt_entry, dt, entry, g);
>>>  }
>>> +
>>> +static inline bool paravirt_iopl(void)
>>> +{
>>> +     return pv_cpu_ops.set_iopl_mask != paravirt_nop;
>>> +}
>>> +
>>>  static inline void set_iopl_mask(unsigned mask)
>>>  {
>>>       PVOP_VCALL1(pv_cpu_ops.set_iopl_mask, mask);
>>> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
>>> index 06c4795..4411d67 100644
>>> --- a/arch/x86/include/asm/processor.h
>>> +++ b/arch/x86/include/asm/processor.h
>>> @@ -523,6 +523,7 @@ static inline void load_sp0(struct tss_struct *tss,
>>>       native_load_sp0(tss, thread);
>>>  }
>>>
>>> +static inline bool paravirt_iopl(void) { return false; }
>>>  static inline void set_iopl_mask(unsigned mask) { }
>>>  #endif /* CONFIG_PARAVIRT */
>>>
>>> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
>>> index e00e1bd..350f3d3 100644
>>> --- a/arch/x86/include/asm/thread_info.h
>>> +++ b/arch/x86/include/asm/thread_info.h
>>> @@ -79,6 +79,7 @@ struct thread_info {
>>>  #define TIF_SIGPENDING               2       /* signal pending */
>>>  #define TIF_NEED_RESCHED     3       /* rescheduling necessary */
>>>  #define TIF_SINGLESTEP               4       /* reenable singlestep on user return*/
>>> +#define TIF_PV_IOPL          5       /* call hypervisor to change IOPL */
>>>  #define TIF_SYSCALL_EMU              6       /* syscall emulation active */
>>>  #define TIF_SYSCALL_AUDIT    7       /* syscall auditing active */
>>>  #define TIF_SECCOMP          8       /* secure computing */
>>> @@ -104,6 +105,7 @@ struct thread_info {
>>>  #define _TIF_SIGPENDING              (1 << TIF_SIGPENDING)
>>>  #define _TIF_NEED_RESCHED    (1 << TIF_NEED_RESCHED)
>>>  #define _TIF_SINGLESTEP              (1 << TIF_SINGLESTEP)
>>> +#define _TIF_PV_IOPL         (1 << TIF_PV_IOPL)
>>>  #define _TIF_SYSCALL_EMU     (1 << TIF_SYSCALL_EMU)
>>>  #define _TIF_SYSCALL_AUDIT   (1 << TIF_SYSCALL_AUDIT)
>>>  #define _TIF_SECCOMP         (1 << TIF_SECCOMP)
>>> @@ -141,7 +143,8 @@ struct thread_info {
>>>
>>>  /* flags to check in __switch_to() */
>>>  #define _TIF_WORK_CTXSW                                                      \
>>> -     (_TIF_IO_BITMAP|_TIF_NOCPUID|_TIF_NOTSC|_TIF_BLOCKSTEP)
>>> +     (_TIF_IO_BITMAP | _TIF_NOCPUID | _TIF_NOTSC | _TIF_BLOCKSTEP |  \
>>> +      _TIF_PV_IOPL)
>>>
>>>  #define _TIF_WORK_CTXSW_PREV (_TIF_WORK_CTXSW|_TIF_USER_RETURN_NOTIFY)
>>>  #define _TIF_WORK_CTXSW_NEXT (_TIF_WORK_CTXSW)
>>> diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
>>> index 39171b3..8b2d4be 100644
>>> --- a/arch/x86/include/asm/xen/hypervisor.h
>>> +++ b/arch/x86/include/asm/xen/hypervisor.h
>>> @@ -62,6 +62,4 @@ void xen_arch_register_cpu(int num);
>>>  void xen_arch_unregister_cpu(int num);
>>>  #endif
>>>
>>> -extern void xen_set_iopl_mask(unsigned mask);
>>> -
>>>  #endif /* _ASM_X86_XEN_HYPERVISOR_H */
>>> diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
>>> index 9c3cf09..2051e7d 100644
>>> --- a/arch/x86/kernel/ioport.c
>>> +++ b/arch/x86/kernel/ioport.c
>>> @@ -126,6 +126,12 @@ SYSCALL_DEFINE1(iopl, unsigned int, level)
>>>       regs->flags = (regs->flags & ~X86_EFLAGS_IOPL) |
>>>               (level << X86_EFLAGS_IOPL_BIT);
>>>       t->iopl = level << X86_EFLAGS_IOPL_BIT;
>>> +     if (paravirt_iopl()) {
>>> +             if (level > 0)
>>> +                     set_thread_flag(TIF_PV_IOPL);
>>> +             else
>>> +                     clear_thread_flag(TIF_PV_IOPL);
>>> +     }
>>
>> You could get rid of paravirt_iopl() by adding a "setflag" boolean
>> parameter to set_iopl_mask(). Only the Xen variant would need above
>> thread flag manipulation.
> 
> After the first two patches, the hook is a no-op except in the Xen
> case, so the thread flag change gets skipped except on Xen.  Moving
> the code to Xen would work, but it would mean that any other
> hypervisor using that hook would also need to duplicate the thread
> flag changes.  Granted, it looks like Xen is doing something unique
> here (running the guest kernel at CPL 1), so that probably won't be an
> issue.

Just create a service function which is doing the job. Xen should call
it and any other future non-standard user of set_iopl_mask(), too.


Juergen

[toc] | [prev] | [next] | [standalone]

#1666107

FromAndy Lutomirski <luto@kernel.org>
Date2017-06-14 19:50 +0200
Message-ID<tSkAx-8cx-1@gated-at.bofh.it>
In reply to#1665761
On Wed, Jun 14, 2017 at 5:40 AM, Brian Gerst <brgerst@gmail.com> wrote:
> Since tasks using IOPL are very rare, move the switching code to the slow
> path for lower impact on normal tasks.

I think that Andrew Cooper added a vmassist that we could opt in to
that makes Xen PV IOPL switching work more or less just like native.
We could maybe opt in to that and avoid needing this stuff at all on
newer hypervisors.

[toc] | [prev] | [next] | [standalone]

#1666120 — Re: [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the path

FromAndrew Cooper <andrew.cooper3@citrix.com>
Date2017-06-14 20:10 +0200
SubjectRe: [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the path
Message-ID<tSkTT-64-1@gated-at.bofh.it>
In reply to#1666107
On 14/06/17 18:40, Andy Lutomirski wrote:
> On Wed, Jun 14, 2017 at 5:40 AM, Brian Gerst <brgerst@gmail.com> wrote:
>> Since tasks using IOPL are very rare, move the switching code to the slow
>> path for lower impact on normal tasks.
> I think that Andrew Cooper added a vmassist that we could opt in to
> that makes Xen PV IOPL switching work more or less just like native.
> We could maybe opt in to that and avoid needing this stuff at all on
> newer hypervisors.

Indeed.

HYPERVISOR_vm_assist(VMASST_CMD_enable, VMASST_TYPE_architectural_iopl);

(if recognised) does two things.

1) virtual IOPL is picked up from EFLAGS in the iret frame, exactly like
native.
2) The guest kernel is assumed to have virtual CPL0 for the purpose of
privilege calculations.

Xen never runs with the real IOPL different to 0, or a PV guests could
disable interrupts with popf.  As a result, all IO port access does trap
to Xen for auditing.  What part 2) does is avoid having the awkward
double-step of Linux needing to set IOPL to 1 for kernel level IO access
to avoid faulting.

The assist should be available in Xen 4.7 and later (or wherever vendors
have backported it to).

~Andrew

[toc] | [prev] | [next] | [standalone]

#1666139

FromBrian Gerst <brgerst@gmail.com>
Date2017-06-14 20:30 +0200
Message-ID<tSldg-cr-15@gated-at.bofh.it>
In reply to#1666120
On Wed, Jun 14, 2017 at 2:02 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 14/06/17 18:40, Andy Lutomirski wrote:
>> On Wed, Jun 14, 2017 at 5:40 AM, Brian Gerst <brgerst@gmail.com> wrote:
>>> Since tasks using IOPL are very rare, move the switching code to the slow
>>> path for lower impact on normal tasks.
>> I think that Andrew Cooper added a vmassist that we could opt in to
>> that makes Xen PV IOPL switching work more or less just like native.
>> We could maybe opt in to that and avoid needing this stuff at all on
>> newer hypervisors.
>
> Indeed.
>
> HYPERVISOR_vm_assist(VMASST_CMD_enable, VMASST_TYPE_architectural_iopl);
>
> (if recognised) does two things.
>
> 1) virtual IOPL is picked up from EFLAGS in the iret frame, exactly like
> native.
> 2) The guest kernel is assumed to have virtual CPL0 for the purpose of
> privilege calculations.
>
> Xen never runs with the real IOPL different to 0, or a PV guests could
> disable interrupts with popf.  As a result, all IO port access does trap
> to Xen for auditing.  What part 2) does is avoid having the awkward
> double-step of Linux needing to set IOPL to 1 for kernel level IO access
> to avoid faulting.
>
> The assist should be available in Xen 4.7 and later (or wherever vendors
> have backported it to).
>
> ~Andrew

Ok.  So do we keep the old code around to support older Xen
hypervisors or just require the newer Xen for guest userspace IOPL
support?  Part of the reason I am making these changes is to sync the
32-bit and 64-bit code in __switch_to(), to ultimately merge them.

--
Brian Gerst

[toc] | [prev] | [next] | [standalone]

#1666356

FromAndy Lutomirski <luto@kernel.org>
Date2017-06-15 03:40 +0200
Message-ID<tSrVo-4m9-13@gated-at.bofh.it>
In reply to#1666139
On Wed, Jun 14, 2017 at 11:25 AM, Brian Gerst <brgerst@gmail.com> wrote:
> On Wed, Jun 14, 2017 at 2:02 PM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> On 14/06/17 18:40, Andy Lutomirski wrote:
>>> On Wed, Jun 14, 2017 at 5:40 AM, Brian Gerst <brgerst@gmail.com> wrote:
>>>> Since tasks using IOPL are very rare, move the switching code to the slow
>>>> path for lower impact on normal tasks.
>>> I think that Andrew Cooper added a vmassist that we could opt in to
>>> that makes Xen PV IOPL switching work more or less just like native.
>>> We could maybe opt in to that and avoid needing this stuff at all on
>>> newer hypervisors.
>>
>> Indeed.
>>
>> HYPERVISOR_vm_assist(VMASST_CMD_enable, VMASST_TYPE_architectural_iopl);
>>
>> (if recognised) does two things.
>>
>> 1) virtual IOPL is picked up from EFLAGS in the iret frame, exactly like
>> native.
>> 2) The guest kernel is assumed to have virtual CPL0 for the purpose of
>> privilege calculations.
>>
>> Xen never runs with the real IOPL different to 0, or a PV guests could
>> disable interrupts with popf.  As a result, all IO port access does trap
>> to Xen for auditing.  What part 2) does is avoid having the awkward
>> double-step of Linux needing to set IOPL to 1 for kernel level IO access
>> to avoid faulting.
>>
>> The assist should be available in Xen 4.7 and later (or wherever vendors
>> have backported it to).
>>
>> ~Andrew
>
> Ok.  So do we keep the old code around to support older Xen
> hypervisors or just require the newer Xen for guest userspace IOPL
> support?  Part of the reason I am making these changes is to sync the
> 32-bit and 64-bit code in __switch_to(), to ultimately merge them.

I think we should keep the old code.

One way to structure this that might be nicer than using paravirt ops
would be to add a new bug X86_BUG_XEN_PV_IOPL that would only be set
on old hypervisors that don't have the assist.  Then the code could
look like:

if (static_cpu_has_bug(X86_XEN_PV_IOPL))
  xen_set_iopl(whatever);

and we wouldn't need the paravirt indirection at all.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web