Xen devel - Nov 2010 - [Patch 1/3] Refining Xsave/Xrestore support - Version 3

Hi, Keir, Jan,

This patch adds xsave support for PV guests. Comments from Jan are included.

Shan Haitao


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 02.11.10 at 04:47, Haitao Shan <maillists.shan@gmail.com>
wrote:
>@@ -1796,7 +1796,10 @@ static int emulate_privileged_op(struct 
> 
>     /* REX prefix. */
>     if ( rex & 8 ) /* REX.W */
>+    {
>+        opsize_prefix = 0; /* 66H is ignored according to SDM 2A */
This I''m sure isn''t correct: The 0x66 prefix is being ignored
as an
operand size override here, but REX.W has no interaction with
0x66 when the latter is used as an opcode extension selector.

Specifically in the case of xsetbv the manual clearly states

#UD	If CPUID.01H:ECX.XSAVE[bit 26] = 0.
	If CR4.OSXSAVE[bit 18] = 0.
	If the LOCK prefix is used.
	If 66H, F3H or F2H prefix is used.
>@@ -2051,13 +2054,48 @@ static int emulate_privileged_op(struct 
>         goto fail;
>     switch ( opcode )
>     {
>-    case 0x1: /* RDTSCP */
>-        if ( (v->arch.guest_context.ctrlreg[4] & X86_CR4_TSD)
&&
>-             !guest_kernel_mode(v, regs) )
>+    case 0x1: /* RDTSCP and XSETBV */
>+        switch ( insn_fetch(u8, code_base, eip, code_limit) )
>+        {
>+        case 0xf9: /* RDTSCP */
>+            if ( (v->arch.guest_context.ctrlreg[4] & X86_CR4_TSD)
&&
>+                 !guest_kernel_mode(v, regs) )
>+                goto fail;
>+            pv_soft_rdtsc(v, regs, 1);
>+            break;
>+        case 0xd1: /* XSETBV */
>+        {
>+            u64 new_xfeature = (u32)regs->eax | ((u64)regs->edx
<< 32);
>+
>+            if ( lock || rep_prefix || opsize_prefix
>+                 || !(v->arch.guest_context.ctrlreg[4] &
X86_CR4_OSXSAVE) )
>+            {
>+                do_guest_trap(TRAP_invalid_op, regs, 0);
>+                break;
I think you need to "goto skip" or "return
EXCRET_fault_fixed" here,
to avoid executing instruction_done().

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Updated...Thanks for Jan''s careful review, indeed.

Shan Haitao

2010/11/2 Jan Beulich <JBeulich@novell.com>:
>>>> On 02.11.10 at 04:47, Haitao Shan
<maillists.shan@gmail.com> wrote:
>>@@ -1796,7 +1796,10 @@ static int emulate_privileged_op(struct
>>
>>     /* REX prefix. */
>>     if ( rex & 8 ) /* REX.W */
>>+    {
>>+        opsize_prefix = 0; /* 66H is ignored according to SDM 2A */
>
> This I''m sure isn''t correct: The 0x66 prefix is being
ignored as an
> operand size override here, but REX.W has no interaction with
> 0x66 when the latter is used as an opcode extension selector.
>
> Specifically in the case of xsetbv the manual clearly states
>
> #UD     If CPUID.01H:ECX.XSAVE[bit 26] = 0.
>        If CR4.OSXSAVE[bit 18] = 0.
>        If the LOCK prefix is used.
>        If 66H, F3H or F2H prefix is used.
>
>>@@ -2051,13 +2054,48 @@ static int emulate_privileged_op(struct
>>         goto fail;
>>     switch ( opcode )
>>     {
>>-    case 0x1: /* RDTSCP */
>>-        if ( (v->arch.guest_context.ctrlreg[4] & X86_CR4_TSD)
&&
>>-             !guest_kernel_mode(v, regs) )
>>+    case 0x1: /* RDTSCP and XSETBV */
>>+        switch ( insn_fetch(u8, code_base, eip, code_limit) )
>>+        {
>>+        case 0xf9: /* RDTSCP */
>>+            if ( (v->arch.guest_context.ctrlreg[4] &
X86_CR4_TSD) &&
>>+                 !guest_kernel_mode(v, regs) )
>>+                goto fail;
>>+            pv_soft_rdtsc(v, regs, 1);
>>+            break;
>>+        case 0xd1: /* XSETBV */
>>+        {
>>+            u64 new_xfeature = (u32)regs->eax | ((u64)regs->edx
<< 32);
>>+
>>+            if ( lock || rep_prefix || opsize_prefix
>>+                 || !(v->arch.guest_context.ctrlreg[4] &
X86_CR4_OSXSAVE) )
>>+            {
>>+                do_guest_trap(TRAP_invalid_op, regs, 0);
>>+                break;
>
> I think you need to "goto skip" or "return
EXCRET_fault_fixed" here,
> to avoid executing instruction_done().
>
> Jan
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 02.11.10 at 10:14, Haitao Shan <maillists.shan@gmail.com>
wrote:
> Updated...Thanks for Jan''s careful review, indeed.
Looks good to me now (including [the hypervisor parts of] the other
two patches).

Thanks, Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel