Re: xl/xm save -c fails - set_vcpucontext EOPNOTSUPP (was Re: xl save -c issues with Windows 7 Ultimate)

["Jan Beulich" <JBeulich@novell.com>]

>>> On 11.05.11 at 21:50, Shriram Rajagopalan <rshriram@cs.ubc.ca> wrote:
> Looking at arch_get_info_guest in domctl.c , I see that cr3 is first copied
> verbatim from the vcpu and
> then modified in the if-else block
> if ( !is_pv_32on64_domain(v->domain) )
>         {
>             c.nat->ctrlreg[3] = xen_pfn_to_cr3(
>                 pagetable_get_pfn(v->arch.guest_table));
> #ifdef __x86_64__
>             c.nat->ctrlreg[1] =
>                 pagetable_is_null(v->arch.guest_table_user) ? 0
>                 :
> xen_pfn_to_cr3(pagetable_get_pfn(v->arch.guest_table_user));
> #endif
> ....
>    } else {
>             l4_pgentry_t *l4e =
> __va(pagetable_get_paddr(v->arch.guest_table));
>             c.cmp->ctrlreg[3] = compat_pfn_to_cr3(l4e_get_pfn(*l4e));
> }
> 
> This seems to account for the difference in the values that libxc supplies
> (obtained from get context)
> and the one validated against by arch_set_info_guest
>  arch_set_context validates cr3 and cr1 against the wrong values (the
> vcpu.cr[1/3]) while it should
>  be validated against the value that results from the operation done in the
> if-else loop in arch_get_info_guest

The problem really is that ->ctrlreg[3] (and also ->ctrlreg[1]) aren't
really being kept up-to-date while the domain is running (they get
written only from arch_set_info_guest()), and as such we could
(should?) actually delete these fields, to not confuse people (like
me in this case).

Here's another shot at it:

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -856,6 +856,15 @@ int arch_set_info_guest(
         goto out;
     }
 
+    init_int80_direct_trap(v);
+
+    /* IOPL privileges are virtualised. */
+    v->arch.pv_vcpu.iopl = (v->arch.user_regs.eflags >> 12) & 3;
+    v->arch.user_regs.eflags &= ~X86_EFLAGS_IOPL;
+
+    /* Ensure real hardware interrupts are enabled. */
+    v->arch.user_regs.eflags |= X86_EFLAGS_IF;
+
     if ( !v->is_initialised )
     {
         v->arch.pv_vcpu.ldt_base = c(ldt_base);
@@ -863,10 +872,25 @@ int arch_set_info_guest(
     }
     else
     {
-        bool_t fail = v->arch.pv_vcpu.ctrlreg[3] != c(ctrlreg[3]);
+        unsigned long pfn = pagetable_get_pfn(v->arch.guest_table);
+        bool_t fail;
 
+        if ( !compat )
+        {
+            fail = xen_pfn_to_cr3(pfn) != c.nat->ctrlreg[3];
 #ifdef CONFIG_X86_64
-        fail |= v->arch.pv_vcpu.ctrlreg[1] != c(ctrlreg[1]);
+            if ( pagetable_is_null(v->arch.guest_table_user) )
+                fail |= c.nat->ctrlreg[1] || !(flags & VGCF_in_kernel);
+            else
+            {
+                pfn = pagetable_get_pfn(v->arch.guest_table_user);
+                fail |= xen_pfn_to_cr3(pfn) != c.nat->ctrlreg[1];
+            }
+#endif
+        }
+#ifdef CONFIG_COMPAT
+        else
+            fail = compat_pfn_to_cr3(pfn) != c.cmp->ctrlreg[3];
 #endif
 
         for ( i = 0; i < ARRAY_SIZE(v->arch.pv_vcpu.gdt_frames); ++i )
@@ -907,15 +931,6 @@ int arch_set_info_guest(
     v->arch.pv_vcpu.ctrlreg[0] &= X86_CR0_TS;
     v->arch.pv_vcpu.ctrlreg[0] |= read_cr0() & ~X86_CR0_TS;
 
-    init_int80_direct_trap(v);
-
-    /* IOPL privileges are virtualised. */
-    v->arch.pv_vcpu.iopl = (v->arch.user_regs.eflags >> 12) & 3;
-    v->arch.user_regs.eflags &= ~X86_EFLAGS_IOPL;
-
-    /* Ensure real hardware interrupts are enabled. */
-    v->arch.user_regs.eflags |= X86_EFLAGS_IF;
-
     cr4 = v->arch.pv_vcpu.ctrlreg[4];
     v->arch.pv_vcpu.ctrlreg[4] = cr4 ? pv_guest_cr4_fixup(v, cr4) :
         real_cr4_to_pv_guest_cr4(mmu_cr4_features);

Jan