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

login
register
mail settings
Submitter Qiaowei Ren
Date Dec. 6, 2013, 6:52 p.m.
Message ID <1386355976-11732-3-git-send-email-qiaowei.ren@intel.com>
Download mbox | patch
Permalink /patch/298043/
State New
Headers show

Comments

Qiaowei Ren - Dec. 6, 2013, 4:08 p.m.
> -----Original Message-----

> From: Borislav Petkov [mailto:bp@alien8.de]

> Sent: Friday, December 06, 2013 9:47 PM

> To: Ren, Qiaowei

> Cc: Paolo Bonzini; H. Peter Anvin; Ingo Molnar; Thomas Gleixner;

> x86@kernel.org; linux-kernel@vger.kernel.org; qemu-devel@nongnu.org;

> kvm@vger.kernel.org; Xudong Hao; Liu, Jinsong

> Subject: Re: [PATCH 3/3] X86, mpx: Intel MPX xstate feature definition

> 

> On Sat, Dec 07, 2013 at 02:52:56AM +0800, Qiaowei Ren wrote:

> 

> Commit message please.

> 

> > 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 |   23 +++++++++++++++++++++++

> >  arch/x86/include/asm/xsave.h     |    6 +++++-

> >  2 files changed, 28 insertions(+), 1 deletions(-)

> >

> > diff --git a/arch/x86/include/asm/processor.h

> > b/arch/x86/include/asm/processor.h

> > index 987c75e..43be6f6 100644

> > --- a/arch/x86/include/asm/processor.h

> > +++ b/arch/x86/include/asm/processor.h

> > @@ -370,6 +370,26 @@ struct ymmh_struct {

> >  	u32 ymmh_space[64];

> >  };

> >

> > +struct lwp_struct {

> > +	u64 lwpcb_addr;

> > +	u32 flags;

> > +	u32 buf_head_offset;

> > +	u64 buf_base;

> > +	u32 buf_size;

> > +	u32 filters;

> > +	u64 saved_event_record[4];

> > +	u32 event_counter[16];

> > +};

> > +

> > +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 +400,9 @@ struct xsave_struct {

> >  	struct i387_fxsave_struct i387;

> >  	struct xsave_hdr_struct xsave_hdr;

> >  	struct ymmh_struct ymmh;

> > +	struct lwp_struct lwp;

> 

> I'm guessing this and the struct lwp_struct above is being added so that you

> can have the LWP XSAVE area size? If so, you don't need it: LWP XSAVE area is

> 128 bytes at offset 832 according to my manuals so I'd guess having a u8

> lwp_area[128] should be fine.

> 

Yes, currently it is only for the LWP XSAVE area size. I will directly use a 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..5cd9de3 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,12 @@

> >  #define XSAVE_YMM_SIZE	    256

> >  #define XSAVE_YMM_OFFSET    (XSAVE_HDR_SIZE +

> XSAVE_HDR_OFFSET)

> >

> > +#define XSTATE_FLEXIBLE (XSTATE_FP | XSTATE_SSE | XSTATE_YMM)

> 

> What's the use of that macro if it is used only once?

> 

Yes. Maybe it is best to use previous pattern.

> > +#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_FLEXIBLE | XSTATE_EAGER)

> >

> >  #ifdef CONFIG_X86_64

> >  #define REX_PREFIX	"0x48, "

> 

Thanks,
Qiaowei
H. Peter Anvin - Dec. 6, 2013, 5:23 p.m.
On 12/06/2013 05:46 AM, Borislav Petkov wrote:
> 
> I'm guessing this and the struct lwp_struct above is being added so that
> you can have the LWP XSAVE area size? If so, you don't need it: LWP
> XSAVE area is 128 bytes at offset 832 according to my manuals so I'd
> guess having a u8 lwp_area[128] should be fine.
> 

Sure, but any reason to *not* document the internal structure?

> 
>> +	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..5cd9de3 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,12 @@
>>  #define XSAVE_YMM_SIZE	    256
>>  #define XSAVE_YMM_OFFSET    (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)
>>  
>> +#define XSTATE_FLEXIBLE (XSTATE_FP | XSTATE_SSE | XSTATE_YMM)
> 
> What's the use of that macro if it is used only once?

Documentation seems good enough.  Explicitly separating out the features
which MUST be eagerly saved seems like a good thing.

>> +#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_FLEXIBLE | XSTATE_EAGER)
>>  

	-hpa
Qiaowei Ren - Dec. 6, 2013, 6:52 p.m.
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 |   23 +++++++++++++++++++++++
 arch/x86/include/asm/xsave.h     |    6 +++++-
 2 files changed, 28 insertions(+), 1 deletions(-)
Borislav Petkov - Dec. 6, 2013, 6:55 p.m.
On Fri, Dec 06, 2013 at 09:23:22AM -0800, H. Peter Anvin wrote:
> On 12/06/2013 05:46 AM, Borislav Petkov wrote:
> > I'm guessing this and the struct lwp_struct above is being added so that
> > you can have the LWP XSAVE area size? If so, you don't need it: LWP
> > XSAVE area is 128 bytes at offset 832 according to my manuals so I'd
> > guess having a u8 lwp_area[128] should be fine.
> > 
> Sure, but any reason to *not* document the internal structure?

Only that you might start getting remove-this-unused-struct patches. :-)

Patch

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 987c75e..43be6f6 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -370,6 +370,26 @@  struct ymmh_struct {
 	u32 ymmh_space[64];
 };
 
+struct lwp_struct {
+	u64 lwpcb_addr;
+	u32 flags;
+	u32 buf_head_offset;
+	u64 buf_base;
+	u32 buf_size;
+	u32 filters;
+	u64 saved_event_record[4];
+	u32 event_counter[16];
+};
+
+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 +400,9 @@  struct xsave_struct {
 	struct i387_fxsave_struct i387;
 	struct xsave_hdr_struct xsave_hdr;
 	struct ymmh_struct ymmh;
+	struct lwp_struct lwp;
+	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..5cd9de3 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,12 @@ 
 #define XSAVE_YMM_SIZE	    256
 #define XSAVE_YMM_OFFSET    (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)
 
+#define XSTATE_FLEXIBLE (XSTATE_FP | XSTATE_SSE | XSTATE_YMM)
+#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_FLEXIBLE | XSTATE_EAGER)
 
 #ifdef CONFIG_X86_64
 #define REX_PREFIX	"0x48, "