[Qemu-devel] [PATCH 1/3] x86, mpx: add documentation on Intel MPX

[Qemu-devel] [PATCH 1/3] x86, mpx: add documentation on Intel MPX

[Qiaowei Ren]

This patch adds the Documentation/intel_mpx.txt file with some
information about 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>
---
 Documentation/intel_mpx.txt |   77 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 77 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/intel_mpx.txt

diff --git a/Documentation/intel_mpx.txt b/Documentation/intel_mpx.txt
new file mode 100644
index 0000000..3d947d0
--- /dev/null
+++ b/Documentation/intel_mpx.txt
@@ -0,0 +1,77 @@
+Intel(R) MPX Overview:
+=====================
+
+Intel(R) Memory Protection Extensions (Intel(R) MPX) is a new
+capability introduced into Intel Architecture. Intel MPX can
+increase the robustness of software when it is used in conjunction
+with compiler changes to check memory references, for those
+references whose compile-time normal intentions are usurped
+at runtime due to buffer overflow or underflow.
+
+Two of the most important goals of Intel MPX are to provide
+this capability at very low performance overhead for newly
+compiled code, and to provide compatibility mechanisms with
+legacy software components. A direct benefit Intel MPX provides
+is hardening software against malicious attacks designed to
+cause or exploit buffer overruns.
+
+For details about the Intel MPX instructions, see "Intel(R)
+Architecture Instruction Set Extensions Programming Reference".
+
+Intel(R) MPX Programming Model
+------------------------------
+
+Intel MPX introduces new registers and new instructions that
+operate on these registers. Some of the registers added are
+bounds registers which store a pointer's lower bound and upper
+bound limits. Whenever the pointer is used, the requested
+reference is checked against the pointer's associated bounds,
+thereby preventing out-of-bound memory access (such as buffer
+overflows and overruns). Out-of-bounds memory references
+initiate a #BR exception which can then be handled in an
+appropriate manner.
+
+Loading and Storing Bounds using Translation
+--------------------------------------------
+
+Intel MPX defines two instructions for load/store of the linear
+address of a pointer to a buffer, along with the bounds of the
+buffer into a paging structure of extended bounds. Specifically
+when storing extended bounds, the processor will perform address
+translation of the address where the pointer is stored to an
+address in the Bound Table (BT) to determine the store location
+of extended bounds. Loading of an extended bounds performs the
+reverse sequence.
+
+The structure in memory to load/store an extended bound is a
+4-tuple consisting of lower bound, upper bound, pointer value
+and a reserved field. Bound loads and stores access 32-bit or
+64-bit operand size according to the operation mode. Thus,
+a bound table entry is 4*32 bits in 32-bit mode and 4*64 bits
+in 64-bit mode.
+
+The linear address of a bound table is stored in a Bound
+Directory (BD) entry. And the linear address of the bound
+directory is derived from either BNDCFGU or BNDCFGS registers.
+Bounds in memory are stored in Bound Tables (BT) as an extended
+bound, which are accessed via Bound Directory (BD) and address
+translation performed by BNDLDX/BNDSTX instructions.
+
+Bounds Directory (BD) and Bounds Tables (BT) are stored in
+application memory and are allocated by the application (in case
+of kernel use, the structures will be in kernel memory). The
+bound directory and each instance of bound table are in contiguous
+linear memory.
+
+XSAVE/XRESTOR Support of Intel MPX State
+----------------------------------------
+
+Enabling Intel MPX requires an OS to manage two bits in XCR0:
+  - BNDREGS for saving and restoring registers BND0-BND3,
+  - BNDCSR for saving and restoring the user-mode configuration
+(BNDCFGU) and the status register (BNDSTATUS).
+
+The reason for having two separate bits is that BND0-BND3 is
+likely to be volatile state, while BNDCFGU and BNDSTATUS are not.
+Therefore, an OS has flexibility in handling these two states
+differently in saving or restoring them.
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/3] x86, mpx: add documentation on Intel MPX

[Borislav Petkov]

On Sat, Dec 07, 2013 at 02:52:54AM +0800, Qiaowei Ren wrote:
> This patch adds the Documentation/intel_mpx.txt file with some
> information about 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>
> ---
>  Documentation/intel_mpx.txt |   77 +++++++++++++++++++++++++++++++++++++++++++

Documentation/x86/ is probably a more fitting place for this.

>  1 files changed, 77 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/intel_mpx.txt
> 
> diff --git a/Documentation/intel_mpx.txt b/Documentation/intel_mpx.txt
> new file mode 100644
> index 0000000..3d947d0
> --- /dev/null
> +++ b/Documentation/intel_mpx.txt
> @@ -0,0 +1,77 @@
> +Intel(R) MPX Overview:
> +=====================
> +
> +Intel(R) Memory Protection Extensions (Intel(R) MPX) is a new
> +capability introduced into Intel Architecture. Intel MPX can
> +increase the robustness of software when it is used in conjunction
> +with compiler changes to check memory references, for those
> +references whose compile-time normal intentions are usurped

That's a strange formulation, what does it actually mean? The intentions
of references??

> +at runtime due to buffer overflow or underflow.
> +
> +Two of the most important goals of Intel MPX are to provide
> +this capability at very low performance overhead for newly
> +compiled code, and to provide compatibility mechanisms with
> +legacy software components. A direct benefit Intel MPX provides
> +is hardening software against malicious attacks designed to
> +cause or exploit buffer overruns.
> +
> +For details about the Intel MPX instructions, see "Intel(R)
> +Architecture Instruction Set Extensions Programming Reference".
> +
> +Intel(R) MPX Programming Model
> +------------------------------
> +
> +Intel MPX introduces new registers and new instructions that
> +operate on these registers. Some of the registers added are
> +bounds registers which store a pointer's lower bound and upper
> +bound limits. Whenever the pointer is used, the requested
> +reference is checked against the pointer's associated bounds,
> +thereby preventing out-of-bound memory access (such as buffer
> +overflows and overruns). Out-of-bounds memory references
> +initiate a #BR exception which can then be handled in an
> +appropriate manner.
> +
> +Loading and Storing Bounds using Translation
> +--------------------------------------------
> +
> +Intel MPX defines two instructions for load/store of the linear
> +address of a pointer to a buffer, along with the bounds of the
> +buffer into a paging structure of extended bounds. Specifically
> +when storing extended bounds, the processor will perform address
> +translation of the address where the pointer is stored to an
> +address in the Bound Table (BT) to determine the store location
> +of extended bounds. Loading of an extended bounds performs the

s/an//

> +reverse sequence.
> +

...

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [Qemu-devel] [PATCH 1/3] x86, mpx: add documentation on Intel MPX

[Ren, Qiaowei]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Friday, December 06, 2013 9:27 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 1/3] x86, mpx: add documentation on Intel MPX
> 
> On Sat, Dec 07, 2013 at 02:52:54AM +0800, Qiaowei Ren wrote:
> > This patch adds the Documentation/intel_mpx.txt file with some
> > information about 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>
> > ---
> >  Documentation/intel_mpx.txt |   77
> +++++++++++++++++++++++++++++++++++++++++++
> 
> Documentation/x86/ is probably a more fitting place for this.
> 
Yes. I will move it to Documentation/x86.

> >  1 files changed, 77 insertions(+), 0 deletions(-)  create mode 100644
> > Documentation/intel_mpx.txt
> >
> > diff --git a/Documentation/intel_mpx.txt b/Documentation/intel_mpx.txt
> > new file mode 100644 index 0000000..3d947d0
> > --- /dev/null
> > +++ b/Documentation/intel_mpx.txt
> > @@ -0,0 +1,77 @@
> > +Intel(R) MPX Overview:
> > +=====================
> > +
> > +Intel(R) Memory Protection Extensions (Intel(R) MPX) is a new
> > +capability introduced into Intel Architecture. Intel MPX can increase
> > +the robustness of software when it is used in conjunction with
> > +compiler changes to check memory references, for those references
> > +whose compile-time normal intentions are usurped
> 
> That's a strange formulation, what does it actually mean? The intentions of
> references??
> 
It is from public introduction and specification, you can refer to http://software.intel.com/en-us/articles/introduction-to-intel-memory-protection-extensions 

Thanks,
Qiaowei

Re: [Qemu-devel] [PATCH 1/3] x86, mpx: add documentation on Intel MPX

[Borislav Petkov]

On Fri, Dec 06, 2013 at 03:55:10PM +0000, Ren, Qiaowei wrote:
> It is from public introduction and specification, you can refer to
http://software.intel.com/en-us/articles/introduction-to-intel-memory-protection-extensions

Yep, saw it there too. Which doesn't make it any less strange :)

Btw, if you're going to quote the public documentation, why even add the
text file here? You can simply add the link above as a comment to the
code or as a oneliner somewhere in Documentation/x86/.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [Qemu-devel] [PATCH 1/3] x86, mpx: add documentation on Intel MPX

[Ren, Qiaowei]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Saturday, December 07, 2013 12:06 AM
> 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 1/3] x86, mpx: add documentation on Intel MPX
> 
> On Fri, Dec 06, 2013 at 03:55:10PM +0000, Ren, Qiaowei wrote:
> > It is from public introduction and specification, you can refer to
> http://software.intel.com/en-us/articles/introduction-to-intel-memory-protecti
> on-extensions
> 
> Yep, saw it there too. Which doesn't make it any less strange :)
> 
> Btw, if you're going to quote the public documentation, why even add the text
> file here? You can simply add the link above as a comment to the code or as a
> oneliner somewhere in Documentation/x86/.
> 
It is just partly from that link, I will modify it to be any less strange. :)

Thanks,
Qiaowei

[Qemu-devel] [PATCH 2/3] X86, mpx: Intel MPX definition

[Qiaowei Ren]

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/cpufeature.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index d3f5c63..6c2738d 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -216,6 +216,7 @@
 #define X86_FEATURE_ERMS	(9*32+ 9) /* Enhanced REP MOVSB/STOSB */
 #define X86_FEATURE_INVPCID	(9*32+10) /* Invalidate Processor Context ID */
 #define X86_FEATURE_RTM		(9*32+11) /* Restricted Transactional Memory */
+#define X86_FEATURE_MPX		(9*32+14) /* Memory Protection Extension */
 #define X86_FEATURE_RDSEED	(9*32+18) /* The RDSEED instruction */
 #define X86_FEATURE_ADX		(9*32+19) /* The ADCX and ADOX instructions */
 #define X86_FEATURE_SMAP	(9*32+20) /* Supervisor Mode Access Prevention */
@@ -330,6 +331,7 @@ extern const char * const x86_power_flags[32];
 #define cpu_has_perfctr_l2	boot_cpu_has(X86_FEATURE_PERFCTR_L2)
 #define cpu_has_cx8		boot_cpu_has(X86_FEATURE_CX8)
 #define cpu_has_cx16		boot_cpu_has(X86_FEATURE_CX16)
+#define cpu_has_mpx		boot_cpu_has(X86_FEATURE_MPX)
 #define cpu_has_eager_fpu	boot_cpu_has(X86_FEATURE_EAGER_FPU)
 #define cpu_has_topoext		boot_cpu_has(X86_FEATURE_TOPOEXT)
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/3] X86, mpx: Intel MPX definition

[Borislav Petkov]

On Sat, Dec 07, 2013 at 02:52:55AM +0800, Qiaowei Ren wrote:
> 
> 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/cpufeature.h |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

This patch should probably be merged with the next one...

> 
> diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> index d3f5c63..6c2738d 100644
> --- a/arch/x86/include/asm/cpufeature.h
> +++ b/arch/x86/include/asm/cpufeature.h
> @@ -216,6 +216,7 @@
>  #define X86_FEATURE_ERMS	(9*32+ 9) /* Enhanced REP MOVSB/STOSB */
>  #define X86_FEATURE_INVPCID	(9*32+10) /* Invalidate Processor Context ID */
>  #define X86_FEATURE_RTM		(9*32+11) /* Restricted Transactional Memory */
> +#define X86_FEATURE_MPX		(9*32+14) /* Memory Protection Extension */
>  #define X86_FEATURE_RDSEED	(9*32+18) /* The RDSEED instruction */
>  #define X86_FEATURE_ADX		(9*32+19) /* The ADCX and ADOX instructions */
>  #define X86_FEATURE_SMAP	(9*32+20) /* Supervisor Mode Access Prevention */
> @@ -330,6 +331,7 @@ extern const char * const x86_power_flags[32];
>  #define cpu_has_perfctr_l2	boot_cpu_has(X86_FEATURE_PERFCTR_L2)
>  #define cpu_has_cx8		boot_cpu_has(X86_FEATURE_CX8)
>  #define cpu_has_cx16		boot_cpu_has(X86_FEATURE_CX16)
> +#define cpu_has_mpx		boot_cpu_has(X86_FEATURE_MPX)

... and we're trying to not have more of those macros so people should be simply
using boot_cpu_has(X86_FEATURE_YYY).

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [Qemu-devel] [PATCH 2/3] X86, mpx: Intel MPX definition

[H. Peter Anvin]

No... we always ask for cpufeature.h patches separately because they sometimes cause conflicts between branches.

Borislav Petkov <bp@alien8.de> wrote:
>On Sat, Dec 07, 2013 at 02:52:55AM +0800, Qiaowei Ren wrote:
>> 
>> 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/cpufeature.h |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>
>This patch should probably be merged with the next one...
>
>> 
>> diff --git a/arch/x86/include/asm/cpufeature.h
>b/arch/x86/include/asm/cpufeature.h
>> index d3f5c63..6c2738d 100644
>> --- a/arch/x86/include/asm/cpufeature.h
>> +++ b/arch/x86/include/asm/cpufeature.h
>> @@ -216,6 +216,7 @@
>>  #define X86_FEATURE_ERMS	(9*32+ 9) /* Enhanced REP MOVSB/STOSB */
>>  #define X86_FEATURE_INVPCID	(9*32+10) /* Invalidate Processor
>Context ID */
>>  #define X86_FEATURE_RTM		(9*32+11) /* Restricted Transactional
>Memory */
>> +#define X86_FEATURE_MPX		(9*32+14) /* Memory Protection Extension */
>>  #define X86_FEATURE_RDSEED	(9*32+18) /* The RDSEED instruction */
>>  #define X86_FEATURE_ADX		(9*32+19) /* The ADCX and ADOX instructions
>*/
>>  #define X86_FEATURE_SMAP	(9*32+20) /* Supervisor Mode Access
>Prevention */
>> @@ -330,6 +331,7 @@ extern const char * const x86_power_flags[32];
>>  #define cpu_has_perfctr_l2	boot_cpu_has(X86_FEATURE_PERFCTR_L2)
>>  #define cpu_has_cx8		boot_cpu_has(X86_FEATURE_CX8)
>>  #define cpu_has_cx16		boot_cpu_has(X86_FEATURE_CX16)
>> +#define cpu_has_mpx		boot_cpu_has(X86_FEATURE_MPX)
>
>... and we're trying to not have more of those macros so people should
>be simply
>using boot_cpu_has(X86_FEATURE_YYY).

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

[Qemu-devel] [PATCH 3/3] X86, mpx: Intel MPX xstate feature definition

[Qiaowei Ren]

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;
+	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, "
-- 
1.7.1

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

[Borislav Petkov]

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.


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

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

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

[Ren, Qiaowei]

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

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

[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

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

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