Re: [Xen-devel] [BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 01/07/14 05:00, Ian Campbell wrote:
> On Mon, 2014-01-06 at 17:53 -0800, Mukesh Rathor wrote:
> > On Sat,  4 Jan 2014 12:52:16 -0500
> > Don Slutz <dslutz@xxxxxxxxxxx> wrote:
> >
> > > The gdbsx code expects that domctl->u.gdbsx_guest_memio.remain is
> > > returned.
> > >
> > > Without this gdb does not report an error.
> > >
> > > With this patch and using a 1G hvm domU:
> > >
> > > (gdb) x/1xh 0x6ae9168b
> > > 0x6ae9168b:     Cannot access memory at address 0x6ae9168b
> > >
> > > Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
> > > ---
> > >   xen/arch/x86/domctl.c | 3 +--
> > >   1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> > > index ef6c140..4aa751f 100644
> > > --- a/xen/arch/x86/domctl.c
> > > +++ b/xen/arch/x86/domctl.c
> > > @@ -997,8 +997,7 @@ long arch_do_domctl(
> > >               domctl->u.gdbsx_guest_memio.len;
> > >   
> > >           ret = gdbsx_guest_mem_io(domctl->domain,
> > > &domctl->u.gdbsx_guest_memio);
> > > -        if ( !ret )
> > > -           copyback = 1;
> > > +        copyback = 1;
> > >       }
> > >       break;
> > >   
> > Ooopsy... my thought was that an application should not even look at
> > remain if the hcall/syscall failed, but forgot when writing the
> > gdbsx itself :). Think of it this way, if the call didn't even make it to
> > xen, and some reason the ioctl returned non-zero rc, then remain would
> > still be zero. So I think we should fix gdbsx instead of here:
> >
> > xg_write_mem():
> >      if ((rc=_domctl_hcall(XEN_DOMCTL_gdbsx_guestmemio, frombuf, buflen)))
> >      {
> >          XGERR("ERROR: failed to write %d bytes. errno:%d rc:%d\n",
> >                 iop->remain, errno, rc);
> Isn't this still using iop->remain on failure which is what you say
> shouldn't be done?
I agree, which is why I took it as a guide line.  What I have tested with is:

diff --git a/tools/debugger/gdbsx/xg/xg_main.c 
b/tools/debugger/gdbsx/xg/xg_main.c
index 3b2a285..53a0ce1 100644
--- a/tools/debugger/gdbsx/xg/xg_main.c
+++ b/tools/debugger/gdbsx/xg/xg_main.c
@@ -787,8 +787,11 @@ xg_read_mem(uint64_t guestva, char *tobuf, int tobuf_len, 
uint64_t pgd3val)
     iop->gwr = 0;       /* not writing to guest */

     if ( (rc = _domctl_hcall(XEN_DOMCTL_gdbsx_guestmemio, tobuf, tobuf_len)) )
-        XGTRC("ERROR: failed to read %d bytes. errno:%d rc:%d\n",
-              iop->remain, errno, rc);
+    {
+        XGTRC("ERROR: failed to read bytes. errno:%d rc:%d\n",
+              errno, rc);
+        return tobuf_len;
+    }

     for(i=0; i < XGMIN(8, tobuf_len); u.buf8[i]=tobuf[i], i++);
     XGTRC("X:remain:%d buf8:0x%llx\n", iop->remain, u.llbuf8);
@@ -818,8 +821,11 @@ xg_write_mem(uint64_t guestva, char *frombuf, int buflen, 
uint64_t pgd3val)
     iop->gwr = 1;       /* writing to guest */

     if ((rc=_domctl_hcall(XEN_DOMCTL_gdbsx_guestmemio, frombuf, buflen)))
-        XGERR("ERROR: failed to write %d bytes. errno:%d rc:%d\n",
-              iop->remain, errno, rc);
+    {
+        XGERR("ERROR: failed to write bytes to %llx. errno:%d rc:%d\n",
+              guestva, errno, rc);
+        return buflen;
+    }
     return iop->remain;
 }


works fine and I plan it to be part of v2.
    -Don


> >          return iop->len;
> >      }
> >
> > Similarly in xg_read_mem().
> >
> > Hope that makes sense. Don't mean to create work for you for my mistake,
> > so if you don't have time, I can submit a patch for this too.
> >
> > thanks
> > Mukesh

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel