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

Groups > linux.kernel > #1346038 > unrolled thread

[PATCH v3 9/9] x86/xsaves: Re-enable XSAVES

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 v3 9/9] x86/xsaves: Re-enable XSAVES Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-02-29 18:50 +0100
    Re: [PATCH v3 9/9] x86/xsaves: Re-enable XSAVES Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-03-02 01:40 +0100
      Re: [PATCH v3 9/9] x86/xsaves: Re-enable XSAVES Dave Hansen <dave.hansen@linux.intel.com> - 2016-03-02 01:50 +0100
        Re: [PATCH v3 9/9] x86/xsaves: Re-enable XSAVES Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-03-02 02:00 +0100
        Re: [PATCH v3 9/9] x86/xsaves: Re-enable XSAVES "H. Peter Anvin" <hpa@zytor.com> - 2016-03-02 02:00 +0100
          Re: [PATCH v3 9/9] x86/xsaves: Re-enable XSAVES Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-03-02 02:10 +0100
    Re: [PATCH v3 9/9] x86/xsaves: Re-enable XSAVES Dave Hansen <dave.hansen@linux.intel.com> - 2016-03-02 02:00 +0100

#1346038 — [PATCH v3 9/9] x86/xsaves: Re-enable XSAVES

We did not handle XSAVES* instructions correctly. There were issues in
converting between standard and compacted format when interfacing with
user-space. These issues have been corrected.

Add a WARN_ONCE() to make it clear that XSAVES supervisor states are not
yet implemented.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 arch/x86/kernel/fpu/init.c   | 16 ++++------------
 arch/x86/kernel/fpu/xstate.c |  8 ++++++++
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index b5952d5..21e0d52 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -228,19 +228,11 @@ static void __init fpu__init_system_xstate_size_legacy(void)
 	user_xstate_size = kernel_xstate_size;
 
 	/*
-	 * Quirk: we don't yet handle the XSAVES* instructions
-	 * correctly, as we don't correctly convert between
-	 * standard and compacted format when interfacing
-	 * with user-space - so disable it for now.
-	 *
-	 * The difference is small: with recent CPUs the
-	 * compacted format is only marginally smaller than
-	 * the standard FPU state format.
-	 *
-	 * ( This is easy to backport while we are fixing
-	 *   XSAVES* support. )
+	 * Most recent CPUs supporting XSAVES can run 64-bit mode.
+	 * Enable XSAVES for 64-bit.
 	 */
-	setup_clear_cpu_cap(X86_FEATURE_XSAVES);
+	if (!config_enabled(CONFIG_X86_64))
+		setup_clear_cpu_cap(X86_FEATURE_XSAVES);
 }
 
 /*
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 2e80d6f..cb2a484 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -204,6 +204,14 @@ void fpu__init_cpu_xstate(void)
 	if (!cpu_has_xsave || !xfeatures_mask)
 		return;
 
+	/*
+	 * Make it clear that XSAVES supervisor states are not yet
+	 * implemented should anyone expect it to work by changing
+	 * bits in XFEATURE_MASK_* macros and XCR0.
+	 */
+	WARN_ONCE((xfeatures_mask & XFEATURE_MASK_SUPERVISOR),
+		"x86/fpu: XSAVES supervisor states are not yet implemented.\n");
+
 	cr4_set_bits(X86_CR4_OSXSAVE);
 	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
 }
-- 
1.9.1

[toc] | [next] | [standalone]

#1347293

On Tue, Mar 01, 2016 at 03:56:12PM -0800, Dave Hansen wrote:
> On 02/29/2016 09:42 AM, Yu-cheng Yu wrote:
> >  	/*
> > -	 * Quirk: we don't yet handle the XSAVES* instructions
> > -	 * correctly, as we don't correctly convert between
> > -	 * standard and compacted format when interfacing
> > -	 * with user-space - so disable it for now.
> > -	 *
> > -	 * The difference is small: with recent CPUs the
> > -	 * compacted format is only marginally smaller than
> > -	 * the standard FPU state format.
> > -	 *
> > -	 * ( This is easy to backport while we are fixing
> > -	 *   XSAVES* support. )
> > +	 * Most recent CPUs supporting XSAVES can run 64-bit mode.
> > +	 * Enable XSAVES for 64-bit.
> >  	 */
> > -	setup_clear_cpu_cap(X86_FEATURE_XSAVES);
> > +	if (!config_enabled(CONFIG_X86_64))
> > +		setup_clear_cpu_cap(X86_FEATURE_XSAVES);
> >  }
> 
> I think we need a much better explanation of this for posterity.  Why
> are we not supporting this now, and what would someone have to do in the
> future in order to enable it?
> 
If anyone is using this newer feature, then that user is most likely using
a 64-bit capable processor and a 64-bit kernel. The intention here is to
take out the complexity and any potential of error. If the user removes 
the restriction and builds a private kernel, it should work but we have
not checked all possible combinations. I will put these in the comments.

> >  /*
> > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> > index 2e80d6f..cb2a484 100644
> > --- a/arch/x86/kernel/fpu/xstate.c
> > +++ b/arch/x86/kernel/fpu/xstate.c
> > @@ -204,6 +204,14 @@ void fpu__init_cpu_xstate(void)
> >  	if (!cpu_has_xsave || !xfeatures_mask)
> >  		return;
> >  
> > +	/*
> > +	 * Make it clear that XSAVES supervisor states are not yet
> > +	 * implemented should anyone expect it to work by changing
> > +	 * bits in XFEATURE_MASK_* macros and XCR0.
> > +	 */
> > +	WARN_ONCE((xfeatures_mask & XFEATURE_MASK_SUPERVISOR),
> > +		"x86/fpu: XSAVES supervisor states are not yet implemented.\n");
> > +
> >  	cr4_set_bits(X86_CR4_OSXSAVE);
> >  	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
> >  }
> 
> Let's also do a:
> 
> 	xfeatures_mask &= ~XFEATURE_MASK_SUPERVISOR;
> 
> Otherwise, we have a broken system at the moment.
> 
Currently, if anyone sets any supervisor state in xfeatures_mask, the
kernel prints out the warning then goes into a protection fault.
That is a very strong indication to the user. Do we want to mute it? 

Yu-cheng

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

#1347310

On 03/01/2016 04:34 PM, Yu-cheng Yu wrote:
> On Tue, Mar 01, 2016 at 03:56:12PM -0800, Dave Hansen wrote:
>> On 02/29/2016 09:42 AM, Yu-cheng Yu wrote:
>>> -	setup_clear_cpu_cap(X86_FEATURE_XSAVES);
>>> +	if (!config_enabled(CONFIG_X86_64))
>>> +		setup_clear_cpu_cap(X86_FEATURE_XSAVES);
>>>  }
>>
>> I think we need a much better explanation of this for posterity.  Why
>> are we not supporting this now, and what would someone have to do in the
>> future in order to enable it?
>>
> If anyone is using this newer feature, then that user is most likely using
> a 64-bit capable processor and a 64-bit kernel. The intention here is to
> take out the complexity and any potential of error. If the user removes 
> the restriction and builds a private kernel, it should work but we have
> not checked all possible combinations. I will put these in the comments.

A user can go download a 32-bit version of Ubuntu or Debian and install
it on a 64-bit processor today.  It's a very easy mistake to make when
downloading the install CD.

In any case, I don't have a _problem_ with leaving i386 in the dust
here.  I just want us to be very explicit about what we are doing.

>>> +	/*
>>> +	 * Make it clear that XSAVES supervisor states are not yet
>>> +	 * implemented should anyone expect it to work by changing
>>> +	 * bits in XFEATURE_MASK_* macros and XCR0.
>>> +	 */
>>> +	WARN_ONCE((xfeatures_mask & XFEATURE_MASK_SUPERVISOR),
>>> +		"x86/fpu: XSAVES supervisor states are not yet implemented.\n");
>>> +
>>>  	cr4_set_bits(X86_CR4_OSXSAVE);
>>>  	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
>>>  }
>>
>> Let's also do a:
>>
>> 	xfeatures_mask &= ~XFEATURE_MASK_SUPERVISOR;
>>
>> Otherwise, we have a broken system at the moment.
>>
> Currently, if anyone sets any supervisor state in xfeatures_mask, the
> kernel prints out the warning then goes into a protection fault.
> That is a very strong indication to the user. Do we want to mute it? 

By "goes into a protection fault", do you mean that it doesn't boot?

I'd just rather we put the kernel in a known-safe configuration (masking
supervisor state out of xfeatures_mask) rather than rely on the general
protection fault continuing to be generated by whatever is generating it.

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

#1347376

On Tue, Mar 01, 2016 at 04:45:41PM -0800, Dave Hansen wrote:
> >>> +	WARN_ONCE((xfeatures_mask & XFEATURE_MASK_SUPERVISOR),
> >>> +		"x86/fpu: XSAVES supervisor states are not yet implemented.\n");
> >>> +
> >>>  	cr4_set_bits(X86_CR4_OSXSAVE);
> >>>  	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
> >>>  }
> >>
> >> Let's also do a:
> >>
> >> 	xfeatures_mask &= ~XFEATURE_MASK_SUPERVISOR;
> >>
> >> Otherwise, we have a broken system at the moment.
> >>
> > Currently, if anyone sets any supervisor state in xfeatures_mask, the
> > kernel prints out the warning then goes into a protection fault.
> > That is a very strong indication to the user. Do we want to mute it? 
> 
> By "goes into a protection fault", do you mean that it doesn't boot?
> 
> I'd just rather we put the kernel in a known-safe configuration (masking
> supervisor state out of xfeatures_mask) rather than rely on the general
> protection fault continuing to be generated by whatever is generating it.
> 
Ok.

Yu-cheng

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

#1347383

On March 1, 2016 4:45:41 PM PST, Dave Hansen <dave.hansen@linux.intel.com> wrote:
>On 03/01/2016 04:34 PM, Yu-cheng Yu wrote:
>> On Tue, Mar 01, 2016 at 03:56:12PM -0800, Dave Hansen wrote:
>>> On 02/29/2016 09:42 AM, Yu-cheng Yu wrote:
>>>> -	setup_clear_cpu_cap(X86_FEATURE_XSAVES);
>>>> +	if (!config_enabled(CONFIG_X86_64))
>>>> +		setup_clear_cpu_cap(X86_FEATURE_XSAVES);
>>>>  }
>>>
>>> I think we need a much better explanation of this for posterity. 
>Why
>>> are we not supporting this now, and what would someone have to do in
>the
>>> future in order to enable it?
>>>
>> If anyone is using this newer feature, then that user is most likely
>using
>> a 64-bit capable processor and a 64-bit kernel. The intention here is
>to
>> take out the complexity and any potential of error. If the user
>removes 
>> the restriction and builds a private kernel, it should work but we
>have
>> not checked all possible combinations. I will put these in the
>comments.
>
>A user can go download a 32-bit version of Ubuntu or Debian and install
>it on a 64-bit processor today.  It's a very easy mistake to make when
>downloading the install CD.
>
>In any case, I don't have a _problem_ with leaving i386 in the dust
>here.  I just want us to be very explicit about what we are doing.
>
>>>> +	/*
>>>> +	 * Make it clear that XSAVES supervisor states are not yet
>>>> +	 * implemented should anyone expect it to work by changing
>>>> +	 * bits in XFEATURE_MASK_* macros and XCR0.
>>>> +	 */
>>>> +	WARN_ONCE((xfeatures_mask & XFEATURE_MASK_SUPERVISOR),
>>>> +		"x86/fpu: XSAVES supervisor states are not yet implemented.\n");
>>>> +
>>>>  	cr4_set_bits(X86_CR4_OSXSAVE);
>>>>  	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
>>>>  }
>>>
>>> Let's also do a:
>>>
>>> 	xfeatures_mask &= ~XFEATURE_MASK_SUPERVISOR;
>>>
>>> Otherwise, we have a broken system at the moment.
>>>
>> Currently, if anyone sets any supervisor state in xfeatures_mask, the
>> kernel prints out the warning then goes into a protection fault.
>> That is a very strong indication to the user. Do we want to mute it? 
>
>By "goes into a protection fault", do you mean that it doesn't boot?
>
>I'd just rather we put the kernel in a known-safe configuration
>(masking
>supervisor state out of xfeatures_mask) rather than rely on the general
>protection fault continuing to be generated by whatever is generating
>it.

Differences between i386 and x86-64 generally add problems, so unless this requires significant 32-bit-specific code we should not exclude i386 just because.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

#1347415

On Tue, Mar 01, 2016 at 04:53:53PM -0800, H. Peter Anvin wrote:
> Differences between i386 and x86-64 generally add problems, so unless this requires significant 32-bit-specific code we should not exclude i386 just because.

I have not seen any issues with 32-bit code, but will do some tests.
Thanks.

Yu-cheng

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

#1347354

On 02/29/2016 09:42 AM, Yu-cheng Yu wrote:
>  	/*
> -	 * Quirk: we don't yet handle the XSAVES* instructions
> -	 * correctly, as we don't correctly convert between
> -	 * standard and compacted format when interfacing
> -	 * with user-space - so disable it for now.
> -	 *
> -	 * The difference is small: with recent CPUs the
> -	 * compacted format is only marginally smaller than
> -	 * the standard FPU state format.
> -	 *
> -	 * ( This is easy to backport while we are fixing
> -	 *   XSAVES* support. )
> +	 * Most recent CPUs supporting XSAVES can run 64-bit mode.
> +	 * Enable XSAVES for 64-bit.
>  	 */
> -	setup_clear_cpu_cap(X86_FEATURE_XSAVES);
> +	if (!config_enabled(CONFIG_X86_64))
> +		setup_clear_cpu_cap(X86_FEATURE_XSAVES);
>  }

I think we need a much better explanation of this for posterity.  Why
are we not supporting this now, and what would someone have to do in the
future in order to enable it?

>  /*
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 2e80d6f..cb2a484 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -204,6 +204,14 @@ void fpu__init_cpu_xstate(void)
>  	if (!cpu_has_xsave || !xfeatures_mask)
>  		return;
>  
> +	/*
> +	 * Make it clear that XSAVES supervisor states are not yet
> +	 * implemented should anyone expect it to work by changing
> +	 * bits in XFEATURE_MASK_* macros and XCR0.
> +	 */
> +	WARN_ONCE((xfeatures_mask & XFEATURE_MASK_SUPERVISOR),
> +		"x86/fpu: XSAVES supervisor states are not yet implemented.\n");
> +
>  	cr4_set_bits(X86_CR4_OSXSAVE);
>  	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
>  }

Let's also do a:

	xfeatures_mask &= ~XFEATURE_MASK_SUPERVISOR;

Otherwise, we have a broken system at the moment.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web