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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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 <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.

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel