[Xen-devel] [BUGFIX][PATCH v2 0/5] gdbsx: fix 3 bugs

[Don Slutz <dslutz@xxxxxxxxxxx>]

Changes v1 to v2:
  From Konrad Rzeszutek Wilk and Ian Campbell:

    ??

  Split out emacs local variables add to it's own new patch (number 1).

  From Andrew Cooper:

    What does matter is that the caller of dbg_hvm_va2mfn() should
    not have to cleanup a reference taken when it returns an error.

  So use his version of the change.

  From Ian Campbell:

    In all three cases what is missing is the "why" and the
    appropriate analysis from
    
http://wiki.xen.org/wiki/Xen_Roadmap/4.4#Exception_guidelines_for_after_the_code_freeze
    i.e. what is the impact of the bug (i..e what are the advantages
    of the fix) and what are the risks of this changing causing
    further breakage? I'm not really in a position to evaluate the
    risk of a change in gdbsx, so someone needs to tell me.

    I think given that gdbsx is a somewhat "peripheral" bit of code
    and that it is targeted at developers (who might be better able
    to tolerate any resulting issues and more able to apply
    subsequent fixups than regular users) we can accept a larger
    risk than we would with a change to the hypervisor itself etc
    (that's assuming that all of these changes cannot potentially
    impact non-debugger usecases which I expect is the case from the
    function names but I would like to see confirmed).
 
  My take on this below.

  From Mukesh Rathor:

    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:

  Dropped old patch 4, Added new patch 5.


Freeze:

  The benefit of this series is that the hypervisor stops calling
  panic (debug=y) or hanging (debug=n).  Also a person using gdbsx
  is not seeing random heap data of gdbsx's heap instead of guest
  data.

  The risk is that gdbsx does something new wrong.

  My understanding is that all the changes here only effect gdbsx
  and so very limited in scope.

Release manager requests:
  patch 1 and 3 are optional for 4.4.0.
  patch 2 should be in 4.4.0
  patch 4 and 5 would be good to be in 4.4.0

While tracking down a bug in seabios/grub I found the bug in patch
2.

There are 2 ways that gfn will not be INVALID_GFN and yet mfn will
be INVALID_MFN.

  1) p2m_is_readonly(gfntype) and writing memory.
  2) the requested vaddr does not exist.

This may only be an issue for a HVM guest that is in real mode
(I.E. no page tables).

Patch 3 is debug logging that was used to find the 2nd way.


Don Slutz (5):
  Add Emacs local variables to source files.
  dbg_rw_guest_mem: need to call put_gfn in error path.
  dbg_rw_guest_mem: Conditionally enable debug log output
  xg_read_mem: Report on error.
  xg_main: If XEN_DOMCTL_gdbsx_guestmemio fails then force error.

 tools/debugger/gdbsx/xg/xg_main.c | 23 ++++++++++---
 xen/arch/x86/debug.c              | 71 +++++++++++++++++++++++----------------
 2 files changed, 61 insertions(+), 33 deletions(-)

-- 
1.8.4


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