Xen devel - Oct 2010 - [Patch 2/4] Refining Xsave/Xrestore support - Version 2

Hi, Keir,

This is patch #2, which adds PV guest Xsave support.

Shan Haitao


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

>>> On 29.10.10 at 03:25, Haitao Shan <maillists.shan@gmail.com>
wrote:
>@@ -376,7 +392,10 @@ int vcpu_initialise(struct vcpu *v)
> 
>     spin_lock_init(&v->arch.shadow_ldt_lock);
> 
>-    return (is_pv_32on64_vcpu(v) ? setup_compat_l4(v) : 0);
>+    if ( (rc = (is_pv_32on64_vcpu(v) ? setup_compat_l4(v) : 0)) != 0 )
>+        xfree(v->arch.xsave_area);
This surely is ugly to read. Why can''t this be simply

    if ( is_pv_32on64_vcpu(v) )
        rc = setup_compat_l4(v);
    if ( rc )
        xfree(v->arch.xsave_area);

with rc zero-initialized at the top of the function.
>...
>+        case 0xd1: /* XSETBV */
>+        {
>+            u64 new_xfeature = regs->eax | ((u64)regs->edx <<
32);
You need (u32)regs->eax here.
>+            if ( !guest_kernel_mode(v, regs)
>+            /* Currently only XCR0 is implemented */
>+                || regs->ecx != XCR_XFEATURE_ENABLED_MASK
>+            /* bit 0 of XCR0 must be set */
>+                || !(new_xfeature & XSTATE_FP)
>+            /* Reserved bit must not be set */
>+                || (new_xfeature & ~xfeature_mask) )
>+                goto fail;
You need to check for the use of invalid prefixes here (as
otherwise we risk mis-emulating future meanings of prefixed
versions of this opcode).

Further, some of the failures here really need to report #UD
(instead of #GP) to the guest.

To minimize future changes I''d also suggest starting with a
switch ( (u32)regs->ecx ) from the beginning (note that in any
case you''ll have to ignore the upper 32 bits).
> #define real_cr4_to_pv_guest_cr4(c) \
>-    ((c) & ~(X86_CR4_PGE | X86_CR4_PSE | X86_CR4_TSD |
X86_CR4_OSXSAVE))
>+    ((c) & ~(X86_CR4_PGE | X86_CR4_PSE | X86_CR4_TSD))
I don''t think this is correct: You don''t want the guest to see
the
actual CR4 value, but its emulated version. And you handle
things accordingly already in pv_guest_cr4_fixup().

Jan


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