Patchwork [v2,3/3] X86, mpx: Intel MPX xstate feature definition

login
register
mail settings
Submitter Qiaowei Ren
Date Dec. 7, 2013, 12:20 a.m.
Message ID <1386375658-2191-3-git-send-email-qiaowei.ren@intel.com>
Download mbox | patch
Permalink /patch/298108/
State New
Headers show

Comments

Paolo Bonzini - Dec. 6, 2013, 5:35 p.m.
Il 07/12/2013 01:20, Qiaowei Ren ha scritto:
> This patch defines xstate feature and extends struct xsave_hdr_struct
> to support Intel MPX.
> 
> Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com>
> Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> Signed-off-by: Liu Jinsong <jinsong.liu@intel.com>
> ---
>  arch/x86/include/asm/processor.h |   12 ++++++++++++
>  arch/x86/include/asm/xsave.h     |    5 ++++-
>  2 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 987c75e..2fe2e75 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -370,6 +370,15 @@ struct ymmh_struct {
>  	u32 ymmh_space[64];
>  };
>  
> +struct bndregs_struct {
> +	u64 bndregs[8];
> +} __packed;
> +
> +struct bndcsr_struct {
> +	u64 cfg_reg_u;
> +	u64 status_reg;
> +} __packed;
> +
>  struct xsave_hdr_struct {
>  	u64 xstate_bv;
>  	u64 reserved1[2];
> @@ -380,6 +389,9 @@ struct xsave_struct {
>  	struct i387_fxsave_struct i387;
>  	struct xsave_hdr_struct xsave_hdr;
>  	struct ymmh_struct ymmh;
> +	u8 lwp_area[128];

Sorry for the back-and-forth, but I think this and the removal of
XSTATE_FLEXIBLE (perhaps XSTATE_LAZY?) makes your v2 worse than v1.

Since Peter already said the same, please undo these changes.

Also, how is XSTATE_EAGER used?  Should MPX be disabled when xsaveopt is
disabled on the kernel command line?  (Liu, how would this affect the
KVM patches, too?)

Paolo

> +#define XSTATE_EAGER	(XSTATE_BNDREGS | XSTATE_BNDCSR)
>  /*
>   * These are the features that the OS can handle currently.
>   */
> -#define XCNTXT_MASK	(XSTATE_FP | XSTATE_SSE | XSTATE_YMM)
> +#define XCNTXT_MASK    (XSTATE_FP | XSTATE_SSE | XSTATE_YMM | XSTATE_EAGER)
>  
>  #ifdef CONFIG_X86_64
>  #define REX_PREFIX	"0x48, "
>
H. Peter Anvin - Dec. 6, 2013, 5:55 p.m.
On 12/06/2013 09:35 AM, Paolo Bonzini wrote:
> 
> Sorry for the back-and-forth, but I think this and the removal of
> XSTATE_FLEXIBLE (perhaps XSTATE_LAZY?) makes your v2 worse than v1.
> 
> Since Peter already said the same, please undo these changes.
> 
> Also, how is XSTATE_EAGER used?  Should MPX be disabled when xsaveopt is
> disabled on the kernel command line?  (Liu, how would this affect the
> KVM patches, too?)
> 

There are two options: we could disable MPX etc. or we could force eager
saving (using xsave) even if xsaveopt is disabled.  It is a hard call to
make, but I guess I'm leaning towards the latter; we could add an
"lazyxsave" option to explicitly disable all eager features if there is
use for that.

	-hpa
Liu, Jinsong - Dec. 6, 2013, 8:05 p.m.
Paolo Bonzini wrote:
> Il 07/12/2013 01:20, Qiaowei Ren ha scritto:
>> This patch defines xstate feature and extends struct xsave_hdr_struct
>> to support Intel MPX.
>> 
>> Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com>
>> Signed-off-by: Xudong Hao <xudong.hao@intel.com>
>> Signed-off-by: Liu Jinsong <jinsong.liu@intel.com>
>> ---
>>  arch/x86/include/asm/processor.h |   12 ++++++++++++
>>  arch/x86/include/asm/xsave.h     |    5 ++++-
>>  2 files changed, 16 insertions(+), 1 deletions(-)
>> 
>> diff --git a/arch/x86/include/asm/processor.h
>> b/arch/x86/include/asm/processor.h index 987c75e..2fe2e75 100644 ---
>> a/arch/x86/include/asm/processor.h +++
>> b/arch/x86/include/asm/processor.h @@ -370,6 +370,15 @@ struct
>>  	ymmh_struct { u32 ymmh_space[64];
>>  };
>> 
>> +struct bndregs_struct {
>> +	u64 bndregs[8];
>> +} __packed;
>> +
>> +struct bndcsr_struct {
>> +	u64 cfg_reg_u;
>> +	u64 status_reg;
>> +} __packed;
>> +
>>  struct xsave_hdr_struct {
>>  	u64 xstate_bv;
>>  	u64 reserved1[2];
>> @@ -380,6 +389,9 @@ struct xsave_struct {
>>  	struct i387_fxsave_struct i387;
>>  	struct xsave_hdr_struct xsave_hdr;
>>  	struct ymmh_struct ymmh;
>> +	u8 lwp_area[128];
> 
> Sorry for the back-and-forth, but I think this and the removal of
> XSTATE_FLEXIBLE (perhaps XSTATE_LAZY?) makes your v2 worse than v1.
> 
> Since Peter already said the same, please undo these changes.
> 
> Also, how is XSTATE_EAGER used?  Should MPX be disabled when xsaveopt
> is disabled on the kernel command line?  (Liu, how would this affect
> the KVM patches, too?)
> 
> Paolo

Currently seems no, and if needed we can add a new patch at kvm side accordingly when native mpx patches checked in.

Jinsong

> 
>> +#define XSTATE_EAGER	(XSTATE_BNDREGS | XSTATE_BNDCSR)  /*
>>   * These are the features that the OS can handle currently.   */
>> -#define XCNTXT_MASK	(XSTATE_FP | XSTATE_SSE | XSTATE_YMM)
>> +#define XCNTXT_MASK    (XSTATE_FP | XSTATE_SSE | XSTATE_YMM |
>> XSTATE_EAGER) 
>> 
>>  #ifdef CONFIG_X86_64
>>  #define REX_PREFIX	"0x48, "
H. Peter Anvin - Dec. 6, 2013, 9:33 p.m.
On 12/06/2013 12:05 PM, Liu, Jinsong wrote:
>>
>> Since Peter already said the same, please undo these changes.
>>
>> Also, how is XSTATE_EAGER used?  Should MPX be disabled when xsaveopt
>> is disabled on the kernel command line?  (Liu, how would this affect
>> the KVM patches, too?)
>>
>> Paolo
> 
> Currently seems no, and if needed we can add a new patch at kvm side accordingly when native mpx patches checked in.
> 

We need to either disable these features in lazy mode, or we need to
force eager mode if these features are to be supported.  The problem
with the latter is that it means forcing eager mode regardless of if
anything actually *uses* these features.

A third option would be to require applications to use a prctl() or
similar to enable eager-save features.

Thoughts?

	-hpa
Liu, Jinsong - Dec. 6, 2013, 10:12 p.m.
H. Peter Anvin wrote:
> On 12/06/2013 12:05 PM, Liu, Jinsong wrote:
>>> 
>>> Since Peter already said the same, please undo these changes.
>>> 
>>> Also, how is XSTATE_EAGER used?  Should MPX be disabled when
>>> xsaveopt is disabled on the kernel command line?  (Liu, how would
>>> this affect the KVM patches, too?) 
>>> 
>>> Paolo
>> 
>> Currently seems no, and if needed we can add a new patch at kvm side
>> accordingly when native mpx patches checked in. 
>> 
> 
> We need to either disable these features in lazy mode, or we need to
> force eager mode if these features are to be supported.  The problem
> with the latter is that it means forcing eager mode regardless of if
> anything actually *uses* these features.
> 
> A third option would be to require applications to use a prctl() or
> similar to enable eager-save features.
> 
> Thoughts?
> 
> 	-hpa

The third option seems better -- how does native mpx patches work, force eager?

Thanks,
Jinsong
Qiaowei Ren - Dec. 7, 2013, 12:20 a.m.
This patch defines xstate feature and extends struct xsave_hdr_struct
to support Intel MPX.

Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com>
Signed-off-by: Xudong Hao <xudong.hao@intel.com>
Signed-off-by: Liu Jinsong <jinsong.liu@intel.com>
---
 arch/x86/include/asm/processor.h |   12 ++++++++++++
 arch/x86/include/asm/xsave.h     |    5 ++++-
 2 files changed, 16 insertions(+), 1 deletions(-)
Qiaowei Ren - Dec. 7, 2013, 12:23 a.m.
> -----Original Message-----
> From: Liu, Jinsong
> Sent: Saturday, December 07, 2013 6:13 AM
> To: H. Peter Anvin; Paolo Bonzini; Ren, Qiaowei
> Cc: kvm@vger.kernel.org; x86@kernel.org; Xudong Hao;
> qemu-devel@nongnu.org; linux-kernel@vger.kernel.org; Ingo Molnar; Thomas
> Gleixner
> Subject: RE: [Qemu-devel] [PATCH v2 3/3] X86, mpx: Intel MPX xstate feature
> definition
> 
> H. Peter Anvin wrote:
> > On 12/06/2013 12:05 PM, Liu, Jinsong wrote:
> >>>
> >>> Since Peter already said the same, please undo these changes.
> >>>
> >>> Also, how is XSTATE_EAGER used?  Should MPX be disabled when
> >>> xsaveopt is disabled on the kernel command line?  (Liu, how would
> >>> this affect the KVM patches, too?)
> >>>
> >>> Paolo
> >>
> >> Currently seems no, and if needed we can add a new patch at kvm side
> >> accordingly when native mpx patches checked in.
> >>
> >
> > We need to either disable these features in lazy mode, or we need to
> > force eager mode if these features are to be supported.  The problem
> > with the latter is that it means forcing eager mode regardless of if
> > anything actually *uses* these features.
> >
> > A third option would be to require applications to use a prctl() or
> > similar to enable eager-save features.
> >
> > Thoughts?
> >
> > 	-hpa
> 
> The third option seems better -- how does native mpx patches work, force
> eager?
> 
It should be the second option, as you can see xsave.c which we remove from this patch. :)

Thanks,
Qiaowei
H. Peter Anvin - Dec. 7, 2013, 1:07 a.m.
On 12/06/2013 04:23 PM, Ren, Qiaowei wrote:
>>>
>>> We need to either disable these features in lazy mode, or we need to
>>> force eager mode if these features are to be supported.  The problem
>>> with the latter is that it means forcing eager mode regardless of if
>>> anything actually *uses* these features.
>>>
>>> A third option would be to require applications to use a prctl() or
>>> similar to enable eager-save features.
>>>
>>
>> The third option seems better -- how does native mpx patches work, force
>> eager?
>>
> It should be the second option, as you can see xsave.c which we remove from this patch. :)
> 

Ah yes... I missed the fact that that chunk had been dropped from this
patch.  It really shouldn't be.

I'll substitute the previous version of the patch.

	-hpa
Qiaowei Ren - Dec. 7, 2013, 1:16 a.m.
> -----Original Message-----

> From: H. Peter Anvin [mailto:hpa@zytor.com]

> Sent: Saturday, December 07, 2013 9:07 AM

> To: Ren, Qiaowei; Liu, Jinsong; Paolo Bonzini

> Cc: kvm@vger.kernel.org; x86@kernel.org; Xudong Hao;

> qemu-devel@nongnu.org; linux-kernel@vger.kernel.org; Ingo Molnar; Thomas

> Gleixner

> Subject: Re: [Qemu-devel] [PATCH v2 3/3] X86, mpx: Intel MPX xstate feature

> definition

> 

> On 12/06/2013 04:23 PM, Ren, Qiaowei wrote:

> >>>

> >>> We need to either disable these features in lazy mode, or we need to

> >>> force eager mode if these features are to be supported.  The problem

> >>> with the latter is that it means forcing eager mode regardless of if

> >>> anything actually *uses* these features.

> >>>

> >>> A third option would be to require applications to use a prctl() or

> >>> similar to enable eager-save features.

> >>>

> >>

> >> The third option seems better -- how does native mpx patches work,

> >> force eager?

> >>

> > It should be the second option, as you can see xsave.c which we remove

> > from this patch. :)

> >

> 

> Ah yes... I missed the fact that that chunk had been dropped from this patch.

> It really shouldn't be.

> 

> I'll substitute the previous version of the patch.

> 

Jinsong think that both kvm and host depend on these feature definition header file, so we firstly submit these files depended on.

Thanks,
Qiaowei
H. Peter Anvin - Dec. 7, 2013, 1:19 a.m.
On 12/06/2013 05:16 PM, Ren, Qiaowei wrote:
> Jinsong think that both kvm and host depend on these feature definition header file, so we firstly submit these files depended on.

Yes, but we can't turn on the feature without proper protection.  Either
way, they are now in tip:x86/cpufeature.

	-hpa

Patch

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 987c75e..2fe2e75 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -370,6 +370,15 @@  struct ymmh_struct {
 	u32 ymmh_space[64];
 };
 
+struct bndregs_struct {
+	u64 bndregs[8];
+} __packed;
+
+struct bndcsr_struct {
+	u64 cfg_reg_u;
+	u64 status_reg;
+} __packed;
+
 struct xsave_hdr_struct {
 	u64 xstate_bv;
 	u64 reserved1[2];
@@ -380,6 +389,9 @@  struct xsave_struct {
 	struct i387_fxsave_struct i387;
 	struct xsave_hdr_struct xsave_hdr;
 	struct ymmh_struct ymmh;
+	u8 lwp_area[128];
+	struct bndregs_struct bndregs;
+	struct bndcsr_struct bndcsr;
 	/* new processor state extensions will go here */
 } __attribute__ ((packed, aligned (64)));
 
diff --git a/arch/x86/include/asm/xsave.h b/arch/x86/include/asm/xsave.h
index 0415cda..7fa8855 100644
--- a/arch/x86/include/asm/xsave.h
+++ b/arch/x86/include/asm/xsave.h
@@ -9,6 +9,8 @@ 
 #define XSTATE_FP	0x1
 #define XSTATE_SSE	0x2
 #define XSTATE_YMM	0x4
+#define XSTATE_BNDREGS	0x8
+#define XSTATE_BNDCSR	0x10
 
 #define XSTATE_FPSSE	(XSTATE_FP | XSTATE_SSE)
 
@@ -20,10 +22,11 @@ 
 #define XSAVE_YMM_SIZE	    256
 #define XSAVE_YMM_OFFSET    (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)
 
+#define XSTATE_EAGER	(XSTATE_BNDREGS | XSTATE_BNDCSR)
 /*
  * These are the features that the OS can handle currently.
  */
-#define XCNTXT_MASK	(XSTATE_FP | XSTATE_SSE | XSTATE_YMM)
+#define XCNTXT_MASK    (XSTATE_FP | XSTATE_SSE | XSTATE_YMM | XSTATE_EAGER)
 
 #ifdef CONFIG_X86_64
 #define REX_PREFIX	"0x48, "