| Message ID | 1386375658-2191-3-git-send-email-qiaowei.ren@intel.com |
|---|---|
| State | New |
| Headers | show |
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, " >
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
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, "
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
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
> -----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
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
> -----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
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
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, "