x86: IOPL switching cleanups

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 <[email protected]>

---
 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(-)

arch/x86/include/asm/paravirt.h +6 -0

index 9a15739..2145dbd 100644
@@ -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);

arch/x86/include/asm/processor.h +1 -0

index 06c4795..4411d67 100644
@@ -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 */
 

arch/x86/include/asm/thread_info.h +4 -1

index e00e1bd..350f3d3 100644
@@ -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)

arch/x86/include/asm/xen/hypervisor.h +0 -2

index 39171b3..8b2d4be 100644
@@ -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 */

arch/x86/kernel/ioport.c +0 -0

index 9c3cf09..2051e7d 100644

arch/x86/kernel/ioport.c +0 -0

 	set_iopl_mask(t->iopl);
 
 	return 0;

arch/x86/kernel/process.c +8 -0

index 0bb88428c..78d1ab2 100644
@@ -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);
 }
 
 /*

arch/x86/kernel/process_32.c +0 -9

index b2d1f7c..19527b4 100644
@@ -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 ||

arch/x86/kernel/process_64.c +0 -11

index 9c39ab8..9525e10 100644
@@ -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

arch/x86/xen/enlighten_pv.c +1 -1

index 05257c0..ea0d269 100644
@@ -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;
 

Quoted 96 lines · Brian Gerst

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 <[email protected]>

---
 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(-)

arch/x86/include/asm/paravirt.h +6 -0

index 9a15739..2145dbd 100644
@@ -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);

arch/x86/include/asm/processor.h +1 -0

index 06c4795..4411d67 100644
@@ -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 */
 

arch/x86/include/asm/thread_info.h +4 -1

index e00e1bd..350f3d3 100644
@@ -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)

arch/x86/include/asm/xen/hypervisor.h +0 -2

index 39171b3..8b2d4be 100644
@@ -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 */

arch/x86/kernel/ioport.c +2 -0

index 9c3cf09..2051e7d 100644
@@ -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)

arch/x86/kernel/ioport.c SYSCALL_DEFINE1(iopl, unsigned int, level) · line 134

Quoted 4 lines

arch/x86/kernel/ioport.c +4 -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.

Quoted 97 lines · Juergen Gross

Quoted 96 lines · Brian Gerst

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 <[email protected]>

---
 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(-)

arch/x86/include/asm/paravirt.h +6 -0

index 9a15739..2145dbd 100644
@@ -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);

arch/x86/include/asm/processor.h +1 -0

index 06c4795..4411d67 100644
@@ -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 */
 

arch/x86/include/asm/thread_info.h +4 -1

index e00e1bd..350f3d3 100644
@@ -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)

arch/x86/include/asm/xen/hypervisor.h +0 -2

index 39171b3..8b2d4be 100644
@@ -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 */

arch/x86/kernel/ioport.c +2 -0

index 9c3cf09..2051e7d 100644
@@ -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)

Quoted 8 lines

arch/x86/kernel/ioport.c SYSCALL_DEFINE1(iopl, unsigned int, level) · line 134

Quoted 4 lines

arch/x86/kernel/ioport.c +4 -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.

Since tasks using IOPL are very rare, move the switching code to the slow path for lower impact on normal tasks.

Quoted 2 lines · Brian Gerst

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.

Quoted 7 lines · Andy Lutomirski

Quoted 2 lines · Brian Gerst

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

Quoted 29 lines · Andrew Cooper

Quoted 7 lines · Andy Lutomirski

Quoted 2 lines · Brian Gerst

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).

Quoted 1 line

~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.