Re: [Xen-devel] [RFC PATCH V2 8/8] x86/hvm: factor out vm_event related functions into separate file

2015-01-23 Thread Tamas K Lengyel 
On Fri, Jan 23, 2015 at 10:03 AM, Jan Beulich jbeul...@suse.com wrote:
 On 23.01.15 at 09:56, rcojoc...@bitdefender.com wrote:
 On 01/22/2015 06:42 PM, Tamas K Lengyel wrote:
 On Thu, Jan 22, 2015 at 5:25 PM, Jan Beulich jbeul...@suse.com wrote:
 On 18.01.15 at 16:18, tamas.leng...@zentific.com wrote:
 +void hvm_event_msr(unsigned long msr, unsigned long value)
 +{
 +vm_event_request_t req = {
 +.reason = VM_EVENT_REASON_MSR,
 +.vcpu_id = current-vcpu_id,
 +.msr_event.msr = msr,
 +.msr_event.new_value = value,
 +};

 The .msr_event sub-structure also has an old_value member - how
 come this doesn't get filled (I realize the old code was this way,
 but I now doubt earlier patches are all correct in the regard).

 Razvan might have more information on this side as I haven't really
 touched MSR events. I vaguely recall some issues with having access to
 the old MSR value?

 Indeed, getting the previous value would be a bit involved. Please see
 xen/arch/x86/hvm/hvm.c, specifically hvm_msr_write_intercept(). At first
 glance, you'd have to call hvm_msr_read_intercept() to get the previous
 value, or create some sort of cache for previous values for all MSRs,
 updated by hvm_msr_write_intercept().

 Since this has not had any real value for my use-case, and since my code
 has only needed to keep track of a handful of MSRs, I took the route of
 caching previous values for them in the userspace application.

 This is, of course, not to say that it can't be done at the HV level,
 just that (at least IMHO) the HV-level costs have outweighed the benefits.

 In which case the respective interface structure field should have a
 comment attached saying that it currently doesn't hold what the field
 name promises (and, without having checked whether this may already
 be the case, it should be made hold a deterministic value, e.g. zero).

 Jan

Ack, I'll remove it as we don't need unused struct members to pollute
the landscape.

Tamas

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

Re: [Xen-devel] [RFC PATCH V2 8/8] x86/hvm: factor out vm_event related functions into separate file

2015-01-23 Thread Razvan Cojocaru 
On 01/22/2015 06:42 PM, Tamas K Lengyel wrote:
 On Thu, Jan 22, 2015 at 5:25 PM, Jan Beulich jbeul...@suse.com wrote:
 On 18.01.15 at 16:18, tamas.leng...@zentific.com wrote:
 +void hvm_event_msr(unsigned long msr, unsigned long value)
 +{
 +vm_event_request_t req = {
 +.reason = VM_EVENT_REASON_MSR,
 +.vcpu_id = current-vcpu_id,
 +.msr_event.msr = msr,
 +.msr_event.new_value = value,
 +};

 The .msr_event sub-structure also has an old_value member - how
 come this doesn't get filled (I realize the old code was this way,
 but I now doubt earlier patches are all correct in the regard).
 
 Razvan might have more information on this side as I haven't really
 touched MSR events. I vaguely recall some issues with having access to
 the old MSR value?

Indeed, getting the previous value would be a bit involved. Please see
xen/arch/x86/hvm/hvm.c, specifically hvm_msr_write_intercept(). At first
glance, you'd have to call hvm_msr_read_intercept() to get the previous
value, or create some sort of cache for previous values for all MSRs,
updated by hvm_msr_write_intercept().

Since this has not had any real value for my use-case, and since my code
has only needed to keep track of a handful of MSRs, I took the route of
caching previous values for them in the userspace application.

This is, of course, not to say that it can't be done at the HV level,
just that (at least IMHO) the HV-level costs have outweighed the benefits.


Thanks for the question,
Razvan

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

Re: [Xen-devel] [RFC PATCH V2 8/8] x86/hvm: factor out vm_event related functions into separate file

2015-01-23 Thread Jan Beulich 
 On 23.01.15 at 09:56, rcojoc...@bitdefender.com wrote:
 On 01/22/2015 06:42 PM, Tamas K Lengyel wrote:
 On Thu, Jan 22, 2015 at 5:25 PM, Jan Beulich jbeul...@suse.com wrote:
 On 18.01.15 at 16:18, tamas.leng...@zentific.com wrote:
 +void hvm_event_msr(unsigned long msr, unsigned long value)
 +{
 +vm_event_request_t req = {
 +.reason = VM_EVENT_REASON_MSR,
 +.vcpu_id = current-vcpu_id,
 +.msr_event.msr = msr,
 +.msr_event.new_value = value,
 +};

 The .msr_event sub-structure also has an old_value member - how
 come this doesn't get filled (I realize the old code was this way,
 but I now doubt earlier patches are all correct in the regard).
 
 Razvan might have more information on this side as I haven't really
 touched MSR events. I vaguely recall some issues with having access to
 the old MSR value?
 
 Indeed, getting the previous value would be a bit involved. Please see
 xen/arch/x86/hvm/hvm.c, specifically hvm_msr_write_intercept(). At first
 glance, you'd have to call hvm_msr_read_intercept() to get the previous
 value, or create some sort of cache for previous values for all MSRs,
 updated by hvm_msr_write_intercept().
 
 Since this has not had any real value for my use-case, and since my code
 has only needed to keep track of a handful of MSRs, I took the route of
 caching previous values for them in the userspace application.
 
 This is, of course, not to say that it can't be done at the HV level,
 just that (at least IMHO) the HV-level costs have outweighed the benefits.

In which case the respective interface structure field should have a
comment attached saying that it currently doesn't hold what the field
name promises (and, without having checked whether this may already
be the case, it should be made hold a deterministic value, e.g. zero).

Jan


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

Re: [Xen-devel] [RFC PATCH V2 8/8] x86/hvm: factor out vm_event related functions into separate file

2015-01-22 Thread Jan Beulich 
 On 18.01.15 at 16:18, tamas.leng...@zentific.com wrote:
 +static int hvm_event_traps(long parameters, vm_event_request_t *req)
 +{
 +int rc;
 +struct vcpu *v = current;
 +struct domain *d = v-domain;

Unless the intention is to have an exact 1:1 copy of the original,
please use curr and currd here respectively.

 +void hvm_event_cr0(unsigned long value, unsigned long old)
 +{
 +vm_event_request_t req = {
 +.reason = VM_EVENT_REASON_CR0,
 +.vcpu_id = current-vcpu_id,
 +.cr_event.new_value = value,
 +.cr_event.old_value = old
 +};
 +
 +long parameters = current-domain-arch.hvm_domain
 +.params[HVM_PARAM_MEMORY_EVENT_CR0];

And latch current into a local variable curr here and below.

 +void hvm_event_msr(unsigned long msr, unsigned long value)
 +{
 +vm_event_request_t req = {
 +.reason = VM_EVENT_REASON_MSR,
 +.vcpu_id = current-vcpu_id,
 +.msr_event.msr = msr,
 +.msr_event.new_value = value,
 +};

The .msr_event sub-structure also has an old_value member - how
come this doesn't get filled (I realize the old code was this way,
but I now doubt earlier patches are all correct in the regard).

Jan


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

Re: [Xen-devel] [RFC PATCH V2 8/8] x86/hvm: factor out vm_event related functions into separate file

2015-01-22 Thread Tamas K Lengyel 
On Thu, Jan 22, 2015 at 4:00 PM, Tim Deegan t...@xen.org wrote:
 At 16:18 +0100 on 18 Jan (1421594281), Tamas K Lengyel wrote:
 To avoid growing hvm.c these functions can be stored
 separately.

 Signed-off-by: Tamas K Lengyel tamas.leng...@zentific.com

 Assuming this is just code motion, it seems like a good idea to me,
 with one nit:

 --- a/xen/arch/x86/hvm/Makefile
 +++ b/xen/arch/x86/hvm/Makefile
 @@ -22,4 +22,5 @@ obj-y += vlapic.o
  obj-y += vmsi.o
  obj-y += vpic.o
  obj-y += vpt.o
 -obj-y += vpmu.o
 \ No newline at end of file
 +obj-y += vpmu.o
 +obj-y += event.o

 This list is in alphabetical order; please keep it that way.

Noted!

Thanks,
Tamas

 With that fixed,

 Acked-by: Tim Deegan t...@xen.org

 Cheers,

 Tim.

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

Re: [Xen-devel] [RFC PATCH V2 8/8] x86/hvm: factor out vm_event related functions into separate file

2015-01-22 Thread Tim Deegan 
At 16:18 +0100 on 18 Jan (1421594281), Tamas K Lengyel wrote:
 To avoid growing hvm.c these functions can be stored
 separately.
 
 Signed-off-by: Tamas K Lengyel tamas.leng...@zentific.com

Assuming this is just code motion, it seems like a good idea to me,
with one nit:

 --- a/xen/arch/x86/hvm/Makefile
 +++ b/xen/arch/x86/hvm/Makefile
 @@ -22,4 +22,5 @@ obj-y += vlapic.o
  obj-y += vmsi.o
  obj-y += vpic.o
  obj-y += vpt.o
 -obj-y += vpmu.o
 \ No newline at end of file
 +obj-y += vpmu.o
 +obj-y += event.o

This list is in alphabetical order; please keep it that way.

With that fixed,

Acked-by: Tim Deegan t...@xen.org

Cheers,

Tim.

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

Re: [Xen-devel] [RFC PATCH V2 8/8] x86/hvm: factor out vm_event related functions into separate file

2015-01-22 Thread Tamas K Lengyel 
On Thu, Jan 22, 2015 at 5:25 PM, Jan Beulich jbeul...@suse.com wrote:
 On 18.01.15 at 16:18, tamas.leng...@zentific.com wrote:
 +static int hvm_event_traps(long parameters, vm_event_request_t *req)
 +{
 +int rc;
 +struct vcpu *v = current;
 +struct domain *d = v-domain;

 Unless the intention is to have an exact 1:1 copy of the original,
 please use curr and currd here respectively.

Ack.


 +void hvm_event_cr0(unsigned long value, unsigned long old)
 +{
 +vm_event_request_t req = {
 +.reason = VM_EVENT_REASON_CR0,
 +.vcpu_id = current-vcpu_id,
 +.cr_event.new_value = value,
 +.cr_event.old_value = old
 +};
 +
 +long parameters = current-domain-arch.hvm_domain
 +.params[HVM_PARAM_MEMORY_EVENT_CR0];

 And latch current into a local variable curr here and below.

Ack.


 +void hvm_event_msr(unsigned long msr, unsigned long value)
 +{
 +vm_event_request_t req = {
 +.reason = VM_EVENT_REASON_MSR,
 +.vcpu_id = current-vcpu_id,
 +.msr_event.msr = msr,
 +.msr_event.new_value = value,
 +};

 The .msr_event sub-structure also has an old_value member - how
 come this doesn't get filled (I realize the old code was this way,
 but I now doubt earlier patches are all correct in the regard).

Razvan might have more information on this side as I haven't really
touched MSR events. I vaguely recall some issues with having access to
the old MSR value?


 Jan


Thanks,
Tamas

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

Re: [Xen-devel] [RFC PATCH V2 8/8] x86/hvm: factor out vm_event related functions into separate file

2015-01-22 Thread Tamas K Lengyel 
On Thu, Jan 22, 2015 at 5:32 PM, Andrew Cooper
andrew.coop...@citrix.com wrote:
 On 18/01/15 15:18, Tamas K Lengyel wrote:
 -void hvm_event_cr0(unsigned long value, unsigned long old)
 -{
 -vm_event_request_t req = {
 -.reason = VM_EVENT_REASON_CR0,
 -.vcpu_id = current-vcpu_id,
 -.cr_event.new_value = value,
 -.cr_event.old_value = old
 -};
 -
 -long parameters = current-domain-arch.hvm_domain
 -.params[HVM_PARAM_MEMORY_EVENT_CR0];

 (I realise this is probably not the best patch, but) As we are redoing
 the API with a hope of including PV and ARM guests, can we remove this
 use of hvm params?  This would probably involve a new setup hypercall
 subop to set up reporting preferences.

 ~Andrew

Ack, that would be certainly a useful addition. We would need to
define some new place to store these preferences for the domain.

Thanks,
Tamas

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

Re: [Xen-devel] [RFC PATCH V2 8/8] x86/hvm: factor out vm_event related functions into separate file

2015-01-22 Thread Tim Deegan 
At 17:42 +0100 on 22 Jan (1421944966), Tamas K Lengyel wrote:
 On Thu, Jan 22, 2015 at 5:25 PM, Jan Beulich jbeul...@suse.com wrote:
  On 18.01.15 at 16:18, tamas.leng...@zentific.com wrote:
  +static int hvm_event_traps(long parameters, vm_event_request_t *req)
  +{
  +int rc;
  +struct vcpu *v = current;
  +struct domain *d = v-domain;
 
  Unless the intention is to have an exact 1:1 copy of the original,
  please use curr and currd here respectively.
 
 Ack.
 
 
  +void hvm_event_cr0(unsigned long value, unsigned long old)
  +{
  +vm_event_request_t req = {
  +.reason = VM_EVENT_REASON_CR0,
  +.vcpu_id = current-vcpu_id,
  +.cr_event.new_value = value,
  +.cr_event.old_value = old
  +};
  +
  +long parameters = current-domain-arch.hvm_domain
  +.params[HVM_PARAM_MEMORY_EVENT_CR0];
 
  And latch current into a local variable curr here and below.
 
 Ack.
 
 
  +void hvm_event_msr(unsigned long msr, unsigned long value)
  +{
  +vm_event_request_t req = {
  +.reason = VM_EVENT_REASON_MSR,
  +.vcpu_id = current-vcpu_id,
  +.msr_event.msr = msr,
  +.msr_event.new_value = value,
  +};
 
  The .msr_event sub-structure also has an old_value member - how
  come this doesn't get filled (I realize the old code was this way,
  but I now doubt earlier patches are all correct in the regard).
 
 Razvan might have more information on this side as I haven't really
 touched MSR events. I vaguely recall some issues with having access to
 the old MSR value?

Yep - ISTR this is also why the MSR event doesn't respect
HVMPME_onchangeonly.

Tim.

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

[Xen-devel] [RFC PATCH V2 8/8] x86/hvm: factor out vm_event related functions into separate file

2015-01-18 Thread Tamas K Lengyel 
To avoid growing hvm.c these functions can be stored
separately.

Signed-off-by: Tamas K Lengyel tamas.leng...@zentific.com
---
 xen/arch/x86/hvm/Makefile   |   3 +-
 xen/arch/x86/hvm/event.c| 195 
 xen/arch/x86/hvm/hvm.c  | 163 +
 xen/arch/x86/hvm/vmx/vmx.c  |   1 +
 xen/include/asm-x86/hvm/event.h |  40 +
 xen/include/asm-x86/hvm/hvm.h   |  11 ---
 6 files changed, 239 insertions(+), 174 deletions(-)
 create mode 100644 xen/arch/x86/hvm/event.c
 create mode 100644 xen/include/asm-x86/hvm/event.h

diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index eea..2389923 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -22,4 +22,5 @@ obj-y += vlapic.o
 obj-y += vmsi.o
 obj-y += vpic.o
 obj-y += vpt.o
-obj-y += vpmu.o
\ No newline at end of file
+obj-y += vpmu.o
+obj-y += event.o
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
new file mode 100644
index 000..96d1748
--- /dev/null
+++ b/xen/arch/x86/hvm/event.c
@@ -0,0 +1,195 @@
+/*
+* event.c: Common hardware virtual machine event abstractions.
+*
+* Copyright (c) 2004, Intel Corporation.
+* Copyright (c) 2005, International Business Machines Corporation.
+* Copyright (c) 2008, Citrix Systems, Inc.
+*
+* This program is free software; you can redistribute it and/or modify it
+* under the terms and conditions of the GNU General Public License,
+* version 2, as published by the Free Software Foundation.
+*
+* This program is distributed in the hope it will be useful, but WITHOUT
+* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+* more details.
+*
+* You should have received a copy of the GNU General Public License along with
+* this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+* Place - Suite 330, Boston, MA 02111-1307 USA.
+*/
+
+#include xen/vm_event.h
+#include xen/paging.h
+#include public/vm_event.h
+
+static void hvm_event_fill_regs(vm_event_request_t *req)
+{
+const struct cpu_user_regs *regs = guest_cpu_user_regs();
+const struct vcpu *curr = current;
+
+req-regs.x86.rax = regs-eax;
+req-regs.x86.rcx = regs-ecx;
+req-regs.x86.rdx = regs-edx;
+req-regs.x86.rbx = regs-ebx;
+req-regs.x86.rsp = regs-esp;
+req-regs.x86.rbp = regs-ebp;
+req-regs.x86.rsi = regs-esi;
+req-regs.x86.rdi = regs-edi;
+
+req-regs.x86.r8  = regs-r8;
+req-regs.x86.r9  = regs-r9;
+req-regs.x86.r10 = regs-r10;
+req-regs.x86.r11 = regs-r11;
+req-regs.x86.r12 = regs-r12;
+req-regs.x86.r13 = regs-r13;
+req-regs.x86.r14 = regs-r14;
+req-regs.x86.r15 = regs-r15;
+
+req-regs.x86.rflags = regs-eflags;
+req-regs.x86.rip= regs-eip;
+
+req-regs.x86.msr_efer = curr-arch.hvm_vcpu.guest_efer;
+req-regs.x86.cr0 = curr-arch.hvm_vcpu.guest_cr[0];
+req-regs.x86.cr3 = curr-arch.hvm_vcpu.guest_cr[3];
+req-regs.x86.cr4 = curr-arch.hvm_vcpu.guest_cr[4];
+}
+
+static int hvm_event_traps(long parameters, vm_event_request_t *req)
+{
+int rc;
+struct vcpu *v = current;
+struct domain *d = v-domain;
+
+if ( !(parameters  HVMPME_MODE_MASK) )
+return 0;
+
+rc = vm_event_claim_slot(d, d-vm_event-monitor);
+if ( rc == -ENOSYS )
+{
+/* If there was no ring to handle the event, then
+ * simple continue executing normally. */
+return 1;
+}
+else if ( rc  0 )
+return rc;
+
+if ( (parameters  HVMPME_MODE_MASK) == HVMPME_mode_sync )
+{
+req-flags |= VM_EVENT_FLAG_VCPU_PAUSED;
+vm_event_vcpu_pause(v);
+}
+
+hvm_event_fill_regs(req);
+vm_event_put_request(d, d-vm_event-monitor, req);
+
+return 1;
+}
+
+void hvm_event_cr0(unsigned long value, unsigned long old)
+{
+vm_event_request_t req = {
+.reason = VM_EVENT_REASON_CR0,
+.vcpu_id = current-vcpu_id,
+.cr_event.new_value = value,
+.cr_event.old_value = old
+};
+
+long parameters = current-domain-arch.hvm_domain
+.params[HVM_PARAM_MEMORY_EVENT_CR0];
+
+if ( (parameters  HVMPME_onchangeonly)  (value == old) )
+return;
+
+hvm_event_traps(parameters, req);
+}
+
+void hvm_event_cr3(unsigned long value, unsigned long old)
+{
+vm_event_request_t req = {
+.reason = VM_EVENT_REASON_CR3,
+.vcpu_id = current-vcpu_id,
+.cr_event.new_value = value,
+.cr_event.old_value = old
+};
+
+long parameters = current-domain-arch.hvm_domain
+.params[HVM_PARAM_MEMORY_EVENT_CR3];
+
+if ( (parameters  HVMPME_onchangeonly)  (value == old) )
+return;
+
+hvm_event_traps(parameters, req);
+}
+
+void hvm_event_cr4(unsigned long value, unsigned long old)
+{
+vm_event_request_t req = {
+.reason = VM_EVENT_REASON_CR4,
+