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

[Petre Ovidiu PIRCALABU <ppircalabu@xxxxxxxxxxxxxxx>]

On Fri, 2019-05-31 at 16:01 -0700, Andrew Cooper wrote:
> 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@xxxxxxxxxxxxxxx>
> > ---
> >  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

The main reason for this patch was to use an uniform calling convention
for all xc_vm_event wrappers. 
However, at this stage I think it's best to drop it altogheter as it's
not a requirement for the new vm_event interface (new domctls are used
along with their own separate wrappers).

Many thanks for your support,
Petre


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