[PATCH] paging_enabled and non-HVM guests

[PATCH] paging_enabled and non-HVM guests

[Simon Kagstrom]

I had a problem with the GDB-server crashing on connections in
xen_ptrace.c:map_domain_va(). paging_enabled() should only be checked
for HVM guests, and the patch adds a check for that.

Signed-off-by: Simon Kagstrom <ska@bth.se>

diff -r 4501d60d6add tools/libxc/xc_ptrace.c
--- a/tools/libxc/xc_ptrace.c	Tue May  9 09:57:05 2006
+++ b/tools/libxc/xc_ptrace.c	Tue May  9 13:26:14 2006
@@ -374,7 +374,7 @@
     if (fetch_regs(xc_handle, cpu, NULL))
         return NULL;
 
-    if (!paging_enabled(&ctxt[cpu])) { 
+    if ( (ctxt[cpu].flags & VGCF_HVM_GUEST) && !paging_enabled(&ctxt[cpu])) { 
         static void * v;
         unsigned long page;

Re: [PATCH] paging_enabled and non-HVM guests

[Hollis Blanchard]

On Tue, 2006-05-09 at 13:31 +0200, Simon Kagstrom wrote:
> I had a problem with the GDB-server crashing on connections in
> xen_ptrace.c:map_domain_va(). paging_enabled() should only be checked
> for HVM guests, and the patch adds a check for that.
> 
> Signed-off-by: Simon Kagstrom <ska@bth.se>
> 
> diff -r 4501d60d6add tools/libxc/xc_ptrace.c
> --- a/tools/libxc/xc_ptrace.c	Tue May  9 09:57:05 2006
> +++ b/tools/libxc/xc_ptrace.c	Tue May  9 13:26:14 2006
> @@ -374,7 +374,7 @@
>      if (fetch_regs(xc_handle, cpu, NULL))
>          return NULL;
>  
> -    if (!paging_enabled(&ctxt[cpu])) { 
> +    if ( (ctxt[cpu].flags & VGCF_HVM_GUEST) && !paging_enabled(&ctxt[cpu])) { 
>          static void * v;
>          unsigned long page;

I looked at this a couple weeks ago, and I think the real problem is
that the CR registers are never updated in Xen's vcpu structure, and so
xc_vcpu_getcontext() doesn't get them either. So Xen should be fixed; we
shouldn't add workarounds to userland.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] paging_enabled and non-HVM guests

[Anthony Liguori]

Hollis Blanchard wrote:
> On Tue, 2006-05-09 at 13:31 +0200, Simon Kagstrom wrote:
>   
>> I had a problem with the GDB-server crashing on connections in
>> xen_ptrace.c:map_domain_va(). paging_enabled() should only be checked
>> for HVM guests, and the patch adds a check for that.
>>
>> Signed-off-by: Simon Kagstrom <ska@bth.se>
>>
>> diff -r 4501d60d6add tools/libxc/xc_ptrace.c
>> --- a/tools/libxc/xc_ptrace.c	Tue May  9 09:57:05 2006
>> +++ b/tools/libxc/xc_ptrace.c	Tue May  9 13:26:14 2006
>> @@ -374,7 +374,7 @@
>>      if (fetch_regs(xc_handle, cpu, NULL))
>>          return NULL;
>>  
>> -    if (!paging_enabled(&ctxt[cpu])) { 
>> +    if ( (ctxt[cpu].flags & VGCF_HVM_GUEST) && !paging_enabled(&ctxt[cpu])) { 
>>          static void * v;
>>          unsigned long page;
>>     
>
> I looked at this a couple weeks ago, and I think the real problem is
> that the CR registers are never updated in Xen's vcpu structure, and so
> xc_vcpu_getcontext() doesn't get them either. So Xen should be fixed; we
> shouldn't add workarounds to userland.
>   

I think that the CR registers are never changed during the life of a PV 
domain.  I think all that's needed is for some sane values to be set 
during domain creation and things start working.  I believe Ryan had a 
patch that did this?

Regards,

Anthony Liguori

Re: [PATCH] paging_enabled and non-HVM guests

[Ryan Harper]

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

* Anthony Liguori <aliguori@us.ibm.com> [2006-05-09 15:15]:
> Hollis Blanchard wrote:
> >On Tue, 2006-05-09 at 13:31 +0200, Simon Kagstrom wrote:
> >  
> >>I had a problem with the GDB-server crashing on connections in
> >>xen_ptrace.c:map_domain_va(). paging_enabled() should only be checked
> >>for HVM guests, and the patch adds a check for that.
> >>
> >>Signed-off-by: Simon Kagstrom <ska@bth.se>
> >>
> >>diff -r 4501d60d6add tools/libxc/xc_ptrace.c
> >>--- a/tools/libxc/xc_ptrace.c	Tue May  9 09:57:05 2006
> >>+++ b/tools/libxc/xc_ptrace.c	Tue May  9 13:26:14 2006
> >>@@ -374,7 +374,7 @@
> >>     if (fetch_regs(xc_handle, cpu, NULL))
> >>         return NULL;
> >> 
> >>-    if (!paging_enabled(&ctxt[cpu])) { 
> >>+    if ( (ctxt[cpu].flags & VGCF_HVM_GUEST) && 
> >>!paging_enabled(&ctxt[cpu])) { static void * v;
> >>         unsigned long page;
> >>    
> >
> >I looked at this a couple weeks ago, and I think the real problem is
> >that the CR registers are never updated in Xen's vcpu structure, and so
> >xc_vcpu_getcontext() doesn't get them either. So Xen should be fixed; we
> >shouldn't add workarounds to userland.
> >  
> 
> I think that the CR registers are never changed during the life of a PV 
> domain.  I think all that's needed is for some sane values to be set 
> during domain creation and things start working.  I believe Ryan had a 
> patch that did this?

Well, I only hacked up enough to get things functional.  I didn't know
what cr4 should look like so I skipped that check.  Here are the two
patches I needed to debug paravirt 64-bit domUs via gdb.  I was going to
look into abstracting out the index into the page_array (it is only
needed on domains with shadow paging; non-shadow page tables
already have mfns).

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@us.ibm.com

[-- Attachment #2: fix_ptrace_paravirt64.patch --]
[-- Type: text/plain, Size: 1673 bytes --]

diff -r c4eead8a925b tools/libxc/xc_ptrace.c
--- a/tools/libxc/xc_ptrace.c	Sun Apr 16 14:41:31 2006
+++ b/tools/libxc/xc_ptrace.c	Thu Apr 20 22:44:35 2006
@@ -281,8 +281,10 @@
     uint64_t *l4, *l3, *l2, *l1;
     static void *v;
 
+#if 0
     if ((ctxt[cpu].ctrlreg[4] & 0x20) == 0 ) /* legacy ia32 mode */
         return map_domain_va_32(xc_handle, cpu, guest_va, perm);
+#endif
 
     l4 = xc_map_foreign_range( xc_handle, current_domid, PAGE_SIZE,
             PROT_READ, ctxt[cpu].ctrlreg[3] >> PAGE_SHIFT);
@@ -290,14 +292,14 @@
         return NULL;
 
     l3p = l4[l4_table_offset(va)] >> PAGE_SHIFT;
-    l3p = page_array[l3p];
+    //l3p = page_array[l3p];
     l3 = xc_map_foreign_range(xc_handle, current_domid, PAGE_SIZE, PROT_READ, l3p);
     munmap(l4, PAGE_SIZE);
     if ( l3 == NULL )
         return NULL;
 
     l2p = l3[l3_table_offset(va)] >> PAGE_SHIFT;
-    l2p = page_array[l2p];
+    //l2p = page_array[l2p];
     l2 = xc_map_foreign_range(xc_handle, current_domid, PAGE_SIZE, PROT_READ, l2p);
     munmap(l3, PAGE_SIZE);
     if ( l2 == NULL )
@@ -309,7 +311,7 @@
     if (l1e & 0x80)  { /* 2M pages */
         p = (l1p + l1_table_offset(va));
     } else { /* 4K pages */
-        l1p = page_array[l1p];
+        //l1p = page_array[l1p];
         l1 = xc_map_foreign_range(xc_handle, current_domid, PAGE_SIZE, perm, l1p);
         munmap(l2, PAGE_SIZE);
         if ( l1 == NULL )
@@ -317,7 +319,7 @@
 
         p = l1[l1_table_offset(va)] >> PAGE_SHIFT;
     }
-    p = page_array[p];
+    //p = page_array[p];
     if ( v != NULL )
         munmap(v, PAGE_SIZE);
     v = xc_map_foreign_range(xc_handle, current_domid, PAGE_SIZE, perm, p);

[-- Attachment #3: setup_sane_cr0.patch --]
[-- Type: text/plain, Size: 1290 bytes --]

diff -r c4eead8a925b tools/libxc/xc_linux_build.c
--- a/tools/libxc/xc_linux_build.c	Sun Apr 16 14:41:31 2006
+++ b/tools/libxc/xc_linux_build.c	Thu Apr 20 22:45:21 2006
@@ -45,6 +45,11 @@
 #ifdef __ia64__
 #define probe_aout9(image,image_size,load_funcs) 1
 #endif
+
+/* from xc_ptrace.h */
+#define X86_CR0_PE              0x00000001 /* Enable Protected Mode    (RW) */
+#define X86_CR0_PG              0x80000000 /* Paging                   (RW) */
+
 
 struct initrd_info {
     enum { INITRD_none, INITRD_file, INITRD_mem } type;
@@ -1174,6 +1179,8 @@
     ctxt->failsafe_callback_eip = 0;
     ctxt->syscall_callback_eip  = 0;
 #endif
+    /* set sane cr0 bits, protected and paging enabled  */
+    ctxt->ctrlreg[0] = X86_CR0_PE|X86_CR0_PG;
 #endif /* x86 */
 
     memset( &launch_op, 0, sizeof(launch_op) );
diff -r c4eead8a925b linux-2.6-xen-sparse/drivers/xen/core/smpboot.c
--- a/linux-2.6-xen-sparse/drivers/xen/core/smpboot.c	Sun Apr 16 14:41:31 2006
+++ b/linux-2.6-xen-sparse/drivers/xen/core/smpboot.c	Thu Apr 20 22:45:36 2006
@@ -216,6 +216,8 @@
 
 	ctxt.gs_base_kernel = (unsigned long)(cpu_pda(vcpu));
 #endif
+   /* set sane cr0 bits, protected and paging enabled  */
+   ctxt.ctrlreg[0] = 0x80000001;
 
 	BUG_ON(HYPERVISOR_vcpu_op(VCPUOP_initialise, vcpu, &ctxt));
 }

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

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

Re: [PATCH] paging_enabled and non-HVM guests

[Simon Kagstrom]

At Tue, 09 May 2006 14:53:46 -0500,
Hollis Blanchard wrote:
> 
> On Tue, 2006-05-09 at 13:31 +0200, Simon Kagstrom wrote:
> > I had a problem with the GDB-server crashing on connections in
> > xen_ptrace.c:map_domain_va(). paging_enabled() should only be checked
> > for HVM guests, and the patch adds a check for that.
> > 
> > Signed-off-by: Simon Kagstrom <ska@bth.se>
> > 
> > diff -r 4501d60d6add tools/libxc/xc_ptrace.c
> > --- a/tools/libxc/xc_ptrace.c	Tue May  9 09:57:05 2006
> > +++ b/tools/libxc/xc_ptrace.c	Tue May  9 13:26:14 2006
> > @@ -374,7 +374,7 @@
> >      if (fetch_regs(xc_handle, cpu, NULL))
> >          return NULL;
> >  
> > -    if (!paging_enabled(&ctxt[cpu])) { 
> > +    if ( (ctxt[cpu].flags & VGCF_HVM_GUEST) && !paging_enabled(&ctxt[cpu])) { 
> >          static void * v;
> >          unsigned long page;
> 
> I looked at this a couple weeks ago, and I think the real problem is
> that the CR registers are never updated in Xen's vcpu structure, and so
> xc_vcpu_getcontext() doesn't get them either. So Xen should be fixed; we
> shouldn't add workarounds to userland.

OK, I guess that sounds reasonable. Checks for HVM guests are done on
a number of other places as well in libxc, however. Is the support
meant to be transparent between HVM and PV guests?

I won't argue for an incorrect fix, but as the code is right now it
segmentation faults because the virtual address passed to

        page = page_array[va >> PAGE_SHIFT] << PAGE_SHIFT;

(with libxc incorrectly believing paging is disabled) accesses outside
of page_array. I'll keep the patch privately for now since gdbserver
breaks without it.

// Simon

Re: [PATCH] paging_enabled and non-HVM guests

[Hollis Blanchard]

On Wed, 2006-05-10 at 08:06 +0200, Simon Kagstrom wrote:
> 
> I won't argue for an incorrect fix, but as the code is right now it
> segmentation faults because the virtual address passed to
> 
>         page = page_array[va >> PAGE_SHIFT] << PAGE_SHIFT;
> 
> (with libxc incorrectly believing paging is disabled) accesses outside
> of page_array. I'll keep the patch privately for now since gdbserver
> breaks without it. 

Yes, and the reason is that page_array is supposed to be indexed with
*physical* addresses. The current code thinks that paging is disabled
(because CR0 is bad), assumes a virtual address is physical, and tries
to index into the array with it.

Pretty much every use of page_array needs to be abstracted so that it
does the right thing on both HVM and normal guests (normal guests will
have machine addresses in its page tables; HVM guests will have
physical). It's very unfortunate that the people who worked on this code
seem not to have tested or even thought about paravirtualized guests.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] paging_enabled and non-HVM guests

[Hollis Blanchard]

On Wed, 2006-05-10 at 09:51 -0500, Hollis Blanchard wrote:
> On Wed, 2006-05-10 at 08:06 +0200, Simon Kagstrom wrote:
> > 
> > I won't argue for an incorrect fix, but as the code is right now it
> > segmentation faults because the virtual address passed to
> > 
> >         page = page_array[va >> PAGE_SHIFT] << PAGE_SHIFT;
> > 
> > (with libxc incorrectly believing paging is disabled) accesses outside
> > of page_array. I'll keep the patch privately for now since gdbserver
> > breaks without it. 
> 
> Yes, and the reason is that page_array is supposed to be indexed with
> *physical* addresses. The current code thinks that paging is disabled
> (because CR0 is bad), assumes a virtual address is physical, and tries
> to index into the array with it.
> 
> Pretty much every use of page_array needs to be abstracted so that it
> does the right thing on both HVM and normal guests (normal guests will
> have machine addresses in its page tables; HVM guests will have
> physical). It's very unfortunate that the people who worked on this
> code seem not to have tested or even thought about paravirtualized
> guests.

To elaborate on my previous mail, it's not just CR0/paging at fault. For
example, this use of page_array:
    ...
    l3p = l4[l4_table_offset(va)] >> PAGE_SHIFT;
    l3p = page_array[l3p];
    ...
in map_domain_va_64() is obviously incorrect for paravirtualized
domains.

Also, I noticed there's another place that already tests VGCF_HVM_GUEST
before paging_enabled(), which I guess is where you got the idea for
your patch.

Simon, would you care to submit the more complete patch?

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] paging_enabled and non-HVM guests

[Simon Kagstrom]

At Wed, 10 May 2006 10:42:34 -0500,
Hollis Blanchard wrote:
> To elaborate on my previous mail, it's not just CR0/paging at fault. For
> example, this use of page_array:
>     ...
>     l3p = l4[l4_table_offset(va)] >> PAGE_SHIFT;
>     l3p = page_array[l3p];
>     ...
> in map_domain_va_64() is obviously incorrect for paravirtualized
> domains.
> 
> Also, I noticed there's another place that already tests VGCF_HVM_GUEST
> before paging_enabled(), which I guess is where you got the idea for
> your patch.

Actually, I found that afterwards :-).

> Simon, would you care to submit the more complete patch?

I'll see what I can do in the next few days!

// Simon