Re: [PATCH V5 12/12] xen/vm_event: Add RESUME option to vm_event_op domctl

[Tamas K Lengyel <tamas.lengyel@zentific.com>]

On Fri, Feb 13, 2015 at 10:44 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 13/02/15 16:33, Tamas K Lengyel wrote:
>> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
>> index 9c41f5d..334f60e 100644
>> --- a/xen/include/public/memory.h
>> +++ b/xen/include/public/memory.h
>> @@ -386,9 +386,8 @@ typedef struct xen_mem_paging_op xen_mem_paging_op_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_mem_paging_op_t);
>>
>>  #define XENMEM_access_op                    21
>> -#define XENMEM_access_op_resume             0
>> -#define XENMEM_access_op_set_access         1
>> -#define XENMEM_access_op_get_access         2
>> +#define XENMEM_access_op_set_access         0
>> +#define XENMEM_access_op_get_access         1
>>
>>  typedef enum {
>>      XENMEM_access_n,
>> @@ -439,12 +438,11 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_access_op_t);
>>  #define XENMEM_sharing_op_nominate_gfn      0
>>  #define XENMEM_sharing_op_nominate_gref     1
>>  #define XENMEM_sharing_op_share             2
>> -#define XENMEM_sharing_op_resume            3
>> -#define XENMEM_sharing_op_debug_gfn         4
>> -#define XENMEM_sharing_op_debug_mfn         5
>> -#define XENMEM_sharing_op_debug_gref        6
>> -#define XENMEM_sharing_op_add_physmap       7
>> -#define XENMEM_sharing_op_audit             8
>> +#define XENMEM_sharing_op_debug_gfn         3
>> +#define XENMEM_sharing_op_debug_mfn         4
>> +#define XENMEM_sharing_op_debug_gref        5
>> +#define XENMEM_sharing_op_add_physmap       6
>> +#define XENMEM_sharing_op_audit             7
>
> Hmm - I am not certain what our API/ABI constraints are in this case.
> MEMOPs are not toolstack exclusive so lack a formal interface version,
> but these bits are inside an #ifdef __XEN_TOOLS__
>
> If we work on the assumption that there are not currently any
> out-of-tree tools trying to use this interface, then its probably ok to
> break the ABI now and get it right, perhaps even including an ABI
> version parameter in the op itself if we wish to be flexible going forwards.

I think there are some out-of-tree tools that make use if these.
Andres did imply couple years ago that he works with proprietary
software that uses the sharing system but he couldn't really tell much
about it.

>
> I also notice that the structures have 32bit gfns baked into them, which
> is going to need to be fixed at some point.

I think that was just me in the first patch in the series..

>
> ~Andrew
>

On a more general note, I'm not actually sure if we need this resume
option on the domctl. Currently there are two methods ending up
signaling to Xen to process the responses from the ring - one with the
memops/domctl here, and one via event channels. I think the event
channel method is sufficient so I'm not actually sure why these resume
memops existed in the first place. It might be sufficient to just
deprecate them altogether.

Thanks,
Tamas