Simon Kagstrom
2006-May-09 11:31 UTC
[Xen-devel] [PATCH] paging_enabled and non-HVM guests
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;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Hollis Blanchard
2006-May-09 19:53 UTC
Re: [Xen-devel] [PATCH] paging_enabled and non-HVM guests
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Anthony Liguori
2006-May-09 20:14 UTC
Re: [Xen-devel] [PATCH] paging_enabled and non-HVM guests
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ryan Harper
2006-May-09 20:26 UTC
Re: [Xen-devel] [PATCH] paging_enabled and non-HVM guests
* 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Simon Kagstrom
2006-May-10 06:06 UTC
Re: [Xen-devel] [PATCH] paging_enabled and non-HVM guests
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hollis Blanchard
2006-May-10 14:51 UTC
Re: [Xen-devel] [PATCH] paging_enabled and non-HVM guests
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hollis Blanchard
2006-May-10 15:42 UTC
Re: [Xen-devel] [PATCH] paging_enabled and non-HVM guests
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Simon Kagstrom
2006-May-10 18:35 UTC
Re: [Xen-devel] [PATCH] paging_enabled and non-HVM guests
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel