[Xen-devel] Re: [Patch 2/4] Refining Xsave/Xrestore support - Version 2

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> On 29.10.10 at 03:25, Haitao Shan <maillists.shan@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel