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

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 01/07/14 05:02, Ian Campbell wrote:
> On Tue, 2014-01-07 at 10:00 +0000, 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.
> Thinking about this for a second longer -- how does this interface deal
> with partial success?
>
> It seems natural to me for it to return an error and also update
> ->remain, but perhaps you have a different scheme in mind?
I had assumed that this patch (which is not need to "fix" the bugs I found), 
was to be dropped in v2.  However I will agree that currently there is no way to know 
about partial success.  The untested change:

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index ef6c140..0add07e 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -43,7 +43,7 @@ static int gdbsx_guest_mem_io(
     iop->remain = dbg_rw_mem(
         (dbgva_t)iop->gva, (dbgbyte_t *)l_uva, iop->len, domid,
         iop->gwr, iop->pgd3val);
-    return (iop->remain ? -EFAULT : 0);
+    return 0;
 }

 long arch_do_domctl(


Would appear to allow partial success to be reported and also meet with remain 
not to be looked at with an error.

   -Don Slutz

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