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

2015-02-18 Thread Jan Beulich 
 On 17.02.15 at 19:32, tamas.leng...@zentific.com wrote:
 On Tue, Feb 17, 2015 at 3:31 PM, Jan Beulich jbeul...@suse.com wrote:
 On 13.02.15 at 17:33, tamas.leng...@zentific.com wrote:
 @@ -611,13 +611,22 @@ int vm_event_domctl(struct domain *d, 
 xen_domctl_vm_event_op_t *vec,
  }
  break;

 -case XEN_VM_EVENT_PAGING_DISABLE:
 +case XEN_VM_EVENT_DISABLE:
  {
  if ( ved-ring_page )
  rc = vm_event_disable(d, ved);
  }
  break;

 +case XEN_VM_EVENT_RESUME:
 +{
 +if ( ved-ring_page )
 +vm_event_resume(d, ved);
 +else
 +rc = -ENODEV;
 +}
 +break;

 Stray braces again.
 
 Ack.
 

 I also find it confusing that the same set of changes repeats three
 times here - is that an indication of a problem with an earlier patch?
 
 No it's not. There are three rings vm_event can use, thus three rings
 that can be resumed.

But if the code ends up being almost identical, this loudly calls for
consolidation into e.g. a helper function.

Jan


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

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

2015-02-18 Thread Tamas K Lengyel 
On Wed Feb 18 2015 10:31:06 AM CET, Jan Beulich jbeul...@suse.com wrote:

On 17.02.15 at 19:32, tamas.leng...@zentific.com wrote:
  On Tue, Feb 17, 2015 at 3:31 PM, Jan Beulich jbeul...@suse.com wrote:
  On 13.02.15 at 17:33, tamas.leng...@zentific.com wrote:
@@ -611,13 +611,22 @@ int vm_event_domctl(struct domain *d, 
  xen_domctl_vm_event_op_t *vec,
}
break;

-               case XEN_VM_EVENT_PAGING_DISABLE:
+               case XEN_VM_EVENT_DISABLE:
{
if ( ved-ring_page )
rc = vm_event_disable(d, ved);
}
break;

+               case XEN_VM_EVENT_RESUME:
+               {
+                       if ( ved-ring_page )
+                               vm_event_resume(d, ved);
+                       else
+                               rc = -ENODEV;
+               }
+               break;
   
   Stray braces again.
  
  Ack.
  
   
   I also find it confusing that the same set of changes repeats three
   times here - is that an indication of a problem with an earlier
   patch?
  
  No it's not. There are three rings vm_event can use, thus three rings
  that can be resumed.
 
 But if the code ends up being almost identical, this loudly calls for
 consolidation into e.g. a helper function.
 
 Jan

That could be done but considering we are talking about only couple lines of 
code, I'm not sure that would improve readability by much.

I think the question I raised earlier, whether we need the resume option to the 
domctl to begin with is what needs discussion. IMHO the event channel method is 
enough so maybe I'll just turn this patch into deprecating the resume options 
in the memops.

Thanks,
Tamas

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

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

2015-02-17 Thread Tamas K Lengyel 
On Tue, Feb 17, 2015 at 3:31 PM, Jan Beulich jbeul...@suse.com wrote:
 On 13.02.15 at 17:33, tamas.leng...@zentific.com wrote:
 @@ -611,13 +611,22 @@ int vm_event_domctl(struct domain *d, 
 xen_domctl_vm_event_op_t *vec,
  }
  break;

 -case XEN_VM_EVENT_PAGING_DISABLE:
 +case XEN_VM_EVENT_DISABLE:
  {
  if ( ved-ring_page )
  rc = vm_event_disable(d, ved);
  }
  break;

 +case XEN_VM_EVENT_RESUME:
 +{
 +if ( ved-ring_page )
 +vm_event_resume(d, ved);
 +else
 +rc = -ENODEV;
 +}
 +break;

 Stray braces again.

Ack.


 I also find it confusing that the same set of changes repeats three
 times here - is that an indication of a problem with an earlier patch?

No it's not. There are three rings vm_event can use, thus three rings
that can be resumed.


 Jan


Thanks,
Tamas

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

[Xen-devel] [PATCH V5 12/12] xen/vm_event: Add RESUME option to vm_event_op domctl

2015-02-13 Thread Tamas K Lengyel 
Thus far mem_access and mem_sharing memops had been able to signal
to Xen to start pulling responses off the corresponding rings. In this patch
we retire these memops and add them to the option to the vm_event_op domctl.

The vm_event_op domctl suboptions are the same for each ring thus we
consolidate them into XEN_VM_EVENT_ENABLE/DISABLE/RESUME.

As part of this patch in libxc we also rename the mem_access_enable/disable
functions to monitor_enable/disable and move them into xc_monitor.c.

Signed-off-by: Tamas K Lengyel tamas.leng...@zentific.com
Acked-by: Wei Liu wei.l...@citrix.com
---
 tools/libxc/include/xenctrl.h   | 22 ++---
 tools/libxc/xc_mem_access.c | 25 
 tools/libxc/xc_mem_paging.c | 12 ++--
 tools/libxc/xc_memshr.c | 15 ++
 tools/libxc/xc_monitor.c| 22 +
 tools/libxc/xc_vm_event.c   |  6 +++---
 tools/tests/xen-access/xen-access.c | 10 +-
 xen/arch/x86/mm/mem_sharing.c   |  9 -
 xen/common/mem_access.c |  9 -
 xen/common/vm_event.c   | 39 +++--
 xen/include/public/domctl.h | 32 ++
 xen/include/public/memory.h | 16 +++
 12 files changed, 112 insertions(+), 105 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 3324132..3042e98 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2269,6 +2269,7 @@ int xc_tmem_restore_extra(xc_interface *xch, int dom, int 
fd);
  */
 int xc_mem_paging_enable(xc_interface *xch, domid_t domain_id, uint32_t *port);
 int xc_mem_paging_disable(xc_interface *xch, domid_t domain_id);
+int xc_mem_paging_resume(xc_interface *xch, domid_t domain_id);
 int xc_mem_paging_nominate(xc_interface *xch, domid_t domain_id,
unsigned long gfn);
 int xc_mem_paging_evict(xc_interface *xch, domid_t domain_id, unsigned long 
gfn);
@@ -2282,17 +2283,6 @@ int xc_mem_paging_load(xc_interface *xch, domid_t 
domain_id,
  */
 
 /*
- * Enables mem_access and returns the mapped ring page.
- * Will return NULL on error.
- * Caller has to unmap this page when done.
- */
-void *xc_mem_access_enable(xc_interface *xch, domid_t domain_id, uint32_t 
*port);
-void *xc_mem_access_enable_introspection(xc_interface *xch, domid_t domain_id,
- uint32_t *port);
-int xc_mem_access_disable(xc_interface *xch, domid_t domain_id);
-int xc_mem_access_resume(xc_interface *xch, domid_t domain_id);
-
-/*
  * Set a range of memory to a specific access.
  * Allowed types are XENMEM_access_default, XENMEM_access_n, any combination of
  * XENMEM_access_ + (rwx), and XENMEM_access_rx2rw
@@ -2309,7 +2299,17 @@ int xc_get_mem_access(xc_interface *xch, domid_t 
domain_id,
 
 /***
  * Monitor control operations.
+ *
+ * Enables the VM event monitor ring and returns the mapped ring page.
+ * This ring is used to deliver mem_access events, as well a set of additional
+ * events that can be enabled with the xc_monitor_* functions.
+ *
+ * Will return NULL on error.
+ * Caller has to unmap this page when done.
  */
+void *xc_monitor_enable(xc_interface *xch, domid_t domain_id, uint32_t *port);
+int xc_monitor_disable(xc_interface *xch, domid_t domain_id);
+int xc_monitor_resume(xc_interface *xch, domid_t domain_id);
 int xc_monitor_mov_to_cr0(xc_interface *xch, domid_t domain_id,
   unsigned int op, unsigned int sync,
   unsigned int onchangeonly);
diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
index 37e776c..7050692 100644
--- a/tools/libxc/xc_mem_access.c
+++ b/tools/libxc/xc_mem_access.c
@@ -24,31 +24,6 @@
 #include xc_private.h
 #include xen/memory.h
 
-void *xc_mem_access_enable(xc_interface *xch, domid_t domain_id, uint32_t 
*port)
-{
-return xc_vm_event_enable(xch, domain_id, HVM_PARAM_MONITOR_RING_PFN,
-  port);
-}
-
-int xc_mem_access_disable(xc_interface *xch, domid_t domain_id)
-{
-return xc_vm_event_control(xch, domain_id,
-   XEN_VM_EVENT_MONITOR_DISABLE,
-   XEN_DOMCTL_VM_EVENT_OP_MONITOR,
-   NULL);
-}
-
-int xc_mem_access_resume(xc_interface *xch, domid_t domain_id)
-{
-xen_mem_access_op_t mao =
-{
-.op= XENMEM_access_op_resume,
-.domid = domain_id
-};
-
-return do_memory_op(xch, XENMEM_access_op, mao, sizeof(mao));
-}
-
 int xc_set_mem_access(xc_interface *xch,
   domid_t domain_id,
   xenmem_access_t access,
diff --git a/tools/libxc/xc_mem_paging.c b/tools/libxc/xc_mem_paging.c
index 083ab60..76e0c05 100644
--- a/tools/libxc/xc_mem_paging.c
+++ b/tools/libxc/xc_mem_paging.c
@@ -48,7 +48,7 @@ int xc_mem_paging_enable(xc_interface *xch,

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

2015-02-13 Thread Tamas K Lengyel 
On Fri, Feb 13, 2015 at 10:44 PM, Andrew Cooper
andrew.coop...@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_op21
 -#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_resume3
 -#define XENMEM_sharing_op_debug_gfn 4
 -#define XENMEM_sharing_op_debug_mfn 5
 -#define XENMEM_sharing_op_debug_gref6
 -#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_gref5
 +#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

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

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

2015-02-13 Thread Andrew Cooper 
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_op21
 -#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_resume3
 -#define XENMEM_sharing_op_debug_gfn 4
 -#define XENMEM_sharing_op_debug_mfn 5
 -#define XENMEM_sharing_op_debug_gref6
 -#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_gref5
 +#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 also notice that the structures have 32bit gfns baked into them, which
is going to need to be fixed at some point.

~Andrew


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