Re: [Xen-devel] [RFC PATCH V3 05/12] xen: Introduce vm_event

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Tue, Feb 3, 2015 at 4:54 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 29.01.15 at 22:46, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> --- a/docs/misc/xsm-flask.txt
>> +++ b/docs/misc/xsm-flask.txt
>> @@ -87,6 +87,7 @@ __HYPERVISOR_domctl (xen/include/public/domctl.h)
>>   * XEN_DOMCTL_set_machine_address_size
>>   * XEN_DOMCTL_debug_op
>>   * XEN_DOMCTL_gethvmcontext_partial
>> + * XEN_DOMCTL_vm_event_op
>>   * XEN_DOMCTL_mem_event_op
>>   * XEN_DOMCTL_mem_sharing_op
>>   * XEN_DOMCTL_setvcpuextstate
>
> No - no additions here. You don't define XEN_DOMCTL_vm_event_op
> in this patch anyway. Once you rename the other one, replacing it
> here will be okay.
>
>> --- /dev/null
>> +++ b/xen/common/vm_event.c
>> @@ -0,0 +1,739 @@
>
> Please clarify in the patch description whether this (and perhaps
> other) copied or cloned code is really just a plain copy with some
> renaming, or whether there are any other changes. Reviewing this
> as a non-renaming change isn't time well spent in the former case.

Ack, this code is identical to mem_event beside the name.

>
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -835,6 +835,84 @@ typedef struct xen_domctl_mem_event_op 
>> xen_domctl_mem_event_op_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_mem_event_op_t);
>>
>>  /*
>> + * VM event operations
>> + */
>> +
>> +/* XEN_DOMCTL_vm_event_op */
>> +
>> +/*
>> + * Domain memory paging
>> + * Page memory in and out.
>> + * Domctl interface to set up and tear down the
>> + * pager<->hypervisor interface. Use XENMEM_paging_op*
>> + * to perform per-page operations.
>> + *
>> + * The XEN_DOMCTL_MEM_EVENT_OP_PAGING_ENABLE domctl returns several
>> + * non-standard error codes to indicate why paging could not be enabled:
>> + * ENODEV - host lacks HAP support (EPT/NPT) or HAP is disabled in guest
>> + * EMLINK - guest has iommu passthrough enabled
>> + * EXDEV  - guest has PoD enabled
>> + * EBUSY  - guest has or had paging enabled, ring buffer still active
>> + */
>> +#define XEN_DOMCTL_VM_EVENT_OP_PAGING            1
>> +
>> +#define XEN_DOMCTL_VM_EVENT_OP_PAGING_ENABLE     0
>> +#define XEN_DOMCTL_VM_EVENT_OP_PAGING_DISABLE    1
>> +
>> +/*
>> + * Monitor permissions.
>> + *
>> + * As with paging, use the domctl for teardown/setup of the
>> + * helper<->hypervisor interface.
>> + *
>> + * There are HVM hypercalls to set the per-page access permissions of every
>> + * page in a domain.  When one of these permissions--independent, read,
>> + * write, and execute--is violated, the VCPU is paused and a memory event
>> + * is sent with what happened.  (See public/vm_event.h) .
>> + *
>> + * The memory event handler can then resume the VCPU and redo the access
>> + * with a XENMEM_access_op_resume hypercall.
>> + *
>> + * The XEN_DOMCTL_VM_EVENT_OP_MONITOR_ENABLE domctl returns several
>> + * non-standard error codes to indicate why access could not be enabled:
>> + * EBUSY  - guest has or had access enabled, ring buffer still active
>> + */
>> +#define XEN_DOMCTL_VM_EVENT_OP_MONITOR                        2
>> +
>> +#define XEN_DOMCTL_VM_EVENT_OP_MONITOR_ENABLE                 0
>> +#define XEN_DOMCTL_VM_EVENT_OP_MONITOR_DISABLE                1
>> +#define XEN_DOMCTL_VM_EVENT_OP_MONITOR_ENABLE_INTROSPECTION   2
>> +
>> +/*
>> + * Sharing ENOMEM helper.
>> + *
>> + * As with paging, use the domctl for teardown/setup of the
>> + * helper<->hypervisor interface.
>> + *
>> + * If setup, this ring is used to communicate failed allocations
>> + * in the unshare path. XENMEM_sharing_op_resume is used to wake up
>> + * vcpus that could not unshare.
>> + *
>> + * Note that shring can be turned on (as per the domctl below)
>> + * *without* this ring being setup.
>> + */
>> +#define XEN_DOMCTL_VM_EVENT_OP_SHARING           3
>> +
>> +#define XEN_DOMCTL_VM_EVENT_OP_SHARING_ENABLE    0
>> +#define XEN_DOMCTL_VM_EVENT_OP_SHARING_DISABLE   1
>> +
>> +/* Use for teardown/setup of helper<->hypervisor interface for paging,
>> + * access and sharing.*/
>> +struct xen_domctl_vm_event_op {
>> +    uint32_t       op;           /* XEN_DOMCTL_VM_EVENT_OP_*_* */
>> +    uint32_t       mode;         /* XEN_DOMCTL_VM_EVENT_OP_* */
>> +
>> +    uint32_t port;              /* OUT: event channel for ring */
>> +};
>> +typedef struct xen_domctl_vm_event_op xen_domctl_vm_event_op_t;
>> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_vm_event_op_t);
>
> I doubt it is the best approach to do this via add and remove steps,
> rather than just a single renaming step (where likely much on the
> commentary would remain untouched). Or if staying with that model,
> maybe interleaving the additions with the to be replaced code might
> be a workable approach.
>
>> @@ -216,6 +217,8 @@ struct vcpu
>>
>>      /* VCPU paused for mem_event replies. */
>>      atomic_t         mem_event_pause_count;
>> +    /* VCPU paused for vm_event replies. */
>> +    atomic_t         vm_event_pause_count;
>
> Or, to avoid odd changes like this, don't hook up the new source file(s)
> to the make system yet.
>
> Jan

The last one I think is the most workable, I'll only hook vm_event
into the build system when it is getting used (next patch in the
series). That avoids a bunch of weird additions we see here.

Thanks,
Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel