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 20:02:41 -0500 Message-ID: <52CCA3B1.7070602@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> <1389088937.31766.107.camel@kazak.uk.xensource.com> <52CC2A2F.7010700@terremark.com> <20140107150148.4cbf1a73@mantra.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140107150148.4cbf1a73@mantra.us.oracle.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: Mukesh Rathor , Don Slutz Cc: Keir Fraser , Ian Campbell , Stefano Stabellini , George Dunlap , Ian Jackson , xen-devel@lists.xen.org, Jan Beulich List-Id: xen-devel@lists.xenproject.org On 01/07/14 18:01, Mukesh Rathor wrote: > On Tue, 07 Jan 2014 11:24:15 -0500 > Don Slutz wrote: > >> 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 wrote: >>>>> >>>>>> The gdbsx code expects that domctl->u.gdbsx_guest_memio.remain is >>>>>> returned. >>>>>> > ..... > >> 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. > No, the partial success is relevant in other cases, like EAGAIN, > but not EFAULT. If we make it pre-emptible in future to return > EAGAIN, we'd need to make sure remain was honored. Again, think of it > this way, if the first copyin failed, remain would have not been > initialized. > > So, because now the only cause of unfinished copy from dbg_rw_mem is > EFAULT, we should leave above xen code alone, and just change gdbsx. > > thanks > mukesh Since it did not look like we would get to an agreement soon on this, I have sent out v2 series. Not at all clear this patch should be in 4.4.0 (domctl api change, high risk, no clear use case (as far as I know gdb does not do anything with partial success)). On this topic; you seem to be overlooking the page crossing case. Using the info that page 1f is good and 20 is bad, a domctl request for 1ffff for 2 bytes would call on dbg_rw_mem(), dbg_rw_guest_mem() which calculate pagecnt == 1, get a valid mfn and return that byte. The 2nd time pagecnt is also 1, but we get INVALID_MFN, so dbg_rw_guest_mem(0 returns 1. dbg_rw_mem(0 also returns 1. gdbsx_guest_mem_io() returns -EFAULT so no copyback. At this point of the 2 requested byte, 1 byte is valid and 1 is not. Since copyback is not done, remain is 0. So the caller get the error and does not have this "partial success" information. The 1st version of this patch I proposed, the copyback is done, so the caller gets remain == 1 and the valid byte and an error. The 2nd version of this patch (which has now been tested) sets remian == 1, the valid byte, and no error. Hope this helps. -Don Slutz