Re: [Xen-devel] [PATCH V5 09/12] xen/vm_event: Decouple vm_event and mem_access.

2015-02-17 Thread Jan Beulich 
 On 13.02.15 at 17:33, tamas.leng...@zentific.com wrote:
 @@ -1293,56 +1293,30 @@ int p2m_mem_paging_prep(struct domain *d, unsigned 
 long gfn, uint64_t buffer)
   *
   * If the gfn was dropped the vcpu needs to be unpaused.
   */
 -void p2m_mem_paging_resume(struct domain *d)
 +
 +void p2m_mem_paging_resume(struct domain *d, vm_event_response_t *rsp)
  {
  struct p2m_domain *p2m = p2m_get_hostp2m(d);
 -vm_event_response_t rsp;
  p2m_type_t p2mt;
  p2m_access_t a;
  mfn_t mfn;
  
 -/* Pull all responses off the ring */
 -while( vm_event_get_response(d, d-vm_event-paging, rsp) )
 +/* Fix p2m entry if the page was not dropped */
 +if ( !(rsp-u.mem_paging.flags  MEM_PAGING_DROP_PAGE) )
  {
 -struct vcpu *v;
 -
 -if ( rsp.version != VM_EVENT_INTERFACE_VERSION )
 -{
 -printk(XENLOG_G_WARNING vm_event interface version mismatch\n);
 -continue;
 -}
 -
 -#ifndef NDEBUG
 -if ( rsp.flags  VM_EVENT_FLAG_DUMMY )
 -continue;
 -#endif
 -
 -/* Validate the vcpu_id in the response. */
 -if ( (rsp.vcpu_id = d-max_vcpus) || !d-vcpu[rsp.vcpu_id] )
 -continue;
 -
 -v = d-vcpu[rsp.vcpu_id];
 -
 -/* Fix p2m entry if the page was not dropped */
 -if ( !(rsp.u.mem_paging.flags  MEM_PAGING_DROP_PAGE) )
 +unsigned long gfn = rsp-u.mem_access.gfn;
 +gfn_lock(p2m, gfn, 0);

Once again - blank line between declarations and statements please.

 +mfn = p2m-get_entry(p2m, gfn, p2mt, a, 0, NULL);
 +/* Allow only pages which were prepared properly, or pages which
 + * were nominated but not evicted */

Coding style.

 @@ -48,15 +46,15 @@ bool_t vm_event_check_ring(struct vm_event_domain *med);
   * succeed.
   */
  int __vm_event_claim_slot(struct domain *d, struct vm_event_domain *med,
 -bool_t allow_sleep);
 +  bool_t allow_sleep);
  static inline int vm_event_claim_slot(struct domain *d,
 -struct vm_event_domain *med)
 +  struct vm_event_domain *med)
  {
  return __vm_event_claim_slot(d, med, 1);
  }
  
  static inline int vm_event_claim_slot_nosleep(struct domain *d,
 -struct vm_event_domain *med)
 +  struct vm_event_domain *med)

All these whitespace changes here and further down don't really
belong in this patch - please again get this right when adding the
code.

Jan


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

Re: [Xen-devel] [PATCH V5 09/12] xen/vm_event: Decouple vm_event and mem_access.

2015-02-17 Thread Andrew Cooper 
On 17/02/15 18:30, Tamas K Lengyel wrote:
 All these whitespace changes here and further down don't really
 belong in this patch - please again get this right when adding the
 code.
 Same issue I mentioned in the other patch: git -M can't track the
 files if indentation is fixed as part of the renaming process. As I
 end up touching all the files that have with minor style issues like
 this in the series as a result of the renaming, I fix them as I go
 along. If that stretches the rules, I will need to add a whole new
 separate patch just for indentation fixing.

Separating the two is best for review.

One patch which git diff -M says is identical for moving the file, and
one patch which git diff -w says is identical for whitespace fixes.

It makes it trivial to confirm that there is no functional change involved.

~Andrew

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

Re: [Xen-devel] [PATCH V5 09/12] xen/vm_event: Decouple vm_event and mem_access.

2015-02-17 Thread Tamas K Lengyel 
On Tue, Feb 17, 2015 at 7:34 PM, Andrew Cooper
andrew.coop...@citrix.com wrote:
 On 17/02/15 18:30, Tamas K Lengyel wrote:
 All these whitespace changes here and further down don't really
 belong in this patch - please again get this right when adding the
 code.
 Same issue I mentioned in the other patch: git -M can't track the
 files if indentation is fixed as part of the renaming process. As I
 end up touching all the files that have with minor style issues like
 this in the series as a result of the renaming, I fix them as I go
 along. If that stretches the rules, I will need to add a whole new
 separate patch just for indentation fixing.

 Separating the two is best for review.

 One patch which git diff -M says is identical for moving the file, and
 one patch which git diff -w says is identical for whitespace fixes.

 It makes it trivial to confirm that there is no functional change involved.

 ~Andrew

Alright, that makes sense, will do so.

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH V5 09/12] xen/vm_event: Decouple vm_event and mem_access.

2015-02-17 Thread Tamas K Lengyel 
On Tue, Feb 17, 2015 at 3:17 PM, Jan Beulich jbeul...@suse.com wrote:
 On 13.02.15 at 17:33, tamas.leng...@zentific.com wrote:
 @@ -1293,56 +1293,30 @@ int p2m_mem_paging_prep(struct domain *d, unsigned 
 long gfn, uint64_t buffer)
   *
   * If the gfn was dropped the vcpu needs to be unpaused.
   */
 -void p2m_mem_paging_resume(struct domain *d)
 +
 +void p2m_mem_paging_resume(struct domain *d, vm_event_response_t *rsp)
  {
  struct p2m_domain *p2m = p2m_get_hostp2m(d);
 -vm_event_response_t rsp;
  p2m_type_t p2mt;
  p2m_access_t a;
  mfn_t mfn;

 -/* Pull all responses off the ring */
 -while( vm_event_get_response(d, d-vm_event-paging, rsp) )
 +/* Fix p2m entry if the page was not dropped */
 +if ( !(rsp-u.mem_paging.flags  MEM_PAGING_DROP_PAGE) )
  {
 -struct vcpu *v;
 -
 -if ( rsp.version != VM_EVENT_INTERFACE_VERSION )
 -{
 -printk(XENLOG_G_WARNING vm_event interface version 
 mismatch\n);
 -continue;
 -}
 -
 -#ifndef NDEBUG
 -if ( rsp.flags  VM_EVENT_FLAG_DUMMY )
 -continue;
 -#endif
 -
 -/* Validate the vcpu_id in the response. */
 -if ( (rsp.vcpu_id = d-max_vcpus) || !d-vcpu[rsp.vcpu_id] )
 -continue;
 -
 -v = d-vcpu[rsp.vcpu_id];
 -
 -/* Fix p2m entry if the page was not dropped */
 -if ( !(rsp.u.mem_paging.flags  MEM_PAGING_DROP_PAGE) )
 +unsigned long gfn = rsp-u.mem_access.gfn;
 +gfn_lock(p2m, gfn, 0);

 Once again - blank line between declarations and statements please.

 +mfn = p2m-get_entry(p2m, gfn, p2mt, a, 0, NULL);
 +/* Allow only pages which were prepared properly, or pages which
 + * were nominated but not evicted */

 Coding style.

 @@ -48,15 +46,15 @@ bool_t vm_event_check_ring(struct vm_event_domain *med);
   * succeed.
   */
  int __vm_event_claim_slot(struct domain *d, struct vm_event_domain *med,
 -bool_t allow_sleep);
 +  bool_t allow_sleep);
  static inline int vm_event_claim_slot(struct domain *d,
 -struct vm_event_domain *med)
 +  struct vm_event_domain *med)
  {
  return __vm_event_claim_slot(d, med, 1);
  }

  static inline int vm_event_claim_slot_nosleep(struct domain *d,
 -struct vm_event_domain *med)
 +  struct vm_event_domain *med)

 All these whitespace changes here and further down don't really
 belong in this patch - please again get this right when adding the
 code.

Same issue I mentioned in the other patch: git -M can't track the
files if indentation is fixed as part of the renaming process. As I
end up touching all the files that have with minor style issues like
this in the series as a result of the renaming, I fix them as I go
along. If that stretches the rules, I will need to add a whole new
separate patch just for indentation fixing.


 Jan

Thanks,
Tamas

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

[Xen-devel] [PATCH V5 09/12] xen/vm_event: Decouple vm_event and mem_access.

2015-02-13 Thread Tamas K Lengyel 
The vm_event subsystem has been artifically tied to the presence of mem_access.
While mem_access does depend on vm_event, vm_event is an entirely independent
subsystem that can be used for arbitrary function-offloading to helper apps in
domains. This patch removes the dependency that mem_access needs to be supported
in order to enable vm_event.

A new vm_event_resume function is introduced which pulls all responses off from
given ring and delegates handling to appropriate helper functions (if
necessary). By default, vm_event_resume just pulls the response from the ring
and unpauses the corresponding vCPU. This approach reduces code duplication
and present a single point of entry for the entire vm_event subsystem's
response handling mechanism.

Signed-off-by: Tamas K Lengyel tamas.leng...@zentific.com
Acked-by: Daniel De Graaf dgde...@tycho.nsa.gov
---
v4: Consolidate resume routines into vm_event_resume
Style fixes
Sort xen/common/Makefile to be alphabetical
v3: Move ring processing out from mem_access.c to monitor.c in common
---
 xen/arch/x86/mm/mem_sharing.c   | 37 +-
 xen/arch/x86/mm/p2m.c   | 66 ++-
 xen/common/Makefile | 18 -
 xen/common/mem_access.c | 36 +
 xen/common/vm_event.c   | 77 +++--
 xen/include/asm-x86/mem_sharing.h   |  1 -
 xen/include/asm-x86/p2m.h   |  2 +-
 xen/include/xen/mem_access.h| 14 +--
 xen/include/xen/vm_event.h  | 70 -
 xen/include/xsm/dummy.h |  2 -
 xen/include/xsm/xsm.h   |  4 --
 xen/xsm/dummy.c |  2 -
 xen/xsm/flask/hooks.c   | 36 -
 xen/xsm/flask/policy/access_vectors |  8 ++--
 14 files changed, 137 insertions(+), 236 deletions(-)

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 0459544..4959407 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -591,40 +591,6 @@ unsigned int mem_sharing_get_nr_shared_mfns(void)
 return (unsigned int)atomic_read(nr_shared_mfns);
 }
 
-int mem_sharing_sharing_resume(struct domain *d)
-{
-vm_event_response_t rsp;
-
-/* Get all requests off the ring */
-while ( vm_event_get_response(d, d-vm_event-share, rsp) )
-{
-struct vcpu *v;
-
-if ( rsp.version != VM_EVENT_INTERFACE_VERSION )
-{
-printk(XENLOG_G_WARNING vm_event interface version mismatch\n);
-continue;
-}
-
-#ifndef NDEBUG
-if ( rsp.flags  VM_EVENT_FLAG_DUMMY )
-continue;
-#endif
-
-/* Validate the vcpu_id in the response. */
-if ( (rsp.vcpu_id = d-max_vcpus) || !d-vcpu[rsp.vcpu_id] )
-continue;
-
-v = d-vcpu[rsp.vcpu_id];
-
-/* Unpause domain/vcpu */
-if ( rsp.flags  VM_EVENT_FLAG_VCPU_PAUSED )
-vm_event_vcpu_unpause(v);
-}
-
-return 0;
-}
-
 /* Functions that change a page's type and ownership */
 static int page_make_sharable(struct domain *d, 
struct page_info *page, 
@@ -1475,7 +1441,8 @@ int mem_sharing_memop(struct domain *d, 
xen_mem_sharing_op_t *mec)
 {
 if ( !mem_sharing_enabled(d) )
 return -EINVAL;
-rc = mem_sharing_sharing_resume(d);
+
+vm_event_resume(d, d-vm_event-share);
 }
 break;
 
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 68d57d7..dab1da1 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1279,13 +1279,13 @@ int p2m_mem_paging_prep(struct domain *d, unsigned long 
gfn, uint64_t buffer)
 }
 
 /**
- * p2m_mem_paging_resume - Resume guest gfn and vcpus
+ * p2m_mem_paging_resume - Resume guest gfn
  * @d: guest domain
- * @gfn: guest page in paging state
+ * @rsp: vm_event response received
+ *
+ * p2m_mem_paging_resume() will forward the p2mt of a gfn to ram_rw. It is
+ * called by the pager.
  *
- * p2m_mem_paging_resume() will forward the p2mt of a gfn to ram_rw and all
- * waiting vcpus will be unpaused again. It is called by the pager.
- * 
  * The gfn was previously either evicted and populated, or nominated and
  * populated. If the page was evicted the p2mt will be p2m_ram_paging_in. If
  * the page was just nominated the p2mt will be p2m_ram_paging_in_start because
@@ -1293,56 +1293,30 @@ int p2m_mem_paging_prep(struct domain *d, unsigned long 
gfn, uint64_t buffer)
  *
  * If the gfn was dropped the vcpu needs to be unpaused.
  */
-void p2m_mem_paging_resume(struct domain *d)
+
+void p2m_mem_paging_resume(struct domain *d, vm_event_response_t *rsp)
 {
 struct p2m_domain *p2m = p2m_get_hostp2m(d);
-vm_event_response_t rsp;
 p2m_type_t p2mt;
 p2m_access_t a;
 mfn_t mfn;
 
-/* Pull all responses off the ring */
-while( vm_event_get_response(d, d-vm_event-paging, rsp) )
+/*

Re: [Xen-devel] [PATCH V5 09/12] xen/vm_event: Decouple vm_event and mem_access.

2015-02-13 Thread Tamas K Lengyel 
On Fri, Feb 13, 2015 at 10:05 PM, Andrew Cooper
andrew.coop...@citrix.com wrote:
 On 13/02/15 16:33, Tamas K Lengyel wrote:
 diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
 index f988291..f89361e 100644
 --- a/xen/common/vm_event.c
 +++ b/xen/common/vm_event.c
 @@ -357,6 +357,67 @@ int vm_event_get_response(struct domain *d, struct 
 vm_event_domain *ved, vm_even
  return 1;
  }

 +/*
 + * Pull all responses from the given ring and unpause the corresponding vCPU
 + * if required. Based on the response type, here we can also call custom
 + * handlers.
 + *
 + * Note: responses are handled the same way regardless of which ring they
 + * arrive on.
 + */
 +void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
 +{
 +vm_event_response_t rsp;
 +
 +/* Pull all responses off the ring. */
 +while ( vm_event_get_response(d, ved, rsp) )
 +{
 +struct vcpu *v;
 +
 +if ( rsp.version != VM_EVENT_INTERFACE_VERSION )
 +{
 +gdprintk(XENLOG_WARNING, vm_event interface version 
 mismatch!);
 +continue;
 +}
 +
 +#ifndef NDEBUG
 +if ( rsp.flags  VM_EVENT_FLAG_DUMMY )
 +continue;
 +#endif
 +
 +/* Validate the vcpu_id in the response. */
 +if ( (rsp.vcpu_id = d-max_vcpus) || !d-vcpu[rsp.vcpu_id] )
 +continue;
 +
 +v = d-vcpu[rsp.vcpu_id];
 +
 +/*
 + * In some cases the response type needs extra handling, so here
 + * we call the appropriate handlers.
 + */
 +switch ( rsp.reason )
 +{
 +
 +#ifdef HAS_MEM_ACCESS
 +case VM_EVENT_REASON_MEM_ACCESS:
 +mem_access_resume(v, rsp);
 +break;
 +#endif
 +
 +#ifdef HAS_MEM_PAGING
 +case VM_EVENT_REASON_MEM_PAGING:
 +p2m_mem_paging_resume(d, rsp);
 +break;
 +#endif
 +

 You need a default clause which captures unknown/invalid responses, logs
 a message for debugging purposes, and ceases any further processing.  It
 is not sensible for an unknown response to unpause the vcpu.

That's how it works today, no validation on the response fields
anywhere. I was also thinking further checking if the response is
arriving on the appropriate ring - we could send arbitrary responses
on all three rings - but I think it is a bit of an overkill.


 This will require you to have a whitelist of known reasons which simply
 break.

Sure, that can be done easily.
Tamas


 ~Andrew

 +};
 +
 +/* Unpause domain. */
 +if ( rsp.flags  VM_EVENT_FLAG_VCPU_PAUSED )
 +vm_event_vcpu_unpause(v);
 +}
 +}
 +
  void vm_event_cancel_slot(struct domain *d, struct vm_event_domain *ved)
  {
  vm_event_ring_lock(ved);




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

Re: [Xen-devel] [PATCH V5 09/12] xen/vm_event: Decouple vm_event and mem_access.

2015-02-13 Thread Andrew Cooper 
On 13/02/15 16:33, Tamas K Lengyel wrote:
 diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
 index f988291..f89361e 100644
 --- a/xen/common/vm_event.c
 +++ b/xen/common/vm_event.c
 @@ -357,6 +357,67 @@ int vm_event_get_response(struct domain *d, struct 
 vm_event_domain *ved, vm_even
  return 1;
  }
  
 +/*
 + * Pull all responses from the given ring and unpause the corresponding vCPU
 + * if required. Based on the response type, here we can also call custom
 + * handlers.
 + *
 + * Note: responses are handled the same way regardless of which ring they
 + * arrive on.
 + */
 +void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
 +{
 +vm_event_response_t rsp;
 +
 +/* Pull all responses off the ring. */
 +while ( vm_event_get_response(d, ved, rsp) )
 +{
 +struct vcpu *v;
 +
 +if ( rsp.version != VM_EVENT_INTERFACE_VERSION )
 +{
 +gdprintk(XENLOG_WARNING, vm_event interface version mismatch!);
 +continue;
 +}
 +
 +#ifndef NDEBUG
 +if ( rsp.flags  VM_EVENT_FLAG_DUMMY )
 +continue;
 +#endif
 +
 +/* Validate the vcpu_id in the response. */
 +if ( (rsp.vcpu_id = d-max_vcpus) || !d-vcpu[rsp.vcpu_id] )
 +continue;
 +
 +v = d-vcpu[rsp.vcpu_id];
 +
 +/*
 + * In some cases the response type needs extra handling, so here
 + * we call the appropriate handlers.
 + */
 +switch ( rsp.reason )
 +{
 +
 +#ifdef HAS_MEM_ACCESS
 +case VM_EVENT_REASON_MEM_ACCESS:
 +mem_access_resume(v, rsp);
 +break;
 +#endif
 +
 +#ifdef HAS_MEM_PAGING
 +case VM_EVENT_REASON_MEM_PAGING:
 +p2m_mem_paging_resume(d, rsp);
 +break;
 +#endif
 +

You need a default clause which captures unknown/invalid responses, logs
a message for debugging purposes, and ceases any further processing.  It
is not sensible for an unknown response to unpause the vcpu.

This will require you to have a whitelist of known reasons which simply
break.

~Andrew

 +};
 +
 +/* Unpause domain. */
 +if ( rsp.flags  VM_EVENT_FLAG_VCPU_PAUSED )
 +vm_event_vcpu_unpause(v);
 +}
 +}
 +
  void vm_event_cancel_slot(struct domain *d, struct vm_event_domain *ved)
  {
  vm_event_ring_lock(ved);




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