On 04/06/2019 15:23, Petre Ovidiu PIRCALABU wrote: > 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 <ppircal...@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 > The main reason for this patch was to use an uniform calling convention > for all xc_vm_event wrappers.
The cleanup is a great, but it needs to be in finer grained patches so it can be followed more easily. > 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). See patch 8 for the discussion on why a new domctl probably isn't the right course of action. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
