* [PATCH] libxc/x86: XSAVE related adjustments
@ 2016-03-22 13:05 Jan Beulich
2016-03-22 13:48 ` Andrew Cooper
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2016-03-22 13:05 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Ian Jackson, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 2435 bytes --]
- 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)
+/* 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;
}
}
[-- Attachment #2: libxc-x86-cpuid-xstate.patch --]
[-- Type: text/plain, Size: 2469 bytes --]
libxc/x86: XSAVE related adjustments
- 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)
+/* 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;
}
}
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] libxc/x86: XSAVE related adjustments
2016-03-22 13:05 [PATCH] libxc/x86: XSAVE related adjustments Jan Beulich
@ 2016-03-22 13:48 ` Andrew Cooper
2016-03-22 14:46 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2016-03-22 13:48 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Ian Jackson, Wei Liu, Stefano Stabellini
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] libxc/x86: XSAVE related adjustments
2016-03-22 13:48 ` Andrew Cooper
@ 2016-03-22 14:46 ` Jan Beulich
2016-03-22 17:23 ` Andrew Cooper
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2016-03-22 14:46 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Ian Jackson, Wei Liu, xen-devel, Stefano Stabellini
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] libxc/x86: XSAVE related adjustments
2016-03-22 14:46 ` Jan Beulich
@ 2016-03-22 17:23 ` Andrew Cooper
2016-03-23 7:15 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2016-03-22 17:23 UTC (permalink / raw)
To: Jan Beulich; +Cc: Ian Jackson, Wei Liu, xen-devel, Stefano Stabellini
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] libxc/x86: XSAVE related adjustments
2016-03-22 17:23 ` Andrew Cooper
@ 2016-03-23 7:15 ` Jan Beulich
0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2016-03-23 7:15 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Ian Jackson, Wei Liu, xen-devel, Stefano Stabellini
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-03-23 7:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-22 13:05 [PATCH] libxc/x86: XSAVE related adjustments Jan Beulich
2016-03-22 13:48 ` Andrew Cooper
2016-03-22 14:46 ` Jan Beulich
2016-03-22 17:23 ` Andrew Cooper
2016-03-23 7:15 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).