From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback. Date: Tue, 07 Jan 2014 11:26:36 -0500 Message-ID: <52CC2ABC.3040505@terremark.com> References: <1388857936-664-1-git-send-email-dslutz@verizon.com> <1388857936-664-5-git-send-email-dslutz@verizon.com> <20140106175349.6cbd190b@mantra.us.oracle.com> <1389088824.31766.105.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1389088824.31766.105.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , Mukesh Rathor Cc: Keir Fraser , Stefano Stabellini , George Dunlap , Ian Jackson , Don Slutz , xen-devel@lists.xen.org, Jan Beulich List-Id: xen-devel@lists.xenproject.org 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 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 >>> --- >>> 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 >