hypercall_xlat_continuation()

hypercall_xlat_continuation()

[Mukesh Rathor]

Can someone please explain the madness in the else part of this function? The 
caller magically passes 2 for mask? Is this already documented anywhere by 
chance for mortals like me :).

thanks,
Mukesh

Re: hypercall_xlat_continuation()

[Keir Fraser]

Jan wrote it and may still understand it. He's your best hope. If as a
result you think you can make it clearer, please send a patch. :-)

 -- Keir

On 22/05/2009 03:57, "Mukesh Rathor" <mukesh.rathor@oracle.com> wrote:

> 
> Can someone please explain the madness in the else part of this function? The
> caller magically passes 2 for mask? Is this already documented anywhere by
> chance for mortals like me :).
> 
> thanks,
> Mukesh
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: hypercall_xlat_continuation()

[Mukesh Rathor]

Ok. Even if I can't make it clearer, at least I'll add few lines of comments 
explaining what's going on, after (and if) I figure it out.

Jan,

It seems assumption is made that a 64bit dom0 will not have a 32bit app making 
hypercall?

BUG_ON(*reg != (unsigned int)*reg);  <====

can you please explain the rationale? Also, can you please comment on other 
checks, dwiddling with mask, setting *id to *reg value, etc. in this function?


thanks,
Mukesh (starting a new campaign against overuse of ## macros in xen)




Keir Fraser wrote:
> Jan wrote it and may still understand it. He's your best hope. If as a
> result you think you can make it clearer, please send a patch. :-)
> 
>  -- Keir
> 
> On 22/05/2009 03:57, "Mukesh Rathor" <mukesh.rathor@oracle.com> wrote:
> 
>> Can someone please explain the madness in the else part of this function? The
>> caller magically passes 2 for mask? Is this already documented anywhere by
>> chance for mortals like me :).
>>
>> thanks,
>> Mukesh
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
> 

Re: hypercall_xlat_continuation()

[Keir Fraser]

On 22/05/2009 22:58, "Mukesh Rathor" <mukesh.rathor@oracle.com> wrote:

> Ok. Even if I can't make it clearer, at least I'll add few lines of comments
> explaining what's going on, after (and if) I figure it out.
> 
> Jan,
> 
> It seems assumption is made that a 64bit dom0 will not have a 32bit app making
> hypercall?
> 
> BUG_ON(*reg != (unsigned int)*reg);  <====

You know that all the 'xlat' stuff in Xen is for 32-bit guests running on
64-bit hypervisor, right? 64-bit dom0 would never execute this logic.

 -- Keir

Re: hypercall_xlat_continuation()

[Ian Campbell]

On Sat, 2009-05-23 at 06:17 -0400, Keir Fraser wrote:
> On 22/05/2009 22:58, "Mukesh Rathor" <mukesh.rathor@oracle.com> wrote:
> 
> > Ok. Even if I can't make it clearer, at least I'll add few lines of comments
> > explaining what's going on, after (and if) I figure it out.
> > 
> > Jan,
> > 
> > It seems assumption is made that a 64bit dom0 will not have a 32bit app making
> > hypercall?
> > 
> > BUG_ON(*reg != (unsigned int)*reg);  <====
> 
> You know that all the 'xlat' stuff in Xen is for 32-bit guests running on
> 64-bit hypervisor, right? 64-bit dom0 would never execute this logic.

It's worth noting though that I don't believe a 32 bit dom0 toolstack on
a 64 bit kernel on a 64 bit hypervisor will work. In particular the
privcmd "make a hypercall" ioctl doesn't do any compat translation so 32
bit xend and friends can't make hypercalls that way and I think the
MMAPBATCH privcmd doesn't work either.

I'm sure there are other cases too (blktap user<->kernel ring layout
maybe?).

Ian.

Re: hypercall_xlat_continuation()

[Mukesh Rathor]

Ian Campbell wrote:
> On Sat, 2009-05-23 at 06:17 -0400, Keir Fraser wrote:
>> On 22/05/2009 22:58, "Mukesh Rathor" <mukesh.rathor@oracle.com> wrote:
>>
>>> Ok. Even if I can't make it clearer, at least I'll add few lines of comments
>>> explaining what's going on, after (and if) I figure it out.
>>>
>>> Jan,
>>>
>>> It seems assumption is made that a 64bit dom0 will not have a 32bit app making
>>> hypercall?
>>>
>>> BUG_ON(*reg != (unsigned int)*reg);  <====
>> You know that all the 'xlat' stuff in Xen is for 32-bit guests running on
>> 64-bit hypervisor, right? 64-bit dom0 would never execute this logic.
> 
> It's worth noting though that I don't believe a 32 bit dom0 toolstack on
> a 64 bit kernel on a 64 bit hypervisor will work. In particular the
> privcmd "make a hypercall" ioctl doesn't do any compat translation so 32
> bit xend and friends can't make hypercalls that way and I think the
> MMAPBATCH privcmd doesn't work either.
> 
> I'm sure there are other cases too (blktap user<->kernel ring layout
> maybe?).
> 
> Ian.

Thanks Ian for good explanation of hypercall_xlat_continuation().

yeah, I'm just exploring that right now. There is MMAPBATCH_32, btw, in
dom0 that looks like was implemented by PPC folks. Also, MMAP_32.
I was able to start PV guest without network. Not sure if that was
because of compatibility or some other issue. I'm just looking at MMAP
stuff right now, think I finally figured out the chain of calls from
libxc to hyp to dom0 ... ia32_sys_call_table to compat_sys_ioctl to
handler to do_ioctl32_pointer .. whew!!

Thanks,
Mukesh

Re: hypercall_xlat_continuation()

[Mukesh Rathor]

Mukesh Rathor wrote:
> 
> 
> Ian Campbell wrote:
>> On Sat, 2009-05-23 at 06:17 -0400, Keir Fraser wrote:
>>> On 22/05/2009 22:58, "Mukesh Rathor" <mukesh.rathor@oracle.com> wrote:
>>>
>>>> Ok. Even if I can't make it clearer, at least I'll add few lines of 
>>>> comments
>>>> explaining what's going on, after (and if) I figure it out.
>>>>
>>>> Jan,
>>>>
>>>> It seems assumption is made that a 64bit dom0 will not have a 32bit 
>>>> app making
>>>> hypercall?
>>>>
>>>> BUG_ON(*reg != (unsigned int)*reg);  <====
>>> You know that all the 'xlat' stuff in Xen is for 32-bit guests 
>>> running on
>>> 64-bit hypervisor, right? 64-bit dom0 would never execute this logic.
>>
>> It's worth noting though that I don't believe a 32 bit dom0 toolstack on
>> a 64 bit kernel on a 64 bit hypervisor will work. In particular the
>> privcmd "make a hypercall" ioctl doesn't do any compat translation so 32
>> bit xend and friends can't make hypercalls that way and I think the
>> MMAPBATCH privcmd doesn't work either.
>>
>> I'm sure there are other cases too (blktap user<->kernel ring layout
>> maybe?).
>>
>> Ian.
> 
> Thanks Ian for good explanation of hypercall_xlat_continuation().
> 
> yeah, I'm just exploring that right now. There is MMAPBATCH_32, btw, in
> dom0 that looks like was implemented by PPC folks. Also, MMAP_32.
> I was able to start PV guest without network. Not sure if that was
> because of compatibility or some other issue. I'm just looking at MMAP
> stuff right now, think I finally figured out the chain of calls from
> libxc to hyp to dom0 ... ia32_sys_call_table to compat_sys_ioctl to
> handler to do_ioctl32_pointer .. whew!!
> 
> Thanks,
> Mukesh
> 

yeah, looks like privcmd_ioctl_32() only fixes the wrapper struct. The
PFN array still is 32bit pfn's, and privcmd_ioctl() expects 64bits. So,
in a dilemma now, not sure if I should fix it up in privcmd_ioctl_32()
or change privcmd_ioctl() which will take some time to reverse engineer.
Not sure how many things I'll discover, if it's too many, it may
not be worth it in the end.

Thanks,
Mukesh

Re: hypercall_xlat_continuation()

[Ian Campbell]

[-- Attachment #1: Type: text/plain, Size: 888 bytes --]

On Thu, 2009-05-28 at 23:01 -0400, Mukesh Rathor wrote:
> yeah, looks like privcmd_ioctl_32() only fixes the wrapper struct. The
> PFN array still is 32bit pfn's, and privcmd_ioctl() expects 64bits. So,
> in a dilemma now, not sure if I should fix it up in privcmd_ioctl_32()
> or change privcmd_ioctl() which will take some time to reverse engineer.
> Not sure how many things I'll discover, if it's too many, it may
> not be worth it in the end.

FWIW here are my very skanky patches from ages ago (patch names are the
originals if that gives a clue to my opinion of them even then ;-)).

I don't even recall if they worked properly (or at all), I think I
remember starting guests so long as they didn't use blktap (which has
issues with the user<->kernel ring protocol in this scenario).

There's an outside change there might be something in them which isn't
complete rubbish.

Ian.


[-- Attachment #2: hypervisor-skankup32on64on64.patch --]
[-- Type: text/x-patch, Size: 13013 bytes --]

diff -r fed20c1dc0ec xen/arch/x86/domain.c
--- a/xen/arch/x86/domain.c	Tue Sep 25 17:10:13 2007 +0100
+++ b/xen/arch/x86/domain.c	Wed Sep 26 09:32:05 2007 +0100
@@ -155,9 +155,6 @@ int setup_arg_xlat_area(struct vcpu *v, 
         clear_page(d->arch.mm_arg_xlat_l3);
     }
 
-    l4tab[l4_table_offset(COMPAT_ARG_XLAT_VIRT_BASE)] =
-        l4e_from_paddr(__pa(d->arch.mm_arg_xlat_l3), __PAGE_HYPERVISOR);
-
     for ( i = 0; i < COMPAT_ARG_XLAT_PAGES; ++i )
     {
         unsigned long va = COMPAT_ARG_XLAT_VIRT_START(v->vcpu_id) + i * PAGE_SIZE;
@@ -227,11 +224,27 @@ static void release_arg_xlat_area(struct
     }
 }
 
-static int setup_compat_l4(struct vcpu *v)
-{
+static int setup_native_l4(struct vcpu *v, int do_xlat)
+{
+    struct domain *d = v->domain;
+    l4_pgentry_t *l4tab = (l4_pgentry_t *)__va(pagetable_get_paddr(v->arch.guest_table));
+    int rc = 0;
+
+    if ( do_xlat )
+        rc = setup_arg_xlat_area(v, l4tab);
+
+    l4tab[l4_table_offset(COMPAT_ARG_XLAT_VIRT_BASE)] =
+        l4e_from_paddr(__pa(d->arch.mm_arg_xlat_l3), __PAGE_HYPERVISOR);
+
+    return rc;
+}
+
+static int setup_compat_l4(struct vcpu *v, int do_xlat)
+{
+    struct domain *d = v->domain;
     struct page_info *pg = alloc_domheap_page(NULL);
     l4_pgentry_t *l4tab;
-    int rc;
+    int rc = 0;
 
     if ( pg == NULL )
         return -ENOMEM;
@@ -247,16 +260,16 @@ static int setup_compat_l4(struct vcpu *
         l4e_from_paddr(__pa(v->domain->arch.mm_perdomain_l3),
                        __PAGE_HYPERVISOR);
 
-    if ( (rc = setup_arg_xlat_area(v, l4tab)) < 0 )
-    {
-        free_domheap_page(pg);
-        return rc;
-    }
+    if ( do_xlat )
+        rc = setup_arg_xlat_area(v, l4tab);
+
+    l4tab[l4_table_offset(COMPAT_ARG_XLAT_VIRT_BASE)] =
+        l4e_from_paddr(__pa(d->arch.mm_arg_xlat_l3), __PAGE_HYPERVISOR);
 
     v->arch.guest_table = pagetable_from_page(pg);
     v->arch.guest_table_user = v->arch.guest_table;
 
-    return 0;
+    return rc;
 }
 
 static void release_compat_l4(struct vcpu *v)
@@ -284,7 +297,6 @@ int switch_native(struct domain *d)
         return 0;
 
     d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
-    release_arg_xlat_area(d);
 
     /* switch gdt */
     gdt_l1e = l1e_from_page(virt_to_page(gdt_table), PAGE_HYPERVISOR);
@@ -320,7 +332,7 @@ int switch_compat(struct domain *d)
     for ( vcpuid = 0; vcpuid < MAX_VIRT_CPUS; vcpuid++ )
     {
         if ( (d->vcpu[vcpuid] != NULL) &&
-             (setup_compat_l4(d->vcpu[vcpuid]) != 0) )
+             (setup_compat_l4(d->vcpu[vcpuid], 0) != 0) )
             goto undo_and_fail;
         d->arch.mm_perdomain_pt[((vcpuid << GDT_LDT_VCPU_SHIFT) +
                                  FIRST_RESERVED_GDT_PAGE)] = gdt_l1e;
@@ -334,7 +346,6 @@ int switch_compat(struct domain *d)
 
  undo_and_fail:
     d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
-    release_arg_xlat_area(d);
     gdt_l1e = l1e_from_page(virt_to_page(gdt_table), PAGE_HYPERVISOR);
     while ( vcpuid-- != 0 )
     {
@@ -348,7 +359,8 @@ int switch_compat(struct domain *d)
 
 #else
 #define release_arg_xlat_area(d) ((void)0)
-#define setup_compat_l4(v) 0
+#define setup_native_l4(v,x) 0
+#define setup_compat_l4(v,x) 0
 #define release_compat_l4(v) ((void)0)
 #endif
 
@@ -393,7 +405,7 @@ int vcpu_initialise(struct vcpu *v)
     v->arch.perdomain_ptes =
         d->arch.mm_perdomain_pt + (v->vcpu_id << GDT_LDT_VCPU_SHIFT);
 
-    return (is_pv_32on64_vcpu(v) ? setup_compat_l4(v) : 0);
+    return (is_pv_32on64_vcpu(v) ? setup_compat_l4(v, 1) : setup_native_l4(v, 1));
 }
 
 void vcpu_destroy(struct vcpu *v)
@@ -1394,7 +1406,7 @@ unsigned long hypercall_create_continuat
 
         for ( i = 0; *p != '\0'; i++ )
             mcs->call.args[i] = next_arg(p, args);
-        if ( is_pv_32on64_domain(current->domain) )
+        if ( in_32on64_hypercall(current) )
         {
             for ( ; i < 6; i++ )
                 mcs->call.args[i] = 0;
@@ -1408,7 +1420,7 @@ unsigned long hypercall_create_continuat
 
 #ifdef __x86_64__
         if ( !is_hvm_vcpu(current) ?
-             !is_pv_32on64_vcpu(current) :
+             !in_32on64_hypercall(current) :
              (hvm_guest_x86_mode(current) == 8) )
         {
             for ( i = 0; *p != '\0'; i++ )
diff -r fed20c1dc0ec xen/arch/x86/domain_build.c
--- a/xen/arch/x86/domain_build.c	Tue Sep 25 17:10:13 2007 +0100
+++ b/xen/arch/x86/domain_build.c	Wed Sep 26 09:32:05 2007 +0100
@@ -650,8 +650,8 @@ int __init construct_dom0(
     if ( is_pv_32on64_domain(d) )
     {
         v->arch.guest_table_user = v->arch.guest_table;
-        if ( setup_arg_xlat_area(v, l4start) < 0 )
-            panic("Not enough RAM for domain 0 hypercall argument translation.\n");
+        l4tab[l4_table_offset(COMPAT_ARG_XLAT_VIRT_BASE)] =
+            l4e_from_paddr(__pa(v->domain->arch.mm_arg_xlat_l3), __PAGE_HYPERVISOR);
     }
 
     l4tab += l4_table_offset(v_start);
diff -r fed20c1dc0ec xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c	Tue Sep 25 17:10:13 2007 +0100
+++ b/xen/arch/x86/mm.c	Wed Sep 26 09:32:05 2007 +0100
@@ -1191,10 +1191,9 @@ static int alloc_l4_table(struct page_in
     pl4e[l4_table_offset(PERDOMAIN_VIRT_START)] =
         l4e_from_page(virt_to_page(d->arch.mm_perdomain_l3),
                       __PAGE_HYPERVISOR);
-    if ( is_pv_32on64_domain(d) )
-        pl4e[l4_table_offset(COMPAT_ARG_XLAT_VIRT_BASE)] =
-            l4e_from_page(virt_to_page(d->arch.mm_arg_xlat_l3),
-                          __PAGE_HYPERVISOR);
+    pl4e[l4_table_offset(COMPAT_ARG_XLAT_VIRT_BASE)] =
+        l4e_from_page(virt_to_page(d->arch.mm_arg_xlat_l3),
+                      __PAGE_HYPERVISOR);
 
     return 1;
 
diff -r fed20c1dc0ec xen/arch/x86/x86_64/asm-offsets.c
--- a/xen/arch/x86/x86_64/asm-offsets.c	Tue Sep 25 17:10:13 2007 +0100
+++ b/xen/arch/x86/x86_64/asm-offsets.c	Wed Sep 26 09:32:05 2007 +0100
@@ -116,6 +116,9 @@ void __dummy__(void)
     OFFSET(VCPU_vmx_cr2, struct vcpu, arch.hvm_vmx.cpu_cr2);
     BLANK();
 
+    OFFSET(VCPU_in_32on64_hypercall, struct vcpu, arch.in_32on64_hypercall);
+    BLANK();
+
     OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.is_32bit_pv);
     BLANK();
 
diff -r fed20c1dc0ec xen/arch/x86/x86_64/compat/entry.S
--- a/xen/arch/x86/x86_64/compat/entry.S	Tue Sep 25 17:10:13 2007 +0100
+++ b/xen/arch/x86/x86_64/compat/entry.S	Wed Sep 26 09:32:05 2007 +0100
@@ -29,6 +29,7 @@ ENTRY(compat_hypercall)
         SAVE_ALL
         GET_CURRENT(%rbx)
 
+        movl $1,VCPU_in_32on64_hypercall(%rbx)
         cmpl  $NR_hypercalls,%eax
         jae   compat_bad_hypercall
 #ifndef NDEBUG
@@ -77,6 +78,7 @@ compat_skip_clobber:
 compat_skip_clobber:
 #endif
         movl  %eax,UREGS_rax(%rsp)       # save the return value
+        movl  $0,VCPU_in_32on64_hypercall(%rbx)
 
 /* %rbx: struct vcpu */
 ENTRY(compat_test_all_events)
diff -r fed20c1dc0ec xen/arch/x86/x86_64/entry.S
--- a/xen/arch/x86/x86_64/entry.S	Tue Sep 25 17:10:13 2007 +0100
+++ b/xen/arch/x86/x86_64/entry.S	Wed Sep 26 09:32:05 2007 +0100
@@ -195,6 +195,65 @@ test_guest_events:
         movb  $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
         call  create_bounce_frame
         jmp   test_all_events
+
+        ALIGN
+ENTRY(compat_privcmd)
+        pushq $0
+        movl  $0x83,4(%rsp)
+        SAVE_ALL
+        GET_CURRENT(%rbx)
+
+        movl $1,VCPU_in_32on64_hypercall(%rbx)
+        cmpl  $NR_hypercalls,%eax
+        jae   bad_hypercall
+#ifndef NDEBUG
+        /* Deliberately corrupt parameter regs not used by this hypercall. */
+        pushq UREGS_rbx(%rsp); pushq %rcx; pushq %rdx; pushq %rsi; pushq %rdi
+        pushq UREGS_rbp+5*8(%rsp)
+        leaq  compat_hypercall_args_table(%rip),%r10
+        movq  $6,%rcx
+        subb  (%r10,%rax,1),%cl
+        movq  %rsp,%rdi
+        movl  $0xDEADBEEF,%eax
+        rep   stosq
+        popq  %r8 ; popq  %r9 ; xchgl %r8d,%r9d /* Args 5&6: zero extend */
+        popq  %rdx; popq  %rcx; xchgl %edx,%ecx /* Args 3&4: zero extend */
+        popq  %rdi; popq  %rsi; xchgl %edi,%esi /* Args 1&2: zero extend */
+        movl  UREGS_rax(%rsp),%eax
+        pushq %rax
+        pushq UREGS_rip+8(%rsp)
+#else
+        /* Relocate argument registers and zero-extend to 64 bits. */
+        movl  %eax,%eax              /* Hypercall #  */
+        xchgl %ecx,%esi              /* Arg 2, Arg 4 */
+        movl  %edx,%edx              /* Arg 3        */
+        movl  %edi,%r8d              /* Arg 5        */
+        movl  %ebp,%r9d              /* Arg 6        */
+        movl  UREGS_rbx(%rsp),%edi   /* Arg 1        */
+#endif
+        leaq  compat_hypercall_table(%rip),%r10
+        PERFC_INCR(PERFC_hypercalls, %rax, %rbx)
+        callq *(%r10,%rax,8)
+#ifndef NDEBUG
+        /* Deliberately corrupt parameter regs used by this hypercall. */
+        popq  %r10         # Shadow RIP
+        cmpq  %r10,UREGS_rip+8(%rsp)
+        popq  %rcx         # Shadow hypercall index
+        jne   privcmd_skip_clobber /* If RIP has changed then don't clobber. */
+        leaq  compat_hypercall_args_table(%rip),%r10
+        movb  (%r10,%rcx,1),%cl
+        movl  $0xDEADBEEF,%r10d
+        testb %cl,%cl; jz privcmd_skip_clobber; movl %r10d,UREGS_rbx(%rsp)
+        cmpb  $2, %cl; jb privcmd_skip_clobber; movl %r10d,UREGS_rcx(%rsp)
+        cmpb  $3, %cl; jb privcmd_skip_clobber; movl %r10d,UREGS_rdx(%rsp)
+        cmpb  $4, %cl; jb privcmd_skip_clobber; movl %r10d,UREGS_rsi(%rsp)
+        cmpb  $5, %cl; jb privcmd_skip_clobber; movl %r10d,UREGS_rdi(%rsp)
+        cmpb  $6, %cl; jb privcmd_skip_clobber; movl %r10d,UREGS_rbp(%rsp)
+privcmd_skip_clobber:
+#endif
+        movl  %eax,UREGS_rax(%rsp)       # save the return value
+        movl  $0,VCPU_in_32on64_hypercall(%rbx)
+        jmp test_all_events
 
         ALIGN
 /* %rbx: struct vcpu */
diff -r fed20c1dc0ec xen/arch/x86/x86_64/traps.c
--- a/xen/arch/x86/x86_64/traps.c	Tue Sep 25 17:10:13 2007 +0100
+++ b/xen/arch/x86/x86_64/traps.c	Wed Sep 26 09:32:05 2007 +0100
@@ -23,6 +23,7 @@
 
 asmlinkage void syscall_enter(void);
 asmlinkage void compat_hypercall(void);
+asmlinkage void compat_privcmd(void);
 asmlinkage void int80_direct_trap(void);
 
 static void print_xen_info(void)
@@ -303,6 +304,7 @@ void __init percpu_traps_init(void)
          * Also note that this is a trap gate, not an interrupt gate.
          */
         _set_gate(idt_table+HYPERCALL_VECTOR, 15, 1, &compat_hypercall);
+        _set_gate(idt_table+0x83, 15, 3, &compat_privcmd);
 
         /* Fast trap for int80 (faster than taking the #GP-fixup path). */
         _set_gate(idt_table+0x80, 15, 3, &int80_direct_trap);
diff -r fed20c1dc0ec xen/include/asm-x86/config.h
--- a/xen/include/asm-x86/config.h	Tue Sep 25 17:10:13 2007 +0100
+++ b/xen/include/asm-x86/config.h	Wed Sep 26 09:32:05 2007 +0100
@@ -237,7 +237,9 @@ extern unsigned char trampoline_cpu_star
 
 #endif
 
-#define COMPAT_ARG_XLAT_VIRT_BASE      (1UL << ROOT_PAGETABLE_SHIFT)
+/* XLAT area actually covers 270-271 since we sometimes check p-<n>
+ * but only actually access from p onwards. */
+#define COMPAT_ARG_XLAT_VIRT_BASE      PML4_ADDR(271)
 #define COMPAT_ARG_XLAT_SHIFT          0
 #define COMPAT_ARG_XLAT_PAGES          (1U << COMPAT_ARG_XLAT_SHIFT)
 #define COMPAT_ARG_XLAT_SIZE           (COMPAT_ARG_XLAT_PAGES << PAGE_SHIFT)
diff -r fed20c1dc0ec xen/include/asm-x86/domain.h
--- a/xen/include/asm-x86/domain.h	Tue Sep 25 17:10:13 2007 +0100
+++ b/xen/include/asm-x86/domain.h	Wed Sep 26 09:32:05 2007 +0100
@@ -17,6 +17,8 @@
 #endif
 #define is_pv_32on64_vcpu(v)   (is_pv_32on64_domain((v)->domain))
 #define IS_COMPAT(d)           (is_pv_32on64_domain(d))
+
+#define in_32on64_hypercall(v) (v->arch.in_32on64_hypercall)
 
 struct trap_bounce {
     uint32_t      error_code;
@@ -320,6 +322,8 @@ struct arch_vcpu
 
     /* Guest-specified relocation of vcpu_info. */
     unsigned long vcpu_info_mfn;
+
+    int in_32on64_hypercall;
 } __cacheline_aligned;
 
 /* shorthands to improve code legibility */
diff -r fed20c1dc0ec xen/include/asm-x86/x86_64/uaccess.h
--- a/xen/include/asm-x86/x86_64/uaccess.h	Tue Sep 25 17:10:13 2007 +0100
+++ b/xen/include/asm-x86/x86_64/uaccess.h	Wed Sep 26 09:32:07 2007 +0100
@@ -9,7 +9,9 @@
  */
 #define __addr_ok(addr) \
     (((unsigned long)(addr) < (1UL<<48)) || \
-     ((unsigned long)(addr) >= HYPERVISOR_VIRT_END))
+     ((unsigned long)(addr) >= HYPERVISOR_VIRT_END) || \
+     (((unsigned long)(addr) >= COMPAT_ARG_XLAT_VIRT_BASE - PML4_ENTRY_BYTES) && \
+      ((unsigned long)(addr) < COMPAT_ARG_XLAT_VIRT_BASE + PML4_ENTRY_BYTES)))
 
 #define access_ok(addr, size) (__addr_ok(addr))
 
@@ -18,7 +20,9 @@
 #ifdef CONFIG_COMPAT
 
 #define __compat_addr_ok(addr) \
-    ((unsigned long)(addr) < HYPERVISOR_COMPAT_VIRT_START(current->domain))
+    (is_pv_32bit_vcpu(current) \
+     ? ((unsigned long)(addr) < HYPERVISOR_COMPAT_VIRT_START(current->domain)) \
+     : __addr_ok(addr))
 
 #define compat_access_ok(addr, size) \
     __compat_addr_ok((unsigned long)(addr) + ((size) ? (size) - 1 : 0))

[-- Attachment #3: kernel-skankup32on64on64.patch --]
[-- Type: text/x-patch, Size: 1070 bytes --]

diff -r 5b92eed96bd3 drivers/xen/privcmd/privcmd.c
--- a/drivers/xen/privcmd/privcmd.c	Tue Sep 18 10:08:36 2007 +0100
+++ b/drivers/xen/privcmd/privcmd.c	Tue Sep 18 10:21:42 2007 +0100
@@ -69,7 +69,25 @@ static long privcmd_ioctl(struct file *f
 			"popl %%ecx; popl %%ebx"
 			: "=a" (ret) : "0" (&hypercall) : "memory" );
 #elif defined (__x86_64__)
-		if (hypercall.op < (PAGE_SIZE >> 5)) {
+		if (hypercall.op >= (PAGE_SIZE >> 5))
+			break;
+
+		if (test_tsk_thread_flag(current, TIF_IA32)) {
+			__asm__ __volatile__ (
+			"pushq %%rbx; pushq %%rcx; pushq %%rdx; "
+			"pushq %%rsi; pushq %%rdi; pushq %%rbp; "
+			"xorq %%rbp,%%rbp ;"
+			"movl  8(%%rax),%%ebx ;"
+			"movl 16(%%rax),%%ecx ;"
+			"movl 24(%%rax),%%edx ;"
+			"movl 32(%%rax),%%esi ;"
+			"movl 40(%%rax),%%edi ;"
+			"movl   (%%rax),%%eax ;"
+			"int  $0x83;"
+			"popq %%rbp; popq %%rdi; popq %%rsi; popq %%rdx; "
+			"popq %%rcx; popq %%rbx"
+			: "=a" (ret) : "0" (&hypercall) : "memory" );
+		} else {
 			long ign1, ign2, ign3;
 			__asm__ __volatile__ (
 				"movq %8,%%r10; movq %9,%%r8;"

[-- Attachment #4: kernel-32on64on64-batch-privcmd-fixup.patch --]
[-- Type: text/x-patch, Size: 2777 bytes --]

diff -r 567da1a48940 drivers/xen/privcmd/compat_privcmd.c
--- a/drivers/xen/privcmd/compat_privcmd.c	Fri Oct 19 10:27:45 2007 +0100
+++ b/drivers/xen/privcmd/compat_privcmd.c	Mon Oct 22 16:45:06 2007 +0100
@@ -33,8 +33,8 @@ int privcmd_ioctl_32(int fd, unsigned in
 
 	switch (cmd) {
 	case IOCTL_PRIVCMD_MMAP_32: {
-		struct privcmd_mmap *p;
-		struct privcmd_mmap_32 *p32;
+		struct privcmd_mmap __user *p;
+		struct privcmd_mmap_32 __user *p32;
 		struct privcmd_mmap_32 n32;
 
 		p32 = compat_ptr(arg);
@@ -49,20 +49,51 @@ int privcmd_ioctl_32(int fd, unsigned in
 	}
 		break;
 	case IOCTL_PRIVCMD_MMAPBATCH_32: {
-		struct privcmd_mmapbatch *p;
-		struct privcmd_mmapbatch_32 *p32;
+		struct privcmd_mmapbatch __user*p;
 		struct privcmd_mmapbatch_32 n32;
+		xen_pfn_t pfn64;
+		xen_pfn_t __user *upfn64;
+		u32 pfn32;
+		u32 __user *upfn32;
+		int i;
 
-		p32 = compat_ptr(arg);
-		p = compat_alloc_user_space(sizeof(*p));
-		if (copy_from_user(&n32, p32, sizeof(n32)) ||
-		    put_user(n32.num, &p->num) ||
-		    put_user(n32.dom, &p->dom) ||
-		    put_user(n32.addr, &p->addr) ||
-		    put_user(compat_ptr(n32.arr), &p->arr))
+		if (copy_from_user(&n32, compat_ptr(arg), sizeof(n32)))
 			return -EFAULT;
-		
+
+		p = compat_alloc_user_space(sizeof(*p) + sizeof(*upfn64) * n32.num);
+		upfn64 = (xen_pfn_t __user *)(p + 1);
+
+		if (put_user(n32.num, &p->num))
+			return -EFAULT;
+		if (put_user(n32.dom, &p->dom))
+			return -EFAULT;
+		if (put_user(n32.addr, &p->addr))
+			return -EFAULT;
+
+		upfn32 = compat_ptr(n32.arr);
+		for (i=0; i<n32.num; i++)
+		{
+			if (copy_from_user(&pfn32, &upfn32[i], sizeof(pfn32)))
+				return -EFAULT;
+			pfn64 = pfn32;
+			if (copy_to_user(&upfn64[i], &pfn64, sizeof(pfn64)))
+				return -EFAULT;
+		}
+
+		if (put_user(upfn64, &p->arr))
+			return -EFAULT;
+
 		ret = sys_ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH, (unsigned long)p);
+
+		for (i=0; i<n32.num; i++)
+		{
+			if (copy_from_user(&pfn64, &upfn64[i], sizeof(pfn64)))
+				return -EFAULT;
+			pfn32 = pfn64;
+			if (copy_to_user(&upfn32[i], &pfn32, sizeof(pfn32)))
+				return -EFAULT;
+		}
+
 	}
 		break;
 	default:
diff -r 567da1a48940 include/xen/compat_ioctl.h
--- a/include/xen/compat_ioctl.h	Fri Oct 19 10:27:45 2007 +0100
+++ b/include/xen/compat_ioctl.h	Mon Oct 22 13:47:28 2007 +0100
@@ -34,9 +34,10 @@ struct privcmd_mmapbatch_32 {
 struct privcmd_mmapbatch_32 {
 	int num;     /* number of pages to populate */
 	domid_t dom; /* target domain */
+	u16 pad0;
 	__u64 addr;  /* virtual address */
 	compat_uptr_t arr; /* array of mfns - top nibble set on err */
-};
+} __attribute__((packed));
 #define IOCTL_PRIVCMD_MMAP_32                   \
 	_IOC(_IOC_NONE, 'P', 2, sizeof(struct privcmd_mmap_32))
 #define IOCTL_PRIVCMD_MMAPBATCH_32                  \

[-- Attachment #5: Type: text/plain, Size: 138 bytes --]

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

Re: hypercall_xlat_continuation()

[Mukesh Rathor]

Ian Campbell wrote:
> On Thu, 2009-05-28 at 23:01 -0400, Mukesh Rathor wrote:
>> yeah, looks like privcmd_ioctl_32() only fixes the wrapper struct. The
>> PFN array still is 32bit pfn's, and privcmd_ioctl() expects 64bits. So,
>> in a dilemma now, not sure if I should fix it up in privcmd_ioctl_32()
>> or change privcmd_ioctl() which will take some time to reverse engineer.
>> Not sure how many things I'll discover, if it's too many, it may
>> not be worth it in the end.
> 
> FWIW here are my very skanky patches from ages ago (patch names are the
> originals if that gives a clue to my opinion of them even then ;-)).
> 
> I don't even recall if they worked properly (or at all), I think I
> remember starting guests so long as they didn't use blktap (which has
> issues with the user<->kernel ring protocol in this scenario).
> 
> There's an outside change there might be something in them which isn't
> complete rubbish.
> 
> Ian.
> 

Hi Ian,

Looks like you also put in quite a bit of effort into it. I'm
realizing that it is more work than it appeared in the beginning,
and probably not worth the gains in the end. So I should also give up.
learnt few new things along the way tho :).

Thanks,
Mukesh

Re: hypercall_xlat_continuation()

[Mukesh Rathor]

Keir Fraser wrote:
> On 22/05/2009 22:58, "Mukesh Rathor" <mukesh.rathor@oracle.com> wrote:
> 
>> Ok. Even if I can't make it clearer, at least I'll add few lines of comments
>> explaining what's going on, after (and if) I figure it out.
>>
>> Jan,
>>
>> It seems assumption is made that a 64bit dom0 will not have a 32bit app making
>> hypercall?
>>
>> BUG_ON(*reg != (unsigned int)*reg);  <====
> 
> You know that all the 'xlat' stuff in Xen is for 32-bit guests running on
> 64-bit hypervisor, right? 64-bit dom0 would never execute this logic.

Unless someone like me, in trying to make 32bit apps on 64bit dom0 work, 
causes it to take compat path if it's 32bit hcall. Yeah, I know whacky :)...

Thanks as always,
Mukesh

PS: If anyone is curious, if the call is 32bit hcall, in entry.S I make it jmp 
   to compat/entry.S and do compat hcall. So far, other than this xlat stuff, 
seems ok! I set a register in dom0 to before hcall to indicate it's from a 
32bit app.

Re: hypercall_xlat_continuation()

[Jan Beulich]

>>> Mukesh Rathor <mukesh.rathor@oracle.com> 26.05.09 20:43 >>>
>Keir Fraser wrote:
>> You know that all the 'xlat' stuff in Xen is for 32-bit guests running on
>> 64-bit hypervisor, right? 64-bit dom0 would never execute this logic.
>
>Unless someone like me, in trying to make 32bit apps on 64bit dom0 work, 
>causes it to take compat path if it's 32bit hcall. Yeah, I know whacky :)...

I don't think that's a good approach - even when dealing with 32-bit apps,
it's a 64-bit kernel, and hence you should rather translate things to the
64-bit format in the kernel layer that sits in between (after all, apps can't
do hypercalls directly).

Jan

Re: hypercall_xlat_continuation()

[Keir Fraser]

On 27/05/2009 09:02, "Jan Beulich" <JBeulich@novell.com> wrote:

>>> You know that all the 'xlat' stuff in Xen is for 32-bit guests running on
>>> 64-bit hypervisor, right? 64-bit dom0 would never execute this logic.
>> 
>> Unless someone like me, in trying to make 32bit apps on 64bit dom0 work,
>> causes it to take compat path if it's 32bit hcall. Yeah, I know whacky :)...
> 
> I don't think that's a good approach - even when dealing with 32-bit apps,
> it's a 64-bit kernel, and hence you should rather translate things to the
> 64-bit format in the kernel layer that sits in between (after all, apps can't
> do hypercalls directly).

Why would you do that when the tedious translation mechanism already exists
in the hypervisor?

 -- Keir

Re: hypercall_xlat_continuation()

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 27.05.09 10:04 >>>
>On 27/05/2009 09:02, "Jan Beulich" <JBeulich@novell.com> wrote:
>
>>>> You know that all the 'xlat' stuff in Xen is for 32-bit guests running on
>>>> 64-bit hypervisor, right? 64-bit dom0 would never execute this logic.
>>> 
>>> Unless someone like me, in trying to make 32bit apps on 64bit dom0 work,
>>> causes it to take compat path if it's 32bit hcall. Yeah, I know whacky :)...
>> 
>> I don't think that's a good approach - even when dealing with 32-bit apps,
>> it's a 64-bit kernel, and hence you should rather translate things to the
>> 64-bit format in the kernel layer that sits in between (after all, apps can't
>> do hypercalls directly).
>
>Why would you do that when the tedious translation mechanism already exists
>in the hypervisor?

Because it's conceptually wrong. And the necessary extra de-muxing that's
needed (because the int $HYPERCALL_VECTOR path can't be used) is not
going to come without a (performance) price.

A secondary reason is that as soon as Xen becomes more easily build-time
configurable (like Linux is), which I expect to happen at some point (unless
we manage to replace all those build time decisions with runtime ones), a
64-bit guest can't really rely on the hypervisor having compat mode support
(and considering backward compatibility, which might not matter in the
context the subject was brought up in, such an assumption would be wrong
in the first place).

Jan

Re: hypercall_xlat_continuation()

[Keir Fraser]

On 27/05/2009 09:22, "Jan Beulich" <JBeulich@novell.com> wrote:

> Because it's conceptually wrong.

Nothing conceptually wrong with it at all. Just another hypervisor service
being made available to a higher level of software.

> And the necessary extra de-muxing that's
> needed (because the int $HYPERCALL_VECTOR path can't be used) is not
> going to come without a (performance) price.

We add a compat32 wrapping hypercall, and shift the actual hypercall number
to a different register. Noone takes a hit but the 32-bit userspace which
has already bounced via ring 1 and this will not add an extra measurable
overhead.

> A secondary reason is that as soon as Xen becomes more easily build-time
> configurable (like Linux is), which I expect to happen at some point (unless
> we manage to replace all those build time decisions with runtime ones), a
> 64-bit guest can't really rely on the hypervisor having compat mode support
> (and considering backward compatibility, which might not matter in the
> context the subject was brought up in, such an assumption would be wrong
> in the first place).

Firstly, I don't see us bringing in extensive build-time configuration any
time soon. Secondly, if someone wanted this new compat32-for-userspace
support, they would simply have to install a new hypervisor configured with
compat32 support -- how is that harder/worse than installing a new kernel
configured with compat32 support?

At the end of the day, the thought of *duplicating* the compat32 stuff and
then trying to stuff it upstream to kernel.org does not appeal. I see that
as two major concrete negatives versus the much weaker negatives on the
other side of the argument.

 -- Keir

Re: hypercall_xlat_continuation()

[Ian Campbell]

On Thu, 2009-05-21 at 22:57 -0400, Mukesh Rathor wrote:
> Can someone please explain the madness in the else part of this function? The 
> caller magically passes 2 for mask? Is this already documented anywhere by 
> chance for mortals like me :).

I was forced to understand this at one point, let's see how much I can
remember ;-)

The mask argument to hypercall_xlat_continuation indicates which of the
6 potential continuation arguments (corresponding to the up to 6
arguments to a hypercall) need to be translated from a native value to a
compat value. The least significant bit == the first argument, the
second least is the second argument etc. For each bit that is set the
varargs should contain a pair of additional arguments, the native value
and the replacement compat value. The native value is compared to the
value in the continuation before replacing it with the compat value. I
would have thought the native value must always match by design so this
might just be a sanity check.

For example do_memory_op takes arguments (cmd, nat.hnd) and therefore we
pass mask==0x2 followed by varargs (nat.hnd, compat). So if the second
continuation argument matches nat.hnd it will be replaced with compat.

Similarly do_mmuext_op takes (nat_ops, otherstuff, etc...) and we pass a
mask==0x01 with varargs (nat_ops, cmp_uops so if the first continuation
argument matches nat_ops we replace it with cmp_uops.

In both cases if the native and compat things are the same we ignore the
bit set in the mask.

I don't recall what the "BUG_ON(nval == (unsigned int)nval)" is all
about. I guess the assumption is that if an argument requires
translation it must be too large to fit in a compat sized variable. This
seems to be true for the existing cases (which are both
XEN_GUEST_HANDLEs), I don't see why it would be true in general. Maybe
the assumption is that only XEN_GUEST_HANDLES ever need translation?

The "BUG_ON(*reg != (unsigned int)*reg)" is there because if we didn't
request translation for a given argument it had better be the same in
both native and compat form.

The two halves of the outermost if-else are just to handle the different
location of the continuation arguments in the multicall vs regular
hypercall cases.

The first argument to hypercall_xlat_continuation (unsigned int *id) is
a pointer to an index which, if non-NULL, is replaced with the value of
the argument at that index in the continuation, I think it's just used
for sanity checking, I'm not all that convinced it is necessary (maybe
it was useful when initially debugging this stuff)

It all seems rather complicated and fragile to me, but it does seem to
work so I'm not inclined to go poking at it...

Ian.

Re: hypercall_xlat_continuation()

[Jan Beulich]

>>> Ian Campbell <Ian.Campbell@citrix.com> 24.05.09 00:36 >>>
>The mask argument to hypercall_xlat_continuation indicates which of the
>6 potential continuation arguments (corresponding to the up to 6
>arguments to a hypercall) need to be translated from a native value to a
>compat value. The least significant bit == the first argument, the
>second least is the second argument etc. For each bit that is set the
>varargs should contain a pair of additional arguments, the native value
>and the replacement compat value. The native value is compared to the
>value in the continuation before replacing it with the compat value. I
>would have thought the native value must always match by design so this
>might just be a sanity check.

It indeed is a sanity check, to avoid silently doing the wrong thing.

>For example do_memory_op takes arguments (cmd, nat.hnd) and therefore we
>pass mask==0x2 followed by varargs (nat.hnd, compat). So if the second
>continuation argument matches nat.hnd it will be replaced with compat.
>
>Similarly do_mmuext_op takes (nat_ops, otherstuff, etc...) and we pass a
>mask==0x01 with varargs (nat_ops, cmp_uops so if the first continuation
>argument matches nat_ops we replace it with cmp_uops.
>
>In both cases if the native and compat things are the same we ignore the
>bit set in the mask.

... the primary purpose of which is to deal with NULL arguments.

>I don't recall what the "BUG_ON(nval == (unsigned int)nval)" is all
>about. I guess the assumption is that if an argument requires
>translation it must be too large to fit in a compat sized variable. This
>seems to be true for the existing cases (which are both
>XEN_GUEST_HANDLEs), I don't see why it would be true in general. Maybe
>the assumption is that only XEN_GUEST_HANDLES ever need translation?

The intention here is to avoid someone trying to put translated arguments
in guest visible space (i.e. va < 4G).

>...
>
>The first argument to hypercall_xlat_continuation (unsigned int *id) is
>a pointer to an index which, if non-NULL, is replaced with the value of
>the argument at that index in the continuation, I think it's just used
>for sanity checking, I'm not all that convinced it is necessary (maybe
>it was useful when initially debugging this stuff)

No, this is not just for sanity checking. For an example, look at
compat_memory_op()'s use of it: It has no other way of knowing how much
of the current batch do_memory_op() actually processed, since the return
value is either an error code or __HYPERVISOR_memory_op.

>It all seems rather complicated and fragile to me, but it does seem to
>work so I'm not inclined to go poking at it...

Indeed, I didn't try to make it artificially complicated, but the main goal
was to keep the users of it simple (which I still believe they are). I'd be
all for simplifying it *without* imposing more complexity on the callers.
Otoh I don't think it's *that* complicated...

Jan

Re: hypercall_xlat_continuation()

[Keir Fraser]

On 25/05/2009 12:47, "Jan Beulich" <JBeulich@novell.com> wrote:

>> It all seems rather complicated and fragile to me, but it does seem to
>> work so I'm not inclined to go poking at it...
> 
> Indeed, I didn't try to make it artificially complicated, but the main goal
> was to keep the users of it simple (which I still believe they are). I'd be
> all for simplifying it *without* imposing more complexity on the callers.
> Otoh I don't think it's *that* complicated...

Some code comments would be nice. :-)

 -- Keir