[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 01/07/14 18:01, Mukesh Rathor wrote:
On Tue, 07 Jan 2014 11:24:15 -0500
Don Slutz <dslutz@xxxxxxxxxxx> 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 <dslutz@xxxxxxxxxxx> 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




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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.