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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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