| Message ID | 1456911982-24357-1-git-send-email-shuai.ruan@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 02.03.16 at 10:46, <shuai.ruan@linux.intel.com> wrote: > Previous patch using all available features calculate xstate_comp_offsets. > This is wrong.This patch fix this bug by calculating the xstate_comp_offset > based on xcomp_bv of current guest. > Also, the xstate_comp_offset should take alignment into consideration. > > V2: Address comments from Jan: > 1. code style fix > 2. setup_xstate_comp take xcomp_bv as param. This belongs after the first --- separator. > @@ -106,26 +107,34 @@ static int setup_xstate_features(bool_t bsp) > xstate_sizes = xzalloc_array(unsigned int, xstate_features); > if ( !xstate_sizes ) > return -ENOMEM; > + > + xstate_align = xzalloc_array(unsigned int, xstate_features); > + if ( !xstate_align ) > + return -ENOMEM; > } > > for ( leaf = 2; leaf < xstate_features; leaf++ ) > { > if ( bsp ) > + { > cpuid_count(XSTATE_CPUID, leaf, &xstate_sizes[leaf], > - &xstate_offsets[leaf], &tmp, &tmp); > + &xstate_offsets[leaf], &ecx, &edx); > + xstate_align[leaf] = ecx & XSTATE_ALIGN64; Am I seeing it right that you're allocating an array of unsigned int just to use a single bit in each element? This should be a bitmap if so. > @@ -134,16 +143,19 @@ static void __init setup_xstate_comp(void) > * in the fixed offsets in the xsave area in either compacted form > * or standard form. > */ > - xstate_comp_offsets[0] = 0; > + memset(xstate_comp_offsets, 0, sizeof(xstate_comp_offsets)); I'm sorry, but sending a new version without addressing _all_ comments on the previous version is a waste of already limited review bandwidth. Using the static xstate_comp_offsets like this is wrong. > xstate_comp_offsets[1] = XSAVE_SSE_OFFSET; > > xstate_comp_offsets[2] = FXSAVE_SIZE + XSAVE_HDR_SIZE; > > for ( i = 3; i < xstate_features; i++ ) > { > - xstate_comp_offsets[i] = xstate_comp_offsets[i - 1] + > - (((1ul << i) & xfeature_mask) > - ? xstate_sizes[i - 1] : 0); > + xstate_comp_offsets[i] = (xstate_align[i] ? > + ROUNDUP(xstate_comp_offsets[i-1], 64) : The coding style issue here persists too. > + xstate_comp_offsets[i - 1]) + > + (((1ul << i) & xcomp_bv) > + ? xstate_sizes[i - 1] : 0); And the - is this actually correct? Why would you enforce alignment for a component not actually present in the save area? I.e. shouldn't the assignment be made conditional upon the bit being set? In which case things might end up being better readable by doing this in two steps - first align if needed, then simply += sizes[i - 1]. > --- a/xen/include/asm-x86/xstate.h > +++ b/xen/include/asm-x86/xstate.h > @@ -46,6 +46,8 @@ > #define XSTATE_LAZY (XSTATE_ALL & ~XSTATE_NONLAZY) > #define XSTATE_COMPACTION_ENABLED (1ULL << 63) > > +#define XSTATE_ALIGN64 (1ULL << 1) Why 1ULL (instead of just 1U)? Jan
On Thu, Mar 03, 2016 at 01:45:03AM -0700, Jan Beulich wrote: > > @@ -134,16 +143,19 @@ static void __init setup_xstate_comp(void) > > */ > > xstate_comp_offsets[1] = XSAVE_SSE_OFFSET; > > > > xstate_comp_offsets[2] = FXSAVE_SIZE + XSAVE_HDR_SIZE; > > > > for ( i = 3; i < xstate_features; i++ ) > > { > > - xstate_comp_offsets[i] = xstate_comp_offsets[i - 1] + > > - (((1ul << i) & xfeature_mask) > > - ? xstate_sizes[i - 1] : 0); > > + xstate_comp_offsets[i] = (xstate_align[i] ? > > + ROUNDUP(xstate_comp_offsets[i-1], 64) : > > The coding style issue here persists too. In the previous comment, I think you mentions this coding style issue is the second line is not align with first line. Obviously, I misunderstand your meaning. Can give me corret coding style when encounter such case? For If I put the ROUNDUP in the first line it will go beyond 80 characters. Also I see CODING_STYLE file there is "split in sensible places".May be I spilt in nonsensible place. Thanks for your time. > Jan >
>>> On 03.03.16 at 10:40, <shuai.ruan@linux.intel.com> wrote: > On Thu, Mar 03, 2016 at 01:45:03AM -0700, Jan Beulich wrote: >> > @@ -134,16 +143,19 @@ static void __init setup_xstate_comp(void) >> > */ >> > xstate_comp_offsets[1] = XSAVE_SSE_OFFSET; >> > >> > xstate_comp_offsets[2] = FXSAVE_SIZE + XSAVE_HDR_SIZE; >> > >> > for ( i = 3; i < xstate_features; i++ ) >> > { >> > - xstate_comp_offsets[i] = xstate_comp_offsets[i - 1] + >> > - (((1ul << i) & xfeature_mask) >> > - ? xstate_sizes[i - 1] : 0); >> > + xstate_comp_offsets[i] = (xstate_align[i] ? >> > + ROUNDUP(xstate_comp_offsets[i-1], 64) : >> >> The coding style issue here persists too. > In the previous comment, I think you mentions this > coding style issue is the second line is not align with first line. > Obviously, I misunderstand your meaning. > Can give me corret coding style when encounter such case? For If I put > the ROUNDUP in the first line it will go beyond 80 characters. > Also I see CODING_STYLE file there is "split in sensible places".May be > I spilt in nonsensible place. No, the splitting isn't the issue (or at least not the one I've noticed). It's the lack of blanks around the - (which all other uses around here have properly in place). With the other comment I gave indentation and/or line splitting would change anyway. Jan
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c index 8316bd9..dd6d37a 100644 --- a/xen/arch/x86/xstate.c +++ b/xen/arch/x86/xstate.c @@ -26,6 +26,7 @@ u64 __read_mostly xfeature_mask; static unsigned int *__read_mostly xstate_offsets; unsigned int *__read_mostly xstate_sizes; +static unsigned int *__read_mostly xstate_align; static unsigned int __read_mostly xstate_features; static unsigned int __read_mostly xstate_comp_offsets[sizeof(xfeature_mask)*8]; @@ -94,7 +95,7 @@ static bool_t xsave_area_compressed(const struct xsave_struct *xsave_area) static int setup_xstate_features(bool_t bsp) { - unsigned int leaf, tmp, eax, ebx; + unsigned int leaf, eax, ebx, ecx, edx; if ( bsp ) { @@ -106,26 +107,34 @@ static int setup_xstate_features(bool_t bsp) xstate_sizes = xzalloc_array(unsigned int, xstate_features); if ( !xstate_sizes ) return -ENOMEM; + + xstate_align = xzalloc_array(unsigned int, xstate_features); + if ( !xstate_align ) + return -ENOMEM; } for ( leaf = 2; leaf < xstate_features; leaf++ ) { if ( bsp ) + { cpuid_count(XSTATE_CPUID, leaf, &xstate_sizes[leaf], - &xstate_offsets[leaf], &tmp, &tmp); + &xstate_offsets[leaf], &ecx, &edx); + xstate_align[leaf] = ecx & XSTATE_ALIGN64; + } else { cpuid_count(XSTATE_CPUID, leaf, &eax, - &ebx, &tmp, &tmp); + &ebx, &ecx, &edx); BUG_ON(eax != xstate_sizes[leaf]); BUG_ON(ebx != xstate_offsets[leaf]); + BUG_ON((ecx & XSTATE_ALIGN64) != xstate_align[leaf]); } } return 0; } -static void __init setup_xstate_comp(void) +static void setup_xstate_comp(const u64 xcomp_bv) { unsigned int i; @@ -134,16 +143,19 @@ static void __init setup_xstate_comp(void) * in the fixed offsets in the xsave area in either compacted form * or standard form. */ - xstate_comp_offsets[0] = 0; + memset(xstate_comp_offsets, 0, sizeof(xstate_comp_offsets)); + xstate_comp_offsets[1] = XSAVE_SSE_OFFSET; xstate_comp_offsets[2] = FXSAVE_SIZE + XSAVE_HDR_SIZE; for ( i = 3; i < xstate_features; i++ ) { - xstate_comp_offsets[i] = xstate_comp_offsets[i - 1] + - (((1ul << i) & xfeature_mask) - ? xstate_sizes[i - 1] : 0); + xstate_comp_offsets[i] = (xstate_align[i] ? + ROUNDUP(xstate_comp_offsets[i-1], 64) : + xstate_comp_offsets[i - 1]) + + (((1ul << i) & xcomp_bv) + ? xstate_sizes[i - 1] : 0); ASSERT(xstate_comp_offsets[i] + xstate_sizes[i] <= xsave_cntxt_size); } } @@ -172,6 +184,8 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size) } ASSERT(xsave_area_compressed(xsave)); + setup_xstate_comp(xsave->xsave_hdr.xcomp_bv); + /* * Copy legacy XSAVE area and XSAVE hdr area. */ @@ -223,6 +237,8 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size) xsave->xsave_hdr.xstate_bv = xstate_bv; xsave->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | XSTATE_COMPACTION_ENABLED; + setup_xstate_comp(xsave->xsave_hdr.xcomp_bv); + /* * Copy each region from the non-compacted offset to the * possibly compacted offset. @@ -575,8 +591,6 @@ void xstate_init(struct cpuinfo_x86 *c) if ( setup_xstate_features(bsp) && bsp ) BUG(); - if ( bsp && (cpu_has_xsaves || cpu_has_xsavec) ) - setup_xstate_comp(); } static bool_t valid_xcr0(u64 xcr0) diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h index c28cea5..61d1ddb 100644 --- a/xen/include/asm-x86/xstate.h +++ b/xen/include/asm-x86/xstate.h @@ -46,6 +46,8 @@ #define XSTATE_LAZY (XSTATE_ALL & ~XSTATE_NONLAZY) #define XSTATE_COMPACTION_ENABLED (1ULL << 63) +#define XSTATE_ALIGN64 (1ULL << 1) + extern u64 xfeature_mask; extern unsigned int *xstate_sizes;
Previous patch using all available features calculate xstate_comp_offsets. This is wrong.This patch fix this bug by calculating the xstate_comp_offset based on xcomp_bv of current guest. Also, the xstate_comp_offset should take alignment into consideration. V2: Address comments from Jan: 1. code style fix 2. setup_xstate_comp take xcomp_bv as param. Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com> Reported-by: Jan Beulich <jbeulich@suse.com> --- xen/arch/x86/xstate.c | 34 ++++++++++++++++++++++++---------- xen/include/asm-x86/xstate.h | 2 ++ 2 files changed, 26 insertions(+), 10 deletions(-)