[PATCH v2 0/2] x86/entry/32: Get rid of paravirt_enabled in ESPFIX

[PATCH v2 0/2] x86/entry/32: Get rid of paravirt_enabled in ESPFIX

[Andy Lutomirski]

[v2 because I screwed up sending it really badly and it's not worth
trying to disentangle the mess]

Hi Luis-

As promised, here are these patches.

Borislav, if you're okay with this (ab)use of the cpufeatures stuff
and if they survive review, I'd be okay with them joining Luis'
series or going straight into tip:x86/asm.

Andy Lutomirski (2):
  x86/entry/32: Introduce and use X86_BUG_ESPFIX instead of
    paravirt_enabled
  x86/asm-offsets: Remove PARAVIRT_enabled

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

-- 
2.5.0

_______________________________________________
Lguest mailing list
Lguest@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/lguest

[PATCH v2 1/2] x86/entry/32: Introduce and use X86_BUG_ESPFIX instead of paravirt_enabled

[Andy Lutomirski]

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 <luto@kernel.org>
---
 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(-)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 6a792603f9f3..4f7615e405a4 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -418,6 +418,8 @@ restore_all:
 	TRACE_IRQS_IRET
 restore_all_notrace:
 #ifdef CONFIG_X86_ESPFIX32
+	ALTERNATIVE	"jmp restore_nocheck", "", X86_BUG_ESPFIX
+
 	movl	PT_EFLAGS(%esp), %eax		# mix EFLAGS, SS and CS
 	/*
 	 * Warning: PT_OLDSS(%esp) contains the wrong/random values if we
@@ -444,19 +446,6 @@ ENTRY(iret_exc	)
 
 #ifdef CONFIG_X86_ESPFIX32
 ldt_ss:
-#ifdef CONFIG_PARAVIRT
-	/*
-	 * The kernel can't run on a non-flat stack if paravirt mode
-	 * is active.  Rather than try to fixup the high bits of
-	 * ESP, bypass this code entirely.  This may break DOSemu
-	 * and/or Wine support in a paravirt VM, although the option
-	 * is still available to implement the setting of the high
-	 * 16-bits in the INTERRUPT_RETURN paravirt-op.
-	 */
-	cmpl	$0, pv_info+PARAVIRT_enabled
-	jne	restore_nocheck
-#endif
-
 /*
  * Setup and switch to ESPFIX stack
  *
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 6663fae71b12..d11a3aaafd96 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -286,4 +286,12 @@
 #define X86_BUG_CLFLUSH_MONITOR	X86_BUG(7) /* AAI65, CLFLUSH required before MONITOR */
 #define X86_BUG_SYSRET_SS_ATTRS	X86_BUG(8) /* SYSRET doesn't fix up SS attrs */
 
+#ifdef CONFIG_X86_32
+/*
+ * 64-bit kernels don't use X86_BUG_ESPFIX.  Make the define conditional
+ * to avoid confusion.
+ */
+#define X86_BUG_ESPFIX		X86_BUG(9) /* "" IRET to 16-bit SS corrupts ESP/RSP high bits */
+#endif
+
 #endif /* _ASM_X86_CPUFEATURES_H */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 91ddae732a36..c6ef4da8e4f4 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -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
 }
 
 /*
-- 
2.5.0

_______________________________________________
Lguest mailing list
Lguest@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/lguest

Re: [PATCH v2 1/2] x86/entry/32: Introduce and use X86_BUG_ESPFIX instead of paravirt_enabled

[Boris Ostrovsky]

On 02/29/2016 06:50 PM, Andy Lutomirski wrote:
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 91ddae732a36..c6ef4da8e4f4 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -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
>   }
>   
>   /*


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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 1/2] x86/entry/32: Introduce and use X86_BUG_ESPFIX instead of paravirt_enabled

[Andy Lutomirski]

On Tue, Mar 1, 2016 at 6:00 AM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
> On 02/29/2016 06:50 PM, Andy Lutomirski wrote:
>>
>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>> index 91ddae732a36..c6ef4da8e4f4 100644
>> --- a/arch/x86/kernel/cpu/common.c
>> +++ b/arch/x86/kernel/cpu/common.c
>> @@ -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
>>   }
>>     /*
>
>
>
> 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.

>
> -boris



-- 
Andy Lutomirski
AMA Capital Management, LLC
_______________________________________________
Lguest mailing list
Lguest@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/lguest

Re: [PATCH v2 1/2] x86/entry/32: Introduce and use X86_BUG_ESPFIX instead of paravirt_enabled

[Luis R. Rodriguez]

On Tue, Mar 01, 2016 at 07:44:10AM -0800, Andy Lutomirski wrote:
> On Tue, Mar 1, 2016 at 6:00 AM, Boris Ostrovsky
> <boris.ostrovsky@oracle.com> wrote:
> > On 02/29/2016 06:50 PM, Andy Lutomirski wrote:
> >>
> >> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> >> index 91ddae732a36..c6ef4da8e4f4 100644
> >> --- a/arch/x86/kernel/cpu/common.c
> >> +++ b/arch/x86/kernel/cpu/common.c
> >> @@ -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
> >>   }
> >>     /*
> >
> >
> >
> > 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.

I would if I was certain of some things and I also understood this really well,
sadly I don't, but I'll ask questions and we'll see. Replies to follow the
thread.

  Luis

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 1/2] x86/entry/32: Introduce and use X86_BUG_ESPFIX instead of paravirt_enabled

[Luis R. Rodriguez]

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

On Tue, Mar 01, 2016 at 09:00:53AM -0500, Boris Ostrovsky wrote:
> On 02/29/2016 06:50 PM, Andy Lutomirski wrote:
> >diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> >index 91ddae732a36..c6ef4da8e4f4 100644
> >--- a/arch/x86/kernel/cpu/common.c
> >+++ b/arch/x86/kernel/cpu/common.c
> >@@ -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.

> >  #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
> >  }
> >  /*
>
> 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.

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

Perhaps there are others. 2) and 3) are similar but 3) ensures the platform
struct is extended / documented per quirk.

  Luis

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 1/2] x86/entry/32: Introduce and use X86_BUG_ESPFIX instead of paravirt_enabled

[Andy Lutomirski]

On Tue, Mar 1, 2016 at 4:15 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> Ingo, your feedback appreciated at the end here, regarding quirks.
>
> On Tue, Mar 01, 2016 at 09:00:53AM -0500, Boris Ostrovsky wrote:
>> On 02/29/2016 06:50 PM, Andy Lutomirski wrote:
>> >diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>> >index 91ddae732a36..c6ef4da8e4f4 100644
>> >--- a/arch/x86/kernel/cpu/common.c
>> >+++ b/arch/x86/kernel/cpu/common.c
>> >@@ -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.
>
>> >  #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
>> >  }
>> >  /*
>>
>> 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.
>
>> (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.
>

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.
_______________________________________________
Lguest mailing list
Lguest@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/lguest

Re: [PATCH v2 1/2] x86/entry/32: Introduce and use X86_BUG_ESPFIX instead of paravirt_enabled

[Luis R. Rodriguez]

On Wed, Mar 02, 2016 at 04:33:06PM -0800, Andy Lutomirski wrote:
> On Tue, Mar 1, 2016 at 4:15 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > Ingo, your feedback appreciated at the end here, regarding quirks.
> >
> > On Tue, Mar 01, 2016 at 09:00:53AM -0500, Boris Ostrovsky wrote:
> >> On 02/29/2016 06:50 PM, Andy Lutomirski wrote:
> >> >diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> >> >index 91ddae732a36..c6ef4da8e4f4 100644
> >> >--- a/arch/x86/kernel/cpu/common.c
> >> >+++ b/arch/x86/kernel/cpu/common.c
> >> >@@ -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.
> >
> >> >  #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
> >> >  }
> >> >  /*
> >>
> >> 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.
> >
> >> (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.
> >
> 
> 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.

OK great, I'd much prefer that, in particular as I could not find any
other obvious CPU "quirk" to really fold this as a platform quirk and
I really did not think this was enough to justify having lguest have
get a new hypervisor struct added. That just seemed overkill.

  Luis

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 1/2] x86/entry/32: Introduce and use X86_BUG_ESPFIX instead of paravirt_enabled

[Luis R. Rodriguez]

On Mon, Feb 29, 2016 at 03:50:19PM -0800, Andy Lutomirski wrote:
> 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.

This I think I get as all the ESP hackery is on native_iret():

arch/x86/entry/entry_64.S
ENTRY(native_iret)                                                              
        /*                                                                      
         * Are we returning to a stack segment from the LDT?  Note: in          
         * 64-bit mode SS:RSP on the exception stack is always valid.           
         */                                                                     
#ifdef CONFIG_X86_ESPFIX64                                                      
        testb   $4, (SS-RIP)(%rsp)                                              
        jnz     native_irq_return_ldt                                           
#endif 

native_iret is simply expected to not be used on paravirt systems.

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

This I don't quite get.

I see on paravirt.h:                                                              
                                                                                
#define INTERRUPT_RETURN                                                \       
        PARA_SITE(PARA_PATCH(pv_cpu_ops, PV_CPU_iret), CLBR_NONE,       \       
                  jmp PARA_INDIRECT(pv_cpu_ops+PV_CPU_iret))  

The rest is unclear, in particular how this would be late.

  Luis

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 1/2] x86/entry/32: Introduce and use X86_BUG_ESPFIX instead of paravirt_enabled

[Borislav Petkov]

On Mon, Feb 29, 2016 at 03:50:19PM -0800, Andy Lutomirski wrote:
> 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 <luto@kernel.org>
> ---
>  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(-)

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
_______________________________________________
Lguest mailing list
Lguest@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/lguest

[PATCH v2 2/2] x86/asm-offsets: Remove PARAVIRT_enabled

[Andy Lutomirski]

It no longer has any users.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/asm-offsets.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index 84a7524b202c..5c042466f274 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -59,7 +59,6 @@ void common(void) {
 
 #ifdef CONFIG_PARAVIRT
 	BLANK();
-	OFFSET(PARAVIRT_enabled, pv_info, paravirt_enabled);
 	OFFSET(PARAVIRT_PATCH_pv_cpu_ops, paravirt_patch_template, pv_cpu_ops);
 	OFFSET(PARAVIRT_PATCH_pv_irq_ops, paravirt_patch_template, pv_irq_ops);
 	OFFSET(PV_IRQ_irq_disable, pv_irq_ops, irq_disable);
-- 
2.5.0

_______________________________________________
Lguest mailing list
Lguest@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/lguest

Re: [PATCH v2 2/2] x86/asm-offsets: Remove PARAVIRT_enabled

[Borislav Petkov]

On Mon, Feb 29, 2016 at 03:50:20PM -0800, Andy Lutomirski wrote:
> It no longer has any users.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/kernel/asm-offsets.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
> index 84a7524b202c..5c042466f274 100644
> --- a/arch/x86/kernel/asm-offsets.c
> +++ b/arch/x86/kernel/asm-offsets.c
> @@ -59,7 +59,6 @@ void common(void) {
>  
>  #ifdef CONFIG_PARAVIRT
>  	BLANK();
> -	OFFSET(PARAVIRT_enabled, pv_info, paravirt_enabled);
>  	OFFSET(PARAVIRT_PATCH_pv_cpu_ops, paravirt_patch_template, pv_cpu_ops);
>  	OFFSET(PARAVIRT_PATCH_pv_irq_ops, paravirt_patch_template, pv_irq_ops);
>  	OFFSET(PV_IRQ_irq_disable, pv_irq_ops, irq_disable);
> -- 

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
_______________________________________________
Lguest mailing list
Lguest@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/lguest

Re: [PATCH v2 0/2] x86/entry/32: Get rid of paravirt_enabled in ESPFIX

[Borislav Petkov]

On Mon, Feb 29, 2016 at 03:50:18PM -0800, Andy Lutomirski wrote:
> 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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 0/2] x86/entry/32: Get rid of paravirt_enabled in ESPFIX

[Andy Lutomirski]

On Mar 1, 2016 2:46 PM, "Borislav Petkov" <bp@alien8.de> wrote:
>
> On Mon, Feb 29, 2016 at 03:50:18PM -0800, Andy Lutomirski wrote:
> > Borislav, if you're okay with this (ab)use of the cpufeatures stuff
>
> Because of X86_BUG_ESPFIX? Why abuse?

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

>
> --
> Regards/Gruss,
>     Boris.
>
> ECO tip #101: Trim your mails when you reply.
_______________________________________________
Lguest mailing list
Lguest@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/lguest

Re: [PATCH v2 0/2] x86/entry/32: Get rid of paravirt_enabled in ESPFIX

[Borislav Petkov]

On Wed, Mar 02, 2016 at 04:47:38PM -0800, Andy Lutomirski wrote:
> 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.
_______________________________________________
Lguest mailing list
Lguest@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/lguest

Re: [PATCH v2 0/2] x86/entry/32: Get rid of paravirt_enabled in ESPFIX

[Andy Lutomirski]

On Thu, Mar 3, 2016 at 2:18 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Mar 02, 2016 at 04:47:38PM -0800, Andy Lutomirski wrote:
>> 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.

So should these patches just go in as is?

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC
_______________________________________________
Lguest mailing list
Lguest@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/lguest