x86/entry/32: Get rid of paravirt_enabled in ESPFIX

arch/x86/kernel/cpu/common.c +18 -0

index 91ddae732a36..c6ef4da8e4f4 100644
@@ -979,6 +979,31 @@ static void identify_cpu(struct cpuinfo_x86 *c)
  #ifdef CONFIG_NUMA
  	numa_add_cpu(smp_processor_id());
  #endif
+
+	/*
+	 * ESPFIX is a strange bug.  All real CPUs have it.  Paravirt
+	 * systems that run Linux at CPL > 0 may or may not have the
+	 * issue, but, even if they have the issue, there's absolutely
+	 * nothing we can do about it because we can't use the real IRET
+	 * instruction.
+	 *
+	 * NB: For the time being, only 32-bit kernels support
+	 * X86_BUG_ESPFIX as such.  64-bit kernels directly choose
+	 * whether to apply espfix using paravirt hooks.  If any
+	 * non-paravirt system ever shows up that does *not* have the
+	 * ESPFIX issue, we can change this.
+	 */
+#ifdef CONFIG_X86_32
+#ifdef CONFIG_PARAVIRT
+	do {
+		extern void native_iret(void);

Quoted 35 lines · Andy Lutomirski

arch/x86/kernel/cpu/common.c +25 -0

index 91ddae732a36..c6ef4da8e4f4 100644
@@ -979,6 +979,31 @@ static void identify_cpu(struct cpuinfo_x86 *c)
  #ifdef CONFIG_NUMA
        numa_add_cpu(smp_processor_id());
  #endif
+
+       /*
+        * ESPFIX is a strange bug.  All real CPUs have it.  Paravirt
+        * systems that run Linux at CPL > 0 may or may not have the
+        * issue, but, even if they have the issue, there's absolutely
+        * nothing we can do about it because we can't use the real IRET
+        * instruction.
+        *
+        * NB: For the time being, only 32-bit kernels support
+        * X86_BUG_ESPFIX as such.  64-bit kernels directly choose
+        * whether to apply espfix using paravirt hooks.  If any
+        * non-paravirt system ever shows up that does *not* have the
+        * ESPFIX issue, we can change this.
+        */
+#ifdef CONFIG_X86_32
+#ifdef CONFIG_PARAVIRT
+       do {
+               extern void native_iret(void);
+               if (pv_cpu_ops.iret == native_iret)
+                       set_cpu_bug(c, X86_BUG_ESPFIX);
+       } while (0);
+#else
+       set_cpu_bug(c, X86_BUG_ESPFIX);
+#endif
+#endif
  }

arch/x86/kernel/cpu/common.c static void identify_cpu(struct cpuinfo_x86 *c) · line 1008

Quoted 1 line

arch/x86/kernel/cpu/common.c +0 -0

    /*

Alternatively, PV guests can clear X86_BUG_ESPFIX in their init code. E.g in .set_cpu_features op, just like we do for X86_BUG_SYSRET_SS_ATTRS (although this may require adding struct hypervisor_x86 for lguests. Which I think they should have anyway).

-boris

Quoted 36 lines · Boris Ostrovsky

Quoted 35 lines · Andy Lutomirski

arch/x86/kernel/cpu/common.c +25 -0

index 91ddae732a36..c6ef4da8e4f4 100644
@@ -979,6 +979,31 @@ static void identify_cpu(struct cpuinfo_x86 *c)
  #ifdef CONFIG_NUMA
        numa_add_cpu(smp_processor_id());
  #endif
+
+       /*
+        * ESPFIX is a strange bug.  All real CPUs have it.  Paravirt
+        * systems that run Linux at CPL > 0 may or may not have the
+        * issue, but, even if they have the issue, there's absolutely
+        * nothing we can do about it because we can't use the real IRET
+        * instruction.
+        *
+        * NB: For the time being, only 32-bit kernels support
+        * X86_BUG_ESPFIX as such.  64-bit kernels directly choose
+        * whether to apply espfix using paravirt hooks.  If any
+        * non-paravirt system ever shows up that does *not* have the
+        * ESPFIX issue, we can change this.
+        */
+#ifdef CONFIG_X86_32
+#ifdef CONFIG_PARAVIRT
+       do {
+               extern void native_iret(void);
+               if (pv_cpu_ops.iret == native_iret)
+                       set_cpu_bug(c, X86_BUG_ESPFIX);
+       } while (0);
+#else
+       set_cpu_bug(c, X86_BUG_ESPFIX);
+#endif
+#endif
  }

Quoted 8 lines

arch/x86/kernel/cpu/common.c static void identify_cpu(struct cpuinfo_x86 *c) · line 1008

Quoted 1 line

arch/x86/kernel/cpu/common.c +0 -0

    /*

Alternatively, PV guests can clear X86_BUG_ESPFIX in their init code. E.g in .set_cpu_features op, just like we do for X86_BUG_SYSRET_SS_ATTRS (although this may require adding struct hypervisor_x86 for lguests. Which I think they should have anyway).

I'm fine with that.

Luis, if you prefer that approach, can you do this and add the resulting patch to your series? You're busily reworking that stuff anyway.

Borislav, if you're okay with this (ab)use of the cpufeatures stuff

x86_64 has very clean espfix handling on paravirt: espfix64 is set up in native_iret, so paravirt systems that override iret bypass espfix64 automatically. This is robust and straightforward.

x86_32 is messier. espfix is set up before the IRET paravirt patch point, so it can't be directly conditionalized on whether we use native_iret.

arch/x86/kernel/cpu/common.c static void identify_cpu(struct cpuinfo_x86 *c)

Quoted 5 lines · Andy Lutomirski

arch/x86/kernel/cpu/common.c +0 -0

index 91ddae732a36..c6ef4da8e4f4 100644
@@ -979,6 +979,31 @@ static void identify_cpu(struct cpuinfo_x86 *c)

Quoted 26 lines

arch/x86/kernel/cpu/common.c +23 -0

 #ifdef CONFIG_NUMA
 	numa_add_cpu(smp_processor_id());
 #endif
+
+	/*
+	 * ESPFIX is a strange bug.  All real CPUs have it.  Paravirt
+	 * systems that run Linux at CPL > 0 may or may not have the
+	 * issue, but, even if they have the issue, there's absolutely
+	 * nothing we can do about it because we can't use the real IRET
+	 * instruction.
+	 *
+	 * NB: For the time being, only 32-bit kernels support
+	 * X86_BUG_ESPFIX as such.  64-bit kernels directly choose
+	 * whether to apply espfix using paravirt hooks.  If any
+	 * non-paravirt system ever shows up that does *not* have the
+	 * ESPFIX issue, we can change this.
+	 */
+#ifdef CONFIG_X86_32
+#ifdef CONFIG_PARAVIRT
+	do {
+		extern void native_iret(void);
+		if (pv_cpu_ops.iret == native_iret)
+			set_cpu_bug(c, X86_BUG_ESPFIX);
+	} while (0);
+#else
+	set_cpu_bug(c, X86_BUG_ESPFIX);

arch/x86/kernel/cpu/common.c static void identify_cpu(struct cpuinfo_x86 *c) · line 1009

Quoted 4 lines

arch/x86/kernel/cpu/common.c +2 -0

+#endif
+#endif
 }
 /*

Alternatively, PV guests can clear X86_BUG_ESPFIX in their init code. E.g in .set_cpu_features op, just like we do for X86_BUG_SYSRET_SS_ATTRS

(although this may require adding struct
hypervisor_x86 for lguests. Which I think they should have anyway).

Ingo, your feedback appreciated at the end here, regarding quirks.

Quoted 6 lines · Boris Ostrovsky

arch/x86/kernel/cpu/common.c static void identify_cpu(struct cpuinfo_x86 *c)

Quoted 5 lines · Andy Lutomirski

arch/x86/kernel/cpu/common.c +0 -0

index 91ddae732a36..c6ef4da8e4f4 100644
@@ -979,6 +979,31 @@ static void identify_cpu(struct cpuinfo_x86 *c)

Note: Andy's change is on identify_cpu() modification here at the end.

Quoted 26 lines

Quoted 26 lines

arch/x86/kernel/cpu/common.c +23 -0

 #ifdef CONFIG_NUMA
    numa_add_cpu(smp_processor_id());
 #endif
+
+    /*
+     * ESPFIX is a strange bug.  All real CPUs have it.  Paravirt
+     * systems that run Linux at CPL > 0 may or may not have the
+     * issue, but, even if they have the issue, there's absolutely
+     * nothing we can do about it because we can't use the real IRET
+     * instruction.
+     *
+     * NB: For the time being, only 32-bit kernels support
+     * X86_BUG_ESPFIX as such.  64-bit kernels directly choose
+     * whether to apply espfix using paravirt hooks.  If any
+     * non-paravirt system ever shows up that does *not* have the
+     * ESPFIX issue, we can change this.
+     */
+#ifdef CONFIG_X86_32
+#ifdef CONFIG_PARAVIRT
+    do {
+            extern void native_iret(void);
+            if (pv_cpu_ops.iret == native_iret)
+                    set_cpu_bug(c, X86_BUG_ESPFIX);
+    } while (0);
+#else
+    set_cpu_bug(c, X86_BUG_ESPFIX);

Quoted 8 lines

arch/x86/kernel/cpu/common.c static void identify_cpu(struct cpuinfo_x86 *c) · line 1009

Quoted 4 lines

arch/x86/kernel/cpu/common.c +2 -0

+#endif
+#endif
 }
 /*

Alternatively, PV guests can clear X86_BUG_ESPFIX in their init code. E.g in .set_cpu_features op, just like we do for X86_BUG_SYSRET_SS_ATTRS

Andy's proposal works out of identify_cpu() and that covers both boot processor and secondary CPUs. The summary is as follows, tracing back in time from left to right.

                --- identify_boot_cpu() --- check_bugs() --- start_kernel()
               /
identify_cpu()<
               \
                --- identify_secondary_cpu() --- cpu_up() --- smp_init()
                    --- kernel_init_freeable() --- kernel_init()
                        --- rest_init() --- start_kernel()

set_cpu_features() is called from both: init_hypervisor_platform() during setup_arch() and identify_cpu(). Since it'll be called on check_bugs() already on identify_boot_cpu() though I think the call from init_hypervisor_platform() seems redundant ?

We assume we just call:

setup_arch() --> init_hypervisor_platform() --> init_hypervisor(&boot_cpu_data)

But the above map on identify_cpu() also shows we call:

start_kernel --> check_bugs() --> identify_boot_cpu() -->

        identify_cpu() --> init_hypervisor() --> set_cpu_features()

void init_hypervisor(struct cpuinfo_x86 *c)

{
        if (x86_hyper && x86_hyper->set_cpu_features)
                x86_hyper->set_cpu_features(c);
}

Anyway, since we're consolidating quirks, and since it turns out the other quirks are being shifted away from paravirt_enabled() out into another struct x86_platform_ops CPU specific quirk, I wonder why not just also replace this set_cpu_features() thing as a struct x86_platform_ops quirk CPU callback.

Quoted 2 lines

(although this may require adding struct
hypervisor_x86 for lguests. Which I think they should have anyway).

lguest already uses x86_platform, and setting up a per CPU quirk would be rather trivial.

CPU feature / CPU quirk...

I've stashed the other quirks into a x86_early_init_platform_quirks(), this was to have all quirks handled in one place. We handle differences with subarch there. vmware has no subarch though, and it uses its own set_cpu_features(). We have a few options I can think of:

1) keep this on set_cpu_features() and modify lguest to add a struct hypervisor_x86 as boris suggests

2) move set_cpu_features() as a platform feature / quirk callback and call it on identify_cpu()

3) Just identify each quirk on struct x86_platform, with a set of defaults set. Then identify_cpu() enables a platform callback to override defaults, and finally then a shared quirk call is issued to set the different set_cpu_features() or clear them.

Quoted 1 line · Andy Lutomirski

Borislav, if you're okay with this (ab)use of the cpufeatures stuff

Because of X86_BUG_ESPFIX? Why abuse?

--
Regards/Gruss,
Boris.

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

Quoted 102 lines · Luis R. Rodriguez

Ingo, your feedback appreciated at the end here, regarding quirks.

Quoted 6 lines · Boris Ostrovsky

arch/x86/kernel/cpu/common.c static void identify_cpu(struct cpuinfo_x86 *c)

Quoted 5 lines · Andy Lutomirski

arch/x86/kernel/cpu/common.c +0 -0

index 91ddae732a36..c6ef4da8e4f4 100644
@@ -979,6 +979,31 @@ static void identify_cpu(struct cpuinfo_x86 *c)

Note: Andy's change is on identify_cpu() modification here at the end.

Quoted 26 lines

Quoted 26 lines

arch/x86/kernel/cpu/common.c +23 -0

 #ifdef CONFIG_NUMA
    numa_add_cpu(smp_processor_id());
 #endif
+
+    /*
+     * ESPFIX is a strange bug.  All real CPUs have it.  Paravirt
+     * systems that run Linux at CPL > 0 may or may not have the
+     * issue, but, even if they have the issue, there's absolutely
+     * nothing we can do about it because we can't use the real IRET
+     * instruction.
+     *
+     * NB: For the time being, only 32-bit kernels support
+     * X86_BUG_ESPFIX as such.  64-bit kernels directly choose
+     * whether to apply espfix using paravirt hooks.  If any
+     * non-paravirt system ever shows up that does *not* have the
+     * ESPFIX issue, we can change this.
+     */
+#ifdef CONFIG_X86_32
+#ifdef CONFIG_PARAVIRT
+    do {
+            extern void native_iret(void);
+            if (pv_cpu_ops.iret == native_iret)
+                    set_cpu_bug(c, X86_BUG_ESPFIX);
+    } while (0);
+#else
+    set_cpu_bug(c, X86_BUG_ESPFIX);

Quoted 8 lines

arch/x86/kernel/cpu/common.c static void identify_cpu(struct cpuinfo_x86 *c) · line 1009

Quoted 4 lines

arch/x86/kernel/cpu/common.c +2 -0

+#endif
+#endif
 }
 /*

Alternatively, PV guests can clear X86_BUG_ESPFIX in their init code. E.g in .set_cpu_features op, just like we do for X86_BUG_SYSRET_SS_ATTRS

Andy's proposal works out of identify_cpu() and that covers both boot processor and secondary CPUs. The summary is as follows, tracing back in time from left to right.

                --- identify_boot_cpu() --- check_bugs() --- start_kernel()
               /
identify_cpu()<
               \
                --- identify_secondary_cpu() --- cpu_up() --- smp_init()
                    --- kernel_init_freeable() --- kernel_init()
                        --- rest_init() --- start_kernel()

set_cpu_features() is called from both: init_hypervisor_platform() during setup_arch() and identify_cpu(). Since it'll be called on check_bugs() already on identify_boot_cpu() though I think the call from init_hypervisor_platform() seems redundant ?

We assume we just call:

setup_arch() --> init_hypervisor_platform() --> init_hypervisor(&boot_cpu_data)

But the above map on identify_cpu() also shows we call:

start_kernel --> check_bugs() --> identify_boot_cpu() -->

        identify_cpu() --> init_hypervisor() --> set_cpu_features()

void init_hypervisor(struct cpuinfo_x86 *c)

{
        if (x86_hyper && x86_hyper->set_cpu_features)
                x86_hyper->set_cpu_features(c);
}

Anyway, since we're consolidating quirks, and since it turns out the other quirks are being shifted away from paravirt_enabled() out into another struct x86_platform_ops CPU specific quirk, I wonder why not just also replace this set_cpu_features() thing as a struct x86_platform_ops quirk CPU callback.

Quoted 2 lines

(although this may require adding struct
hypervisor_x86 for lguests. Which I think they should have anyway).

lguest already uses x86_platform, and setting up a per CPU quirk would be rather trivial.

CPU feature / CPU quirk...

I've stashed the other quirks into a x86_early_init_platform_quirks(), this was to have all quirks handled in one place. We handle differences with subarch there. vmware has no subarch though, and it uses its own set_cpu_features(). We have a few options I can think of:

1) keep this on set_cpu_features() and modify lguest to add a struct hypervisor_x86 as boris suggests

Quoted 8 lines

2) move set_cpu_features() as a platform feature / quirk callback and call it on identify_cpu()

3) Just identify each quirk on struct x86_platform, with a set of defaults set. Then identify_cpu() enables a platform callback to override defaults, and finally then a shared quirk call is issued to set the different set_cpu_features() or clear them.

I think this is severely overcomplicating the issue.

The issue is that IRET is a pile of shit. It may be quirky, but it affects *everything*.

On x86_64, the kernel assumes that the "iret" implementation works around the quirk. xen_iret doesn't, and that's Xen's problem.

On x86_32, it's inconvenient for native_iret to directly work around the quirk. Instead, some other asm code in the exit path sets up the workaround under the assumption that native_iret is just plain IRET. It's the responsibility of other IRET implementations to have their own implementations of the workaround and, again, they don't in practice and this is Xen and lguest's problem.

So I think the condition we want really is (iret == native_iret), and that's what my patch does. So I think we should leave it at that.

Because I'm mixing paravirt and cpufeatures a bit oddly.

Quoted 1 line · Andy Lutomirski

Because I'm mixing paravirt and cpufeatures a bit oddly.

That's fine. All X86_BUG_* are synthetic and exactly for stuff like that.

--
Regards/Gruss,
Boris.

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

It no longer has any users.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/kernel/asm-offsets.c | 1 -
 1 file changed, 1 deletion(-)

arch/x86/kernel/asm-offsets.c +0 -0

index 84a7524b202c..5c042466f274 100644

x86_64 has very clean espfix handling on paravirt: espfix64 is set up in native_iret, so paravirt systems that override iret bypass espfix64 automatically. This is robust and straightforward.

x86_32 is messier. espfix is set up before the IRET paravirt patch point, so it can't be directly conditionalized on whether we use native_iret. We also can't easily move it into native_iret without regressing performance due to a bizarre consideration. Specifically, on 64-bit kernels, the logic is:

if (regs->ss & 0x4)

          setup_espfix;

On 32-bit kernels, the logic is:

  if ((regs->ss & 0x4) && (regs->cs & 0x3) == 3 &&
      (regs->flags & X86_EFLAGS_VM) == 0)
          setup_espfix;

The performance of setup_espfix itself is essentially irrelevant, but the comparison happens on every IRET so its performance matters. On x86_64, there's no need for any registers except flags to implement the comparison, so we fold the whole thing into native_iret. On x86_32, we don't do that because we need a free register to implement the comparison efficiently. We therefore do espfix setup before restoring registers on x86_32.

This patch gets rid of the explicit paravirt_enabled check by introducing X86_BUG_ESPFIX on 32-bit systems and using an ALTERNATIVE to skip espfix on paravirt systems where iret != native_iret. This is also messy, but it's at least in line with other things we do.

This improves espfix performance by removing a branch, but no one cares. More importantly, it removes a paravirt_enabled user, which is good because paravirt_enabled is ill-defined and is going away.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_32.S          | 15 ++-------------
 arch/x86/include/asm/cpufeatures.h |  8 ++++++++
 arch/x86/kernel/cpu/common.c       | 25 +++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 13 deletions(-)