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

2013-12-06 Thread Borislav Petkov 
On Sat, Dec 07, 2013 at 02:52:56AM +0800, Qiaowei Ren wrote:

Commit message please.

 Signed-off-by: Qiaowei Ren qiaowei@intel.com
 Signed-off-by: Xudong Hao xudong@intel.com
 Signed-off-by: Liu Jinsong jinsong@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.


 + 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_FP0x1
  #define XSTATE_SSE   0x2
  #define XSTATE_YMM   0x4
 +#define XSTATE_BNDREGS   0x8
 +#define XSTATE_BNDCSR0x10
  
  #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?

 +#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, 

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-12-06 Thread Ren, Qiaowei 

 -Original Message-
 From: Borislav Petkov [mailto:b...@alien8.de]
 Sent: Friday, December 06, 2013 9:47 PM
 To: Ren, Qiaowei
 Cc: Paolo Bonzini; H. Peter Anvin; Ingo Molnar; Thomas Gleixner;
 x...@kernel.org; linux-ker...@vger.kernel.org; qemu-de...@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@intel.com
  Signed-off-by: Xudong Hao xudong@intel.com
  Signed-off-by: Liu Jinsong jinsong@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
N�r��yb�X��ǧv�^�)޺{.n�+h����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

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

2013-12-06 Thread H. Peter Anvin 
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


--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-12-06 Thread Borislav Petkov 
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. :-)

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html