| Message ID | 56F1514302000078000DF2CD@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22/03/16 13:05, Jan Beulich wrote: > - don't unintentionally increase features reported by sub-leaf 0 > EDX:EAX > - don't discard the known flags in sub-leaves 2..63 ECX > - handle components 32...62 (EDX) in sub-leaf 1 consistently with > 0...31 (ECX) > - zap sub-leaves beyond 62 > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > While obviously requiring re-basing on either end when taking Andrew's > CPUID levelling series into account, the changes done here appear to > be orthogonal to those done in his series. > > --- a/tools/libxc/xc_cpuid_x86.c > +++ b/tools/libxc/xc_cpuid_x86.c > @@ -281,10 +281,14 @@ static void intel_xc_cpuid_policy(xc_int > } > } > > +/* Leaf 1, EAX: */ > #define XSAVEOPT (1 << 0) > #define XSAVEC (1 << 1) > #define XGETBV1 (1 << 2) > #define XSAVES (1 << 3) Hmm - I should convert these to be X86_FEATURESET_xxx values. > +/* Leaves beyond 1, ECX: */ > +#define XSTATE_XSS (1 << 0) > +#define XSTATE_ALIGN64 (1 << 1) > /* Configure extended state enumeration leaves (0x0000000D for xsave) */ > static void xc_cpuid_config_xsave(xc_interface *xch, > const struct cpuid_domain_info *info, > @@ -300,9 +304,9 @@ static void xc_cpuid_config_xsave(xc_int > { > case 0: > /* EAX: low 32bits of xfeature_enabled_mask */ > - regs[0] = info->xfeature_mask & 0xFFFFFFFF; > + regs[0] &= info->xfeature_mask; > /* EDX: high 32bits of xfeature_enabled_mask */ > - regs[3] = (info->xfeature_mask >> 32) & 0xFFFFFFFF; > + regs[3] &= info->xfeature_mask >> 32; > /* ECX: max size required by all HW features */ > { > unsigned int _input[2] = {0xd, 0x0}, _regs[4]; This is an improvement on the code currently present, but is still superseded by the final patch of my cpuid series. > @@ -325,16 +329,20 @@ static void xc_cpuid_config_xsave(xc_int Between these two hunks, there is a loop bound which is also wrong. > if ( !info->hvm ) > regs[0] &= ~XSAVES; > regs[2] &= info->xfeature_mask; > - regs[3] = 0; > + regs[3] &= info->xfeature_mask >> 32; > break; > - case 2 ... 63: /* sub-leaves */ > + case 2 ... 62: /* per-component sub-leaves */ > if ( !(info->xfeature_mask & (1ULL << input[1])) ) Now I think about it, this check is incomplete. xfeature_mask doesn't contain xss values. For now its fine, but it will cause problems when support for Processor Trace is added. > { > regs[0] = regs[1] = regs[2] = regs[3] = 0; > break; > } > /* Don't touch EAX, EBX. Also cleanup ECX and EDX */ > - regs[2] = regs[3] = 0; > + regs[2] &= XSTATE_XSS | XSTATE_ALIGN64; > + regs[3] = 0; > + break; > + default: > + regs[0] = regs[1] = regs[2] = regs[3] = 0; > break; If you wish, I can fold this patch into the final patch of my cpuid series. ~Andrew
>>> On 22.03.16 at 14:48, <andrew.cooper3@citrix.com> wrote: >> @@ -300,9 +304,9 @@ static void xc_cpuid_config_xsave(xc_int >> { >> case 0: >> /* EAX: low 32bits of xfeature_enabled_mask */ >> - regs[0] = info->xfeature_mask & 0xFFFFFFFF; >> + regs[0] &= info->xfeature_mask; >> /* EDX: high 32bits of xfeature_enabled_mask */ >> - regs[3] = (info->xfeature_mask >> 32) & 0xFFFFFFFF; >> + regs[3] &= info->xfeature_mask >> 32; >> /* ECX: max size required by all HW features */ >> { >> unsigned int _input[2] = {0xd, 0x0}, _regs[4]; > > This is an improvement on the code currently present, but is still > superseded by the final patch of my cpuid series. Is it? I did check your tree before sending, and you do only mechanical adjustments. In particular you don't switch to &= and you don't drop the pointless and-ing with 0xFFFFFFFF. >> @@ -325,16 +329,20 @@ static void xc_cpuid_config_xsave(xc_int > > Between these two hunks, there is a loop bound which is also wrong. But seeing that your patches fix it I didn't bother stealing the fix from your patches. >> if ( !info->hvm ) >> regs[0] &= ~XSAVES; >> regs[2] &= info->xfeature_mask; >> - regs[3] = 0; >> + regs[3] &= info->xfeature_mask >> 32; >> break; >> - case 2 ... 63: /* sub-leaves */ >> + case 2 ... 62: /* per-component sub-leaves */ >> if ( !(info->xfeature_mask & (1ULL << input[1])) ) > > Now I think about it, this check is incomplete. xfeature_mask doesn't > contain xss values. For now the XSS bitmask is blank. Looking at everything together I do think though that once it becomes non-zero, info->xfeature_mask will need to become the OR of both masks. >> { >> regs[0] = regs[1] = regs[2] = regs[3] = 0; >> break; >> } >> /* Don't touch EAX, EBX. Also cleanup ECX and EDX */ >> - regs[2] = regs[3] = 0; >> + regs[2] &= XSTATE_XSS | XSTATE_ALIGN64; >> + regs[3] = 0; >> + break; >> + default: >> + regs[0] = regs[1] = regs[2] = regs[3] = 0; >> break; > > If you wish, I can fold this patch into the final patch of my cpuid series. I'd be fine with that, albeit (as said in the submission) the changes are independent of one another despite them causing conflicts. Jan
On 22/03/16 14:46, Jan Beulich wrote: >>>> On 22.03.16 at 14:48, <andrew.cooper3@citrix.com> wrote: >>> @@ -300,9 +304,9 @@ static void xc_cpuid_config_xsave(xc_int >>> { >>> case 0: >>> /* EAX: low 32bits of xfeature_enabled_mask */ >>> - regs[0] = info->xfeature_mask & 0xFFFFFFFF; >>> + regs[0] &= info->xfeature_mask; >>> /* EDX: high 32bits of xfeature_enabled_mask */ >>> - regs[3] = (info->xfeature_mask >> 32) & 0xFFFFFFFF; >>> + regs[3] &= info->xfeature_mask >> 32; >>> /* ECX: max size required by all HW features */ >>> { >>> unsigned int _input[2] = {0xd, 0x0}, _regs[4]; >> This is an improvement on the code currently present, but is still >> superseded by the final patch of my cpuid series. > Is it? I did check your tree before sending, and you do only > mechanical adjustments. In particular you don't switch to > &= and you don't drop the pointless and-ing with 0xFFFFFFFF. Using &= is specifically wrong and buggy. My patch replaces info->xfeature_mask with guest_xfeature_mask, which itself is calculated from the guest feature availability. The value in regs[] is dom0's view of the cpuid leaves, and are inappropriate to be combined to make the guests view. Observe that I have specifically been replacing masks with assignments. Consider the (admittedly contrived scenario of) dom0 being denied access to xsave, while domU is intended to have access. A less contrived scenario is a 32bit dom0 trying to construct a 64bit PV guest. It only worked previously because dom0 used native cpuid which bypassed Xen hiding the LM bit. > >>> @@ -325,16 +329,20 @@ static void xc_cpuid_config_xsave(xc_int >> Between these two hunks, there is a loop bound which is also wrong. > But seeing that your patches fix it I didn't bother stealing the fix > from your patches. > >>> if ( !info->hvm ) >>> regs[0] &= ~XSAVES; >>> regs[2] &= info->xfeature_mask; >>> - regs[3] = 0; >>> + regs[3] &= info->xfeature_mask >> 32; >>> break; >>> - case 2 ... 63: /* sub-leaves */ >>> + case 2 ... 62: /* per-component sub-leaves */ >>> if ( !(info->xfeature_mask & (1ULL << input[1])) ) >> Now I think about it, this check is incomplete. xfeature_mask doesn't >> contain xss values. > For now the XSS bitmask is blank. Looking at everything together I > do think though that once it becomes non-zero, info->xfeature_mask > will need to become the OR of both masks. > >>> { >>> regs[0] = regs[1] = regs[2] = regs[3] = 0; >>> break; >>> } >>> /* Don't touch EAX, EBX. Also cleanup ECX and EDX */ >>> - regs[2] = regs[3] = 0; >>> + regs[2] &= XSTATE_XSS | XSTATE_ALIGN64; >>> + regs[3] = 0; >>> + break; >>> + default: >>> + regs[0] = regs[1] = regs[2] = regs[3] = 0; >>> break; >> If you wish, I can fold this patch into the final patch of my cpuid series. > I'd be fine with that, albeit (as said in the submission) the changes > are independent of one another despite them causing conflicts. It would be clearer than having two different patches both fixing part of the code. ~Andrew
>>> On 22.03.16 at 18:23, <andrew.cooper3@citrix.com> wrote: > On 22/03/16 14:46, Jan Beulich wrote: >>>>> On 22.03.16 at 14:48, <andrew.cooper3@citrix.com> wrote: >>>> @@ -300,9 +304,9 @@ static void xc_cpuid_config_xsave(xc_int >>>> { >>>> case 0: >>>> /* EAX: low 32bits of xfeature_enabled_mask */ >>>> - regs[0] = info->xfeature_mask & 0xFFFFFFFF; >>>> + regs[0] &= info->xfeature_mask; >>>> /* EDX: high 32bits of xfeature_enabled_mask */ >>>> - regs[3] = (info->xfeature_mask >> 32) & 0xFFFFFFFF; >>>> + regs[3] &= info->xfeature_mask >> 32; >>>> /* ECX: max size required by all HW features */ >>>> { >>>> unsigned int _input[2] = {0xd, 0x0}, _regs[4]; >>> This is an improvement on the code currently present, but is still >>> superseded by the final patch of my cpuid series. >> Is it? I did check your tree before sending, and you do only >> mechanical adjustments. In particular you don't switch to >> &= and you don't drop the pointless and-ing with 0xFFFFFFFF. > > Using &= is specifically wrong and buggy. My patch replaces > info->xfeature_mask with guest_xfeature_mask, which itself is calculated > from the guest feature availability. Oh, right - if you imply buggy just if it was done with or after your change. >>>> { >>>> regs[0] = regs[1] = regs[2] = regs[3] = 0; >>>> break; >>>> } >>>> /* Don't touch EAX, EBX. Also cleanup ECX and EDX */ >>>> - regs[2] = regs[3] = 0; >>>> + regs[2] &= XSTATE_XSS | XSTATE_ALIGN64; >>>> + regs[3] = 0; >>>> + break; >>>> + default: >>>> + regs[0] = regs[1] = regs[2] = regs[3] = 0; >>>> break; >>> If you wish, I can fold this patch into the final patch of my cpuid series. >> I'd be fine with that, albeit (as said in the submission) the changes >> are independent of one another despite them causing conflicts. > > It would be clearer than having two different patches both fixing part > of the code. Especially with the above in mind - go ahead. Jan
--- a/tools/libxc/xc_cpuid_x86.c +++ b/tools/libxc/xc_cpuid_x86.c @@ -281,10 +281,14 @@ static void intel_xc_cpuid_policy(xc_int } } +/* Leaf 1, EAX: */ #define XSAVEOPT (1 << 0) #define XSAVEC (1 << 1) #define XGETBV1 (1 << 2) #define XSAVES (1 << 3) +/* Leaves beyond 1, ECX: */ +#define XSTATE_XSS (1 << 0) +#define XSTATE_ALIGN64 (1 << 1) /* Configure extended state enumeration leaves (0x0000000D for xsave) */ static void xc_cpuid_config_xsave(xc_interface *xch, const struct cpuid_domain_info *info, @@ -300,9 +304,9 @@ static void xc_cpuid_config_xsave(xc_int { case 0: /* EAX: low 32bits of xfeature_enabled_mask */ - regs[0] = info->xfeature_mask & 0xFFFFFFFF; + regs[0] &= info->xfeature_mask; /* EDX: high 32bits of xfeature_enabled_mask */ - regs[3] = (info->xfeature_mask >> 32) & 0xFFFFFFFF; + regs[3] &= info->xfeature_mask >> 32; /* ECX: max size required by all HW features */ { unsigned int _input[2] = {0xd, 0x0}, _regs[4]; @@ -325,16 +329,20 @@ static void xc_cpuid_config_xsave(xc_int if ( !info->hvm ) regs[0] &= ~XSAVES; regs[2] &= info->xfeature_mask; - regs[3] = 0; + regs[3] &= info->xfeature_mask >> 32; break; - case 2 ... 63: /* sub-leaves */ + case 2 ... 62: /* per-component sub-leaves */ if ( !(info->xfeature_mask & (1ULL << input[1])) ) { regs[0] = regs[1] = regs[2] = regs[3] = 0; break; } /* Don't touch EAX, EBX. Also cleanup ECX and EDX */ - regs[2] = regs[3] = 0; + regs[2] &= XSTATE_XSS | XSTATE_ALIGN64; + regs[3] = 0; + break; + default: + regs[0] = regs[1] = regs[2] = regs[3] = 0; break; } }