Re: [PATCH 1/9] tools/libxc: Consistent usage of xc_vm_event_* interface

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 30/05/2019 07:18, Petre Pircalabu wrote:
> Modified xc_mem_paging_enable to use directly xc_vm_event_enable and
> moved the ring_page handling from client to libxc (xenpaging).
>
> Restricted vm_event_control usage only to simplest domctls which do
> not expect any return values and change xc_vm_event_enable to call do_domctl
> directly.
>
> Removed xc_memshr_ring_enable/disable and xc_memshr_domain_resume.
>
> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
> ---
>  tools/libxc/include/xenctrl.h | 49 +--------------------------------
>  tools/libxc/xc_mem_paging.c   | 23 +++++-----------
>  tools/libxc/xc_memshr.c       | 34 -----------------------
>  tools/libxc/xc_monitor.c      | 31 +++++++++++++++++----
>  tools/libxc/xc_private.h      |  2 +-
>  tools/libxc/xc_vm_event.c     | 64 ++++++++++++++++---------------------------
>  tools/xenpaging/xenpaging.c   | 42 +++-------------------------

So, the diffstat here is very impressive, and judging by the delta, its
all about not opencoding the use of the HVM_PARAM interface.  Overall,
this is clearly a good thing.

However, it is quite difficult to follow exactly what is going on.

AFAICT, this wants splitting into $N patches.

One patch should refactor xc_mem_paging_enable() to use
xc_vm_event_enable(), with the associated xenpaging cleanup.

One patch should be the deletion of xc_memshr_* on its own, because
AFAICT it is an isolated change.  It also needs some justification, even
if it is "interface is unused and/or redundant with $X".

One patch (alone) should be the repositioning of the domain_pause()
calls.  This does certainly look like a vast improvement WRT error
handling in xc_vm_event_enable(), but you've definitely changed the API
(insofar as the expectation that the caller has paused the domain) goes,
and its not at all obvious that this change is safe.  It may very well
be, but you need to convince people as to why in the commit message.


I still haven't figured out what the purpose behind dropping the port
parameter from xc_vm_event_control() is.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel