Re: [Xen-devel] [PATCH 0/1] ARM: Implement support for write-ctrlreg vm-events

[Corneliu ZUZU]

On 3/7/2016 11:12 AM, Tamas K Lengyel wrote:
EPT is not really required for CR3 monitoring, it just has been the 
case that vm_events have been only implemented for hap-enabled domains.


I suppose this is not valid for vm-events in their entirety, right? I 
mean it seems to me that @ least for monitor vm-events VMX is enough.



AFAIK for non-hap case CR3 needs to be trapped unconditionally, yes.

If the former is true, shouldn't we do a check like this in
vm_event_monitor_get_capabilities instead?


Yes, it should now, this code was just written before 
vm_event_monitor_get_capabilities was introduced and we haven't gotten 
around converting this check to it.


Is there any reason why monitor vm-events in their current state 
wouldn't work on non-hap domains?
If they would work, shouldn't we instead simply move the 
monitor.write_ctrlreg_enabled part out of the if ( paging_mode_hap(...) ) ?




2). I was also wondering why CR3 load/stores are trapped if paging
is disabled for a domain.


Good question, I was wondering about that myself at some point but I 
haven't found an answer to it. Maybe some git archaeology can help 
determining when that was added and why ;)


Cheers,
Tamas


Yep, will "blame into it".

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

Re: [Xen-devel] [PATCH 0/1] ARM: Implement support for write-ctrlreg vm-events

[Corneliu ZUZU]

On 3/3/2016 4:10 PM, Corneliu ZUZU wrote:

Then,
QUESTIONS (FOR VM-EVENTS & ARM MAINTAINERS ESPECIALLY):

Q1) [...]

Q2) [...]

Q3) [...]

Q4) [...]


Hey all,

I have a question relating to this part of code @ vmx_update_guest_cr:

if ( paging_mode_hap(v->domain) )
{
/* Manage GUEST_CR3 when CR0.PE=0. */
uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
 CPU_BASED_CR3_STORE_EXITING);
v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
v->arch.hvm_vmx.exec_control |= cr3_ctls;

/* Trap CR3 updates if CR3 memory events are enabled. */
if ( v->domain->arch.monitor.write_ctrlreg_enabled &
 monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;

vmx_update_cpu_exec_control(v);
}

While trying to move the check for VM_EVENT_X86_CR3 to the scheduling 
tail, a few questions came to my mind.


1). Tamas, Razvan, maybe you guys could clarify this. I noticed this 
part of code is only executed if paging_mode_hap(v->domain). Is EPT 
mandatory to monitor CR3 writes or is it just that when shadow paging is 
enabled, CR3 r/w are unconditionally trapped? If the former is true, 
shouldn't we do a check like this in vm_event_monitor_get_capabilities 
instead?


2). I was also wondering why CR3 load/stores are trapped if paging is 
disabled for a domain.


Thanks,
Corneliu.

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

Re: [Xen-devel] [PATCH 0/1] ARM: Implement support for write-ctrlreg vm-events

[Corneliu ZUZU]

On 3/3/2016 4:10 PM, Corneliu ZUZU wrote:

Then,
QUESTIONS (FOR VM-EVENTS & ARM MAINTAINERS ESPECIALLY):

Q1) [...]

Q2) [...]

Q3) [...]

Q4) [...]



JSYK, I've realized I can find the answer for these easily (besides Q2, 
for which

Razvan already gave feedback) with some tests (as soon as I get access to my
ARMv8 machine). So, no need to think of that anymore ;)

Thanks,
Corneliu.

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

Re: [Xen-devel] [PATCH 0/1] ARM: Implement support for write-ctrlreg vm-events

[Corneliu ZUZU]

On 3/3/2016 8:51 PM, Razvan Cojocaru wrote:

On 03/03/2016 08:04 PM, Corneliu ZUZU wrote:

On 3/3/2016 6:15 PM, Razvan Cojocaru wrote:

On 03/03/2016 04:10 PM, Corneliu ZUZU wrote:

Q2) About VM_EVENT_FLAG_DENY

Q2.1)
  Doesn't it require sync = 1 (i.e. the vcpu to be paused) to work?
  If so, shouldn't we call vm_event_register_write_resume only
after checking
  that VM_EVENT_FLAG_VCPU_PAUSED flag is set (vm_event_resume).
Moreover, if
  we do that, wouldn't it be 'cleaner' to rename
  vm_event_register_write_resume->vm_event_deny, check for the
  VM_EVENT_FLAG_DENY flag in vm_event_resume instead and call
vm_event_deny
  from there after this check?

Yes, it does require the VCPU to be paused to work, and yes, it's a good
idea to check that that flag is set in the response.

Beyond that, I'd prefer we keep vm_event_register_write_resume()
because, while today all we do there is check that VM_EVENT_FLAG_DENY is
set, we might want do to other things as well there as well (for
example, maybe validate the content of some register). That was really
the point of the "bigger-named" function.

But that's just my opinion, if Tamas prefers your rename suggestion I'll
consider myself outnumbered.

Oh, ok, then the check for VM_EVENT_FLAG_VCPU_PAUSED would be done in
vm_event_register_write_resume instead of vm_event_resume,
since I suppose vm_event_register_write_resume shouldn't be called only
when the vcpu is paused, we only apply that to the DENY flag, correct?

Yes, I think that's the path of least resistance.


Q2.2)
  VM_EVENT_FLAG_DENY functionality is not implemented w/ this
change-set.
  What is done instead is that the control-register write is done
*before*
  sending the vm-event (vm_event_put_request). This way, the user can
  override the written register value after receiving the
vm-event, which
  in effect provides the same flexibility as VM_EVENT_FLAG_DENY does.
  Using this strategy instead would simplify both Xen's code and
the libxc
  user's code.
  Thoughts?

That's how I initially did it with CR events, but with an application
dealing with huge numbers of events, an extra hypercall (to re-set the
register) can be quite expensive, so I had to rework it to the present
state. On these grounds I'm opposed to it - and for consistency I would
prefer that all register write events are pre-write events, and deniable
with a single vm_event reply.


Ah, I understand. I figured the utility of the DENY flag was only for
cases where you'd want to actively
override the value written to the register (set register to overridden
value + resume w/ DENY), instead
of just forbidding the write and leaving the register untouched (i.e.
set on the old value).
If the latter is a  desirable functionality then it indeed makes sense
to have a DENY flag.

Yes, that's the goal.


With that said, another thing crossed my mind. Since the DENY flag will
be implemented for ARM w/ the next
revision and the actual write will be done on the scheduler tail,
similarly to X86 ((hvm_do_resume), wouldn't
it be good if we separated the code that checks monitor_write_data from
there into an arch-dependent function,
e.g. vm_event_monitor_write_data? That way the scheduler tail function
won't be 'polluted' w/ that code and IMHO
it will make the vm-events design more clear (since that functionality
will also be in vm_event.c along w/ the other
vm_event_* functions).

Sounds good, except perhaps for the function name but I'm not sure what
a better one might be unfortunately, maybe someone else with chime in
with a suggestion.


Thanks,
Razvan




I was thinking of the option of giving this function the significance of 
it being called by the
scheduler tail, i.e. just before "entering" a vcpu, in which case we 
could use a name

like vm_event_schedtail.
To me this sounds pretty good since it generalizes the function and it 
makes sense to have
a vm_event_* function that is called just before a vcpu is scheduled, 
i.e. a vm-events function
that would add-in a "final touch" before the "final stage" of actually 
entering the vcpu.


Let me know what you think.

Thanks,
Corneliu.

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

Re: [Xen-devel] [PATCH 0/1] ARM: Implement support for write-ctrlreg vm-events

[Corneliu ZUZU]

On 3/3/2016 6:15 PM, Razvan Cojocaru wrote:

On 03/03/2016 04:10 PM, Corneliu ZUZU wrote:

Q2) About VM_EVENT_FLAG_DENY

   Q2.1)
 Doesn't it require sync = 1 (i.e. the vcpu to be paused) to work?
 If so, shouldn't we call vm_event_register_write_resume only after checking
 that VM_EVENT_FLAG_VCPU_PAUSED flag is set (vm_event_resume). Moreover, if
 we do that, wouldn't it be 'cleaner' to rename
 vm_event_register_write_resume->vm_event_deny, check for the
 VM_EVENT_FLAG_DENY flag in vm_event_resume instead and call vm_event_deny
 from there after this check?

Yes, it does require the VCPU to be paused to work, and yes, it's a good
idea to check that that flag is set in the response.

Beyond that, I'd prefer we keep vm_event_register_write_resume()
because, while today all we do there is check that VM_EVENT_FLAG_DENY is
set, we might want do to other things as well there as well (for
example, maybe validate the content of some register). That was really
the point of the "bigger-named" function.

But that's just my opinion, if Tamas prefers your rename suggestion I'll
consider myself outnumbered.


Oh, ok, then the check for VM_EVENT_FLAG_VCPU_PAUSED would be done in 
vm_event_register_write_resume instead of vm_event_resume,
since I suppose vm_event_register_write_resume shouldn't be called only 
when the vcpu is paused, we only apply that to the DENY flag, correct?



   Q2.2)
 VM_EVENT_FLAG_DENY functionality is not implemented w/ this change-set.
 What is done instead is that the control-register write is done *before*
 sending the vm-event (vm_event_put_request). This way, the user can
 override the written register value after receiving the vm-event, which
 in effect provides the same flexibility as VM_EVENT_FLAG_DENY does.
 Using this strategy instead would simplify both Xen's code and the libxc
 user's code.
 Thoughts?

That's how I initially did it with CR events, but with an application
dealing with huge numbers of events, an extra hypercall (to re-set the
register) can be quite expensive, so I had to rework it to the present
state. On these grounds I'm opposed to it - and for consistency I would
prefer that all register write events are pre-write events, and deniable
with a single vm_event reply.



Ah, I understand. I figured the utility of the DENY flag was only for 
cases where you'd want to actively
override the value written to the register (set register to overridden 
value + resume w/ DENY), instead
of just forbidding the write and leaving the register untouched (i.e. 
set on the old value).
If the latter is a  desirable functionality then it indeed makes sense 
to have a DENY flag.


With that said, another thing crossed my mind. Since the DENY flag will 
be implemented for ARM w/ the next
revision and the actual write will be done on the scheduler tail, 
similarly to X86 ((hvm_do_resume), wouldn't
it be good if we separated the code that checks monitor_write_data from 
there into an arch-dependent function,
e.g. vm_event_monitor_write_data? That way the scheduler tail function 
won't be 'polluted' w/ that code and IMHO
it will make the vm-events design more clear (since that functionality 
will also be in vm_event.c along w/ the other

vm_event_* functions).


HTH,
Razvan



Thanks,
Corneliu.

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

Re: [Xen-devel] [PATCH 0/1] ARM: Implement support for write-ctrlreg vm-events

[Corneliu ZUZU]

On 3/3/2016 4:10 PM, Corneliu ZUZU wrote:

The patch was currently tested on an ARMv8 (CONFIG_ARM_64) machine (after
applying a fix I'll send soon).


Shannon beat me to it with the fix ;) See "[PATCH] arm/timer: fix panic 
when booting with DT".


Corneliu.

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

[Xen-devel] [PATCH 1/1] arm/monitor vm-events: implement write-ctrlreg support

[Corneliu ZUZU]

This patch adds ARM support for write-ctrlreg monitor vm-events.
The ARM control-registers that can be monitored are:
 - VM_EVENT_ARM_SCTLR:  AArch32 SCTLR, AArch64 SCTLR_EL1
 - VM_EVENT_ARM_TTBR{0,1}:  AArch32 TTBR{0,1}, AArch64 TTBR{0,1}_EL1
 - VM_EVENT_ARM_TTBCR:  AArch32 TTBCR, AArch64 TCR_EL1

Trapping of write operations for these registers was attained
by setting the HCR_EL2.TVM / HCR.TVM bit.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/arm/p2m.c  |   5 +
 xen/arch/arm/traps.c| 128 +-
 xen/arch/x86/hvm/event.c|  27 -
 xen/arch/x86/hvm/hvm.c  |   2 +-
 xen/arch/x86/hvm/vmx/vmx.c  |   2 +-
 xen/arch/x86/monitor.c  |  45 
 xen/common/monitor.c|  48 +
 xen/common/vm_event.c   |  29 +
 xen/include/asm-arm/domain.h|   8 ++
 xen/include/asm-arm/traps.h | 227 
 xen/include/asm-arm/vm_event.h  |   4 +-
 xen/include/asm-x86/hvm/event.h |  13 +--
 xen/include/asm-x86/monitor.h   |   2 -
 xen/include/public/vm_event.h   |   8 +-
 xen/include/xen/monitor.h   |   2 +
 xen/include/xen/vm_event.h  |   8 ++
 16 files changed, 467 insertions(+), 91 deletions(-)
 create mode 100644 xen/include/asm-arm/traps.h

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index a2a9c4b..a32dfdd 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -108,6 +108,11 @@ void p2m_restore_state(struct vcpu *n)
 else
 hcr |= HCR_RW;
 
+if ( likely(0 == n->domain->arch.monitor.write_ctrlreg_enabled) )
+hcr &= ~HCR_TVM;
+else
+hcr |= HCR_TVM;
+
 WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
 isb();
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 83744e8..3e1c8ee 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -31,8 +31,10 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -41,6 +43,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "decode.h"
 #include "vtimer.h"
@@ -122,7 +125,12 @@ void init_traps(void)
 WRITE_SYSREG((HCPTR_CP_MASK & ~(HCPTR_CP(10) | HCPTR_CP(11))) | HCPTR_TTA,
  CPTR_EL2);
 
-/* Setup hypervisor traps */
+/* Setup hypervisor traps
+ *
+ * Note: HCR_TVM bit is also set for system-register write monitoring
+ * purposes (see vm_event_monitor_cr), but (for performance reasons) that's
+ * done selectively (i.e. on the scheduling tail, see p2m_restore_state).
+ */
 WRITE_SYSREG(HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
  HCR_TWE|HCR_TWI|HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP, HCR_EL2);
 isb();
@@ -398,7 +406,6 @@ static void inject_abt32_exception(struct cpu_user_regs 
*regs,
 far |= addr << 32;
 WRITE_SYSREG(far, FAR_EL1);
 WRITE_SYSREG(fsr, IFSR32_EL2);
-
 #endif
 }
 else
@@ -1686,6 +1693,61 @@ static void do_cp15_32(struct cpu_user_regs *regs,
 switch ( hsr.bits & HSR_CP32_REGS_MASK )
 {
 /*
+ * HCR_EL2.TVM / HCR.TVM
+ *
+ * ARMv7 (DDI 0406C.b): B1.14.13
+ * ARMv8 (DDI 0487A.e): D1-1569 Table D1-34
+ */
+case HSR_CPREG32(SCTLR):
+TVM_EMUL_VMEVT(regs, hsr, *r, VM_EVENT_ARM_SCTLR, SCTLR);
+break;
+case HSR_CPREG32(TTBR0_32):
+TVM_EMUL_VMEVT(regs, hsr, *r, VM_EVENT_ARM_TTBR0, TTBR0_32);
+break;
+case HSR_CPREG32(TTBR1_32):
+TVM_EMUL_VMEVT(regs, hsr, *r, VM_EVENT_ARM_TTBR1, TTBR1_32);
+break;
+case HSR_CPREG32(TTBCR):
+TVM_EMUL_VMEVT(regs, hsr, *r, VM_EVENT_ARM_TTBCR, TTBCR);
+break;
+case HSR_CPREG32(DACR):
+TVM_EMUL(regs, hsr, *r, DACR);
+break;
+case HSR_CPREG32(DFSR):
+TVM_EMUL(regs, hsr, *r, DFSR);
+break;
+case HSR_CPREG32(IFSR):
+TVM_EMUL(regs, hsr, *r, IFSR);
+break;
+case HSR_CPREG32(DFAR):
+TVM_EMUL(regs, hsr, *r, DFAR);
+break;
+case HSR_CPREG32(IFAR):
+TVM_EMUL(regs, hsr, *r, IFAR);
+break;
+case HSR_CPREG32(ADFSR):
+TVM_EMUL(regs, hsr, *r, ADFSR);
+break;
+case HSR_CPREG32(AIFSR):
+TVM_EMUL(regs, hsr, *r, AIFSR);
+break;
+case HSR_CPREG32(MAIR0):
+TVM_EMUL(regs, hsr, *r, MAIR0);
+break;
+case HSR_CPREG32(MAIR1):
+TVM_EMUL(regs, hsr, *r, MAIR1);
+break;
+case HSR_CPREG32(AMAIR0):
+TVM_EMUL(regs, hsr, *r, AMAIR0);
+break;
+case HSR_CPREG32(AMAIR1):
+TVM_EMUL(regs, hsr, *r, AMAIR1);
+break;
+case HSR_CPREG32(CONTEXTIDR):
+TVM_EMUL(regs, hsr, *r, CONTEXTIDR);
+break;
+
+/*
  * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
  *
  * ARMv7 (DDI 0406C.b): B4.1.22
@@ -1809,6 +1871,14 @@ static void do_cp15_32(struct cpu_user_regs *reg

[Xen-devel] [PATCH 0/1] ARM: Implement support for write-ctrlreg vm-events

[Corneliu ZUZU]

I've decided to include a cover-letter for this patch to ask for some feedback
on a few matters (and also to detail the changes that were done here instead
of writing that in the commit message).

First,
DETAILS OF CHANGES:

== Moved to common-side:
  1) monitor_ctrlreg_bitmask macro

  2) hvm_event_cr->vm_event_monitor_cr

  3) XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG handling (moved from X86
  arch_monitor_domctl_event to common monitor_domctl)

== ARM implementations:

  1) add VM_EVENT_ARM_{SCTLR,TTBR{0,1},TTBCR} as possible
vm_event_write_ctrlreg indexes

  2) add write_ctrlreg_* bits in arch_domain

  3) vm_event_monitor_get_capabilities updated to reflect support
for XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG

  4) on the scheduling tail, p2m_restore_state sets the HCR_TVM bit if
write_ctrlreg_enabled <> 0 for the domain

  5) traps.c, traps.h:
do_cp15_32, do_cp15_64 and do_sysreg now handle HCR_TVM traps,
emulating writes as needed and also calling vm_event_monitor_cr in
the process for the targeted monitored registers

The patch was currently tested on an ARMv8 (CONFIG_ARM_64) machine (after
applying a fix I'll send soon). With CONFIG_ARM_32 I only checked that Xen
clean-builds ok.

Then,
QUESTIONS (FOR VM-EVENTS & ARM MAINTAINERS ESPECIALLY):

Q1) Getting rid of the "#ifdef CONFIG_X86" @
monitor_domctl->XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG case.
To trap targeted control-registers writes, on ARM we check if
write_ctrlreg_enabled is non-zero on the scheduling tail and set/unset the
HCR_TVM bit accordingly (see the p2m_restore_state function).

I see that on X86 for CR3 that's done @ vmx_update_guest_cr by this code:
   
/* Trap CR3 updates if CR3 memory events are enabled. */
if ( v->domain->arch.monitor.write_ctrlreg_enabled &
 monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;

Couldn't we move this part on the X86 scheduling tail as it is done for ARM
in this changeset?

Q2) About VM_EVENT_FLAG_DENY

  Q2.1)
Doesn't it require sync = 1 (i.e. the vcpu to be paused) to work?
If so, shouldn't we call vm_event_register_write_resume only after checking
that VM_EVENT_FLAG_VCPU_PAUSED flag is set (vm_event_resume). Moreover, if
we do that, wouldn't it be 'cleaner' to rename
vm_event_register_write_resume->vm_event_deny, check for the
VM_EVENT_FLAG_DENY flag in vm_event_resume instead and call vm_event_deny
from there after this check?

  Q2.2)
VM_EVENT_FLAG_DENY functionality is not implemented w/ this change-set.
What is done instead is that the control-register write is done *before*
sending the vm-event (vm_event_put_request). This way, the user can
override the written register value after receiving the vm-event, which
in effect provides the same flexibility as VM_EVENT_FLAG_DENY does.
Using this strategy instead would simplify both Xen's code and the libxc
user's code.
Thoughts?

Q3) About security: i.e. making sure I don't crash Xen :)

When a system-register write is trapped in Xen due to the HCR_TVM bit,
besides triggering the vm-event where it's the case, we also need to emulate
this write operation. This is done with the WRITE_SYSREG* macros (see
TVM_EMUL/TVM_EMUL_VMEVT macros and their usage).

A potential security issue I see here is a case where WRITE_SYSREG* would
cause some kind of hardware fault/exception from EL2 that I did not take
into account. Since the value that must be written is determined by the
domain, that would mean that a 'bad' domain could willingly cause Xen to
crash.

I've read the ARM manuals and of course I've kept this in mind while doing
the emulation, but I still need some clarifications.
Concretely, this is the information I haven't found while reading the docs:
Q3.1)
If an AArch64 domain does:

MSR SCTLR_EL1, 

and given that SCTLR_EL1 is a 32-bit system register and that Xt is
a 64-bit register, what happens if the *high* 32-bits of Xt are not
set to zero? Are they simply ignored or would that cause a fault? If
so, would that fault happen before trapping to EL2?

Q3.2)
What would happen if a domain writes an invalid value to these regs.
By invalid I mean writing 1 to RES0/UNK/SBZP/RAZ/SBZP bits or
vice-versa (0 to RES1,etc). Again, could such behavior cause faults
when doing WRITE_SYSREG*? Note that this also concerns functions
like xc_vcpu_setcontext.

Q4) The ARMv7 manual doesn't include AMAIR0/AMAIR1 in the list of registers that
cause HYP traps on write when the HCR.TVM bit is set. Is that correct?
If so, should I surround parts relevant to them in this changeset w/ 
CONFIG_ARM_64?

Corneliu ZUZU (1)

[Xen-devel] [PATCH v3] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

This patch adds ARM support for guest-request monitor vm-events.
Note: on ARM hypercall instruction skipping must be done manually
by the caller. This will probably be changed in a future patch.

Summary of changes:
== Moved to common-side:
  * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86
  arch_monitor_domctl_event to common monitor_domctl)
  * hvm_event_guest_request->vm_event_monitor_guest_request
  * hvm_event_traps->vm_event_monitor_traps (also added target vcpu as param)
  * guest-request bits from X86 'struct arch_domain' (to common 'struct domain')
== ARM implementations:
  * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls
  vm_event_monitor_guest_request (as on X86)
  * arch_monitor_get_capabilities->vm_event_monitor_get_capabilities,
updated to reflect support for XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST
  * vm_event_init_domain (does nothing), vm_event_cleanup_domain
== Misc:
  * vm_event_fill_regs, no longer X86-specific. ARM-side implementation of this
  function currently does nothing, that will be added in a separate patch.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
Acked-by: Jan Beulich <jbeul...@suse.com>
Acked-by: Razvan Cojocaru <rcojoc...@bitdefender.com>
Acked-by: Stefano Stabellini <stefano.stabell...@eu.citrix.com>
Acked-by: Tamas K Lengyel <ta...@tklengyel.com>
---
Changed since v2:
  * added 'const' attribute to altp2m_vcpu_idx vcpu param
---
 xen/arch/arm/hvm.c  |  8 
 xen/arch/x86/hvm/event.c| 82 -
 xen/arch/x86/hvm/hvm.c  |  3 +-
 xen/arch/x86/monitor.c  | 18 +
 xen/arch/x86/vm_event.c |  1 +
 xen/common/monitor.c| 36 +++---
 xen/common/vm_event.c   | 60 ++
 xen/include/asm-arm/altp2m.h| 39 
 xen/include/asm-arm/monitor.h   |  8 +---
 xen/include/asm-arm/vm_event.h  | 19 +-
 xen/include/asm-x86/altp2m.h| 10 +++--
 xen/include/asm-x86/domain.h|  4 +-
 xen/include/asm-x86/hvm/event.h | 13 +--
 xen/include/asm-x86/monitor.h   | 23 
 xen/include/asm-x86/vm_event.h  | 23 
 xen/include/xen/sched.h |  6 +++
 xen/include/xen/vm_event.h  |  9 +
 17 files changed, 232 insertions(+), 130 deletions(-)
 create mode 100644 xen/include/asm-arm/altp2m.h

diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 056db1a..c01123a 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -72,6 +73,13 @@ long do_hvm_op(unsigned long op, 
XEN_GUEST_HANDLE_PARAM(void) arg)
 break;
 }
 
+case HVMOP_guest_request_vm_event:
+if ( guest_handle_is_null(arg) )
+vm_event_monitor_guest_request();
+else
+rc = -EINVAL;
+break;
+
 default:
 {
 gdprintk(XENLOG_DEBUG, "HVMOP op=%lu: not implemented\n", op);
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index d0b7d90..56c5514 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -6,6 +6,7 @@
  * Copyright (c) 2004, Intel Corporation.
  * Copyright (c) 2005, International Business Machines Corporation.
  * Copyright (c) 2008, Citrix Systems, Inc.
+ * Copyright (c) 2016, Bitdefender S.R.L.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -21,71 +22,32 @@
  */
 
 #include 
-#include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
-static int hvm_event_traps(uint8_t sync, vm_event_request_t *req)
-{
-int rc;
-struct vcpu *curr = current;
-struct domain *currd = curr->domain;
-
-rc = vm_event_claim_slot(currd, >vm_event->monitor);
-switch ( rc )
-{
-case 0:
-break;
-case -ENOSYS:
-/*
- * If there was no ring to handle the event, then
- * simply continue executing normally.
- */
-return 1;
-default:
-return rc;
-};
-
-if ( sync )
-{
-req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
-vm_event_vcpu_pause(curr);
-}
-
-if ( altp2m_active(currd) )
-{
-req->flags |= VM_EVENT_FLAG_ALTERNATE_P2M;
-req->altp2m_idx = vcpu_altp2m(curr).p2midx;
-}
-
-vm_event_fill_regs(req);
-vm_event_put_request(currd, >vm_event->monitor, req);
-
-return 1;
-}
-
 bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
 {
-struct arch_domain *currad = >domain->arch;
+struct vcpu *curr = current;
+struct arch_domain *ad = >domain->arch;
 unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index);
 
-if ( (currad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) &&
- (!(currad

Re: [Xen-devel] [PATCH v2] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

On 2/26/2016 2:31 PM, Jan Beulich wrote:

On 26.02.16 at 13:20, <cz...@bitdefender.com> wrote:

On 2/26/2016 2:14 PM, Razvan Cojocaru wrote:

On 02/26/2016 02:05 PM, Corneliu ZUZU wrote:

On 2/26/2016 1:56 PM, Jan Beulich wrote:

On 26.02.16 at 12:07, <cz...@bitdefender.com> wrote:

--- a/xen/include/asm-x86/altp2m.h
+++ b/xen/include/asm-x86/altp2m.h
@@ -15,8 +15,8 @@
 * this program; If not, see <http://www.gnu.org/licenses/>.
 */
-#ifndef _X86_ALTP2M_H
-#define _X86_ALTP2M_H
+#ifndef __ASM_X86_ALTP2M_H
+#define __ASM_X86_ALTP2M_H

Unrelated change? (No need to undo, but please don't mix such
into patches especially when they are quite large already anyway.)

Noted.


@@ -33,5 +33,9 @@ void altp2m_vcpu_initialise(struct vcpu *v);
void altp2m_vcpu_destroy(struct vcpu *v);
void altp2m_vcpu_reset(struct vcpu *v);
-#endif /* _X86_ALTP2M_H */
+static inline uint16_t altp2m_vcpu_idx(struct vcpu *v)

const

'const', as in:

+static inline const uint16_t altp2m_vcpu_idx(struct vcpu *v)

Since there's no functional difference between returning const uint6_t
and plain uint16_t, I assume that Jan meant "const struct vcpu *v".

I thought the functional difference would be when calling:

uint16_t idx = altp2m_vcpu_idx(v); // => can subsequently modify idx
const uint16_t idx = altp2m_vcpu_idx(v); // => cannot subsequently
modify idx (unless const is casted to non-const)

That's correct, but for this the return type of the function doesn't
matter. In fact I'd expect the compiler to warn about a meaningless
modifier placed on a function return type.

Jan




I find having

static inline const uint16_t altp2m_vcpu_idx(struct vcpu *v)

and subsequently writing

uint16_t idx = altp2m_vcpu_idx(v);

instead of

const uint16_t idx = altp2m_vcpu_idx(v);

without the compiler throwing at least a warning counter-intuitive.

Reminds me of something Linus said in an email: "it would be *stupid* 
for a C compiler to do anything but what we assume it does".


Noted, will change to 'const struct vcpu *v'.

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

Re: [Xen-devel] [PATCH v2] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

On 2/26/2016 2:14 PM, Razvan Cojocaru wrote:

On 02/26/2016 02:05 PM, Corneliu ZUZU wrote:

On 2/26/2016 1:56 PM, Jan Beulich wrote:

On 26.02.16 at 12:07, <cz...@bitdefender.com> wrote:

--- a/xen/include/asm-x86/altp2m.h
+++ b/xen/include/asm-x86/altp2m.h
@@ -15,8 +15,8 @@
* this program; If not, see <http://www.gnu.org/licenses/>.
*/
   -#ifndef _X86_ALTP2M_H
-#define _X86_ALTP2M_H
+#ifndef __ASM_X86_ALTP2M_H
+#define __ASM_X86_ALTP2M_H

Unrelated change? (No need to undo, but please don't mix such
into patches especially when they are quite large already anyway.)

Noted.


@@ -33,5 +33,9 @@ void altp2m_vcpu_initialise(struct vcpu *v);
   void altp2m_vcpu_destroy(struct vcpu *v);
   void altp2m_vcpu_reset(struct vcpu *v);
   -#endif /* _X86_ALTP2M_H */
+static inline uint16_t altp2m_vcpu_idx(struct vcpu *v)

const

'const', as in:

+static inline const uint16_t altp2m_vcpu_idx(struct vcpu *v)

Since there's no functional difference between returning const uint6_t
and plain uint16_t, I assume that Jan meant "const struct vcpu *v".


I thought the functional difference would be when calling:

uint16_t idx = altp2m_vcpu_idx(v); // => can subsequently modify idx
const uint16_t idx = altp2m_vcpu_idx(v); // => cannot subsequently 
modify idx (unless const is casted to non-const)





Cheers,
Razvan



Corneliu.

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

Re: [Xen-devel] [PATCH v2] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

On 2/26/2016 1:56 PM, Jan Beulich wrote:

On 26.02.16 at 12:07,  wrote:

--- a/xen/include/asm-x86/altp2m.h
+++ b/xen/include/asm-x86/altp2m.h
@@ -15,8 +15,8 @@
   * this program; If not, see .
   */
  
-#ifndef _X86_ALTP2M_H

-#define _X86_ALTP2M_H
+#ifndef __ASM_X86_ALTP2M_H
+#define __ASM_X86_ALTP2M_H

Unrelated change? (No need to undo, but please don't mix such
into patches especially when they are quite large already anyway.)


Noted.


@@ -33,5 +33,9 @@ void altp2m_vcpu_initialise(struct vcpu *v);
  void altp2m_vcpu_destroy(struct vcpu *v);
  void altp2m_vcpu_reset(struct vcpu *v);
  
-#endif /* _X86_ALTP2M_H */

+static inline uint16_t altp2m_vcpu_idx(struct vcpu *v)

const


'const', as in:

+static inline const uint16_t altp2m_vcpu_idx(struct vcpu *v)

?



With that, non-event x86 source changes
Acked-by: Jan Beulich 

Jan




Thanks,
Corneliu.

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

[Xen-devel] [PATCH v2] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

This patch adds ARM support for guest-request monitor vm-events.
Note: on ARM hypercall instruction skipping must be done manually
by the caller. This will probably be changed in a future patch.

Summary of changes:
== Moved to common-side:
  * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86
  arch_monitor_domctl_event to common monitor_domctl)
  * hvm_event_guest_request->vm_event_monitor_guest_request
  * hvm_event_traps->vm_event_monitor_traps (also added target vcpu as param)
  * guest-request bits from X86 'struct arch_domain' (to common 'struct domain')
== ARM implementations:
  * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls
  vm_event_monitor_guest_request (as on X86)
  * arch_monitor_get_capabilities->vm_event_monitor_get_capabilities,
updated to reflect support for XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST
  * vm_event_init_domain (does nothing), vm_event_cleanup_domain
== Misc:
  * vm_event_fill_regs, no longer X86-specific. ARM-side implementation of this
  function currently does nothing, that will be added in a separate patch.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
Changed since v1:
  * hvm_event_traps, hvm_event_guest_request moved to common/vm_event.c and
  and renamed to vm_event_monitor_traps and vm_event_monitor_guest_request
  * arch_monitor_get_capabilities moved to vm_event.h and renamed to
  vm_event_monitor_get_capabilities
  * arch_hvm_event_fill_regs replaced w/ existing vm_event_fill_regs
  (see commit adc75eba8b15c7103a010f736fe62e3fb2383964)
  * defined altp2m_active for ARM and altp2m_vcpu_idx to remove #ifdef in
  vm_event_monitor_traps
  * change bitfield members type to unsigned int
---
 xen/arch/arm/hvm.c  |  8 
 xen/arch/x86/hvm/event.c| 82 -
 xen/arch/x86/hvm/hvm.c  |  3 +-
 xen/arch/x86/monitor.c  | 18 +
 xen/arch/x86/vm_event.c |  1 +
 xen/common/monitor.c| 36 +++---
 xen/common/vm_event.c   | 60 ++
 xen/include/asm-arm/altp2m.h| 39 
 xen/include/asm-arm/monitor.h   |  8 +---
 xen/include/asm-arm/vm_event.h  | 19 +-
 xen/include/asm-x86/altp2m.h| 10 +++--
 xen/include/asm-x86/domain.h|  4 +-
 xen/include/asm-x86/hvm/event.h | 13 +--
 xen/include/asm-x86/monitor.h   | 23 
 xen/include/asm-x86/vm_event.h  | 23 
 xen/include/xen/sched.h |  6 +++
 xen/include/xen/vm_event.h  |  9 +
 17 files changed, 232 insertions(+), 130 deletions(-)
 create mode 100644 xen/include/asm-arm/altp2m.h

diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 056db1a..c01123a 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -72,6 +73,13 @@ long do_hvm_op(unsigned long op, 
XEN_GUEST_HANDLE_PARAM(void) arg)
 break;
 }
 
+case HVMOP_guest_request_vm_event:
+if ( guest_handle_is_null(arg) )
+vm_event_monitor_guest_request();
+else
+rc = -EINVAL;
+break;
+
 default:
 {
 gdprintk(XENLOG_DEBUG, "HVMOP op=%lu: not implemented\n", op);
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index d0b7d90..56c5514 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -6,6 +6,7 @@
  * Copyright (c) 2004, Intel Corporation.
  * Copyright (c) 2005, International Business Machines Corporation.
  * Copyright (c) 2008, Citrix Systems, Inc.
+ * Copyright (c) 2016, Bitdefender S.R.L.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -21,71 +22,32 @@
  */
 
 #include 
-#include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
-static int hvm_event_traps(uint8_t sync, vm_event_request_t *req)
-{
-int rc;
-struct vcpu *curr = current;
-struct domain *currd = curr->domain;
-
-rc = vm_event_claim_slot(currd, >vm_event->monitor);
-switch ( rc )
-{
-case 0:
-break;
-case -ENOSYS:
-/*
- * If there was no ring to handle the event, then
- * simply continue executing normally.
- */
-return 1;
-default:
-return rc;
-};
-
-if ( sync )
-{
-req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
-vm_event_vcpu_pause(curr);
-}
-
-if ( altp2m_active(currd) )
-{
-req->flags |= VM_EVENT_FLAG_ALTERNATE_P2M;
-req->altp2m_idx = vcpu_altp2m(curr).p2midx;
-}
-
-vm_event_fill_regs(req);
-vm_event_put_request(currd, >vm_event->monitor, req);
-
-return 1;
-}
-
 bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
 {
-struct arch_domain *currad = >domain->arch;
+struct vcpu *c

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

On 2/22/2016 12:14 PM, Jan Beulich wrote:

On 19.02.16 at 19:01,  wrote:

On 2/19/2016 7:15 PM, Jan Beulich wrote:

On 19.02.16 at 17:25,  wrote:

On 2/19/2016 4:26 PM, Jan Beulich wrote:

On 18.02.16 at 20:35,  wrote:

---
MAINTAINERS |   1 +
xen/arch/arm/hvm.c  |   8 +++
xen/arch/x86/hvm/event.c| 116 
++--
xen/arch/x86/hvm/hvm.c  |   1 +
xen/arch/x86/monitor.c  |  14 -
xen/arch/x86/vm_event.c |   1 +
xen/common/Makefile |   2 +-
xen/common/hvm/Makefile |   3 +-
xen/common/hvm/event.c  |  96 +

So here you _again_ try to introduce something HVM-ish for ARM.
Why? Why can't this code live in common/vm_event.c?

Are you referring to "xen/arch/arm/hvm.c"? If so, I did not introduce
that file, it was already there, all I did is add handling of
HVMOP_guest_request_vm_event to ARM-side's already existing do_hvm_op :).

No, I'm referring to your initial attempt to create arch/arm/hvm/...

I don't understand. Have I done that again with this patch?

Not the exact same thing, but something going along those same
lines of thinking.


On the "HVM-ish" note, is there some incompatibility between ARM and the
concept of HVM?

ARM guests are neither PV nor HVM right now, but somewhere in
the middle (PVHv2 may come closest).

I did not know that, but the fact that there is already "hvm-like" code
written for ARM didn't hint me towards that fact either :)
I'm aware that I'm far from familiar with the codebase right now, I'm
browsing more of the code these days and taking notes to try and
understand in depth at least the parts I'm sending contributions for.
I've already got some questions I want to post to the mailing list soon,
*including* exactly how the distinction between the guest-types comes
into play w/ the vm-events code.
Specifically, I'm talking for example about the following piece of code
from the X86 arch_monitor_get_capabilities:

  /*
   * At the moment only Intel HVM domains are supported. However, event
   * delivery could be extended to AMD and PV domains.
   */
  if ( !is_hvm_domain(d) || !cpu_has_vmx )
  return capabilities;

== "However, event delivery could be extended to AMD and PV domains".
This comment begs for questions like:
* what would be necessary to extend support to PV domains?
* can we really do this operation without hardware assisted
virtualization whatsoever? If not, how much can we do without that?
* what about pvh?

Since I have other questions like the above and I'll probably have more
while I'm trying to get a better picture of the code, would it be ok if
we defer addressing these issues to then?

Yes, you should definitely not hijack this thread for other, more
general inquiries.


Ok then, should I also understand then that for now it's ok to keep the 
"HVM-ish" hvm_event_traps & hvm_event_guest_request (I suppose you were 
referring to these 2 functions above) on the common-side event.c until 
we address these issues?
Or I could try to move them to common/vm_event.c as you suggest renamed 
to vm_event_traps & vm_event_guest_request and also rename 
arch_hvm_event_fill_regs to arch_vm_event_fill_regs (?).



--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -376,17 +376,15 @@ struct arch_domain
unsigned long *pirq_eoi_map;
unsigned long pirq_eoi_map_mfn;

-/* Monitor options */

+/* Arch-specific monitor options */
struct {
-unsigned int write_ctrlreg_enabled   : 4;
-unsigned int write_ctrlreg_sync  : 4;
-unsigned int write_ctrlreg_onchangeonly  : 4;
-unsigned int mov_to_msr_enabled  : 1;
-unsigned int mov_to_msr_extended : 1;
-unsigned int singlestep_enabled  : 1;
-unsigned int software_breakpoint_enabled : 1;
-unsigned int guest_request_enabled   : 1;
-unsigned int guest_request_sync  : 1;
+uint16_t write_ctrlreg_enabled   : 4;
+uint16_t write_ctrlreg_sync  : 4;
+uint16_t write_ctrlreg_onchangeonly  : 4;
+uint16_t mov_to_msr_enabled  : 1;
+uint16_t mov_to_msr_extended : 1;
+uint16_t singlestep_enabled  : 1;
+uint16_t software_breakpoint_enabled : 1;
} monitor;

What is this type change supposed to achieve in general, and in
particular in the context of this patch?

Some time before this patch there was a discussion I had w/ ARM
maintainer Ian Campbell who made the suggestion to try and move parts of
the monitor vm-events code to the common-side.
(you can find that discussion here:
https://www.mail-archive.com/xen-devel@lists.xen.org/msg54139.html).
In short, the reason for his suggestion was that that previous 

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

On 2/19/2016 8:42 PM, Tamas K Lengyel wrote:



On Fri, Feb 19, 2016 at 11:33 AM, Corneliu ZUZU <cz...@bitdefender.com 
<mailto:cz...@bitdefender.com>> wrote:


On 2/19/2016 8:27 PM, Tamas K Lengyel wrote:



On Fri, Feb 19, 2016 at 11:11 AM, Corneliu ZUZU
<cz...@bitdefender.com <mailto:cz...@bitdefender.com>> wrote:

On 2/19/2016 7:54 PM, Tamas K Lengyel wrote:



On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini
<stefano.stabell...@eu.citrix.com
<mailto:stefano.stabell...@eu.citrix.com>> wrote:

    On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> On 2/19/2016 6:05 PM, Andrew Cooper wrote:
> > On 19/02/16 16:00, Stefano Stabellini wrote:
    > > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote:
> > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote:
> > > > > > +
> > > > > > +if ( sync )
> > > > > > +{
> > > > > > + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
> > > > > > + vm_event_vcpu_pause(v);
> > > > > > +}
> > > > > > +
> > > > > > +#if CONFIG_X86
> > > > > > +if ( altp2m_active(d) )
> > > > > I would rather
> > > > >
> > > > > #define altp2m_active(d) (0)
> > > > >
> > > > > on ARM, removing the two ifdefs in this file.
> > > > Yeah, I actually wanted to get rid of that too
at some point, the
> > > > question is,
> > > > what do I do with "req->altp2m_idx =
vcpu_altp2m(v).p2midx"? I'm not
> > > > familiar
> > > > w/ altp2m design, maybe someone that knows more
of the internals of that
> > > > can
> > > > give a suggestion.
> > > If you #define altp2m_active to (0), gcc will
automatically avoid the if
> > > statement.
> > You will still get the compile error from ARM's
struct vcpu not having
> > altp2m information.
> >
> > ~Andrew
> >
>
> Yep.

Yes, you are right, especially given that Xen is
compiled -Wall -Werror.

How do you plan to introduce altp2m support on ARM? Is
there going to be
a struct altp2mvcpu on ARM too? It is not nice to access
stuff under
v->arch from common code. Maybe we need another
arch_blah function to
set altp2m_idx.


As altp2m could be implemented for ARM as well it might make
sense to start introducing bits and pieces that would make
it easier to do that work in the future. But I agree,
accessing v->arch directly from common is not a good way to
go about it.

Tamas


I am not at all familiar w/ altp2m at the moment, but I'll
try to look into it.
Since that doesn't relate so much with the code motion of
this changeset and it might not be that straightforward to
implement, would it be ok to leave the #ifdef CONFIG_X86
there for now and remove it in a separate patch?


We are trying to avoid having to do ifdefs where-ever possible.
So in this case too introducing arch-specific function(s) that
are empty for ARM would be more appropriate.

Tamas



I understand that, I was merely asking if it would be okay to do
it in another patch, because it didn't seem that straightforward.
More concretely, are you suggesting to:
* do the "#define altp2m_active(d) (0)" as Stefano suggested


That is easy enough to implement so go with that.

* incorporate "vcpu_altp2m(v).p2midx" into an arch_foo function


Implement an arch-specific function for this, say 
p2m_get_vcpu_altp2m_idx(v) which would just return 0 on ARM for now.


Tamas



Got it.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

On 2/19/2016 8:27 PM, Tamas K Lengyel wrote:



On Fri, Feb 19, 2016 at 11:11 AM, Corneliu ZUZU <cz...@bitdefender.com 
<mailto:cz...@bitdefender.com>> wrote:


On 2/19/2016 7:54 PM, Tamas K Lengyel wrote:



On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini
<stefano.stabell...@eu.citrix.com
<mailto:stefano.stabell...@eu.citrix.com>> wrote:

    On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> On 2/19/2016 6:05 PM, Andrew Cooper wrote:
> > On 19/02/16 16:00, Stefano Stabellini wrote:
    > > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote:
> > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote:
> > > > > > +
> > > > > > +if ( sync )
> > > > > > +{
> > > > > > + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
> > > > > > + vm_event_vcpu_pause(v);
> > > > > > +}
> > > > > > +
> > > > > > +#if CONFIG_X86
> > > > > > +if ( altp2m_active(d) )
> > > > > I would rather
> > > > >
> > > > > #define altp2m_active(d) (0)
> > > > >
> > > > > on ARM, removing the two ifdefs in this file.
> > > > Yeah, I actually wanted to get rid of that too at
some point, the
> > > > question is,
> > > > what do I do with "req->altp2m_idx =
vcpu_altp2m(v).p2midx"? I'm not
> > > > familiar
> > > > w/ altp2m design, maybe someone that knows more of
the internals of that
> > > > can
> > > > give a suggestion.
> > > If you #define altp2m_active to (0), gcc will
automatically avoid the if
> > > statement.
> > You will still get the compile error from ARM's struct
vcpu not having
> > altp2m information.
> >
> > ~Andrew
> >
>
> Yep.

Yes, you are right, especially given that Xen is compiled
-Wall -Werror.

How do you plan to introduce altp2m support on ARM? Is there
going to be
a struct altp2mvcpu on ARM too? It is not nice to access
stuff under
v->arch from common code. Maybe we need another arch_blah
function to
set altp2m_idx.


As altp2m could be implemented for ARM as well it might make
sense to start introducing bits and pieces that would make it
easier to do that work in the future. But I agree, accessing
v->arch directly from common is not a good way to go about it.

Tamas


I am not at all familiar w/ altp2m at the moment, but I'll try to
look into it.
Since that doesn't relate so much with the code motion of this
changeset and it might not be that straightforward to implement,
would it be ok to leave the #ifdef CONFIG_X86 there for now and
remove it in a separate patch?


We are trying to avoid having to do ifdefs where-ever possible. So in 
this case too introducing arch-specific function(s) that are empty for 
ARM would be more appropriate.


Tamas



I understand that, I was merely asking if it would be okay to do it in 
another patch, because it didn't seem that straightforward.

More concretely, are you suggesting to:
* do the "#define altp2m_active(d) (0)" as Stefano suggested
* incorporate "vcpu_altp2m(v).p2midx" into an arch_foo function
?

That seems easy enough to do. If so, how should I call this arch_foo 
function and where would it be appropriate to put it?


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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

On 2/19/2016 7:54 PM, Tamas K Lengyel wrote:



On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini 
<stefano.stabell...@eu.citrix.com 
<mailto:stefano.stabell...@eu.citrix.com>> wrote:


On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> On 2/19/2016 6:05 PM, Andrew Cooper wrote:
> > On 19/02/16 16:00, Stefano Stabellini wrote:
> > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote:
    > > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote:
> > > > > > +
> > > > > > +if ( sync )
> > > > > > +{
> > > > > > +req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
> > > > > > + vm_event_vcpu_pause(v);
> > > > > > +}
> > > > > > +
> > > > > > +#if CONFIG_X86
> > > > > > +if ( altp2m_active(d) )
> > > > > I would rather
> > > > >
> > > > > #define altp2m_active(d) (0)
> > > > >
> > > > > on ARM, removing the two ifdefs in this file.
> > > > Yeah, I actually wanted to get rid of that too at some
point, the
> > > > question is,
> > > > what do I do with "req->altp2m_idx =
vcpu_altp2m(v).p2midx"? I'm not
> > > > familiar
> > > > w/ altp2m design, maybe someone that knows more of the
internals of that
> > > > can
> > > > give a suggestion.
> > > If you #define altp2m_active to (0), gcc will automatically
avoid the if
> > > statement.
> > You will still get the compile error from ARM's struct vcpu
not having
> > altp2m information.
> >
> > ~Andrew
> >
>
> Yep.

Yes, you are right, especially given that Xen is compiled -Wall
-Werror.

How do you plan to introduce altp2m support on ARM? Is there going
to be
a struct altp2mvcpu on ARM too? It is not nice to access stuff under
v->arch from common code. Maybe we need another arch_blah function to
set altp2m_idx.


As altp2m could be implemented for ARM as well it might make sense to 
start introducing bits and pieces that would make it easier to do that 
work in the future. But I agree, accessing v->arch directly from 
common is not a good way to go about it.


Tamas


I am not at all familiar w/ altp2m at the moment, but I'll try to look 
into it.
Since that doesn't relate so much with the code motion of this changeset 
and it might not be that straightforward to implement, would it be ok to 
leave the #ifdef CONFIG_X86 there for now and remove it in a separate patch?


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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

On 2/19/2016 7:15 PM, Jan Beulich wrote:

On 19.02.16 at 17:25,  wrote:

On 2/19/2016 4:26 PM, Jan Beulich wrote:

On 18.02.16 at 20:35,  wrote:

---
   MAINTAINERS |   1 +
   xen/arch/arm/hvm.c  |   8 +++
   xen/arch/x86/hvm/event.c| 116 
++--
   xen/arch/x86/hvm/hvm.c  |   1 +
   xen/arch/x86/monitor.c  |  14 -
   xen/arch/x86/vm_event.c |   1 +
   xen/common/Makefile |   2 +-
   xen/common/hvm/Makefile |   3 +-
   xen/common/hvm/event.c  |  96 +

So here you _again_ try to introduce something HVM-ish for ARM.
Why? Why can't this code live in common/vm_event.c?

Are you referring to "xen/arch/arm/hvm.c"? If so, I did not introduce
that file, it was already there, all I did is add handling of
HVMOP_guest_request_vm_event to ARM-side's already existing do_hvm_op :).

No, I'm referring to your initial attempt to create arch/arm/hvm/...


I don't understand. Have I done that again with this patch?




On the "HVM-ish" note, is there some incompatibility between ARM and the
concept of HVM?

ARM guests are neither PV nor HVM right now, but somewhere in
the middle (PVHv2 may come closest).


I did not know that, but the fact that there is already "hvm-like" code 
written for ARM didn't hint me towards that fact either :)
I'm aware that I'm far from familiar with the codebase right now, I'm 
browsing more of the code these days and taking notes to try and 
understand in depth at least the parts I'm sending contributions for.
I've already got some questions I want to post to the mailing list soon, 
*including* exactly how the distinction between the guest-types comes 
into play w/ the vm-events code.
Specifically, I'm talking for example about the following piece of code 
from the X86 arch_monitor_get_capabilities:


/*
 * At the moment only Intel HVM domains are supported. However, event
 * delivery could be extended to AMD and PV domains.
 */
if ( !is_hvm_domain(d) || !cpu_has_vmx )
return capabilities;

== "However, event delivery could be extended to AMD and PV domains".
This comment begs for questions like:
* what would be necessary to extend support to PV domains?
* can we really do this operation without hardware assisted 
virtualization whatsoever? If not, how much can we do without that?

* what about pvh?

Since I have other questions like the above and I'll probably have more 
while I'm trying to get a better picture of the code, would it be ok if 
we defer addressing these issues to then?





--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -376,17 +376,15 @@ struct arch_domain
   unsigned long *pirq_eoi_map;
   unsigned long pirq_eoi_map_mfn;
   
-/* Monitor options */

+/* Arch-specific monitor options */
   struct {
-unsigned int write_ctrlreg_enabled   : 4;
-unsigned int write_ctrlreg_sync  : 4;
-unsigned int write_ctrlreg_onchangeonly  : 4;
-unsigned int mov_to_msr_enabled  : 1;
-unsigned int mov_to_msr_extended : 1;
-unsigned int singlestep_enabled  : 1;
-unsigned int software_breakpoint_enabled : 1;
-unsigned int guest_request_enabled   : 1;
-unsigned int guest_request_sync  : 1;
+uint16_t write_ctrlreg_enabled   : 4;
+uint16_t write_ctrlreg_sync  : 4;
+uint16_t write_ctrlreg_onchangeonly  : 4;
+uint16_t mov_to_msr_enabled  : 1;
+uint16_t mov_to_msr_extended : 1;
+uint16_t singlestep_enabled  : 1;
+uint16_t software_breakpoint_enabled : 1;
   } monitor;

What is this type change supposed to achieve in general, and in
particular in the context of this patch?

Some time before this patch there was a discussion I had w/ ARM
maintainer Ian Campbell who made the suggestion to try and move parts of
the monitor vm-events code to the common-side.
(you can find that discussion here:
https://www.mail-archive.com/xen-devel@lists.xen.org/msg54139.html).
In short, the reason for his suggestion was that that previous attempt
of mine to add guest-request support for ARM highlighted code
duplication between X86 and ARM if I were to leave the monitor vm-events
code as it was (that is, completely arch-specific). In an effort to
avoid that, I first submitted a 7 patch-series which tried to move as
much as possible to the common-side.
"As much as possible" meant guest-request, ctrl-reg write, single-step
and software breakpoints, all moved to the common-side. That also meant
introducing some Kconfigs to selectively activate some parts of that
now-common code, since not all of it was used on ARM at that moment.
Although conceptually that code could have remained on the common-side,
Tamas pointed out that we could get rid of 

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

On 2/19/2016 6:02 PM, Tamas K Lengyel wrote:



On Fri, Feb 19, 2016 at 7:26 AM, Jan Beulich > wrote:


>>> On 18.02.16 at 20:35, > wrote:
> ---
>  MAINTAINERS |   1 +
>  xen/arch/arm/hvm.c  |   8 +++
>  xen/arch/x86/hvm/event.c| 116
++--
>  xen/arch/x86/hvm/hvm.c  |   1 +
>  xen/arch/x86/monitor.c  |  14 -
>  xen/arch/x86/vm_event.c |   1 +
>  xen/common/Makefile |   2 +-
>  xen/common/hvm/Makefile |   3 +-
>  xen/common/hvm/event.c  |  96
+

So here you _again_ try to introduce something HVM-ish for ARM.
Why? Why can't this code live in common/vm_event.c?


I too am wondering if this is the right way to architect this. It 
would be better to move the guest-requested stuff into the generic 
vm_event component as it doesn't seem to be HVM specific other then it 
using an HVMOP hypercall to be triggered.


Tamas



Oh, that. "xen/common/hvm/event.c". I too don't know if it's the right 
way, but Jan, please at least don't attribute the way the code already 
is to me, I did not architect it.
And it's not human to expect doing everything perfectly in a single 
shot. If you're of the opinion that it should be in vm_event.c I will 
gladly try to put it there. Of course, that

could also be done in another patch.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

On 2/19/2016 4:26 PM, Jan Beulich wrote:

On 18.02.16 at 20:35,  wrote:

---
  MAINTAINERS |   1 +
  xen/arch/arm/hvm.c  |   8 +++
  xen/arch/x86/hvm/event.c| 116 ++--
  xen/arch/x86/hvm/hvm.c  |   1 +
  xen/arch/x86/monitor.c  |  14 -
  xen/arch/x86/vm_event.c |   1 +
  xen/common/Makefile |   2 +-
  xen/common/hvm/Makefile |   3 +-
  xen/common/hvm/event.c  |  96 +

So here you _again_ try to introduce something HVM-ish for ARM.
Why? Why can't this code live in common/vm_event.c?


Are you referring to "xen/arch/arm/hvm.c"? If so, I did not introduce 
that file, it was already there, all I did is add handling of 
HVMOP_guest_request_vm_event to ARM-side's already existing do_hvm_op :).
On the "HVM-ish" note, is there some incompatibility between ARM and the 
concept of HVM?





--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -376,17 +376,15 @@ struct arch_domain
  unsigned long *pirq_eoi_map;
  unsigned long pirq_eoi_map_mfn;
  
-/* Monitor options */

+/* Arch-specific monitor options */
  struct {
-unsigned int write_ctrlreg_enabled   : 4;
-unsigned int write_ctrlreg_sync  : 4;
-unsigned int write_ctrlreg_onchangeonly  : 4;
-unsigned int mov_to_msr_enabled  : 1;
-unsigned int mov_to_msr_extended : 1;
-unsigned int singlestep_enabled  : 1;
-unsigned int software_breakpoint_enabled : 1;
-unsigned int guest_request_enabled   : 1;
-unsigned int guest_request_sync  : 1;
+uint16_t write_ctrlreg_enabled   : 4;
+uint16_t write_ctrlreg_sync  : 4;
+uint16_t write_ctrlreg_onchangeonly  : 4;
+uint16_t mov_to_msr_enabled  : 1;
+uint16_t mov_to_msr_extended : 1;
+uint16_t singlestep_enabled  : 1;
+uint16_t software_breakpoint_enabled : 1;
  } monitor;

What is this type change supposed to achieve in general, and in
particular in the context of this patch?


Some time before this patch there was a discussion I had w/ ARM 
maintainer Ian Campbell who made the suggestion to try and move parts of 
the monitor vm-events code to the common-side.
(you can find that discussion here: 
https://www.mail-archive.com/xen-devel@lists.xen.org/msg54139.html).
In short, the reason for his suggestion was that that previous attempt 
of mine to add guest-request support for ARM highlighted code 
duplication between X86 and ARM if I were to leave the monitor vm-events 
code as it was (that is, completely arch-specific). In an effort to 
avoid that, I first submitted a 7 patch-series which tried to move as 
much as possible to the common-side.
"As much as possible" meant guest-request, ctrl-reg write, single-step 
and software breakpoints, all moved to the common-side. That also meant 
introducing some Kconfigs to selectively activate some parts of that 
now-common code, since not all of it was used on ARM at that moment. 
Although conceptually that code could have remained on the common-side, 
Tamas pointed out that we could get rid of the Kconfigs (which in his 
opinion bloated the code) by only moving at the moment what is 
implemented on both X86 and ARM. As for the rest, he noted that when I 
add implementations for the other monitor vm-events on ARM as well, I 
could move that to common as well. The above is a result of that - 
guest-request is implemented on both X86 and ARM with this patch, hence 
I also moved it to common.



--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -1,5 +1,9 @@
  /*
- * event.h: Hardware virtual machine assist events.
+ * include/asm-x86/hvm/event.h
+ *
+ * Arch-specific hardware virtual machine event abstractions.
+ *
+ * Copyright (c) 2016, Bitdefender S.R.L.

Is this true? Namely, was _all_ of the code here written by people
on your company, including what may have got moved here from
elsewhere?

Jan




My bad. Removing that line would be satisfactory, yes?

Corneliu.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

On 2/19/2016 6:05 PM, Andrew Cooper wrote:

On 19/02/16 16:00, Stefano Stabellini wrote:

On Fri, 19 Feb 2016, Corneliu ZUZU wrote:

On 2/19/2016 3:49 PM, Stefano Stabellini wrote:

On Thu, 18 Feb 2016, Corneliu ZUZU wrote:

+
+if ( sync )
+{
+req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
+vm_event_vcpu_pause(v);
+}
+
+#if CONFIG_X86
+if ( altp2m_active(d) )

I would rather

#define altp2m_active(d) (0)

on ARM, removing the two ifdefs in this file.

Yeah, I actually wanted to get rid of that too at some point, the question is,
what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm not familiar
w/ altp2m design, maybe someone that knows more of the internals of that can
give a suggestion.

If you #define altp2m_active to (0), gcc will automatically avoid the if
statement.

You will still get the compile error from ARM's struct vcpu not having
altp2m information.

~Andrew



Yep.

Corneliu.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

On 2/19/2016 3:49 PM, Stefano Stabellini wrote:

On Thu, 18 Feb 2016, Corneliu ZUZU wrote:

+
+if ( sync )
+{
+req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
+vm_event_vcpu_pause(v);
+}
+
+#if CONFIG_X86
+if ( altp2m_active(d) )

I would rather

#define altp2m_active(d) (0)

on ARM, removing the two ifdefs in this file.


Yeah, I actually wanted to get rid of that too at some point, the 
question is, what do I do with "req->altp2m_idx = 
vcpu_altp2m(v).p2midx"? I'm not familiar w/ altp2m design, maybe someone 
that knows more of the internals of that can give a suggestion.


Thanks,
Corneliu.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

On 2/18/2016 11:25 PM, Tamas K Lengyel wrote:



On Thu, Feb 18, 2016 at 1:08 PM, Razvan Cojocaru 
<rcojoc...@bitdefender.com <mailto:rcojoc...@bitdefender.com>> wrote:


On 02/18/2016 09:35 PM, Corneliu ZUZU wrote:
> This patch adds ARM support for guest-request monitor vm-events.
>
> Summary of changes:
> == Moved to common-side:
>   * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86
>   arch_monitor_domctl_event to common monitor_domctl)
>   * hvm_event_guest_request, hvm_event_traps (also added target
vcpu as param)
>   * guest-request bits from X86 'struct arch_domain' (to common
'struct domain')
> == ARM implementations:
>   * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls
>   hvm_event_guest_request (as on X86)
>   * arch_monitor_get_capabilities: updated to reflect support for
>   XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST
>   * vm_event_init_domain (does nothing), vm_event_cleanup_domain
> == Misc:
>   * hvm_event_fill_regs renamed to arch_hvm_event_fill_regs, no
longer
>   X86-specific. ARM-side implementation of this function
currently does
>   nothing, that will be added in a separate patch.

We should probably take into account what happens with Tamas'
"vm_event:
consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs" patch
here.
That patch already affects hvm_event_fill_regs().


Well it seems one of us will have to rebase depending which patch gets 
accepted & merged first. The conflict is minimal so it's not a major 
issue. If my patch gets merged first then just have to introduce the 
empty function in the ARM header with the new name.


Tamas



Okay then, for me it's fine either way.

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

[Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

[Corneliu ZUZU]

This patch adds ARM support for guest-request monitor vm-events.

Summary of changes:
== Moved to common-side:
  * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86
  arch_monitor_domctl_event to common monitor_domctl)
  * hvm_event_guest_request, hvm_event_traps (also added target vcpu as param)
  * guest-request bits from X86 'struct arch_domain' (to common 'struct domain')
== ARM implementations:
  * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls
  hvm_event_guest_request (as on X86)
  * arch_monitor_get_capabilities: updated to reflect support for
  XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST
  * vm_event_init_domain (does nothing), vm_event_cleanup_domain
== Misc:
  * hvm_event_fill_regs renamed to arch_hvm_event_fill_regs, no longer
  X86-specific. ARM-side implementation of this function currently does
  nothing, that will be added in a separate patch.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 MAINTAINERS |   1 +
 xen/arch/arm/hvm.c  |   8 +++
 xen/arch/x86/hvm/event.c| 116 ++--
 xen/arch/x86/hvm/hvm.c  |   1 +
 xen/arch/x86/monitor.c  |  14 -
 xen/arch/x86/vm_event.c |   1 +
 xen/common/Makefile |   2 +-
 xen/common/hvm/Makefile |   3 +-
 xen/common/hvm/event.c  |  96 +
 xen/common/monitor.c|  31 +--
 xen/include/asm-arm/hvm/event.h |  41 ++
 xen/include/asm-arm/monitor.h   |   7 ++-
 xen/include/asm-arm/vm_event.h  |   4 +-
 xen/include/asm-x86/domain.h|  18 +++
 xen/include/asm-x86/hvm/event.h |  43 ++-
 xen/include/xen/hvm/event.h |  41 ++
 xen/include/xen/sched.h |   6 +++
 17 files changed, 299 insertions(+), 134 deletions(-)
 create mode 100644 xen/common/hvm/event.c
 create mode 100644 xen/include/asm-arm/hvm/event.h
 create mode 100644 xen/include/xen/hvm/event.h

diff --git a/MAINTAINERS b/MAINTAINERS
index cd4da04..768ad32 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -356,6 +356,7 @@ M:  Tamas K Lengyel <ta...@tklengyel.com>
 S: Supported
 F: xen/common/vm_event.c
 F: xen/common/mem_access.c
+F: xen/common/hvm/event.c
 F: xen/common/monitor.c
 F: xen/arch/x86/hvm/event.c
 F: xen/arch/x86/monitor.c
diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 056db1a..767edd1 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -72,6 +73,13 @@ long do_hvm_op(unsigned long op, 
XEN_GUEST_HANDLE_PARAM(void) arg)
 break;
 }
 
+case HVMOP_guest_request_vm_event:
+if ( guest_handle_is_null(arg) )
+hvm_event_guest_request();
+else
+rc = -EINVAL;
+break;
+
 default:
 {
 gdprintk(XENLOG_DEBUG, "HVMOP op=%lu: not implemented\n", op);
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index cb9c152..bbb0dc8 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -6,6 +6,7 @@
  * Copyright (c) 2004, Intel Corporation.
  * Copyright (c) 2005, International Business Machines Corporation.
  * Copyright (c) 2008, Citrix Systems, Inc.
+ * Copyright (c) 2016, Bitdefender S.R.L.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -20,103 +21,32 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include 
-#include 
+#include 
 #include 
 #include 
-#include 
 #include 
 
-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->data.regs.x86.rax = regs->eax;
-req->data.regs.x86.rcx = regs->ecx;
-req->data.regs.x86.rdx = regs->edx;
-req->data.regs.x86.rbx = regs->ebx;
-req->data.regs.x86.rsp = regs->esp;
-req->data.regs.x86.rbp = regs->ebp;
-req->data.regs.x86.rsi = regs->esi;
-req->data.regs.x86.rdi = regs->edi;
-
-req->data.regs.x86.r8  = regs->r8;
-req->data.regs.x86.r9  = regs->r9;
-req->data.regs.x86.r10 = regs->r10;
-req->data.regs.x86.r11 = regs->r11;
-req->data.regs.x86.r12 = regs->r12;
-req->data.regs.x86.r13 = regs->r13;
-req->data.regs.x86.r14 = regs->r14;
-req->data.regs.x86.r15 = regs->r15;
-
-req->data.regs.x86.rflags = regs->eflags;
-req->data.regs.x86.rip= regs->eip;
-
-req->data.regs.x86.msr_efer = curr->arch.hvm_vcpu.guest_efer;
-req->data.regs.x86.cr0 = curr->arch.hvm_vcpu.guest_cr[0];
-req->data.regs.x86.cr3 = curr->arch.hvm_vcpu.guest_cr[3];
-req->data.regs.x86.cr4 = curr->arch.hvm_vcpu.gues

[Xen-devel] [PATCH] x86/monitor: minor left-shift undefined behavior checks

[Corneliu ZUZU]

This minor patch adds a range-check to avoid left-shift caused undefined
behavior. Also replaces '1 <<' w/ '1U <<' @ x86 monitor.h in an effort to avoid
a future potential '1 << 31' that would cause a similar issue.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/x86/monitor.c| 13 +
 xen/include/asm-x86/monitor.h | 10 +-
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index a507edb..b4bd008 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -32,10 +32,15 @@ int arch_monitor_domctl_event(struct domain *d,
 {
 case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG:
 {
-unsigned int ctrlreg_bitmask =
-monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
-bool_t old_status =
-!!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask);
+unsigned int ctrlreg_bitmask;
+bool_t old_status;
+
+/* sanity check: avoid left-shift undefined behavior */
+if ( unlikely(mop->u.mov_to_cr.index > 31) )
+return -EINVAL;
+
+ctrlreg_bitmask = monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
+old_status = !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask);
 
 if ( unlikely(old_status == requested_status) )
 return -EEXIST;
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index c789f71..f1bf4bd 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -40,14 +40,14 @@ static inline uint32_t arch_monitor_get_capabilities(struct 
domain *d)
 if ( !is_hvm_domain(d) || !cpu_has_vmx )
 return capabilities;
 
-capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
-   (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
-   (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
-   (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
+capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
+   (1U << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
+   (1U << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
+   (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
 
 /* Since we know this is on VMX, we can just call the hvm func */
 if ( hvm_is_singlestep_supported() )
-capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
+capabilities |= (1U << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
 
 return capabilities;
 }
-- 
2.5.0


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

[Xen-devel] [PATCH] x86/hvm_event: fix uninitialized struct field usage introduced by c/s f5365e6

[Corneliu ZUZU]

c/s f5365e6: "xen/vm-events: Move parts of monitor_domctl code to common-side",
introduced a use without initialization issue.
hvm_event_breakpoint calls hvm_event_traps() and if sync is true that
ors some bits into req->flags which was never initialised.
Reported by Coverity Scan.

Initializes req @ hvm_event_breakpoint entry.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/x86/hvm/event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index 874a36c..cb9c152 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -173,7 +173,7 @@ int hvm_event_breakpoint(unsigned long rip,
 {
 struct vcpu *curr = current;
 struct arch_domain *ad = >domain->arch;
-vm_event_request_t req;
+vm_event_request_t req = {};
 
 switch ( type )
 {
-- 
2.5.0


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

Re: [Xen-devel] Uninitialized variables in hvm_event_breakpoint (Re: New Defects reported by Coverity Scan for XenProject)

[Corneliu ZUZU]

On 2/18/2016 12:01 PM, Ian Campbell wrote:

On Wed, 2016-02-17 at 16:02 -0800, scan-ad...@coverity.com wrote:

Hi,

Please find the latest report on new defect(s) introduced to XenProject
found with Coverity Scan.

1 new defect(s) introduced to XenProject found with Coverity Scan.
4 defect(s), reported by Coverity Scan earlier, were marked fixed in the
recent build analyzed by Coverity Scan.

New defect(s) Reported-by: Coverity Scan
Showing 1 of 1 defect(s)


** CID 1353192:  Uninitialized variables  (UNINIT)
/xen/arch/x86/hvm/event.c: 176 in hvm_event_breakpoint()

This appears to have been introduced by:
 commit
 557c7873f35aa39bd84977b28948457b1b342f92
 Author: Corneliu ZUZU <czuzu@bitdef
 ender.com>
 Date:   Mon Feb 15 14:14:16 2016 +0100

 x86: merge 2 hvm_event_... functions into 1
 
 This patch merges almost identical functions hvm_event_int3 and

 hvm_event_single_step into a single function called 
hvm_event_breakpoint.
 Also fixes event.c file header comment in the process.
 
 Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>

 Acked-by: Razvan Cojocaru <rcojoc...@bitdefender.com>
 Acked-by: Jan Beulich <jbeul...@suse.com>


hvm_event_breakpoint calls hvm_event_traps() and if sync is true that
ors some bits into req->flags which was never initialised.



_
___
*** CID 1353192:  Uninitialized variables  (UNINIT)
/xen/arch/x86/hvm/event.c: 176 in hvm_event_breakpoint()
170
171 int hvm_event_breakpoint(unsigned long rip,
172  enum hvm_event_breakpoint_type type)
173 {
174 struct vcpu *curr = current;
175 struct arch_domain *ad = >domain->arch;

 CID 1353192:  Uninitialized variables  (UNINIT)
 Declaring variable "req" without initializer.

176 vm_event_request_t req;
177
178 switch ( type )
179 {
180 case HVM_EVENT_SOFTWARE_BREAKPOINT:
181 if ( !ad->monitor.software_breakpoint_enabled )


_
___
To view the defects in Coverity Scan visit, https://scan.coverity.com/pro
jects/xenproject?tab=overview

To manage Coverity Scan email notifications for
"ian.campb...@citrix.com", click https://scan.coverity.com/subscriptions/
edit?email=ian.campbell%40citrix.com=1ce0fc428b9f94f66fd8d1ecf6cbb7
6a



Sorry, my bad, I didn't know struct-initialization using labels sets all 
the other fields to zero.

Shall I submit a fix for this issue?

Corneliu.

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

Re: [Xen-devel] [PATCH v5 0/2] Vm-events: move monitor vm-events code to common-side.

[Corneliu ZUZU]

On 2/17/2016 5:09 PM, Konrad Rzeszutek Wilk wrote:

On Wed, Feb 17, 2016 at 09:33:58AM +0200, Corneliu ZUZU wrote:

This patch series is an attempt to move some of the monitor vm-events code to
the common-side. Done to make it easier to move additional parts that can be
moved to common when ARM-side implementations are to be added.

Both applied.

Patches summary:
1. Fix file comment
 Acked-by: Stefano Stabellini <stefano.stabell...@eu.citrix.com>
2. Move monitor_domctl to common-side.
 Acked-by: Stefano Stabellini <stefano.stabell...@eu.citrix.com>
 Acked-by: Razvan Cojocaru <rcojoc...@bitdefender.com>

Note: ARM support for guest-request, control-register write monitor vm-events
will follow after review of this patch-series.

---
Changed since v4:
   1/2: nothing changed
   2/2: arch_monitor_domctl_event:
 replaced !old_status w/ requested_status (equivalent but more readable)

Corneliu ZUZU (2):
   xen/arm: fix file comments
   xen/vm-events: Move parts of monitor_domctl code to common-side.

  MAINTAINERS   |   1 +
  xen/arch/arm/hvm.c|  29 +++-
  xen/arch/x86/monitor.c| 153 +++---
  xen/common/Makefile   |   1 +
  xen/common/domctl.c   |   2 +-
  xen/common/monitor.c  |  69 +++
  xen/include/asm-arm/monitor.h |  30 +++--
  xen/include/asm-x86/monitor.h |  53 +--
  xen/include/xen/monitor.h |  30 +
  9 files changed, 245 insertions(+), 123 deletions(-)
  create mode 100644 xen/common/monitor.c
  create mode 100644 xen/include/xen/monitor.h

--
2.5.0


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

Thanks!

Corneliu.

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

Re: [Xen-devel] EXTRA_CFLAGS when compiling Xen

[Corneliu ZUZU]

On 2/17/2016 3:54 PM, Jan Beulich wrote:

On 17.02.16 at 14:11,  wrote:

On 2/17/2016 2:43 PM, Jan Beulich wrote:

On 17.02.16 at 13:09,  wrote:

On 2/17/2016 12:34 PM, Jan Beulich wrote:

The reason I need this is to pass '-save-temps' to GCC, I want to inspect
some code
and it would be easier to do that on the preprocessed files.

... there's absolutely no need to for a case like this, at least as
long as the xen/ subtree is where you want to do this.
xen/Rules.mk has rules for what you want (and also for
producing the intermediate assembly file), just that you can't
achakieve this by invoking me from the top level directory -
you need to run make directly in xen/ and manually specify
the intended target (including leading sub-directories).

I wouldn't want to needlessly insist, but of course a canonical way to
do this would be preferred.
I do see the %.i targets there in Rules.mk invoking the preprocessor,
but I haven't yet figured how to make those execute.

Could you detail what make args would activate execution of the %.i targets?

Makes me wonder how the rather detailed reply I've already given
(which also answers this question) hasn't been sufficient.

I don't see why there's need for insults when one asks politely.

I'm sorry, but your reply gave the impression that you didn't read
mine, which I considered an attempt to be wasting my time.


Of course, 'rather detailed' is always a rather subjective assessment.
Wonder no more. The original response:
* doesn't specify what additional make arguments/environment vars should
be set if I'm to run make from ./xen/ rather then ./

Why would I mention env vars when there are none to set (at least
I don't need any that aren't set in my ./.config).


* doesn't make it clear what you mean by "intended target". I suppose
you weren't suggesting to manually run make for *each*
source file I want to preprocess one by one, since I was asking how to
do that for all sources ('make dist-xen'...)

"I want to inspect some code" didn't read like you needed to inspect
every translation unit; I'm sorry if this ended up being a mis-
interpretation. I don't think tree-wide saving of intermediate files
would work reliably, since iirc paths get dropped from file names
when determining the names of the intermediate files.

Jan




I'm sorry too :) I honestly thought you were suggesting that there is a 
target which would build Xen (as I would build it normally, i.e. 'make 
dist-xen') and also call the %.i targets in the process. I spent like 
half an hour trying to figure out what that target was before asking for 
details, heh, it didn't cross my mind to ask you directly if you meant 
the actual files to be the targets. In my mind it seemed obvious what I 
mean, but now that you mention it I wasn't actually that clear, I 
apologize for that.


Regards,
Corneliu.

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

Re: [Xen-devel] EXTRA_CFLAGS when compiling Xen

[Corneliu ZUZU]

On 2/17/2016 2:43 PM, Jan Beulich wrote:

On 17.02.16 at 13:09,  wrote:

On 2/17/2016 12:34 PM, Jan Beulich wrote:

The reason I need this is to pass '-save-temps' to GCC, I want to inspect
some code
and it would be easier to do that on the preprocessed files.

... there's absolutely no need to for a case like this, at least as
long as the xen/ subtree is where you want to do this.
xen/Rules.mk has rules for what you want (and also for
producing the intermediate assembly file), just that you can't
achakieve this by invoking me from the top level directory -
you need to run make directly in xen/ and manually specify
the intended target (including leading sub-directories).

I wouldn't want to needlessly insist, but of course a canonical way to
do this would be preferred.
I do see the %.i targets there in Rules.mk invoking the preprocessor,
but I haven't yet figured how to make those execute.

Could you detail what make args would activate execution of the %.i targets?

Makes me wonder how the rather detailed reply I've already given
(which also answers this question) hasn't been sufficient.

Jan



I don't see why there's need for insults when one asks politely.
Of course, 'rather detailed' is always a rather subjective assessment.
Wonder no more. The original response:
* doesn't specify what additional make arguments/environment vars should 
be set if I'm to run make from ./xen/ rather then ./
* doesn't make it clear what you mean by "intended target". I suppose 
you weren't suggesting to manually run make for *each*
source file I want to preprocess one by one, since I was asking how to 
do that for all sources ('make dist-xen'...)


Thanks anyway,
Corneliu.

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

Re: [Xen-devel] EXTRA_CFLAGS when compiling Xen

[Corneliu ZUZU]

On 2/17/2016 12:34 PM, Jan Beulich wrote:



The reason I need this is to pass '-save-temps' to GCC, I want to inspect
some code
and it would be easier to do that on the preprocessed files.

... there's absolutely no need to for a case like this, at least as
long as the xen/ subtree is where you want to do this.
xen/Rules.mk has rules for what you want (and also for
producing the intermediate assembly file), just that you can't
achieve this by invoking make from the top level directory -
you need to run make directly in xen/ and manually specify
the intended target (including leading sub-directories).

Jan



I wouldn't want to needlessly insist, but of course a canonical way to 
do this would be preferred.
I do see the %.i targets there in Rules.mk invoking the preprocessor, 
but I haven't yet figured how to make those execute.


Could you detail what make args would activate execution of the %.i targets?
This is not so important since for now Razvan's suggestion does the 
trick (or adding %.i as a dependency for %.o in Rules.mk),
but I thought it would be nice for future reference, so only respond 
when/if you have the time/disposition.


Thanks,
Corneliu.

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

Re: [Xen-devel] EXTRA_CFLAGS when compiling Xen

[Corneliu ZUZU]

On 2/17/2016 12:23 PM, Razvan Cojocaru wrote:

Could anyone tell me if and how I could pass some GCC extra compilation
flags when building Xen (i.e. make dist-xen)?
I tried:
* when running make: passing EXTRA_CFLAGS
* when running make: passing CFLAGS (this "works" but overrides the rest
of the CFLAGS)
* when running ./configure: passing --extra-cflags.
None works.

Is there an established modality for this or must I manually modify the
makefiles?
The reason I need this is to pass '-save-temps' to GCC, I want to
inspect some code
and it would be easier to do that on the preprocessed files.

Does editing Config.mk not work?


I was wondering also if there is a "proper" way to do that, but that 
easily does the job, thanks.


For whoever else needs this, I simply added CFLAGS += -save-temps=obj in 
Config.mk (see similar "CFLAGS +=" lines).


Corneliu.

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

[Xen-devel] EXTRA_CFLAGS when compiling Xen

[Corneliu ZUZU]

Hi all,

Could anyone tell me if and how I could pass some GCC extra compilation flags 
when building Xen (i.e. make dist-xen)?
I tried:
* when running make: passing EXTRA_CFLAGS
* when running make: passing CFLAGS (this "works" but overrides the rest of the 
CFLAGS)
* when running ./configure: passing --extra-cflags.
None works.

Is there an established modality for this or must I manually modify the 
makefiles?
The reason I need this is to pass '-save-temps' to GCC, I want to inspect some 
code
and it would be easier to do that on the preprocessed files.

Thank you,
Corneliu.


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

[Xen-devel] [PATCH v5 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

This patch moves monitor_domctl to common-side.
Purpose: move what's common to common, prepare for implementation
of such vm-events on ARM.

* move get_capabilities to arch-side => arch_monitor_get_capabilities.
* add arch-side monitor op handling function => arch_monitor_domctl_op.
  e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
* add arch-side monitor event handling function => arch_monitor_domctl_event.
  e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event enable/disable
* remove status_check

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
Acked-by: Stefano Stabellini <stefano.stabell...@eu.citrix.com>
Acked-by: Razvan Cojocaru <rcojoc...@bitdefender.com>

---
Changed since v4:
  * arch_monitor_domctl_event @ x86/monitor.c:
replaced !old_status w/ requested_status (equivalent but more readable)
---
 MAINTAINERS   |   1 +
 xen/arch/x86/monitor.c| 153 +++---
 xen/common/Makefile   |   1 +
 xen/common/domctl.c   |   2 +-
 xen/common/monitor.c  |  69 +++
 xen/include/asm-arm/monitor.h |  30 +++--
 xen/include/asm-x86/monitor.h |  53 +--
 xen/include/xen/monitor.h |  30 +
 8 files changed, 217 insertions(+), 122 deletions(-)
 create mode 100644 xen/common/monitor.c
 create mode 100644 xen/include/xen/monitor.h

diff --git a/MAINTAINERS b/MAINTAINERS
index f07384c..5cbb1dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -355,6 +355,7 @@ M:  Tamas K Lengyel <ta...@tklengyel.com>
 S: Supported
 F: xen/common/vm_event.c
 F: xen/common/mem_access.c
+F: xen/common/monitor.c
 F: xen/arch/x86/hvm/event.c
 F: xen/arch/x86/monitor.c
 F: xen/arch/*/vm_event.c
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 1d43880..a507edb 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -1,9 +1,10 @@
 /*
  * arch/x86/monitor.c
  *
- * Architecture-specific monitor_op domctl handler.
+ * Arch-specific monitor_op domctl handler.
  *
  * Copyright (c) 2015 Tamas K Lengyel (ta...@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public
@@ -18,87 +19,14 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include 
-#include 
-#include 
-#include 
 #include 
-#include 
-#include 
+#include 
 
-/*
- * Sanity check whether option is already enabled/disabled
- */
-static inline
-int status_check(struct xen_domctl_monitor_op *mop, bool_t status)
-{
-bool_t requested_status = (mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE);
-
-if ( status == requested_status )
-return -EEXIST;
-
-return 0;
-}
-
-static inline uint32_t get_capabilities(struct domain *d)
-{
-uint32_t capabilities = 0;
-
-/*
- * At the moment only Intel HVM domains are supported. However, event
- * delivery could be extended to AMD and PV domains.
- */
-if ( !is_hvm_domain(d) || !cpu_has_vmx )
-return capabilities;
-
-capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
-   (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
-   (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
-   (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
-
-/* Since we know this is on VMX, we can just call the hvm func */
-if ( hvm_is_singlestep_supported() )
-capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
-
-return capabilities;
-}
-
-int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
+int arch_monitor_domctl_event(struct domain *d,
+  struct xen_domctl_monitor_op *mop)
 {
-int rc;
 struct arch_domain *ad = >arch;
-uint32_t capabilities = get_capabilities(d);
-
-if ( current->domain == d ) /* no domain_pause() */
-return -EPERM;
-
-rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event);
-if ( rc )
-return rc;
-
-switch ( mop->op )
-{
-case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
-mop->event = capabilities;
-return 0;
-
-case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
-domain_pause(d);
-ad->mem_access_emulate_each_rep = !!mop->event;
-domain_unpause(d);
-return 0;
-}
-
-/*
- * Sanity check
- */
-if ( mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE &&
- mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE )
-return -EOPNOTSUPP;
-
-/* Check if event type is available. */
-if ( !(capabilities & (1 << mop->event)) )
-return -EOPNOTSUPP;
+bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
 
 switch ( mop->event )
 {
@@ -106,1

[Xen-devel] [PATCH v5 1/2] xen/arm: fix file comments

[Corneliu ZUZU]

Add file header comment and local variable block @ EOF
of xen/arch/arm/hvm.c.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
Acked-by: Stefano Stabellini <stefano.stabell...@eu.citrix.com>

---
Changed since v4: 
---
 xen/arch/arm/hvm.c | 29 -
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 5fd0753..056db1a 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -1,3 +1,21 @@
+/*
+ * arch/arm/hvm.c
+ *
+ * Arch-specific hardware virtual machine abstractions.
+ *
+ * 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, see <http://www.gnu.org/licenses/>.
+ */
+
 #include 
 #include 
 #include 
@@ -14,7 +32,6 @@
 #include 
 
 long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
-
 {
 long rc = 0;
 
@@ -65,3 +82,13 @@ long do_hvm_op(unsigned long op, 
XEN_GUEST_HANDLE_PARAM(void) arg)
 
 return rc;
 }
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.5.0


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

[Xen-devel] [PATCH v5 0/2] Vm-events: move monitor vm-events code to common-side.

[Corneliu ZUZU]

This patch series is an attempt to move some of the monitor vm-events code to
the common-side. Done to make it easier to move additional parts that can be
moved to common when ARM-side implementations are to be added.

Patches summary:
1. Fix file comment
Acked-by: Stefano Stabellini <stefano.stabell...@eu.citrix.com>
2. Move monitor_domctl to common-side.
Acked-by: Stefano Stabellini <stefano.stabell...@eu.citrix.com>
Acked-by: Razvan Cojocaru <rcojoc...@bitdefender.com>

Note: ARM support for guest-request, control-register write monitor vm-events
will follow after review of this patch-series.

---
Changed since v4:
  1/2: nothing changed
  2/2: arch_monitor_domctl_event:
replaced !old_status w/ requested_status (equivalent but more readable)

Corneliu ZUZU (2):
  xen/arm: fix file comments
  xen/vm-events: Move parts of monitor_domctl code to common-side.

 MAINTAINERS   |   1 +
 xen/arch/arm/hvm.c|  29 +++-
 xen/arch/x86/monitor.c| 153 +++---
 xen/common/Makefile   |   1 +
 xen/common/domctl.c   |   2 +-
 xen/common/monitor.c  |  69 +++
 xen/include/asm-arm/monitor.h |  30 +++--
 xen/include/asm-x86/monitor.h |  53 +--
 xen/include/xen/monitor.h |  30 +
 9 files changed, 245 insertions(+), 123 deletions(-)
 create mode 100644 xen/common/monitor.c
 create mode 100644 xen/include/xen/monitor.h

-- 
2.5.0


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

Re: [Xen-devel] [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/16/2016 6:02 PM, Tamas K Lengyel wrote:



@@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct
xen_domctl_monitor_op *mop)

 case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
 {


So since we will now have two separate booleans, requested_status and 
old_status and then manually verify they are opposite..


-bool_t status = ad->monitor.mov_to_msr_enabled;
+bool_t old_status = ad->monitor.mov_to_msr_enabled;


...here we should set the field to requested_status, not !old_status. 
While they are technically equivalent, the code would read better to 
other way around.



-ad->monitor.mov_to_msr_enabled = !status;
+ad->monitor.mov_to_msr_enabled = !old_status; 


 domain_unpause(d);
 break;
 }

 case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
 {
-bool_t status = ad->monitor.singlestep_enabled;
+bool_t old_status = ad->monitor.singlestep_enabled;

-rc = status_check(mop, status);
-if ( rc )
-return rc;
+if ( unlikely(old_status == requested_status) )
+return -EEXIST;

 domain_pause(d);


Here as well..

-ad->monitor.singlestep_enabled = !status;
+ad->monitor.singlestep_enabled = !old_status;
 domain_unpause(d);
 break;
 }

 case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
 {
-bool_t status = ad->monitor.software_breakpoint_enabled;
+bool_t old_status = ad->monitor.software_breakpoint_enabled;

-rc = status_check(mop, status);
-if ( rc )
-return rc;
+if ( unlikely(old_status == requested_status) )
+return -EEXIST;

 domain_pause(d);


..and here..

-ad->monitor.software_breakpoint_enabled = !status;
+ad->monitor.software_breakpoint_enabled = !old_status;
 domain_unpause(d);
 break;
 }

 case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
 {
-bool_t status = ad->monitor.guest_request_enabled;
+bool_t old_status = ad->monitor.guest_request_enabled;

-rc = status_check(mop, status);
-if ( rc )
-return rc;
+if ( unlikely(old_status == requested_status) )
+return -EEXIST;

 domain_pause(d);
 ad->monitor.guest_request_sync = mop->u.guest_request.sync;


..and here.

-ad->monitor.guest_request_enabled = !status;
+ad->monitor.guest_request_enabled = !old_status;
 domain_unpause(d);
 break;
 }


Otherwise the patch looks good.

Thanks,
Tamas



Oh, right, that would look better. Shall I send a v5 then with that 
change? (and if yes I guess it won't hurt if I also include the 
left-shift sanity checks I mentioned I should have added (?))


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

Re: [Xen-devel] [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/16/2016 2:34 PM, Jan Beulich wrote:

On 16.02.16 at 12:20, <cz...@bitdefender.com> wrote:

On 2/16/2016 12:45 PM, Jan Beulich wrote:

On 16.02.16 at 09:13, <cz...@bitdefender.com> wrote:

On 2/16/2016 9:08 AM, Corneliu ZUZU wrote:

This patch moves monitor_domctl to common-side.
Purpose: move what's common to common, prepare for implementation
of such vm-events on ARM.

* move get_capabilities to arch-side => arch_monitor_get_capabilities.
* add arch-side monitor op handling function => arch_monitor_domctl_op.
 e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
* add arch-side monitor event handling function => arch_monitor_domctl_event.
 e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event

enable/disable

* remove status_check

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>

---
Changed since v3:
 * monitor_domctl @ common/monitor.c:
   - remove unused requested_status
   - sanity check mop->event range to avoid left-shift undefined behavior

Due to left-shift undefined behavior situations, shouldn't I also:

* in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<'

There's no undefinedness there, since the right side operands of
<< are all constant. Using 1U here would be okay, but is not
strictly needed.

I reasoned based on this ISO C99 quote:
[for an E1 << E2 operation, ]
"If E1 has a signed type and nonnegative value, and E1 × 2^E2 is
representable in the result type, then that is the resulting value;
otherwise, the behavior is undefined."

I inferred that this means that code such as '(1 << 31)' would render
undefined behavior, since (1 x 2^31) is not representable on 'int'.
The standard doesn't seem to mention different behavior if the operands
are constants.
This would render undefined behavior if bit 31 of capabilities would be
used @ some point, i.e. if one day someone would e.g. unknowingly:
  #define XEN_DOMCTL_MONITOR_EVENT_GRAVITATIONAL_WAVE 31
Have I misinterpreted the 'representable in the result type' part?

No, that's all correct. It's just that right now no
XEN_DOMCTL_MONITOR_EVENT_* has value 31, and hence
there's only a very minor latent issue here (someone blindly
copying the existing 1 << ... without adding the necessary U at
that point; one might hope the compiler would then point this out
though).

Jan



Ah, I see. Did a fast test earlier w/ GCC 5.1, unfortunately I think the 
compiler doesn't
issue any warning in this situation, would have preferred a heads-up too 
(couldn't even force it to do so).
(it would be nice if Xen shipped w/ a gravitational-waves detector 
though)


Corneliu.

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

Re: [Xen-devel] [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/16/2016 12:45 PM, Jan Beulich wrote:

On 16.02.16 at 09:13, <cz...@bitdefender.com> wrote:

On 2/16/2016 9:08 AM, Corneliu ZUZU wrote:

This patch moves monitor_domctl to common-side.
Purpose: move what's common to common, prepare for implementation
of such vm-events on ARM.

* move get_capabilities to arch-side => arch_monitor_get_capabilities.
* add arch-side monitor op handling function => arch_monitor_domctl_op.
e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
* add arch-side monitor event handling function => arch_monitor_domctl_event.
e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event

enable/disable

* remove status_check

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>

---
Changed since v3:
* monitor_domctl @ common/monitor.c:
  - remove unused requested_status
  - sanity check mop->event range to avoid left-shift undefined behavior

Due to left-shift undefined behavior situations, shouldn't I also:

* in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<'

There's no undefinedness there, since the right side operands of
<< are all constant. Using 1U here would be okay, but is not
strictly needed.


I reasoned based on this ISO C99 quote:
[for an E1 << E2 operation, ]
"If E1 has a signed type and nonnegative value, and E1 × 2^E2 is 
representable in the result type, then that is the resulting value; 
otherwise, the behavior is undefined."


I inferred that this means that code such as '(1 << 31)' would render 
undefined behavior, since (1 x 2^31) is not representable on 'int'.
The standard doesn't seem to mention different behavior if the operands 
are constants.
This would render undefined behavior if bit 31 of capabilities would be 
used @ some point, i.e. if one day someone would e.g. unknowingly:

#define XEN_DOMCTL_MONITOR_EVENT_GRAVITATIONAL_WAVE 31
Have I misinterpreted the 'representable in the result type' part?




* in X86 arch_monitor_domctl_event,
XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG case
add a sanity check of mop->u.mov_to_cr.index before:
  unsigned int ctrlreg_bitmask =
monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
, which basically translates to:
  unsigned int ctrlreg_bitmask = (1U << mop->u.mov_to_cr.index);

? (especially since mop->u.mov_to_cr.index is set by the caller).

Yes, there a range check would be needed, but preferably as a
separate patch (as this has nothing to do with the code motion
you perform here).

Jan




Great, I'll do these changes in a separate patch then.
Let me know if you have any other comments on this.

Thanks,
Corneliu.

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

Re: [Xen-devel] [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/16/2016 9:08 AM, Corneliu ZUZU wrote:

This patch moves monitor_domctl to common-side.
Purpose: move what's common to common, prepare for implementation
of such vm-events on ARM.

* move get_capabilities to arch-side => arch_monitor_get_capabilities.
* add arch-side monitor op handling function => arch_monitor_domctl_op.
   e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
* add arch-side monitor event handling function => arch_monitor_domctl_event.
   e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event 
enable/disable
* remove status_check

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>

---
Changed since v3:
   * monitor_domctl @ common/monitor.c:
 - remove unused requested_status
 - sanity check mop->event range to avoid left-shift undefined behavior


Due to left-shift undefined behavior situations, shouldn't I also:

* in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<'

* in X86 arch_monitor_domctl_event, 
XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG case

add a sanity check of mop->u.mov_to_cr.index before:
unsigned int ctrlreg_bitmask = 
monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);

, which basically translates to:
unsigned int ctrlreg_bitmask = (1U << mop->u.mov_to_cr.index);

? (especially since mop->u.mov_to_cr.index is set by the caller).

Would have been good if I'd thought of that before sending this patch 
series :).


Thanks,
Corneliu.

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

[Xen-devel] [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

This patch moves monitor_domctl to common-side.
Purpose: move what's common to common, prepare for implementation
of such vm-events on ARM.

* move get_capabilities to arch-side => arch_monitor_get_capabilities.
* add arch-side monitor op handling function => arch_monitor_domctl_op.
  e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
* add arch-side monitor event handling function => arch_monitor_domctl_event.
  e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event enable/disable
* remove status_check

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>

---
Changed since v3:
  * monitor_domctl @ common/monitor.c:
- remove unused requested_status
- sanity check mop->event range to avoid left-shift undefined behavior
  * arch_monitor_domctl_event: use ASSERT_UNREACHABLE() instead of bug warning
  * xen/monitor.h: replace includes w/ structs forward-declare, fix #ifndef
---
 MAINTAINERS   |   1 +
 xen/arch/x86/monitor.c| 153 +++---
 xen/common/Makefile   |   1 +
 xen/common/domctl.c   |   2 +-
 xen/common/monitor.c  |  69 +++
 xen/include/asm-arm/monitor.h |  30 +++--
 xen/include/asm-x86/monitor.h |  53 +--
 xen/include/xen/monitor.h |  30 +
 8 files changed, 217 insertions(+), 122 deletions(-)
 create mode 100644 xen/common/monitor.c
 create mode 100644 xen/include/xen/monitor.h

diff --git a/MAINTAINERS b/MAINTAINERS
index f07384c..5cbb1dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -355,6 +355,7 @@ M:  Tamas K Lengyel <ta...@tklengyel.com>
 S: Supported
 F: xen/common/vm_event.c
 F: xen/common/mem_access.c
+F: xen/common/monitor.c
 F: xen/arch/x86/hvm/event.c
 F: xen/arch/x86/monitor.c
 F: xen/arch/*/vm_event.c
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 1d43880..660b92c 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -1,9 +1,10 @@
 /*
  * arch/x86/monitor.c
  *
- * Architecture-specific monitor_op domctl handler.
+ * Arch-specific monitor_op domctl handler.
  *
  * Copyright (c) 2015 Tamas K Lengyel (ta...@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public
@@ -18,87 +19,14 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include 
-#include 
-#include 
-#include 
 #include 
-#include 
-#include 
+#include 
 
-/*
- * Sanity check whether option is already enabled/disabled
- */
-static inline
-int status_check(struct xen_domctl_monitor_op *mop, bool_t status)
-{
-bool_t requested_status = (mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE);
-
-if ( status == requested_status )
-return -EEXIST;
-
-return 0;
-}
-
-static inline uint32_t get_capabilities(struct domain *d)
-{
-uint32_t capabilities = 0;
-
-/*
- * At the moment only Intel HVM domains are supported. However, event
- * delivery could be extended to AMD and PV domains.
- */
-if ( !is_hvm_domain(d) || !cpu_has_vmx )
-return capabilities;
-
-capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
-   (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
-   (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
-   (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
-
-/* Since we know this is on VMX, we can just call the hvm func */
-if ( hvm_is_singlestep_supported() )
-capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
-
-return capabilities;
-}
-
-int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
+int arch_monitor_domctl_event(struct domain *d,
+  struct xen_domctl_monitor_op *mop)
 {
-int rc;
 struct arch_domain *ad = >arch;
-uint32_t capabilities = get_capabilities(d);
-
-if ( current->domain == d ) /* no domain_pause() */
-return -EPERM;
-
-rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event);
-if ( rc )
-return rc;
-
-switch ( mop->op )
-{
-case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
-mop->event = capabilities;
-return 0;
-
-case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
-domain_pause(d);
-ad->mem_access_emulate_each_rep = !!mop->event;
-domain_unpause(d);
-return 0;
-}
-
-/*
- * Sanity check
- */
-if ( mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE &&
- mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE )
-return -EOPNOTSUPP;
-
-/* Check if event type is available. */
-if ( !(capabilities & (1 << mop->event)) )
-return -EOPNOTSUPP;
+bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
 
 swit

[Xen-devel] [PATCH v4 0/2] Vm-events: move monitor vm-events code to common-side.

[Corneliu ZUZU]

This patch series is an attempt to move some of the monitor vm-events code to
the common-side. Done to make it easier to move additional parts that can be
moved to common when ARM-side implementations are to be added.

Patches summary:
1. Fix file comment
Acked-by: Stefano Stabellini 
2. Move monitor_domctl to common-side.

Note: ARM support for guest-request, control-register write monitor vm-events
will follow after review of this patch-series.

---
Changed since v3:
  1/2: nothing changed
  2/2: only minor changes
* sanity check mop->event range to avoid left-shift undefined behavior
* remove unused requested_status
* use ASSERT_UNREACHABLE() instead of bug warning
* replace includes w/ structs forward-declare, fix #ifndef

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

[Xen-devel] [PATCH v4 1/2] xen/arm: fix file comments

[Corneliu ZUZU]

Add file header comment and local variable block @ EOF
of xen/arch/arm/hvm.c.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
Acked-by: Stefano Stabellini <stefano.stabell...@eu.citrix.com>

---
Changed since v2 (mistakenly not included in v3): 
---
 xen/arch/arm/hvm.c | 29 -
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 5fd0753..056db1a 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -1,3 +1,21 @@
+/*
+ * arch/arm/hvm.c
+ *
+ * Arch-specific hardware virtual machine abstractions.
+ *
+ * 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, see <http://www.gnu.org/licenses/>.
+ */
+
 #include 
 #include 
 #include 
@@ -14,7 +32,6 @@
 #include 
 
 long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
-
 {
 long rc = 0;
 
@@ -65,3 +82,13 @@ long do_hvm_op(unsigned long op, 
XEN_GUEST_HANDLE_PARAM(void) arg)
 
 return rc;
 }
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.5.0


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

Re: [Xen-devel] [PATCH v3 1/2] xen/x86: merge 2 hvm_event_... functions into 1

[Corneliu ZUZU]

On 2/15/2016 7:47 PM, Tamas K Lengyel wrote:



On Mon, Feb 15, 2016 at 10:40 AM, Corneliu ZUZU <cz...@bitdefender.com 
<mailto:cz...@bitdefender.com>> wrote:


On 2/15/2016 10:30 AM, Razvan Cojocaru wrote:

On 02/15/2016 08:35 AM, Corneliu ZUZU wrote:

This patch merges almost identical functions
hvm_event_int3 and
hvm_event_single_step into a single function called
hvm_event_breakpoint.
Also fixes event.c file header comment in the process.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com
<mailto:cz...@bitdefender.com>>
---
  xen/arch/x86/hvm/event.c| 108
+++-
  xen/arch/x86/hvm/vmx/vmx.c  |  15 +++---
  xen/include/asm-x86/hvm/event.h |  11 ++--
  3 files changed, 67 insertions(+), 67 deletions(-)

Looks good to me.

Acked-by: Razvan Cojocaru <rcojoc...@bitdefender.com
<mailto:rcojoc...@bitdefender.com>>


Thanks,
Razvan


If the patch hasn't been merged to staging yet then include it. If 
it's a longer series the cover page can indicate which patches are 
still in need of review and which ones have been already acked. 
Furthermore, for each patch under the Signed-off-by tag you can say 
what changed in each revision. If nothing changed, you can say no 
change or just don't put anything for that revision. See 
http://lists.xen.org/archives/html/xen-devel/2016-02/msg00943.html for 
an example. Also keep in mind that if the changes in a revision are 
significant enough you can't keep the Acked-by tag on the patch, it 
will need to be re-acked.


Tamas


Got it, thanks.

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

Re: [Xen-devel] [PATCH v3 1/2] xen/x86: merge 2 hvm_event_... functions into 1

[Corneliu ZUZU]

On 2/15/2016 10:30 AM, Razvan Cojocaru wrote:

On 02/15/2016 08:35 AM, Corneliu ZUZU wrote:

This patch merges almost identical functions hvm_event_int3 and
hvm_event_single_step into a single function called hvm_event_breakpoint.
Also fixes event.c file header comment in the process.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
  xen/arch/x86/hvm/event.c| 108 +++-
  xen/arch/x86/hvm/vmx/vmx.c  |  15 +++---
  xen/include/asm-x86/hvm/event.h |  11 ++--
  3 files changed, 67 insertions(+), 67 deletions(-)

Looks good to me.

Acked-by: Razvan Cojocaru <rcojoc...@bitdefender.com>


Thanks,
Razvan



I forgot to ask: when getting an Acked-By response, should I include 
that patch in the next patch-series
or ommit it? I've done that w/ the first patch in the last patch-series, 
but IDK if I was correct to do so.


Thanks,
Corneliu.

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

Re: [Xen-devel] [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/15/2016 6:51 PM, Tamas K Lengyel wrote:



On Mon, Feb 15, 2016 at 9:44 AM, Jan Beulich > wrote:


>>> On 15.02.16 at 17:28, > wrote:
> On 2/15/2016 4:08 PM, Jan Beulich wrote:
>>
>>> After changing 1 to 1U though, I don't understand why we
should also
>>> range-check mop->event.
>>> I'm imagining when (mop->event > 31):
>>> * (1U << mop->event) = 0 or >= (0x1 + 0x) (?)
>> No, it's plain undefined.
>
> Weirdo C, didn't know that!
> I've just read
http://www.danielvik.com/2010/05/c-language-quirks.html .
> That's crazy, I can't believe such 'quirks' exist and that I
never knew
> of them.
> So then, would this do:
>
> /* sanity check - avoid '<<' operator undefined behavior */
> if ( unlikely(mop->event > 31) )
>  return -EINVAL;
> if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U <<
mop->event))) )
>  return -EOPNOTSUPP;

I'd say -EOPNOTSUPP in both cases, but if the maintainers like
-EINVAL better I wouldn't insist on my preference.


The best approach of course would be if we had __MAX values defined 
for such enums to check against but that doesn't seem to be part of 
Xen's coding practice. So in this case I would say leave it as -EINVAL 
as it's more descriptive of the problem and may even signal to the 
caller some inherent bug on their side, not just that the requested 
option is not supported.


Thanks,
Tamas



Good points, noted.

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

Re: [Xen-devel] [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/15/2016 4:08 PM, Jan Beulich wrote:



After changing 1 to 1U though, I don't understand why we should also
range-check mop->event.
I'm imagining when (mop->event > 31):
* (1U << mop->event) = 0 or >= (0x1 + 0x) (?)

No, it's plain undefined.


Weirdo C, didn't know that!
I've just read http://www.danielvik.com/2010/05/c-language-quirks.html .
That's crazy, I can't believe such 'quirks' exist and that I never knew 
of them.

So then, would this do:

/* sanity check - avoid '<<' operator undefined behavior */
if ( unlikely(mop->event > 31) )
return -EINVAL;
if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) )
return -EOPNOTSUPP;


?

Thanks,
Corneliu.

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

Re: [Xen-devel] [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/15/2016 2:44 PM, Jan Beulich wrote:



  switch ( mop->op )
  {
  case XEN_DOMCTL_MONITOR_OP_ENABLE:
  case XEN_DOMCTL_MONITOR_OP_DISABLE:
  /* Check if event type is available. */
  if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << 
mop->event))) )
  return -EOPNOTSUPP;
  /* Arch-side handles enable/disable ops. */
  return arch_monitor_domctl_event(d, mop);

Ah, I see now that I've mis-read the default: code further below,
which actually calls arch_monitor_domctl_op(), not ..._event().
However, there's an "undefined behavior" issue with the code
above then when mop->event >= 31 - I think you want to left
shift 1U instead of plain 1, and you need to range check
mop->event first.

Jan


Never looked @ that part before, used it the way it was.
I suppose that's because "according to the C specification, the result 
of a bit shift
operation on a signed argument gives implementation-defined results, so 
in/theory/|1U << i|is

more portable than|1 << i|" (taken from a stackoverflow post).

After changing 1 to 1U though, I don't understand why we should also 
range-check mop->event.

I'm imagining when (mop->event > 31):
* (1U << mop->event) = 0 or >= (0x1 + 0x) (?)
* in both cases arch_monitor_get_capabilities(d) & (1U << mop->event) 
would be = 0

* in which case we would return -EOPNOTSUPP
, no?

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

Re: [Xen-devel] [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/15/2016 2:25 PM, Stefano Stabellini wrote:

On Mon, 15 Feb 2016, Jan Beulich wrote:

On 15.02.16 at 07:37,  wrote:

  default:
-return -EOPNOTSUPP;
+/*
+ * Should not be reached unless arch_monitor_get_capabilities() is not
+ * properly implemented. In that case, since reaching this point does
+ * not really break anything, don't crash the hypervisor, issue a
+ * warning instead of BUG().
+ */
+printk(XENLOG_WARNING
+"WARNING, BUG: arch_monitor_get_capabilities() not implemented"
+"properly.\n");
  
-};

+return -EOPNOTSUPP;
+}

I disagree with the issuing of a message here. At the very least this
should be a dprintk(). Perhaps an ASSERT_UNREACHABLE() would be
the way to go? What's worse though is that I can't see the checking
which would make true the "should not be reached" statement above
(not that you must not rely on the caller of the hypercall to be well
behaved).

ASSERT_UNREACHABLE() is appropriate here



Noted.

Corneliu.

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

Re: [Xen-devel] [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/15/2016 1:41 PM, Jan Beulich wrote:

+++ b/xen/include/xen/monitor.h
@@ -0,0 +1,30 @@
+/*
+ * include/xen/monitor.h
+ *
+ * Common monitor_op domctl handler.
+ *
+ * Copyright (c) 2015 Tamas K Lengyel (ta...@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that 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, see .
+ */
+
+#ifndef __MONITOR_H__
+#define __MONITOR_H__

__XEN_MONITOR_H__ please.


Hadn't noticed this comment, also noted.

Thanks,
Corneliu.

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

Re: [Xen-devel] [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/15/2016 1:41 PM, Jan Beulich wrote:

On 15.02.16 at 07:37,  wrote:

  default:
-return -EOPNOTSUPP;
+/*
+ * Should not be reached unless arch_monitor_get_capabilities() is not
+ * properly implemented. In that case, since reaching this point does
+ * not really break anything, don't crash the hypervisor, issue a
+ * warning instead of BUG().
+ */
+printk(XENLOG_WARNING
+"WARNING, BUG: arch_monitor_get_capabilities() not implemented"
+"properly.\n");
  
-};

+return -EOPNOTSUPP;
+}

I disagree with the issuing of a message here. At the very least this
should be a dprintk(). Perhaps an ASSERT_UNREACHABLE() would be
the way to go?


IDK, but I agree that it doesn't look so elegant like that.
Won't ASSERT_UNREACHABLE crash the hypervisor?


What's worse though is that I can't see the checking
which would make true the "should not be reached" statement above
(not that you must not rely on the caller of the hypercall to be well
behaved).


The reasoning is as follows.
From this part in monitor_domctl:

switch ( mop->op )
{
case XEN_DOMCTL_MONITOR_OP_ENABLE:
case XEN_DOMCTL_MONITOR_OP_DISABLE:
/* Check if event type is available. */
if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << 
mop->event))) )

return -EOPNOTSUPP;
/* Arch-side handles enable/disable ops. */
return arch_monitor_domctl_event(d, mop);

we can see that arch_monitor_domctl_event gets to be called only when 
arch_monitor_get_capabilities reports

an 'available' mop->event.
But if then in arch_monitor_domctl_event the default case is reached, it 
would mean
that arch_monitor_get_capabilities reported a mop->event as being 
available/supported

when is *not actually handled* anywhere.




--- /dev/null
+++ b/xen/include/xen/monitor.h
@@ -0,0 +1,30 @@
+/*
+ * include/xen/monitor.h
+ *
+ * Common monitor_op domctl handler.
+ *
+ * Copyright (c) 2015 Tamas K Lengyel (ta...@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that 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, see .
+ */
+
+#ifndef __MONITOR_H__
+#define __MONITOR_H__

__XEN_MONITOR_H__ please.


+#include 
+#include 
+
+int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);

Neither of the two includes seem necessary for this prototype, all
you need are forward declarations of the two involved structures.

Jan


Noted.

Corneliu.

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

Re: [Xen-devel] [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/15/2016 8:37 AM, Corneliu ZUZU wrote:

diff --git a/xen/common/monitor.c b/xen/common/monitor.c
new file mode 100644
index 000..b708cab
--- /dev/null
+++ b/xen/common/monitor.c
+int rc;
+bool_t requested_status = 0;
+
+if ( unlikely(current->domain == d) ) /* no domain_pause() */
+return -EPERM;
+
+rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event);
+if ( unlikely(rc) )
+return rc;
+
+switch ( mop->op )
+{
+case XEN_DOMCTL_MONITOR_OP_ENABLE:
+requested_status = 1;
+/* fallthrough */
+case XEN_DOMCTL_MONITOR_OP_DISABLE:
+/* Check if event type is available. */
+if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << mop->event))) 
)
+return -EOPNOTSUPP;
+/* Arch-side handles enable/disable ops. */
+return arch_monitor_domctl_event(d, mop);



Only noticed now, requested_status now became unused in this function.
Will remove in v4.

Corneliu.

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

[Xen-devel] [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

This patch moves monitor_domctl to common-side.
Purpose: move what's common to common, prepare for implementation
of such vm-events on ARM.

* move get_capabilities to arch-side => arch_monitor_get_capabilities.
* add arch-side monitor op handling function => arch_monitor_domctl_op.
  e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
* add arch-side monitor event handling function => arch_monitor_domctl_event.
  e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event enable/disable
* remove status_check

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 MAINTAINERS   |   1 +
 xen/arch/x86/monitor.c| 158 --
 xen/common/Makefile   |   1 +
 xen/common/domctl.c   |   2 +-
 xen/common/monitor.c  |  69 ++
 xen/include/asm-arm/monitor.h |  34 +++--
 xen/include/asm-x86/monitor.h |  53 --
 xen/include/xen/monitor.h |  30 
 8 files changed, 226 insertions(+), 122 deletions(-)
 create mode 100644 xen/common/monitor.c
 create mode 100644 xen/include/xen/monitor.h

diff --git a/MAINTAINERS b/MAINTAINERS
index f07384c..5cbb1dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -355,6 +355,7 @@ M:  Tamas K Lengyel <ta...@tklengyel.com>
 S: Supported
 F: xen/common/vm_event.c
 F: xen/common/mem_access.c
+F: xen/common/monitor.c
 F: xen/arch/x86/hvm/event.c
 F: xen/arch/x86/monitor.c
 F: xen/arch/*/vm_event.c
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 1d43880..069c3c7 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -1,9 +1,10 @@
 /*
  * arch/x86/monitor.c
  *
- * Architecture-specific monitor_op domctl handler.
+ * Arch-specific monitor_op domctl handler.
  *
  * Copyright (c) 2015 Tamas K Lengyel (ta...@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public
@@ -18,87 +19,14 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include 
-#include 
-#include 
-#include 
 #include 
-#include 
-#include 
+#include 
 
-/*
- * Sanity check whether option is already enabled/disabled
- */
-static inline
-int status_check(struct xen_domctl_monitor_op *mop, bool_t status)
-{
-bool_t requested_status = (mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE);
-
-if ( status == requested_status )
-return -EEXIST;
-
-return 0;
-}
-
-static inline uint32_t get_capabilities(struct domain *d)
-{
-uint32_t capabilities = 0;
-
-/*
- * At the moment only Intel HVM domains are supported. However, event
- * delivery could be extended to AMD and PV domains.
- */
-if ( !is_hvm_domain(d) || !cpu_has_vmx )
-return capabilities;
-
-capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
-   (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
-   (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
-   (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
-
-/* Since we know this is on VMX, we can just call the hvm func */
-if ( hvm_is_singlestep_supported() )
-capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
-
-return capabilities;
-}
-
-int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
+int arch_monitor_domctl_event(struct domain *d,
+  struct xen_domctl_monitor_op *mop)
 {
-int rc;
 struct arch_domain *ad = >arch;
-uint32_t capabilities = get_capabilities(d);
-
-if ( current->domain == d ) /* no domain_pause() */
-return -EPERM;
-
-rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event);
-if ( rc )
-return rc;
-
-switch ( mop->op )
-{
-case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
-mop->event = capabilities;
-return 0;
-
-case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
-domain_pause(d);
-ad->mem_access_emulate_each_rep = !!mop->event;
-domain_unpause(d);
-return 0;
-}
-
-/*
- * Sanity check
- */
-if ( mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE &&
- mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE )
-return -EOPNOTSUPP;
-
-/* Check if event type is available. */
-if ( !(capabilities & (1 << mop->event)) )
-return -EOPNOTSUPP;
+bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
 
 switch ( mop->event )
 {
@@ -106,13 +34,11 @@ int monitor_domctl(struct domain *d, struct 
xen_domctl_monitor_op *mop)
 {
 unsigned int ctrlreg_bitmask =
 monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
-bool_t status =
+bool_t old_status =
 !!(ad->monitor.wri

[Xen-devel] [PATCH v3 1/2] xen/x86: merge 2 hvm_event_... functions into 1

[Corneliu ZUZU]

This patch merges almost identical functions hvm_event_int3 and
hvm_event_single_step into a single function called hvm_event_breakpoint.
Also fixes event.c file header comment in the process.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/x86/hvm/event.c| 108 +++-
 xen/arch/x86/hvm/vmx/vmx.c  |  15 +++---
 xen/include/asm-x86/hvm/event.h |  11 ++--
 3 files changed, 67 insertions(+), 67 deletions(-)

diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index a3d4892..874a36c 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -1,22 +1,24 @@
 /*
-* 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, see <http://www.gnu.org/licenses/>.
-*/
+ * arch/x86/hvm/event.c
+ *
+ * Arch-specific 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, see <http://www.gnu.org/licenses/>.
+ */
 
 #include 
 #include 
@@ -151,61 +153,51 @@ void hvm_event_guest_request(void)
 }
 }
 
-int hvm_event_int3(unsigned long rip)
+static inline unsigned long gfn_of_rip(unsigned long rip)
 {
-int rc = 0;
 struct vcpu *curr = current;
+struct segment_register sreg;
+uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
 
-if ( curr->domain->arch.monitor.software_breakpoint_enabled )
-{
-struct segment_register sreg;
-uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
-vm_event_request_t req = {
-.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT,
-.vcpu_id = curr->vcpu_id,
-};
-
-hvm_get_segment_register(curr, x86_seg_ss, );
-if ( sreg.attr.fields.dpl == 3 )
-pfec |= PFEC_user_mode;
+hvm_get_segment_register(curr, x86_seg_ss, );
+if ( sreg.attr.fields.dpl == 3 )
+pfec |= PFEC_user_mode;
 
-hvm_get_segment_register(curr, x86_seg_cs, );
-req.u.software_breakpoint.gfn = paging_gva_to_gfn(curr,
-  sreg.base + rip,
-  );
-
-rc = hvm_event_traps(1, );
-}
+hvm_get_segment_register(curr, x86_seg_cs, );
 
-return rc;
+return paging_gva_to_gfn(curr, sreg.base + rip, );
 }
 
-int hvm_event_single_step(unsigned long rip)
+int hvm_event_breakpoint(unsigned long rip,
+ enum hvm_event_breakpoint_type type)
 {
-int rc = 0;
 struct vcpu *curr = current;
+struct arch_domain *ad = >domain->arch;
+vm_event_request_t req;
 
-if ( curr->domain->arch.monitor.singlestep_enabled )
+switch ( type )
 {
-struct segment_register sreg;
-uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
-vm_event_request_t req = {
-.reason = VM_EVENT_REASON_SINGLESTEP,
-.vcpu_id = curr->vcpu_id,
-};
-
-hvm_get_segment_register(curr, x86_seg_ss, );
-if ( sreg.attr.fields.dpl == 3 )
-pfec |= PFEC_user_mode;
+case HVM_EVENT_SOFTWARE_BREAKPOINT:
+if ( !ad->monitor.software_breakpoint_enabled )
+return 0;
+req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
+req.u.software_breakpoint.gfn = gfn_of_rip(rip);
+break;
 
-hvm_get_segment_register(curr, x86_seg_cs, );
-req.u.singlestep.gfn = paging_gva_to_gfn(curr, sreg.base + rip,
- );
+case HVM_EVENT_SINGLESTEP_BREAKPOINT:
+if ( !ad->mo

[Xen-devel] [PATCH v3 0/2] Vm-events: move monitor vm-events code to common-side.

[Corneliu ZUZU]

This patch series is an attempt to move some of the monitor vm-events code to
the common-side. Done to make it easier to move additional parts that can be
moved to common when ARM-side implementations are to be added.

Patches summary:
1. Merge almost identical functions hvm_event_int3 + hvm_event_single_step ->
   hvm_event_breakpoint.
2. Move monitor_domctl to common-side.

Note: ARM support for guest-request, control-register write monitor vm-events
will follow after review of this patch-series.

---
Changed since v2:
  Major:
* moved back all monitor vm-event handling on X86-side
* removed Kconfigs
* arch_monitor_domctl_event & arch_monitor_domctl_op now directly return rc
  Minor:
* squashed commits 4 & 6 on their previous commits
* fixed coding-style mistakes (e.g. use C-style comments only)
* drop xen/config.h include

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

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

On 2/12/2016 8:05 AM, Corneliu ZUZU wrote:

On 2/11/2016 5:44 PM, Tamas K Lengyel wrote:


* the #ifdefs make it possible for that code to be put in common
=> that makes it *clear* that those code parts are NOT
architecture specific and their implementation can be safely used
for all architectures.


The current practice has been to put empty static inline functions 
into architecture-specific headers if the part is not 
handled/implemented for the specific architecture. This has already 
been the case for monitor_domctl (there is already separate 
asm-arm/monitor.h and asm-x86/monitor.h) so it should be followed as 
more of the code moves into common.


Tamas


Point is, they *are* implemented, because that's *common* code, it 
doesn't make sense to be moved to the arch-side
when you know that their implementation will be *the same* from 
arch-to-arch.
Not *everything* needs to stay on the arch-side, just what is 
architecture-specific - that's why e.g. arch_hvm_event_fill_regs,
arch_hvm_event_gfn_of_ip are not in common and are static inline 
functions as you say, because they have *different*

implementations *depending on the architecture*.

Finally, if Ian or any other ARM maintainer feels the same with moving 
code that can be moved and has been moved to common
back on the arch-side (effectively undoing 50% of my efforts),  I will 
do so.


Corneliu.


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


Actually, noted, will move single-step and software-breakpoint back on 
the arch-side as you suggest in v3.

If someone else disagrees, I suppose it will be discussed then.

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

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

On 2/11/2016 5:44 PM, Tamas K Lengyel wrote:


* the #ifdefs make it possible for that code to be put in common
=> that makes it *clear* that those code parts are NOT
architecture specific and their implementation can be safely used
for all architectures.


The current practice has been to put empty static inline functions 
into architecture-specific headers if the part is not 
handled/implemented for the specific architecture. This has already 
been the case for monitor_domctl (there is already separate 
asm-arm/monitor.h and asm-x86/monitor.h) so it should be followed as 
more of the code moves into common.


Tamas


Point is, they *are* implemented, because that's *common* code, it 
doesn't make sense to be moved to the arch-side
when you know that their implementation will be *the same* from 
arch-to-arch.
Not *everything* needs to stay on the arch-side, just what is 
architecture-specific - that's why e.g. arch_hvm_event_fill_regs,
arch_hvm_event_gfn_of_ip are not in common and are static inline 
functions as you say, because they have *different*

implementations *depending on the architecture*.

Finally, if Ian or any other ARM maintainer feels the same with moving 
code that can be moved and has been moved to common
back on the arch-side (effectively undoing 50% of my efforts),  I will 
do so.


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

[Xen-devel] [PATCH v2 0/7] Vm-events: move monitor vm-events code to common-side.

[Corneliu ZUZU]

This patch series is an attempt to move (most) of the monitor vm-events code to
the common-side.

Patches summary:

1. Minor file-comment fix

2. Merge almost identical functions hvm_event_int3 + hvm_event_single_step ->
hvm_event_breakpoint.

3. Add Kconfigs:
HAS_VM_EVENT_WRITE_CTRLREG, HAS_VM_EVENT_SINGLESTEP,
HAS_VM_EVENT_SOFTWARE_BREAKPOINT, HAS_VM_EVENT_GUEST_REQUEST
and move monitor_domctl to common-side. Used the Kconfigs to selectively
activate implemented monitor vm-events code for each architecture.

4. Minor file renames. Read (*) below.

5. Move hvm_event_traps, hvm_event_cr, hvm_event_guest_request,
hvm_event_breakpoint functions to common-side. (note: arch_hvm_event_fill_regs
on ARM-side will be implemented in a separate patch)

6. Minor file renames + comment fixes. Read (*) below.

7. Move arch_domain.monitor bitfield members to struct domain (common-side).
   Moved bits: single-step, software-breakpoint and guest-request.
(note: write_ctrlreg_* were left on the arch-side, since control-registers
number can vary across architectures)

(*) was only necessary to avoid git seeing a file as being modified, rather than
moved (would have made the diff unnecessarily bulky).

---
Changed since v1:
  Major:
* not moving arch/arm/hvm.c to arch/arm/hvm/hvm.c anymore
* hvm_event_software_breakpoint renamed to hvm_event_breakpoint
* hvm_event_breakpoint single_step param replaced by enum
  Minor:
* formatting & comment fixes: e.g. removed blank lines around #if/#endif
* improved code readability (used switch/case instead of if in some places)

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

[Xen-devel] [PATCH v2 1/7] xen/arm: fix file comments

[Corneliu ZUZU]

Add file header comment and local variable block @ EOF
of xen/arch/arm/hvm.c.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/arm/hvm.c | 29 -
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 5fd0753..056db1a 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -1,3 +1,21 @@
+/*
+ * arch/arm/hvm.c
+ *
+ * Arch-specific hardware virtual machine abstractions.
+ *
+ * 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, see <http://www.gnu.org/licenses/>.
+ */
+
 #include 
 #include 
 #include 
@@ -14,7 +32,6 @@
 #include 
 
 long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
-
 {
 long rc = 0;
 
@@ -65,3 +82,13 @@ long do_hvm_op(unsigned long op, 
XEN_GUEST_HANDLE_PARAM(void) arg)
 
 return rc;
 }
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.5.0


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

[Xen-devel] [PATCH v2 5/7] xen/vm-events: Move hvm_event_* functions to common-side.

[Corneliu ZUZU]

1. Moved hvm_event_traps, hvm_event_cr, hvm_event_guest_request,
hvm_event_breakpoint from arch-side to common-side

  1.1. Moved arch/x86/hvm/event.c to common/hvm/event.c
# see files: arch/x86/hvm/Makefile, xen/common/Makefile,
 xen/common/hvm/Makefile, xen/common/hvm/event.c
# changes:
- moved hvm_event_fill_regs to arch-side (arch_hvm_event_fill_regs)
- added vcpu parameter to hvm_event_traps
- surrounded common hvm_event_* implementations w/
   CONFIG_HAS_VM_EVENT_*
- moved hvm_event_msr to arch-side (see x86/hvm/event_x86.c)
- moved rip->gfn code in hvm_event_breakpoint to arch-side
   (see arch_hvm_event_gfn_of_ip) and renamed rip param to
   ip (i.e. now the parameter signifies a 'generic' instruction
   pointer, rather than the X86_64 RIP register)

  1.2. Moved asm-x86/hvm/event.h to xen/hvm/event.h
# see files: arch/x86/hvm/hvm.c, arch/x86/hvm/vmx/vmx.c

2. Added x86/hvm/event_x86.c => will rename in next commit to event.c (not done
in this commit to avoid git seeing this as being the modified old event.c =>
keeping the same name would have rendered an unnecessarily bulky diff)
# see files: arch/x86/hvm/Makefile
# implements X86-specific hvm_event_msr

3. Added asm-x86/hvm/event_arch.h, asm-arm/hvm/event_arch.h (renamed to
event.h in next commit, reason is the same as @ (2.).
# define/implement: arch_hvm_event_fill_regs, arch_hvm_event_gfn_of_ip and
hvm_event_msr (X86 side only)

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/x86/hvm/Makefile|   2 +-
 xen/arch/x86/hvm/event_x86.c |  51 
 xen/arch/x86/hvm/hvm.c   |   3 +-
 xen/arch/x86/hvm/vmx/vmx.c   |   2 +-
 xen/common/Makefile  |   2 +-
 xen/common/hvm/Makefile  |   3 +-
 xen/{arch/x86 => common}/hvm/event.c | 139 +++
 xen/include/asm-arm/hvm/event_arch.h |  40 +
 xen/include/asm-x86/hvm/event_arch.h |  93 +
 xen/include/{asm-x86 => xen}/hvm/event.h |  46 +++---
 10 files changed, 273 insertions(+), 108 deletions(-)
 create mode 100644 xen/arch/x86/hvm/event_x86.c
 rename xen/{arch/x86 => common}/hvm/event.c (44%)
 create mode 100644 xen/include/asm-arm/hvm/event_arch.h
 create mode 100644 xen/include/asm-x86/hvm/event_arch.h
 rename xen/include/{asm-x86 => xen}/hvm/event.h (59%)

diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 794e793..15daa09 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -3,7 +3,7 @@ subdir-y += vmx
 
 obj-y += asid.o
 obj-y += emulate.o
-obj-y += event.o
+obj-y += event_x86.o
 obj-y += hpet.o
 obj-y += hvm.o
 obj-y += i8254.o
diff --git a/xen/arch/x86/hvm/event_x86.c b/xen/arch/x86/hvm/event_x86.c
new file mode 100644
index 000..7b54f18
--- /dev/null
+++ b/xen/arch/x86/hvm/event_x86.c
@@ -0,0 +1,51 @@
+/*
+ * arch/x86/hvm/event_x86.c
+ *
+ * Arch-specific hardware virtual machine event abstractions.
+ *
+ * Copyright (c) 2004, Intel Corporation.
+ * Copyright (c) 2005, International Business Machines Corporation.
+ * Copyright (c) 2008, Citrix Systems, Inc.
+ * Copyright (c) 2016, Bitdefender S.R.L.
+ *
+ * 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, see <http://www.gnu.org/licenses/>.
+ */
+
+#include 
+
+void hvm_event_msr(unsigned int msr, uint64_t value)
+{
+struct vcpu *curr = current;
+
+if ( curr->domain->arch.monitor.mov_to_msr_enabled )
+{
+vm_event_request_t req = {
+.reason = VM_EVENT_REASON_MOV_TO_MSR,
+.vcpu_id = curr->vcpu_id,
+.u.mov_to_msr.msr = msr,
+.u.mov_to_msr.value = value,
+};
+
+hvm_event_traps(curr, 1, );
+}
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e93a648..2dec80e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -58,7 +59,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include  /* for cpu_has_tsc_ratio */
 #in

[Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

[Corneliu ZUZU]

This patch merges almost identical functions hvm_event_int3 and
hvm_event_single_step into a single function called hvm_event_breakpoint.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/x86/hvm/event.c| 76 +++--
 xen/arch/x86/hvm/vmx/vmx.c  | 15 
 xen/include/asm-x86/hvm/event.h | 11 --
 3 files changed, 52 insertions(+), 50 deletions(-)

diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index a3d4892..e3444db 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -1,9 +1,12 @@
 /*
-* event.c: Common hardware virtual machine event abstractions.
+* arch/x86/hvm/event.c
+*
+* Arch-specific hardware virtual machine event abstractions.
 *
 * Copyright (c) 2004, Intel Corporation.
 * Copyright (c) 2005, International Business Machines Corporation.
 * Copyright (c) 2008, Citrix Systems, Inc.
+* Copyright (c) 2016, Bitdefender S.R.L.
 *
 * This program is free software; you can redistribute it and/or modify it
 * under the terms and conditions of the GNU General Public License,
@@ -151,61 +154,52 @@ void hvm_event_guest_request(void)
 }
 }
 
-int hvm_event_int3(unsigned long rip)
+static inline
+uint64_t gfn_of_rip(unsigned long rip)
 {
-int rc = 0;
 struct vcpu *curr = current;
+struct segment_register sreg;
+uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
 
-if ( curr->domain->arch.monitor.software_breakpoint_enabled )
-{
-struct segment_register sreg;
-uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
-vm_event_request_t req = {
-.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT,
-.vcpu_id = curr->vcpu_id,
-};
-
-hvm_get_segment_register(curr, x86_seg_ss, );
-if ( sreg.attr.fields.dpl == 3 )
-pfec |= PFEC_user_mode;
+hvm_get_segment_register(curr, x86_seg_ss, );
+if ( sreg.attr.fields.dpl == 3 )
+pfec |= PFEC_user_mode;
 
-hvm_get_segment_register(curr, x86_seg_cs, );
-req.u.software_breakpoint.gfn = paging_gva_to_gfn(curr,
-  sreg.base + rip,
-  );
-
-rc = hvm_event_traps(1, );
-}
+hvm_get_segment_register(curr, x86_seg_cs, );
 
-return rc;
+return (uint64_t) paging_gva_to_gfn(curr, sreg.base + rip, );
 }
 
-int hvm_event_single_step(unsigned long rip)
+int hvm_event_breakpoint(unsigned long rip,
+ enum hvm_event_breakpoint_type type)
 {
-int rc = 0;
 struct vcpu *curr = current;
+struct arch_domain *ad = >domain->arch;
+vm_event_request_t req;
 
-if ( curr->domain->arch.monitor.singlestep_enabled )
+switch ( type )
 {
-struct segment_register sreg;
-uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
-vm_event_request_t req = {
-.reason = VM_EVENT_REASON_SINGLESTEP,
-.vcpu_id = curr->vcpu_id,
-};
-
-hvm_get_segment_register(curr, x86_seg_ss, );
-if ( sreg.attr.fields.dpl == 3 )
-pfec |= PFEC_user_mode;
+case HVM_EVENT_SOFTWARE_BREAKPOINT:
+if ( !ad->monitor.software_breakpoint_enabled )
+return 0;
+req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
+req.u.software_breakpoint.gfn = gfn_of_rip(rip);
+break;
 
-hvm_get_segment_register(curr, x86_seg_cs, );
-req.u.singlestep.gfn = paging_gva_to_gfn(curr, sreg.base + rip,
- );
+case HVM_EVENT_SINGLESTEP_BREAKPOINT:
+if ( !ad->monitor.singlestep_enabled )
+return 0;
+req.reason = VM_EVENT_REASON_SINGLESTEP;
+req.u.singlestep.gfn = gfn_of_rip(rip);
+break;
 
-rc = hvm_event_traps(1, );
+default:
+return -EOPNOTSUPP;
 }
 
-return rc;
+req.vcpu_id = curr->vcpu_id;
+
+return hvm_event_traps(1, );
 }
 
 /*
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b9f340c..cf0e642 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3192,8 +3192,10 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
 break;
 }
 else {
-int handled = hvm_event_int3(regs->eip);
-
+int handled =
+hvm_event_breakpoint(regs->eip,
+ HVM_EVENT_SOFTWARE_BREAKPOINT);
+
 if ( handled < 0 ) 
 {
 struct hvm_trap trap = {
@@ -3517,10 +3519,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
 case EXIT_REASON_MONITOR_TRAP_FLAG:
 v->arch.hvm_vmx.exec_control &= ~CPU_BASED_MONITOR_TRAP_FLAG;
 vmx_update_cpu_exec_control(v);
-if ( v->arch.hvm_vcpu.

[Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

1. Kconfig:
  * Added Kconfigs for common monitor vm-events:
  # see files: common/Kconfig, x86/Kconfig
HAS_VM_EVENT_WRITE_CTRLREG
HAS_VM_EVENT_SINGLESTEP
HAS_VM_EVENT_SOFTWARE_BREAKPOINT
HAS_VM_EVENT_GUEST_REQUEST

2. Moved monitor_domctl from arch-side to common-side
  2.1. Moved arch/x86/monitor.c to common/monitor.c
# see files: arch/x86/Makefile, xen/common/Makefile, xen/common/monitor.c
# changes:
- removed status_check (we would have had to duplicate it in X86
arch_monitor_domctl_event otherwise)
- moved get_capabilities to arch-side (arch_monitor_get_capabilities)
- moved XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP to arch-side (see
arch_monitor_domctl_op)
- put XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR to x86-side (see
arch_monitor_domctl_event)
- surrounded switch cases w/ CONFIG_HAS_VM_EVENT_*

  2.2. Moved asm-x86/monitor.h to xen/monitor.h
# see files: arch/x86/hvm/event.c, arch/x86/hvm/hvm.c,
 arch/x86/hvm/vmx/vmx.c, xen/common/domctl.c

  2.3. Removed asm-arm/monitor.h (no longer needed)

3. Added x86/monitor_x86.c => will rename in next commit to monitor.c (not done
in this commit to avoid git seeing this as being the modified old monitor.c =>
keeping the same name would have rendered an unnecessarily bulky diff)
# see files: arch/x86/Makefile
# implements X86-side arch_monitor_domctl_event

4. Added asm-x86/monitor_arch.h, asm-arm/monitor_arch.h (renamed to
monitor.h in next commit, reason is the same as @ (3.).
# define/implement: arch_monitor_get_capabilities, arch_monitor_domctl_op
and arch_monitor_domctl_event

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/x86/Kconfig  |   4 +
 xen/arch/x86/Makefile |   2 +-
 xen/arch/x86/hvm/event.c  |   2 +-
 xen/arch/x86/hvm/hvm.c|   2 +-
 xen/arch/x86/hvm/vmx/vmx.c|   2 +-
 xen/arch/x86/monitor_x86.c|  72 
 xen/common/Kconfig|  20 +++
 xen/common/Makefile   |   1 +
 xen/common/domctl.c   |   2 +-
 xen/{arch/x86 => common}/monitor.c| 195 +-
 xen/include/asm-arm/{monitor.h => monitor_arch.h} |  34 +++-
 xen/include/asm-x86/monitor_arch.h|  74 
 xen/include/{asm-x86 => xen}/monitor.h|  17 +-
 13 files changed, 293 insertions(+), 134 deletions(-)
 create mode 100644 xen/arch/x86/monitor_x86.c
 rename xen/{arch/x86 => common}/monitor.c (44%)
 rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%)
 create mode 100644 xen/include/asm-x86/monitor_arch.h
 rename xen/include/{asm-x86 => xen}/monitor.h (74%)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 3a90f47..e46be1b 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -14,6 +14,10 @@ config X86
select HAS_MEM_ACCESS
select HAS_MEM_PAGING
select HAS_MEM_SHARING
+   select HAS_VM_EVENT_WRITE_CTRLREG
+   select HAS_VM_EVENT_SINGLESTEP
+   select HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+   select HAS_VM_EVENT_GUEST_REQUEST
select HAS_NS16550
select HAS_PASSTHROUGH
select HAS_PCI
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 8e6e901..6e80cf0 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -36,7 +36,7 @@ obj-y += microcode_intel.o
 # This must come after the vendor specific files.
 obj-y += microcode.o
 obj-y += mm.o x86_64/mm.o
-obj-y += monitor.o
+obj-y += monitor_x86.o
 obj-y += mpparse.o
 obj-y += nmi.o
 obj-y += numa.o
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index e3444db..04faa72 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -23,8 +23,8 @@
 
 #include 
 #include 
+#include 
 #include 
-#include 
 #include 
 #include 
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 930d0e3..e93a648 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -51,7 +52,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index cf0e642..be67b60 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -57,7 +58,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 
 static bool_t __initdata opt_force_ept;
diff --git a/xen/arch/x86/monitor_x86.c b/xen/arch/x86/monitor_x86.c
new file mode 100644
index 000..d19fd15
--- /dev/null
+++ b/xen/arch/x86/monitor_x86.c
@@ -0,0 +1,72 @@
+/*
+ * arch/x86/monitor_x86.c
+ 

[Xen-devel] [PATCH v2 4/7] Rename monitor_x86.c to monitor.c and monitor_arch.h to monitor.h

[Corneliu ZUZU]

Rename:
- arch/x86/monitor_x86.c -> arch/x86/monitor.c
- asm-{x86,arm}/monitor_arch.h -> asm-{x86,arm}/monitor.h

(previous commit explains why these renames were necessary)

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/x86/Makefile | 2 +-
 xen/arch/x86/{monitor_x86.c => monitor.c} | 4 ++--
 xen/common/monitor.c  | 2 +-
 xen/include/asm-arm/{monitor_arch.h => monitor.h} | 8 
 xen/include/asm-x86/{monitor_arch.h => monitor.h} | 8 
 5 files changed, 12 insertions(+), 12 deletions(-)
 rename xen/arch/x86/{monitor_x86.c => monitor.c} (97%)
 rename xen/include/asm-arm/{monitor_arch.h => monitor.h} (90%)
 rename xen/include/asm-x86/{monitor_arch.h => monitor.h} (94%)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 6e80cf0..8e6e901 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -36,7 +36,7 @@ obj-y += microcode_intel.o
 # This must come after the vendor specific files.
 obj-y += microcode.o
 obj-y += mm.o x86_64/mm.o
-obj-y += monitor_x86.o
+obj-y += monitor.o
 obj-y += mpparse.o
 obj-y += nmi.o
 obj-y += numa.o
diff --git a/xen/arch/x86/monitor_x86.c b/xen/arch/x86/monitor.c
similarity index 97%
rename from xen/arch/x86/monitor_x86.c
rename to xen/arch/x86/monitor.c
index d19fd15..568def2 100644
--- a/xen/arch/x86/monitor_x86.c
+++ b/xen/arch/x86/monitor.c
@@ -1,5 +1,5 @@
 /*
- * arch/x86/monitor_x86.c
+ * arch/x86/monitor.c
  *
  * Arch-specific monitor_op domctl handler.
  *
@@ -19,7 +19,7 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include 
+#include 
 
 bool_t arch_monitor_domctl_event(struct domain *d,
  struct xen_domctl_monitor_op *mop,
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index a4899c3..03063bb 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -25,7 +25,7 @@
 #include 
 #include 
 
-#include/* for monitor_arch_# */
+#include /* for monitor_arch_# */
 #if CONFIG_X86
 #include /* for VM_EVENT_X86_CR3 */
 #include /* for hvm_update_guest_cr, ... */
diff --git a/xen/include/asm-arm/monitor_arch.h b/xen/include/asm-arm/monitor.h
similarity index 90%
rename from xen/include/asm-arm/monitor_arch.h
rename to xen/include/asm-arm/monitor.h
index d0df66c..eb770da 100644
--- a/xen/include/asm-arm/monitor_arch.h
+++ b/xen/include/asm-arm/monitor.h
@@ -1,5 +1,5 @@
 /*
- * include/asm-arm/monitor_arch.h
+ * include/asm-arm/monitor.h
  *
  * Arch-specific monitor_op domctl handler.
  *
@@ -19,8 +19,8 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef __ASM_ARM_MONITOR_ARCH_H__
-#define __ASM_ARM_MONITOR_ARCH_H__
+#ifndef __ASM_ARM_MONITOR_H__
+#define __ASM_ARM_MONITOR_H__
 
 #include 
 #include 
@@ -50,4 +50,4 @@ bool_t arch_monitor_domctl_event(struct domain *d,
 return 0;
 }
 
-#endif /* __ASM_ARM_MONITOR_ARCH_H__ */
+#endif /* __ASM_ARM_MONITOR_H__ */
diff --git a/xen/include/asm-x86/monitor_arch.h b/xen/include/asm-x86/monitor.h
similarity index 94%
rename from xen/include/asm-x86/monitor_arch.h
rename to xen/include/asm-x86/monitor.h
index d9daf65..b12823c 100644
--- a/xen/include/asm-x86/monitor_arch.h
+++ b/xen/include/asm-x86/monitor.h
@@ -1,5 +1,5 @@
 /*
- * include/asm-x86/monitor_arch.h
+ * include/asm-x86/monitor.h
  *
  * Arch-specific monitor_op domctl handler.
  *
@@ -19,8 +19,8 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef __ASM_X86_MONITOR_ARCH_H__
-#define __ASM_X86_MONITOR_ARCH_H__
+#ifndef __ASM_X86_MONITOR_H__
+#define __ASM_X86_MONITOR_H__
 
 #include   /* for struct domain, is_hvm_domain, ... */
 #include   /* for XEN_DOMCTL_MONITOR_#, ... */
@@ -71,4 +71,4 @@ bool_t arch_monitor_domctl_event(struct domain *d,
  struct xen_domctl_monitor_op *mop,
  int *rc);
 
-#endif /* __ASM_X86_MONITOR_ARCH_H__ */
+#endif /* __ASM_X86_MONITOR_H__ */
-- 
2.5.0


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

Re: [Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

[Corneliu ZUZU]

On 2/10/2016 6:18 PM, Jan Beulich wrote:

On 10.02.16 at 16:50,  wrote:

@@ -151,61 +154,52 @@ void hvm_event_guest_request(void)
  }
  }
  
-int hvm_event_int3(unsigned long rip)

+static inline
+uint64_t gfn_of_rip(unsigned long rip)

This should be a single line and the return value should be
unsigned long.

Noted.

+return (uint64_t) paging_gva_to_gfn(curr, sreg.base + rip, );
  }

Pointless cast.

Noted.



--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -17,6 +17,12 @@
  #ifndef __ASM_X86_HVM_EVENT_H__
  #define __ASM_X86_HVM_EVENT_H__
  
+enum hvm_event_breakpoint_type

+{
+HVM_EVENT_SOFTWARE_BREAKPOINT,
+HVM_EVENT_SINGLESTEP_BREAKPOINT,
+};

I don't see what good it does to put existing constants into an
enum.
As Andrew pointed out, an enum was requested in v1 instead of the 
single_step param.
One could use the already existing VM_EVENT_REASON_* constants, but 
conceptually this

function only involves a subset of those (i.e. *breakpoint vm-events*).



@@ -27,9 +33,8 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value,
  #define hvm_event_crX(what, new, old) \
  hvm_event_cr(VM_EVENT_X86_##what, new, old)
  void hvm_event_msr(unsigned int msr, uint64_t value);
-/* Called for current VCPU: returns -1 if no listener */
-int hvm_event_int3(unsigned long rip);
-int hvm_event_single_step(unsigned long rip);
+int hvm_event_breakpoint(unsigned long rip,
+ enum hvm_event_breakpoint_type type);

I guess the comment was here for a reason, and this reason doesn't
go away with this code folding. But I'll leave it to the VM event code
maintainers to judge.

Jan


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



That comment seemed & still seems wrong to me, I don't see any code 
paths out of which that function would return -1.


Thank you,
Corneliu.

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

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

On 2/10/2016 6:26 PM, Jan Beulich wrote:

On 10.02.16 at 16:52,  wrote:

  xen/arch/x86/Kconfig  |   4 +
  xen/arch/x86/Makefile |   2 +-
  xen/arch/x86/hvm/event.c  |   2 +-
  xen/arch/x86/hvm/hvm.c|   2 +-
  xen/arch/x86/hvm/vmx/vmx.c|   2 +-
  xen/arch/x86/monitor_x86.c|  72 
  xen/common/Kconfig|  20 +++
  xen/common/Makefile   |   1 +
  xen/common/domctl.c   |   2 +-
  xen/{arch/x86 => common}/monitor.c| 195 +-
  xen/include/asm-arm/{monitor.h => monitor_arch.h} |  34 +++-
  xen/include/asm-x86/monitor_arch.h|  74 
  xen/include/{asm-x86 => xen}/monitor.h|  17 +-
  13 files changed, 293 insertions(+), 134 deletions(-)
  create mode 100644 xen/arch/x86/monitor_x86.c
  rename xen/{arch/x86 => common}/monitor.c (44%)
  rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%)
  create mode 100644 xen/include/asm-x86/monitor_arch.h
  rename xen/include/{asm-x86 => xen}/monitor.h (74%)

With percentages as low as 44 I'm not sure all this strange
renaming and introduction of oddly named new files is actually a
good idea.
The diff would have actually looked a lot worse if I didn't do that, at 
least IMO.

The "strange renaming and ..." was an effort I made to make reviewing these
patches easier :). The reason is explained in the introductory message 
and in this commit

message.

--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -14,6 +14,10 @@ config X86
select HAS_MEM_ACCESS
select HAS_MEM_PAGING
select HAS_MEM_SHARING
+   select HAS_VM_EVENT_WRITE_CTRLREG
+   select HAS_VM_EVENT_SINGLESTEP
+   select HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+   select HAS_VM_EVENT_GUEST_REQUEST
select HAS_NS16550
select HAS_PASSTHROUGH
select HAS_PCI

Please don't break the alphabetic ordering.


Noted. Didn't realise they were in alphabetic order.


+#include  /* for XENLOG_WARNING */
Please simply drop this include, it's being automatically included via
compiler command line option. Also please avoid comments like this
unless they explain an otherwise unexpected or unreasonable
inclusion.

Jan


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



Noted, noted.

That you,
Corneliu.

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

Re: [Xen-devel] [PATCH v2 7/7] xen/vm-events: move arch_domain.monitor bits to common

[Corneliu ZUZU]

On 2/10/2016 6:29 PM, Jan Beulich wrote:

On 10.02.16 at 17:00,  wrote:

+#if CONFIG_HAS_VM_EVENT_GUEST_REQUEST
+uint8_t guest_request_enabled   : 1;
+uint8_t guest_request_sync  : 1;
+#endif // HAS_VM_EVENT_GUEST_REQUEST
+} monitor;
  };

Please use C-style comments only (see ./CODING_STYLE).

Jan


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


Noted.

Corneliu.

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

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

On 2/10/2016 6:39 PM, Tamas K Lengyel wrote:



On Wed, Feb 10, 2016 at 8:52 AM, Corneliu ZUZU <cz...@bitdefender.com 
<mailto:cz...@bitdefender.com>> wrote:


1. Kconfig:
  * Added Kconfigs for common monitor vm-events:
  # see files: common/Kconfig, x86/Kconfig
HAS_VM_EVENT_WRITE_CTRLREG
HAS_VM_EVENT_SINGLESTEP
HAS_VM_EVENT_SOFTWARE_BREAKPOINT
HAS_VM_EVENT_GUEST_REQUEST

2. Moved monitor_domctl from arch-side to common-side
  2.1. Moved arch/x86/monitor.c to common/monitor.c
# see files: arch/x86/Makefile, xen/common/Makefile,
xen/common/monitor.c
# changes:
- removed status_check (we would have had to duplicate it
in X86
arch_monitor_domctl_event otherwise)
- moved get_capabilities to arch-side
(arch_monitor_get_capabilities)
- moved XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP to
arch-side (see
arch_monitor_domctl_op)
- put XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR to x86-side (see
arch_monitor_domctl_event)
- surrounded switch cases w/ CONFIG_HAS_VM_EVENT_*

  2.2. Moved asm-x86/monitor.h to xen/monitor.h
# see files: arch/x86/hvm/event.c, arch/x86/hvm/hvm.c,
 arch/x86/hvm/vmx/vmx.c, xen/common/domctl.c

  2.3. Removed asm-arm/monitor.h (no longer needed)

3. Added x86/monitor_x86.c => will rename in next commit to
monitor.c (not done
in this commit to avoid git seeing this as being the modified old
monitor.c =>
keeping the same name would have rendered an unnecessarily bulky diff)
# see files: arch/x86/Makefile
# implements X86-side arch_monitor_domctl_event

4. Added asm-x86/monitor_arch.h, asm-arm/monitor_arch.h (renamed to
monitor.h in next commit, reason is the same as @ (3.).
# define/implement: arch_monitor_get_capabilities,
arch_monitor_domctl_op
and arch_monitor_domctl_event


So these commit messages are not very good IMHO. Rather then just 
summarizing what the patch does you should describe why the patch is 
needed in the first place. Usually for longer series having a cover 
page is also helpful to outline how the patches in the series fit 
together.


The intention was to make review easier (following the changes in 
parallel w/

the commit message). But indeed I was maybe too focused on that, should have
started w/ stating the purpose of these changes rather than jumping 
directly to detailing
them. Will try to compact these commit messages next time (maybe move 
details to the

introductory section instead, as you suggest).



    Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com
<mailto:cz...@bitdefender.com>>
---
 xen/arch/x86/Kconfig  |   4 +
 xen/arch/x86/Makefile |   2 +-
 xen/arch/x86/hvm/event.c  |   2 +-
 xen/arch/x86/hvm/hvm.c|   2 +-
 xen/arch/x86/hvm/vmx/vmx.c|   2 +-
 xen/arch/x86/monitor_x86.c|  72 
 xen/common/Kconfig|  20 +++
 xen/common/Makefile   |   1 +
 xen/common/domctl.c   |   2 +-
 xen/{arch/x86 => common}/monitor.c| 195
+-
 xen/include/asm-arm/{monitor.h => monitor_arch.h} | 34 +++-
 xen/include/asm-x86/monitor_arch.h|  74 
 xen/include/{asm-x86 => xen}/monitor.h| 17 +-
 13 files changed, 293 insertions(+), 134 deletions(-)
 create mode 100644 xen/arch/x86/monitor_x86.c
 rename xen/{arch/x86 => common}/monitor.c (44%)
 rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%)
 create mode 100644 xen/include/asm-x86/monitor_arch.h
 rename xen/include/{asm-x86 => xen}/monitor.h (74%)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 3a90f47..e46be1b 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -14,6 +14,10 @@ config X86
select HAS_MEM_ACCESS
select HAS_MEM_PAGING
select HAS_MEM_SHARING
+   select HAS_VM_EVENT_WRITE_CTRLREG


You mentioned in the previous revision of this series that currently 
you only have plans to implement write_ctrleg and guest_request events 
for ARM. I think singlestep and software_breakpoint should not be 
moved to common without a clear plan to have those implemented. Now, 
if you were to include the implementation of write_ctrlreg and 
guest_request in this series and leave the others in x86 as they are 
now, I don't think there would be any reason to have these Kconfig 
options present at all.


Moving what made sense to be moved to common was a suggestion of Ian's.
The purpose of this patch is to --avoid

[Xen-devel] [PATCH v2 6/7] Rename event_x86.c to event.c and event_arch.h to event.h + minor fixes

[Corneliu ZUZU]

Rename:
- arch/x86/hvm/event_x86.c -> arch/x86/hvm/event.c
- asm-{x86,arm}/hvm/event_arch.h -> asm-{x86/arm}/hvm/event.h

(previous commit explains why these renames were necessary)

Minor fixes:
* xen/common/hvm/event.c: fix malformed file header comment
* xen/hvm/event.h: fix comment & change hvm_event_crX first param name to a 
more
  descriptive one

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/x86/hvm/Makefile |  2 +-
 xen/arch/x86/hvm/{event_x86.c => event.c} |  2 +-
 xen/arch/x86/hvm/hvm.c|  2 +-
 xen/common/hvm/event.c| 44 +++
 xen/include/asm-arm/hvm/{event_arch.h => event.h} |  8 ++---
 xen/include/asm-x86/hvm/{event_arch.h => event.h} |  8 ++---
 xen/include/xen/hvm/event.h   |  6 ++--
 7 files changed, 36 insertions(+), 36 deletions(-)
 rename xen/arch/x86/hvm/{event_x86.c => event.c} (98%)
 rename xen/include/asm-arm/hvm/{event_arch.h => event.h} (86%)
 rename xen/include/asm-x86/hvm/{event_arch.h => event.h} (94%)

diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 15daa09..794e793 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -3,7 +3,7 @@ subdir-y += vmx
 
 obj-y += asid.o
 obj-y += emulate.o
-obj-y += event_x86.o
+obj-y += event.o
 obj-y += hpet.o
 obj-y += hvm.o
 obj-y += i8254.o
diff --git a/xen/arch/x86/hvm/event_x86.c b/xen/arch/x86/hvm/event.c
similarity index 98%
rename from xen/arch/x86/hvm/event_x86.c
rename to xen/arch/x86/hvm/event.c
index 7b54f18..3349c34 100644
--- a/xen/arch/x86/hvm/event_x86.c
+++ b/xen/arch/x86/hvm/event.c
@@ -1,5 +1,5 @@
 /*
- * arch/x86/hvm/event_x86.c
+ * arch/x86/hvm/event.c
  *
  * Arch-specific hardware virtual machine event abstractions.
  *
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 2dec80e..69e2854 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -59,7 +59,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include  /* for cpu_has_tsc_ratio */
 #include 
diff --git a/xen/common/hvm/event.c b/xen/common/hvm/event.c
index 604e902..0b532d5 100644
--- a/xen/common/hvm/event.c
+++ b/xen/common/hvm/event.c
@@ -1,30 +1,30 @@
 /*
-* xen/common/hvm/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.
-* Copyright (c) 2016, Bitdefender S.R.L.
-*
-* 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, see <http://www.gnu.org/licenses/>.
-*/
+ * xen/common/hvm/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.
+ * Copyright (c) 2016, Bitdefender S.R.L.
+ *
+ * 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, see <http://www.gnu.org/licenses/>.
+ */
 
 #include 
 #include/* for vm_event_# calls */
 #include 
-#include  /* for hvm_event_arch_# calls */
+#include   /* for hvm_event_arch_# calls */
 #if CONFIG_X86
 #include 
 #endif
diff --git a/xen/include/asm-arm/hvm/event_arch.h 
b/xen/include/asm-arm/hvm/event.h
similarity index 86%
rename from xen/include/asm-arm/hvm/event_arch.h
rename to xen/include/asm-arm/hvm/event.h
index beebca2..4aa797b 100644
--- a/xen/include/asm-arm/hvm/event_arch.h
+++ b/xen/include/asm-arm/hvm/event.h
@@ -1,5 +1,5 @@
 /*
- * include/asm-arm/hvm/event_arch.h
+ * include/asm-arm/hvm/event.h
  *
  * Arch-specific hardware virtual machine event abstractions.
  *
@@ -18,8 +18,8 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef __ASM_ARM_HVM_EVENT_ARCH_H__
-#define __ASM_ARM_HVM_EVENT_ARCH_H__
+#ifndef __ASM_ARM_HVM_

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

On 2/10/2016 6:30 PM, Jan Beulich wrote:

On 10.02.16 at 17:26,  wrote:

On 10.02.16 at 16:52,  wrote:

  xen/arch/x86/Kconfig  |   4 +
  xen/arch/x86/Makefile |   2 +-
  xen/arch/x86/hvm/event.c  |   2 +-
  xen/arch/x86/hvm/hvm.c|   2 +-
  xen/arch/x86/hvm/vmx/vmx.c|   2 +-
  xen/arch/x86/monitor_x86.c|  72 
  xen/common/Kconfig|  20 +++
  xen/common/Makefile   |   1 +
  xen/common/domctl.c   |   2 +-
  xen/{arch/x86 => common}/monitor.c| 195 +-
  xen/include/asm-arm/{monitor.h => monitor_arch.h} |  34 +++-
  xen/include/asm-x86/monitor_arch.h|  74 
  xen/include/{asm-x86 => xen}/monitor.h|  17 +-
  13 files changed, 293 insertions(+), 134 deletions(-)
  create mode 100644 xen/arch/x86/monitor_x86.c
  rename xen/{arch/x86 => common}/monitor.c (44%)
  rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%)
  create mode 100644 xen/include/asm-x86/monitor_arch.h
  rename xen/include/{asm-x86 => xen}/monitor.h (74%)

With percentages as low as 44 I'm not sure all this strange
renaming and introduction of oddly named new files is actually a
good idea.

Also it looks like this moving around of files needs to be
accompanied by adjustments to ./MAINTAINERS.

Jan


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



Will check that, indeed these might have broken something there.

Corneliu.

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

[Xen-devel] [PATCH v2 7/7] xen/vm-events: move arch_domain.monitor bits to common

[Corneliu ZUZU]

This patch moves bitfield members for single-step, software-breakpoint and
guest-request monitor vm-events from the arch-side (arch_domain.monitor) to
the common-side (domain.monitor).
Ctrl-reg bits (i.e. write_ctrlreg_* members) are left on the arch-side, because
control-registers number can vary across architectures.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c  |  4 ++--
 xen/common/hvm/event.c   | 12 ++--
 xen/common/monitor.c | 17 +++--
 xen/include/asm-x86/domain.h | 16 ++--
 xen/include/xen/sched.h  | 16 
 5 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 5bc3c74..07acbf2 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1706,8 +1706,8 @@ void vmx_do_resume(struct vcpu *v)
 }
 
 debug_state = v->domain->debugger_attached
-  || v->domain->arch.monitor.software_breakpoint_enabled
-  || v->domain->arch.monitor.singlestep_enabled;
+  || v->domain->monitor.software_breakpoint_enabled
+  || v->domain->monitor.singlestep_enabled;
 
 if ( unlikely(v->arch.hvm_vcpu.debug_state_latch != debug_state) )
 {
diff --git a/xen/common/hvm/event.c b/xen/common/hvm/event.c
index 0b532d5..0e5f30e 100644
--- a/xen/common/hvm/event.c
+++ b/xen/common/hvm/event.c
@@ -107,16 +107,16 @@ bool_t hvm_event_cr(unsigned int index,
 void hvm_event_guest_request(void)
 {
 struct vcpu *curr = current;
-struct arch_domain *ad = >domain->arch;
+struct domain *d = curr->domain;
 
-if ( ad->monitor.guest_request_enabled )
+if ( d->monitor.guest_request_enabled )
 {
 vm_event_request_t req = {
 .reason = VM_EVENT_REASON_GUEST_REQUEST,
 .vcpu_id = curr->vcpu_id,
 };
 
-hvm_event_traps(curr, ad->monitor.guest_request_sync, );
+hvm_event_traps(curr, d->monitor.guest_request_sync, );
 }
 }
 #endif // HAS_VM_EVENT_GUEST_REQUEST
@@ -126,14 +126,14 @@ int hvm_event_breakpoint(unsigned long ip,
  enum hvm_event_breakpoint_type type)
 {
 struct vcpu *curr = current;
-struct arch_domain *ad = >domain->arch;
+struct domain *d = curr->domain;
 vm_event_request_t req;
 
 switch ( type )
 {
 #if CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT
 case HVM_EVENT_SOFTWARE_BREAKPOINT:
-if ( !ad->monitor.software_breakpoint_enabled )
+if ( !d->monitor.software_breakpoint_enabled )
 return 0;
 req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
 req.u.software_breakpoint.gfn = arch_hvm_event_gfn_of_ip(ip);
@@ -142,7 +142,7 @@ int hvm_event_breakpoint(unsigned long ip,
 
 #if CONFIG_HAS_VM_EVENT_SINGLESTEP
 case HVM_EVENT_SINGLESTEP_BREAKPOINT:
-if ( !ad->monitor.singlestep_enabled )
+if ( !d->monitor.singlestep_enabled )
 return 0;
 req.reason = VM_EVENT_REASON_SINGLESTEP;
 req.u.singlestep.gfn = arch_hvm_event_gfn_of_ip(ip);
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index 03063bb..3c69b5e 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -116,14 +116,13 @@ int monitor_domctl(struct domain *d, struct 
xen_domctl_monitor_op *mop)
 #if CONFIG_HAS_VM_EVENT_SINGLESTEP
 case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
 {
-struct arch_domain *ad = >arch;
-bool_t old_status = ad->monitor.singlestep_enabled;
+bool_t old_status = d->monitor.singlestep_enabled;
 
 if ( unlikely(old_status == requested_status) )
 return -EEXIST;
 
 domain_pause(d);
-ad->monitor.singlestep_enabled = !old_status;
+d->monitor.singlestep_enabled = !old_status;
 domain_unpause(d);
 break;
 }
@@ -132,14 +131,13 @@ int monitor_domctl(struct domain *d, struct 
xen_domctl_monitor_op *mop)
 #if CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT
 case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
 {
-struct arch_domain *ad = >arch;
-bool_t old_status = ad->monitor.software_breakpoint_enabled;
+bool_t old_status = d->monitor.software_breakpoint_enabled;
 
 if ( unlikely(old_status == requested_status) )
 return -EEXIST;
 
 domain_pause(d);
-ad->monitor.software_breakpoint_enabled = !old_status;
+d->monitor.software_breakpoint_enabled = !old_status;
 domain_unpause(d);
 break;
 }
@@ -148,15 +146,14 @@ int monitor_domctl(struct domain *d, struct 
xen_domctl_monitor_op *mop)
 #if CONFIG_HAS_VM_EVENT_GUEST_REQUEST
 case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
 {
-struct arch_domain *ad = >arch;
-bool_t old_status = ad->monitor.guest_request_enabled

Re: [Xen-devel] [PATCH v2 4/7] Rename monitor_x86.c to monitor.c and monitor_arch.h to monitor.h

[Corneliu ZUZU]

On 2/10/2016 7:16 PM, Jan Beulich wrote:

On 10.02.16 at 17:44, <ta...@tklengyel.com> wrote:

On Wed, Feb 10, 2016 at 8:54 AM, Corneliu ZUZU <cz...@bitdefender.com>
wrote:


Rename:
 - arch/x86/monitor_x86.c -> arch/x86/monitor.c
 - asm-{x86,arm}/monitor_arch.h -> asm-{x86,arm}/monitor.h

(previous commit explains why these renames were necessary)


Referencing a "previous commit" like this is not acceptable as you don't
know how these patches will get applied and there might be other patches
that get committed in-between yours. In each patch explain clearly why the
patch is needed. I still find it odd that you need to rename x86/monitor.c
-> x86/monitor_x86.c -> x86/monitor.c in the same series.

Indeed. Thinking about it again perhaps the operations should be
"move to common/ (mostly) verbatim" followed by "break out x86
specific code" (if that's the smaller portion compared to what is to
stay).

Jan


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



That's a great alternative! Will do so in v3.

Corneliu.

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

Re: [Xen-devel] [PATCH v2 4/7] Rename monitor_x86.c to monitor.c and monitor_arch.h to monitor.h

[Corneliu ZUZU]

On 2/10/2016 6:44 PM, Tamas K Lengyel wrote:



On Wed, Feb 10, 2016 at 8:54 AM, Corneliu ZUZU <cz...@bitdefender.com 
<mailto:cz...@bitdefender.com>> wrote:


Rename:
- arch/x86/monitor_x86.c -> arch/x86/monitor.c
- asm-{x86,arm}/monitor_arch.h -> asm-{x86,arm}/monitor.h

(previous commit explains why these renames were necessary)


Referencing a "previous commit" like this is not acceptable as you 
don't know how these patches will get applied and there might be other 
patches that get committed in-between yours. In each patch explain 
clearly why the patch is needed. I still find it odd that you need to 
rename x86/monitor.c -> x86/monitor_x86.c -> x86/monitor.c in the same 
series.



Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com
<mailto:cz...@bitdefender.com>>
---



As I stated earlier, those commits (4 & 6) are the result of a git diff
limitation (also see my responses to these patches in v1).
The following simple experiment illustrates my point precisely:
1) create a local git repo, add file ./a.txt there with many lines (e.g. 
100) and make that commit 1
2) now create a dir ./d, move a.txt -> ./d/a.txt and add another ./a.txt 
w/ few lines, e.g. 2, make that commit 2


Git diff (even w/ the -M, -C options) of commit 2 would then report that:

# ./a.txt has been modified => diff w/ 100 deletions & 2 additions
# ./d/a.txt has been added => diff w/ 100 additions
=> 202 additions & deletions total, *even though* similarity 
between ./a.txt in commit 1 and
./d/a.txt in commit 2 is 100% ! (also even if you use -M, -C 
diff options)


*instead of* reporting

# ./a.txt moved to ./d/a.txt, similarity index 100% => 0 deletions 
& 0 additions

# added ./a.txt => 2 additions
=> 2 additions total

You could see why this limitation would have made the diffs somewhat bulky,
especially when those files also get to be modified in the process.
This is also the reason why accepting patch 3 without 4 or 5 without 6 
wouldn't

make sense, 3+4 should be treated as a single patch, 5+6 similarly.

As a proposed resolution: since I took these measures *only to ease code 
review*,
patches 4 & 6 can be safely squashed onto their previous commits (3 & 5) 
when and if acking them.
Or I could just worry less about this next time and simply squash them 
myself before sending

them for review, less to do on my part :).

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

Re: [Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

[Corneliu ZUZU]

On 2/10/2016 7:11 PM, Jan Beulich wrote:

On 10.02.16 at 18:04,  wrote:

On 2/10/2016 6:18 PM, Jan Beulich wrote:

On 10.02.16 at 16:50,  wrote:

--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -17,6 +17,12 @@
   #ifndef __ASM_X86_HVM_EVENT_H__
   #define __ASM_X86_HVM_EVENT_H__
   
+enum hvm_event_breakpoint_type

+{
+HVM_EVENT_SOFTWARE_BREAKPOINT,
+HVM_EVENT_SINGLESTEP_BREAKPOINT,
+};

I don't see what good it does to put existing constants into an
enum.

As Andrew pointed out, an enum was requested in v1 instead of the
single_step param.
One could use the already existing VM_EVENT_REASON_* constants, but
conceptually this
function only involves a subset of those (i.e. *breakpoint vm-events*).

Re-using existing constants would seem fine to me.


Yes but then IMHO conceptually that would be wrong, since it would be 
like saying
"that param can be any type of vm-event", even though it can actually be 
only from a

subset of vm-event types and it will never be *any* type of vm-event.


I only now realize that I've made a mistake while looking at the
above - the capitals made it implicitly "obvious" to me that they're
on the right side of an assignment. Please use capitals only for
#define-d constants, not enumerated ones.

Jan


Most examples of existing enums I've looked at were capital-case, I thought
they're supposed to be like that. Could you please provide an example on how
enum values should look? (there isn't any in CODING_STYLE)

Let me know if you still want me to change this and how.

Corneliu.

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

Re: [Xen-devel] [PATCH v2 2/7] xen/x86: merge 2 hvm_event_... functions into 1

[Corneliu ZUZU]

On 2/10/2016 7:11 PM, Jan Beulich wrote:

On 10.02.16 at 18:04,  wrote:

On 2/10/2016 6:18 PM, Jan Beulich wrote:

On 10.02.16 at 16:50,  wrote:

--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -17,6 +17,12 @@
   #ifndef __ASM_X86_HVM_EVENT_H__
   #define __ASM_X86_HVM_EVENT_H__
   
+enum hvm_event_breakpoint_type

+{
+HVM_EVENT_SOFTWARE_BREAKPOINT,
+HVM_EVENT_SINGLESTEP_BREAKPOINT,
+};

I don't see what good it does to put existing constants into an
enum.

As Andrew pointed out, an enum was requested in v1 instead of the
single_step param.
One could use the already existing VM_EVENT_REASON_* constants, but
conceptually this
function only involves a subset of those (i.e. *breakpoint vm-events*).

Re-using existing constants would seem fine to me.

I only now realize that I've made a mistake while looking at the
above - the capitals made it implicitly "obvious" to me that they're
on the right side of an assignment. Please use capitals only for
#define-d constants, not enumerated ones.

Jan




Also, why this bias towards #define vs enums? I usually always prefer 
the latter,
they make the code more readable, e.g. it's clearer what can or cannot 
be passed
to a corresponding function parameter. And from an IDE user's 
point-of-view, I don't

understand why I have to read a comment and search for a list of identifiers
rather than just clicking on a type name and getting there in a flash.
They also make debugging easier, etc.. :).

Corneliu.

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

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

On 2/10/2016 7:56 PM, Tamas K Lengyel wrote:



On Wed, Feb 10, 2016 at 10:34 AM, Corneliu ZUZU <cz...@bitdefender.com 
<mailto:cz...@bitdefender.com>> wrote:



On 2/10/2016 6:39 PM, Tamas K Lengyel wrote:



diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 3a90f47..e46be1b 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -14,6 +14,10 @@ config X86
select HAS_MEM_ACCESS
select HAS_MEM_PAGING
select HAS_MEM_SHARING
+   select HAS_VM_EVENT_WRITE_CTRLREG


You mentioned in the previous revision of this series that
currently you only have plans to implement write_ctrleg and
guest_request events for ARM. I think singlestep and
software_breakpoint should not be moved to common without a clear
plan to have those implemented. Now, if you were to include the
implementation of write_ctrlreg and guest_request in this series
and leave the others in x86 as they are now, I don't think there
would be any reason to have these Kconfig options present at all.


Moving what made sense to be moved to common was a suggestion of
Ian's.
The purpose of this patch is to --avoid-- having to go through
this process again
when an implementation of feature X for architecture A != X86
comes into place.
IMHO what is common should stay in common and I don't see any
issues w/
having them there, only advantages (future implementations of
these features will
be easier).
Maybe Ian can chime in on this.


That's the upside. The downside is that in the interim, while those 
features are not implemented, we need to add a bunch of Kconfig 
variables to decide under what build they are available.


Technically, you don't need to add anything unless you implement that 
feature.
E.g. the ARM Kconfig currently contains no mention of these options, 
since they're not implemented there @ all.
And when implemented they're only added once and it's one line "select 
HAS_", it's not like you have to specify a command-line
parameter when building Xen or something like that, so IMO they don't 
add considerable complexity.
And after all, these kind of situations (i.e. activating/deactivating 
code based on architecture) are why arch-specific Kconfigs exist, right? 
Why not use them? :)


If it was moved to common only when the feature is available for all 
architectures, we wouldn't need that many ifdefs and the code would be 
clearer. So I do see why it would be beneficial having these moved to 
common now, but I still rather have it happen when it's necessary and 
without the Kconfig settings.


What if a 3rd architecture comes into place, you'd have to move them 
back from common to the arch-side and get back to the code duplication 
we just got rid of?
And if you then also implement it for the 3rd architecture, you move 
them back to common from the arch-side?
It seems uncomfortable having to keep track of all architectures when 
wanting to add such a feature implementation for just one of them.
I don't know what & if such plans exist for Xen, but why make that 
future process of adding in support for other architectures 
unnecessarily complicated,

even if it won't happen soon?

Also, IMO the code is clearer like this:
* you know what parts interest you when implementing parts of these 
features/when debugging/when simply looking @ the code
* the #ifdefs make it possible for that code to be put in common => that 
makes it *clear* that those code parts are NOT
architecture specific and their implementation can be safely used for 
all architectures.
* code duplication is avoided => avoid having to reflect a modification 
when it happens in more places than it would be necessary


There are disadvantages, everything has a downside but IMHO they are 
minor compared to the alternative.


Ian, any comments on this? :)

Thank you,
Corneliu.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

On 2/10/2016 6:39 PM, Tamas K Lengyel wrote:
I think it would be better if this function just had a single rc 
instead of two (not passing one rc as a pointer on input).


Good point. Would it be ok if:
* I remove the rc param
* make return type int
* rc = 0 if arch-side didn't handle the event, but no errors occurred
* rc = 1 if arch-side handled the event and no errors occurred
* rc < 0 if errors occurred
* return rc
?

Didn't cross my mind why error rcs are < 0, I only now realize that
probably just these kind of situations are the reason for that.

Corneliu.

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

Re: [Xen-devel] [PATCH 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Corneliu ZUZU]

On 2/9/2016 7:40 PM, Tamas K Lengyel wrote:



On Tue, Feb 9, 2016 at 5:32 AM, Corneliu ZUZU <cz...@bitdefender.com 
<mailto:cz...@bitdefender.com>> wrote:


There are already directories w/ just one/a few files in them,
even small (e.g. common/gcov/gcov.c).
IMHO no harm is done if a file is put in its proper directory even
before it grows too large.
This way one can find the file more easily, future additions are
more visibly encouraged to
also be separated in that directory when it makes sense, symmetry
between arch directories remains
intact (=> easier to compare between code for different
arches/find equivalent files between them).

But I am not a Xen maintainer, I'm only a contributor so I can
only suggest :).
If ARM maintainers (e.g. Tamas) feel the same, I will move the
file back.


I don't really have a strong opinion about this either way, so if the 
others prefer it staying as it is then there is no point moving it.


Tamas



Ok, will do, thanks for the feedback.

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

Re: [Xen-devel] [PATCH 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Corneliu ZUZU]

On 2/9/2016 1:03 PM, Stefano Stabellini wrote:

On Mon, 8 Feb 2016, Corneliu ZUZU wrote:

X86-side hvm.c is @ arch/x86/hvm/hvm.c. To maintain arm<->x86 symmetry,
also move arch/arm/hvm.c to arch/arm/hvm/hvm.c.

Why are we doing this? These are not header files, their paths don't
necessarily need to be the same and xen/arch/x86/hvm/hvm.c is very
different from xen/arch/arm/hvm.c.

Please state the reason more clearly.



Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
  xen/arch/arm/Makefile |  2 +-
  xen/arch/arm/hvm.c| 67 ---
  xen/arch/arm/hvm/Makefile |  1 +
  xen/arch/arm/hvm/hvm.c| 66 ++



Because the ARM side hvm.c currently exists solely to add an implementation for
do_hvm_op, which on x86 is @ arch/x86/hvm/hvm.c. I presume the ARM hvm.c got 
its name
from the X86 file, so I thought a symmetry between the two was intended from 
the start.
Also, the hvm directory was created to separate hvm-specific code, which is the 
case w/
do_hvm_op on any arch.

Corneliu.


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

Re: [Xen-devel] [PATCH 2/7] x86: hvm events: merge 2 functions into 1

[Corneliu ZUZU]

On 2/9/2016 1:19 PM, Jan Beulich wrote:

On 08.02.16 at 17:57,  wrote:

This patch merges almost identical functions hvm_event_int3
and hvm_event_single_step into a single function called
hvm_event_software_breakpoint.

Except that "software breakpoint" is rather questionable a name
here, considering that on x86 this is basically an alias for "int3".
If it was "breakpoint", one might argue (see the other responses
you've got) that breakpoint event resulting from debug register
settings might then be candidates to come here too.

Jan


Yeah..should I then:
* keep both functions and only rename hvm_event_int3 to 
hvm_event_software_breakpoint
* separate the code that gets the GFN of the instruction pointer in a 
static inline function

?

Corneliu.

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

Re: [Xen-devel] [PATCH 2/7] x86: hvm events: merge 2 functions into 1

[Corneliu ZUZU]

On 2/9/2016 2:12 PM, Jan Beulich wrote:

On 09.02.16 at 12:52,  wrote:

On 2/9/2016 1:19 PM, Jan Beulich wrote:

On 08.02.16 at 17:57,  wrote:

This patch merges almost identical functions hvm_event_int3
and hvm_event_single_step into a single function called
hvm_event_software_breakpoint.

Except that "software breakpoint" is rather questionable a name
here, considering that on x86 this is basically an alias for "int3".
If it was "breakpoint", one might argue (see the other responses
you've got) that breakpoint event resulting from debug register
settings might then be candidates to come here too.

Yeah..should I then:
* keep both functions and only rename hvm_event_int3 to
hvm_event_software_breakpoint

I actually think that the intention of folding two almost identical
functions is a good one. I'm merely suggesting to think of a
better name - perhaps just "breakpoint" or "debug event"?




SGTM. I'll change it to hvm_event_breakpoint then.

Corneliu.

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

Re: [Xen-devel] [PATCH 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Corneliu ZUZU]

On 2/9/2016 1:55 PM, Jan Beulich wrote:

On 09.02.16 at 12:28, <cz...@bitdefender.com> wrote:

On 2/9/2016 1:03 PM, Stefano Stabellini wrote:

On Mon, 8 Feb 2016, Corneliu ZUZU wrote:

X86-side hvm.c is @ arch/x86/hvm/hvm.c. To maintain arm<->x86 symmetry,
also move arch/arm/hvm.c to arch/arm/hvm/hvm.c.

Why are we doing this? These are not header files, their paths don't
necessarily need to be the same and xen/arch/x86/hvm/hvm.c is very
different from xen/arch/arm/hvm.c.

Please state the reason more clearly.



Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
   xen/arch/arm/Makefile |  2 +-
   xen/arch/arm/hvm.c| 67 
---
   xen/arch/arm/hvm/Makefile |  1 +
   xen/arch/arm/hvm/hvm.c| 66 ++


Because the ARM side hvm.c currently exists solely to add an implementation
for
do_hvm_op, which on x86 is @ arch/x86/hvm/hvm.c. I presume the ARM hvm.c got
its name
from the X86 file, so I thought a symmetry between the two was intended from
the start.
Also, the hvm directory was created to separate hvm-specific code, which is
the case w/
do_hvm_op on any arch.

While I'm not an ARM maintainer, this change still strikes me as odd
(or a change for the change's sake). A directory with just one file
in it (and - afaict - no current perspective to gain more) is just
pointless. In fact it's usually the other way around: When a file
grows (or would grow) too large, a similarly named subdirectory
gets introduced with the contents "scattered" across multiple files
in that directory.

Jan


There are already directories w/ just one/a few files in them, even 
small (e.g. common/gcov/gcov.c).
IMHO no harm is done if a file is put in its proper directory even 
before it grows too large.
This way one can find the file more easily, future additions are more 
visibly encouraged to
also be separated in that directory when it makes sense, symmetry 
between arch directories remains
intact (=> easier to compare between code for different arches/find 
equivalent files between them).


But I am not a Xen maintainer, I'm only a contributor so I can only 
suggest :).

If ARM maintainers (e.g. Tamas) feel the same, I will move the file back.

Corneliu.

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

[Xen-devel] [PATCH 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Corneliu ZUZU]

X86-side hvm.c is @ arch/x86/hvm/hvm.c. To maintain arm<->x86 symmetry,
also move arch/arm/hvm.c to arch/arm/hvm/hvm.c.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/arm/Makefile |  2 +-
 xen/arch/arm/hvm.c| 67 ---
 xen/arch/arm/hvm/Makefile |  1 +
 xen/arch/arm/hvm/hvm.c| 66 ++
 4 files changed, 68 insertions(+), 68 deletions(-)
 delete mode 100644 xen/arch/arm/hvm.c
 create mode 100644 xen/arch/arm/hvm/Makefile
 create mode 100644 xen/arch/arm/hvm/hvm.c

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 1783912..22f1c75 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -1,6 +1,7 @@
 subdir-$(CONFIG_ARM_32) += arm32
 subdir-$(CONFIG_ARM_64) += arm64
 subdir-y += platforms
+subdir-y += hvm
 subdir-$(CONFIG_ARM_64) += efi
 
 obj-$(EARLY_PRINTK) += early_printk.o
@@ -34,7 +35,6 @@ obj-y += vgic.o vgic-v2.o
 obj-$(CONFIG_ARM_64) += vgic-v3.o
 obj-y += vtimer.o
 obj-y += vuart.o
-obj-y += hvm.o
 obj-y += device.o
 obj-y += decode.o
 obj-y += processor.o
diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
deleted file mode 100644
index 5fd0753..000
--- a/xen/arch/arm/hvm.c
+++ /dev/null
@@ -1,67 +0,0 @@
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-#include 
-
-#include 
-#include 
-#include 
-
-#include 
-
-long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
-
-{
-long rc = 0;
-
-switch ( op )
-{
-case HVMOP_set_param:
-case HVMOP_get_param:
-{
-struct xen_hvm_param a;
-struct domain *d;
-
-if ( copy_from_guest(, arg, 1) )
-return -EFAULT;
-
-if ( a.index >= HVM_NR_PARAMS )
-return -EINVAL;
-
-d = rcu_lock_domain_by_any_id(a.domid);
-if ( d == NULL )
-return -ESRCH;
-
-rc = xsm_hvm_param(XSM_TARGET, d, op);
-if ( rc )
-goto param_fail;
-
-if ( op == HVMOP_set_param )
-{
-d->arch.hvm_domain.params[a.index] = a.value;
-}
-else
-{
-a.value = d->arch.hvm_domain.params[a.index];
-rc = copy_to_guest(arg, , 1) ? -EFAULT : 0;
-}
-
-param_fail:
-rcu_unlock_domain(d);
-break;
-}
-
-default:
-{
-gdprintk(XENLOG_DEBUG, "HVMOP op=%lu: not implemented\n", op);
-rc = -ENOSYS;
-break;
-}
-}
-
-return rc;
-}
diff --git a/xen/arch/arm/hvm/Makefile b/xen/arch/arm/hvm/Makefile
new file mode 100644
index 000..6ee4054
--- /dev/null
+++ b/xen/arch/arm/hvm/Makefile
@@ -0,0 +1 @@
+obj-y += hvm.o
diff --git a/xen/arch/arm/hvm/hvm.c b/xen/arch/arm/hvm/hvm.c
new file mode 100644
index 000..1ae681f
--- /dev/null
+++ b/xen/arch/arm/hvm/hvm.c
@@ -0,0 +1,66 @@
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include 
+#include 
+#include 
+
+#include 
+
+long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
+{
+long rc = 0;
+
+switch ( op )
+{
+case HVMOP_set_param:
+case HVMOP_get_param:
+{
+struct xen_hvm_param a;
+struct domain *d;
+
+if ( copy_from_guest(, arg, 1) )
+return -EFAULT;
+
+if ( a.index >= HVM_NR_PARAMS )
+return -EINVAL;
+
+d = rcu_lock_domain_by_any_id(a.domid);
+if ( d == NULL )
+return -ESRCH;
+
+rc = xsm_hvm_param(XSM_TARGET, d, op);
+if ( rc )
+goto param_fail;
+
+if ( op == HVMOP_set_param )
+{
+d->arch.hvm_domain.params[a.index] = a.value;
+}
+else
+{
+a.value = d->arch.hvm_domain.params[a.index];
+rc = copy_to_guest(arg, , 1) ? -EFAULT : 0;
+}
+
+param_fail:
+rcu_unlock_domain(d);
+break;
+}
+
+default:
+{
+gdprintk(XENLOG_DEBUG, "HVMOP op=%lu: not implemented\n", op);
+rc = -ENOSYS;
+break;
+}
+}
+
+return rc;
+}
-- 
2.5.0


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

[Xen-devel] [PATCH 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

1. Kconfig:
  * Added Kconfigs for common monitor vm-events:
  # see files: common/Kconfig, x86/Kconfig
HAS_VM_EVENT_WRITE_CTRLREG
HAS_VM_EVENT_SINGLESTEP
HAS_VM_EVENT_SOFTWARE_BREAKPOINT
HAS_VM_EVENT_GUEST_REQUEST

2. Moved monitor_domctl from arch-side to common-side
  2.1. Moved arch/x86/monitor.c to common/monitor.c
# see files: arch/x86/Makefile, xen/common/Makefile, xen/common/monitor.c
# changes:
- removed status_check (we would have had to duplicate it in X86
arch_monitor_domctl_event otherwise)
- moved get_capabilities to arch-side (arch_monitor_get_capabilities)
- moved XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP to arch-side (see
arch_monitor_domctl_op)
- put XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR to x86-side (see
arch_monitor_domctl_event)
- surrounded switch cases w/ CONFIG_HAS_VM_EVENT_*

  2.2. Moved asm-x86/monitor.h to xen/monitor.h
# see files: arch/x86/hvm/event.c, arch/x86/hvm/hvm.c,
 arch/x86/hvm/vmx/vmx.c, xen/common/domctl.c

  2.3. Removed asm-arm/monitor.h (no longer needed)

3. Added x86/monitor_x86.c => will rename in next commit to monitor.c (not done
in this commit to avoid git seeing this as being the modified old monitor.c =>
keeping the same name would have rendered an unnecessarily bulky diff)
# see files: arch/x86/Makefile
# implements X86-side arch_monitor_domctl_event

4. Added asm-x86/monitor_arch.h, asm-arm/monitor_arch.h (renamed to
monitor.h in next commit, reason is the same as @ (3.).
# define/implement: arch_monitor_get_capabilities, arch_monitor_domctl_op
and arch_monitor_domctl_event

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/x86/Kconfig   |   4 +
 xen/arch/x86/Makefile  |   2 +-
 xen/arch/x86/hvm/event.c   |   2 +-
 xen/arch/x86/hvm/hvm.c |   2 +-
 xen/arch/x86/hvm/vmx/vmx.c |   2 +-
 xen/arch/x86/monitor.c | 228 -
 xen/arch/x86/monitor_x86.c |  72 
 xen/common/Kconfig |  20 
 xen/common/Makefile|   1 +
 xen/common/domctl.c|   2 +-
 xen/common/monitor.c   | 203 +
 xen/include/asm-arm/monitor.h  |  33 --
 xen/include/asm-arm/monitor_arch.h |  53 +
 xen/include/asm-x86/monitor.h  |  31 -
 xen/include/asm-x86/monitor_arch.h |  74 
 xen/include/xen/monitor.h  |  36 ++
 16 files changed, 468 insertions(+), 297 deletions(-)
 delete mode 100644 xen/arch/x86/monitor.c
 create mode 100644 xen/arch/x86/monitor_x86.c
 create mode 100644 xen/common/monitor.c
 delete mode 100644 xen/include/asm-arm/monitor.h
 create mode 100644 xen/include/asm-arm/monitor_arch.h
 delete mode 100644 xen/include/asm-x86/monitor.h
 create mode 100644 xen/include/asm-x86/monitor_arch.h
 create mode 100644 xen/include/xen/monitor.h

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 3a90f47..e46be1b 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -14,6 +14,10 @@ config X86
select HAS_MEM_ACCESS
select HAS_MEM_PAGING
select HAS_MEM_SHARING
+   select HAS_VM_EVENT_WRITE_CTRLREG
+   select HAS_VM_EVENT_SINGLESTEP
+   select HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+   select HAS_VM_EVENT_GUEST_REQUEST
select HAS_NS16550
select HAS_PASSTHROUGH
select HAS_PCI
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 8e6e901..6e80cf0 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -36,7 +36,7 @@ obj-y += microcode_intel.o
 # This must come after the vendor specific files.
 obj-y += microcode.o
 obj-y += mm.o x86_64/mm.o
-obj-y += monitor.o
+obj-y += monitor_x86.o
 obj-y += mpparse.o
 obj-y += nmi.o
 obj-y += numa.o
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index 9dc533b..5ffc485 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -20,8 +20,8 @@
 
 #include 
 #include 
+#include 
 #include 
-#include 
 #include 
 #include 
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 35ec6c9..9063eb5 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -51,7 +52,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 1a24788..f7708fe 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -57,7 +58,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 
 static bool_t __initdata opt_force_ept;
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
deleted file mode 100644
index 1d43

[Xen-devel] [PATCH 5/7] xen/vm-events: Move hvm_event_* functions to common-side.

[Corneliu ZUZU]

1. Moved hvm_event_traps, hvm_event_cr, hvm_event_guest_request,
hvm_event_software_breakpoint from arch-side to common-side

  1.1. Moved arch/x86/hvm/event.c to common/hvm/event.c
# see files: arch/x86/hvm/Makefile, xen/common/hvm/Makefile,
 xen/common/hvm/event.c
# changes:
- moved hvm_event_fill_regs to arch-side 
(arch_hvm_event_fill_regs)
- added vcpu parameter to hvm_event_traps
- surrounded common hvm_event_* implementations w/ 
CONFIG_HAS_VM_EVENT_*
- moved hvm_event_msr to arch-side (see x86/hvm/event_x86.c)
- moved rip->gfn code in hvm_event_software_breakpoint to 
arch-side
(see arch_hvm_event_gfn_of_ip) and renamed rip param to 
ip - i.e.
now the parameter is a 'generic' instruction pointer, 
rather than
the Intel X86_64 RIP register)

  1.2. Moved asm-x86/hvm/event.h to xen/hvm/event.h
# see files: arch/x86/hvm/hvm.c, arch/x86/hvm/vmx/vmx.c

2. Added x86/hvm/event_x86.c => will rename in next commit to event.c (not done
in this commit to avoid git seeing this as being the modified old event.c =>
keeping the same name would have rendered an unnecessarily bulky diff)
# see files: arch/x86/hvm/Makefile
# implements X86-specific hvm_event_msr

3. Added asm-x86/hvm/event_arch.h, asm-arm/hvm/event_arch.h (renamed to
event.h in next commit, reason is the same as @ (2.).
# define/implement: arch_hvm_event_fill_regs, arch_hvm_event_gfn_of_ip,
hvm_event_msr (X86 side only)

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/x86/hvm/Makefile|   2 +-
 xen/arch/x86/hvm/event.c | 203 ---
 xen/arch/x86/hvm/event_x86.c |  51 +
 xen/arch/x86/hvm/hvm.c   |   3 +-
 xen/arch/x86/hvm/vmx/vmx.c   |   2 +-
 xen/common/hvm/Makefile  |   1 +
 xen/common/hvm/event.c   | 172 +
 xen/include/asm-arm/hvm/event_arch.h |  40 +++
 xen/include/asm-x86/hvm/event.h  |  44 
 xen/include/asm-x86/hvm/event_arch.h |  93 
 xen/include/xen/hvm/event.h  |  71 
 11 files changed, 432 insertions(+), 250 deletions(-)
 delete mode 100644 xen/arch/x86/hvm/event.c
 create mode 100644 xen/arch/x86/hvm/event_x86.c
 create mode 100644 xen/common/hvm/event.c
 create mode 100644 xen/include/asm-arm/hvm/event_arch.h
 delete mode 100644 xen/include/asm-x86/hvm/event.h
 create mode 100644 xen/include/asm-x86/hvm/event_arch.h
 create mode 100644 xen/include/xen/hvm/event.h

diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 794e793..15daa09 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -3,7 +3,7 @@ subdir-y += vmx
 
 obj-y += asid.o
 obj-y += emulate.o
-obj-y += event.o
+obj-y += event_x86.o
 obj-y += hpet.o
 obj-y += hvm.o
 obj-y += i8254.o
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
deleted file mode 100644
index 5ffc485..000
--- a/xen/arch/x86/hvm/event.c
+++ /dev/null
@@ -1,203 +0,0 @@
-/*
-* 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, see <http://www.gnu.org/licenses/>.
-*/
-
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-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->data.regs.x86.rax = regs->eax;
-req->data.regs.x86.rcx = regs->ecx;
-req->data.regs.x86.rdx = regs->edx;
-req->data.regs.x86.rbx = regs->ebx;
-req->data.regs.x86.rsp = regs->esp;
-req->data.regs.x86.rbp = regs->ebp;
-req->data.regs.x86.rsi = regs->esi;
-req->data.regs.x86.rdi = regs->edi;
-
-req->data.regs.x86.r8  = regs->r8;
-req->data.regs.x86.r9  = regs->r9;
-req->data.regs.x86.r10 = regs->r10;
-req->data.regs.x86.r11 = regs->r11;
-req->data.regs.x86.r12 = regs->r12;
-req->data.regs.x86.r13 = r

[Xen-devel] [PATCH 7/7] arch.monitor: move bits to common (arch_domain to domain)

[Corneliu ZUZU]

This patch moves bitfield members for single-step, software-breakpoint and
guest-request monitor vm-events from the arch-side (struct arch_domain) to
the common-side (struct domain). Ctrl-reg bits (i.e. write_ctrlreg_* members)
are left on the arch-side, because control-registers number can vary across
architectures.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c  |  4 ++--
 xen/common/hvm/event.c   | 12 ++--
 xen/common/monitor.c | 17 +++--
 xen/include/asm-x86/domain.h | 16 ++--
 xen/include/xen/sched.h  | 16 
 5 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 5bc3c74..07acbf2 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1706,8 +1706,8 @@ void vmx_do_resume(struct vcpu *v)
 }
 
 debug_state = v->domain->debugger_attached
-  || v->domain->arch.monitor.software_breakpoint_enabled
-  || v->domain->arch.monitor.singlestep_enabled;
+  || v->domain->monitor.software_breakpoint_enabled
+  || v->domain->monitor.singlestep_enabled;
 
 if ( unlikely(v->arch.hvm_vcpu.debug_state_latch != debug_state) )
 {
diff --git a/xen/common/hvm/event.c b/xen/common/hvm/event.c
index d4ce97a..2660093 100644
--- a/xen/common/hvm/event.c
+++ b/xen/common/hvm/event.c
@@ -110,16 +110,16 @@ bool_t hvm_event_cr(unsigned int index,
 void hvm_event_guest_request(void)
 {
 struct vcpu *curr = current;
-struct arch_domain *ad = >domain->arch;
+struct domain *d = curr->domain;
 
-if ( ad->monitor.guest_request_enabled )
+if ( d->monitor.guest_request_enabled )
 {
 vm_event_request_t req = {
 .reason = VM_EVENT_REASON_GUEST_REQUEST,
 .vcpu_id = curr->vcpu_id,
 };
 
-hvm_event_traps(curr, ad->monitor.guest_request_sync, );
+hvm_event_traps(curr, d->monitor.guest_request_sync, );
 }
 }
 
@@ -131,9 +131,9 @@ int hvm_event_software_breakpoint(unsigned long ip, bool_t 
single_step)
 {
 int rc = 0;
 struct vcpu *curr = current;
-struct arch_domain *ad = >domain->arch;
-bool_t enabled = ( single_step ? ad->monitor.singlestep_enabled
-   : ad->monitor.software_breakpoint_enabled );
+struct domain *d = curr->domain;
+bool_t enabled = ( single_step ? d->monitor.singlestep_enabled
+   : d->monitor.software_breakpoint_enabled );
 
 if ( enabled )
 {
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index 1165aeb..b745aa4 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -121,14 +121,13 @@ int monitor_domctl(struct domain *d, struct 
xen_domctl_monitor_op *mop)
 
 case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
 {
-struct arch_domain *ad = >arch;
-bool_t old_status = ad->monitor.singlestep_enabled;
+bool_t old_status = d->monitor.singlestep_enabled;
 
 if ( unlikely(old_status == requested_status) )
 return -EEXIST;
 
 domain_pause(d);
-ad->monitor.singlestep_enabled = !old_status;
+d->monitor.singlestep_enabled = !old_status;
 domain_unpause(d);
 break;
 }
@@ -139,14 +138,13 @@ int monitor_domctl(struct domain *d, struct 
xen_domctl_monitor_op *mop)
 
 case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
 {
-struct arch_domain *ad = >arch;
-bool_t old_status = ad->monitor.software_breakpoint_enabled;
+bool_t old_status = d->monitor.software_breakpoint_enabled;
 
 if ( unlikely(old_status == requested_status) )
 return -EEXIST;
 
 domain_pause(d);
-ad->monitor.software_breakpoint_enabled = !old_status;
+d->monitor.software_breakpoint_enabled = !old_status;
 domain_unpause(d);
 break;
 }
@@ -157,15 +155,14 @@ int monitor_domctl(struct domain *d, struct 
xen_domctl_monitor_op *mop)
 
 case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
 {
-struct arch_domain *ad = >arch;
-bool_t old_status = ad->monitor.guest_request_enabled;
+bool_t old_status = d->monitor.guest_request_enabled;
 
 if ( unlikely(old_status == requested_status) )
 return -EEXIST;
 
 domain_pause(d);
-ad->monitor.guest_request_sync = mop->u.guest_request.sync;
-ad->monitor.guest_request_enabled = !old_status;
+d->monitor.guest_request_sync = mop->u.guest_request.sync;
+d->monitor.guest_request_enabled = !old_status;
 domain_unpause(d);
 break;
 }
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 4072e27..6254060 100644
--- a/xen/include/asm-

[Xen-devel] [PATCH 0/7] Vm-events: move monitor vm-events code to common code.

[Corneliu ZUZU]

This patch series is an attempt to move (most) of the monitor vm-events code to
the common-side.

Patches summary:

1. Move xen/arch/arm/hvm.c to xen/arch/arm/hvm/hvm.c

2. Merge almost identical functions hvm_event_int3 + This patch series is an 
attempt to move (most) of the monitor vm-events code to
the common-side.

Patches summary:

1. Move xen/arch/arm/hvm.c to xen/arch/arm/hvm/hvm.c

2. Merge almost identical functions hvm_event_int3 + hvm_event_single_step ->
hvm_event_software_breakpoint.

3. Add Kconfigs:
HAS_VM_EVENT_WRITE_CTRLREG, HAS_VM_EVENT_SINGLESTEP,
HAS_VM_EVENT_SOFTWARE_BREAKPOINT, HAS_VM_EVENT_GUEST_REQUEST
and move monitor_domctl to common-side. Used the Kconfigs to selectively
activate implemented monitor vm-events code for each architecture.

4. Some file renames. Read (*) below.

5. Move hvm_event_traps, hvm_event_cr, hvm_event_guest_request,
hvm_event_software_breakpoint functions to common-side.
(note: arch_hvm_event_fill_regs on ARM-side will be implemented in a separate
patch)

6. Some file renames, plus some minor fixes. Read (*) below.

7. Move monitor bitfield members from struct arch_domain to struct domain.
Moved bits: single-step, software-breakpoint and guest-request.
(note: write_ctrlreg_* were left on the arch-side, since control-registers
number can vary across architectures)

(*) was only necessary to avoid git seeing a file as being modified, rather than
moved (would have made the diff unnecessarily bulky).hvm_event_single_step ->
hvm_event_software_breakpoint.

3. Add Kconfigs:
HAS_VM_EVENT_WRITE_CTRLREG, HAS_VM_EVENT_SINGLESTEP,
HAS_VM_EVENT_SOFTWARE_BREAKPOINT, HAS_VM_EVENT_GUEST_REQUEST
and move monitor_domctl to common-side. Used the Kconfigs to selectively
activate implemented monitor vm-events code for each architecture.

4. Some file renames. Read (*) below.

5. Move hvm_event_traps, hvm_event_cr, hvm_event_guest_request,
hvm_event_software_breakpoint functions to common-side.
(note: arch_hvm_event_fill_regs on ARM-side will be implemented in a separate
patch)

6. Some file renames, plus some minor fixes. Read (*) below.

7. Move monitor bitfield members from struct arch_domain to struct domain.
Moved bits: single-step, software-breakpoint and guest-request.
(note: write_ctrlreg_* were left on the arch-side, since control-registers
number can vary across architectures)

(*) was only necessary to avoid git seeing a file as being modified, rather than
moved (would have made the diff unnecessarily bulky).

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

[Xen-devel] [PATCH 6/7] Rename event_x86.c to event.c and event_arch.h to event.h + minor fixes

[Corneliu ZUZU]

* rename arch/x86/hvm/event_x86.c to arch/x86/hvm/event.c and
asm-{x86,arm}/hvm/event_arch.h to asm-{x86/arm}/hvm/event.h (last commit
before this one explains why this was necessary)

Minor fixes:
* ARM fix: xen/common/hvm/event.c was not actually compiled
(see file-changes: xen/common/Makefile, xen/common/hvm/Makefile)
* xen/common/hvm/event.c: fix malformed file header comment
* xen/hvm/event.h: fix comment & change hvm_event_crX first param name to a more
descriptive one

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/x86/hvm/Makefile|  2 +-
 xen/arch/x86/hvm/event.c | 51 
 xen/arch/x86/hvm/event_x86.c | 51 
 xen/arch/x86/hvm/hvm.c   |  2 +-
 xen/common/Makefile  |  2 +-
 xen/common/hvm/Makefile  |  2 +-
 xen/common/hvm/event.c   | 44 -
 xen/include/asm-arm/hvm/event.h  | 40 
 xen/include/asm-arm/hvm/event_arch.h | 40 
 xen/include/asm-x86/hvm/event.h  | 93 
 xen/include/asm-x86/hvm/event_arch.h | 93 
 xen/include/xen/hvm/event.h  |  6 +--
 12 files changed, 213 insertions(+), 213 deletions(-)
 create mode 100644 xen/arch/x86/hvm/event.c
 delete mode 100644 xen/arch/x86/hvm/event_x86.c
 create mode 100644 xen/include/asm-arm/hvm/event.h
 delete mode 100644 xen/include/asm-arm/hvm/event_arch.h
 create mode 100644 xen/include/asm-x86/hvm/event.h
 delete mode 100644 xen/include/asm-x86/hvm/event_arch.h

diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 15daa09..794e793 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -3,7 +3,7 @@ subdir-y += vmx
 
 obj-y += asid.o
 obj-y += emulate.o
-obj-y += event_x86.o
+obj-y += event.o
 obj-y += hpet.o
 obj-y += hvm.o
 obj-y += i8254.o
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
new file mode 100644
index 000..3349c34
--- /dev/null
+++ b/xen/arch/x86/hvm/event.c
@@ -0,0 +1,51 @@
+/*
+ * arch/x86/hvm/event.c
+ *
+ * Arch-specific hardware virtual machine event abstractions.
+ *
+ * Copyright (c) 2004, Intel Corporation.
+ * Copyright (c) 2005, International Business Machines Corporation.
+ * Copyright (c) 2008, Citrix Systems, Inc.
+ * Copyright (c) 2016, Bitdefender S.R.L.
+ *
+ * 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, see <http://www.gnu.org/licenses/>.
+ */
+
+#include 
+
+void hvm_event_msr(unsigned int msr, uint64_t value)
+{
+struct vcpu *curr = current;
+
+if ( curr->domain->arch.monitor.mov_to_msr_enabled )
+{
+vm_event_request_t req = {
+.reason = VM_EVENT_REASON_MOV_TO_MSR,
+.vcpu_id = curr->vcpu_id,
+.u.mov_to_msr.msr = msr,
+.u.mov_to_msr.value = value,
+};
+
+hvm_event_traps(curr, 1, );
+}
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/hvm/event_x86.c b/xen/arch/x86/hvm/event_x86.c
deleted file mode 100644
index 7b54f18..000
--- a/xen/arch/x86/hvm/event_x86.c
+++ /dev/null
@@ -1,51 +0,0 @@
-/*
- * arch/x86/hvm/event_x86.c
- *
- * Arch-specific hardware virtual machine event abstractions.
- *
- * Copyright (c) 2004, Intel Corporation.
- * Copyright (c) 2005, International Business Machines Corporation.
- * Copyright (c) 2008, Citrix Systems, Inc.
- * Copyright (c) 2016, Bitdefender S.R.L.
- *
- * 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, see <http://www.gnu.org/licenses/>.
- */
-
-#include 
-
-void hvm_event_msr(unsigned int msr, uint64_t value)
-{
-struct vcpu *curr = current;
-
-if ( curr->domain->arch.monitor.mov_to_msr_enabled )
-{
-vm_event_request_t req = {
- 

[Xen-devel] [PATCH 4/7] Rename monitor_x86.c to monitor.c and monitor_arch.h to monitor.h

[Corneliu ZUZU]

(last commit before this one explains why this was necessary)

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/x86/Makefile  |  2 +-
 xen/arch/x86/monitor.c | 72 +
 xen/arch/x86/monitor_x86.c | 72 -
 xen/common/monitor.c   |  2 +-
 xen/include/asm-arm/monitor.h  | 53 +++
 xen/include/asm-arm/monitor_arch.h | 53 ---
 xen/include/asm-x86/monitor.h  | 74 ++
 xen/include/asm-x86/monitor_arch.h | 74 --
 8 files changed, 201 insertions(+), 201 deletions(-)
 create mode 100644 xen/arch/x86/monitor.c
 delete mode 100644 xen/arch/x86/monitor_x86.c
 create mode 100644 xen/include/asm-arm/monitor.h
 delete mode 100644 xen/include/asm-arm/monitor_arch.h
 create mode 100644 xen/include/asm-x86/monitor.h
 delete mode 100644 xen/include/asm-x86/monitor_arch.h

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 6e80cf0..8e6e901 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -36,7 +36,7 @@ obj-y += microcode_intel.o
 # This must come after the vendor specific files.
 obj-y += microcode.o
 obj-y += mm.o x86_64/mm.o
-obj-y += monitor_x86.o
+obj-y += monitor.o
 obj-y += mpparse.o
 obj-y += nmi.o
 obj-y += numa.o
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
new file mode 100644
index 000..568def2
--- /dev/null
+++ b/xen/arch/x86/monitor.c
@@ -0,0 +1,72 @@
+/*
+ * arch/x86/monitor.c
+ *
+ * Arch-specific monitor_op domctl handler.
+ *
+ * Copyright (c) 2015 Tamas K Lengyel (ta...@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that 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, see <http://www.gnu.org/licenses/>.
+ */
+
+#include 
+
+bool_t arch_monitor_domctl_event(struct domain *d,
+ struct xen_domctl_monitor_op *mop,
+ int *rc)
+{
+struct arch_domain *ad = >arch;
+bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
+
+switch ( mop->event )
+{
+case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
+{
+bool_t old_status = ad->monitor.mov_to_msr_enabled;
+
+if ( unlikely(old_status == requested_status) )
+return -EEXIST;
+
+if ( XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op &&
+ mop->u.mov_to_msr.extended_capture &&
+ !hvm_enable_msr_exit_interception(d) )
+return -EOPNOTSUPP;
+
+domain_pause(d);
+
+if ( XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op &&
+ mop->u.mov_to_msr.extended_capture )
+ad->monitor.mov_to_msr_extended = 1;
+else
+ad->monitor.mov_to_msr_extended = 0;
+
+ad->monitor.mov_to_msr_enabled = !old_status;
+domain_unpause(d);
+break;
+}
+
+default:
+return 0;
+}
+
+return 1;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/monitor_x86.c b/xen/arch/x86/monitor_x86.c
deleted file mode 100644
index d19fd15..000
--- a/xen/arch/x86/monitor_x86.c
+++ /dev/null
@@ -1,72 +0,0 @@
-/*
- * arch/x86/monitor_x86.c
- *
- * Arch-specific monitor_op domctl handler.
- *
- * Copyright (c) 2015 Tamas K Lengyel (ta...@tklengyel.com)
- * Copyright (c) 2016, Bitdefender S.R.L.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public
- * License v2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that 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, see <http://www.gnu.org/licenses/>.
- */
-
-#include 
-
-bool_t arch_monitor_domctl_event(struct domain *d,
- struct xen_domctl_monitor_op *mop,
- int *rc)
-{
-struct arch_domain *ad = >arch;
-bool_t 

[Xen-devel] [PATCH 2/7] x86: hvm events: merge 2 functions into 1

[Corneliu ZUZU]

This patch merges almost identical functions hvm_event_int3
and hvm_event_single_step into a single function called
hvm_event_software_breakpoint.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>
---
 xen/arch/x86/hvm/event.c| 52 ++---
 xen/arch/x86/hvm/vmx/vmx.c  | 11 +
 xen/include/asm-x86/hvm/event.h |  5 ++--
 3 files changed, 26 insertions(+), 42 deletions(-)

diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index a3d4892..9dc533b 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -151,17 +151,20 @@ void hvm_event_guest_request(void)
 }
 }
 
-int hvm_event_int3(unsigned long rip)
+int hvm_event_software_breakpoint(unsigned long rip, bool_t single_step)
 {
 int rc = 0;
 struct vcpu *curr = current;
+struct arch_domain *ad = >domain->arch;
+bool_t enabled = ( single_step ? ad->monitor.singlestep_enabled
+   : ad->monitor.software_breakpoint_enabled );
 
-if ( curr->domain->arch.monitor.software_breakpoint_enabled )
+if ( enabled )
 {
+uint64_t gfn;
 struct segment_register sreg;
 uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
 vm_event_request_t req = {
-.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT,
 .vcpu_id = curr->vcpu_id,
 };
 
@@ -170,37 +173,18 @@ int hvm_event_int3(unsigned long rip)
 pfec |= PFEC_user_mode;
 
 hvm_get_segment_register(curr, x86_seg_cs, );
-req.u.software_breakpoint.gfn = paging_gva_to_gfn(curr,
-  sreg.base + rip,
-  );
-
-rc = hvm_event_traps(1, );
-}
-
-return rc;
-}
-
-int hvm_event_single_step(unsigned long rip)
-{
-int rc = 0;
-struct vcpu *curr = current;
-
-if ( curr->domain->arch.monitor.singlestep_enabled )
-{
-struct segment_register sreg;
-uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
-vm_event_request_t req = {
-.reason = VM_EVENT_REASON_SINGLESTEP,
-.vcpu_id = curr->vcpu_id,
-};
-
-hvm_get_segment_register(curr, x86_seg_ss, );
-if ( sreg.attr.fields.dpl == 3 )
-pfec |= PFEC_user_mode;
-
-hvm_get_segment_register(curr, x86_seg_cs, );
-req.u.singlestep.gfn = paging_gva_to_gfn(curr, sreg.base + rip,
- );
+gfn = paging_gva_to_gfn(curr, sreg.base + rip, );
+
+if ( single_step )
+{
+req.reason = VM_EVENT_REASON_SINGLESTEP;
+req.u.singlestep.gfn = gfn;
+}
+else
+{
+req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
+req.u.software_breakpoint.gfn = gfn;
+}
 
 rc = hvm_event_traps(1, );
 }
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b9f340c..1a24788 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3192,7 +3192,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
 break;
 }
 else {
-int handled = hvm_event_int3(regs->eip);
+int handled = hvm_event_software_breakpoint(regs->eip, 0);
 
 if ( handled < 0 ) 
 {
@@ -3517,10 +3517,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
 case EXIT_REASON_MONITOR_TRAP_FLAG:
 v->arch.hvm_vmx.exec_control &= ~CPU_BASED_MONITOR_TRAP_FLAG;
 vmx_update_cpu_exec_control(v);
-if ( v->arch.hvm_vcpu.single_step ) {
-  hvm_event_single_step(regs->eip);
-  if ( v->domain->debugger_attached )
-  domain_pause_for_debugger();
+if ( v->arch.hvm_vcpu.single_step )
+{
+hvm_event_software_breakpoint(regs->eip, 1);
+if ( v->domain->debugger_attached )
+domain_pause_for_debugger();
 }
 
 break;
diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h
index 11eb1fe..7c2252b 100644
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -27,9 +27,8 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value,
 #define hvm_event_crX(what, new, old) \
 hvm_event_cr(VM_EVENT_X86_##what, new, old)
 void hvm_event_msr(unsigned int msr, uint64_t value);
-/* Called for current VCPU: returns -1 if no listener */
-int hvm_event_int3(unsigned long rip);
-int hvm_event_single_step(unsigned long rip);
+int hvm_event_software_breakpoint(unsigned long rip,
+  bool_t single_step);
 void hvm_event_guest_request(void);
 
 #endif /* __ASM_X86_HVM_EVENT_H__ */
-- 
2.5.0


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

Re: [Xen-devel] [PATCH 7/7] arch.monitor: move bits to common (arch_domain to domain)

[Corneliu ZUZU]

On 2/8/2016 8:29 PM, Tamas K Lengyel wrote:



On Mon, Feb 8, 2016 at 9:58 AM, Corneliu ZUZU <cz...@bitdefender.com 
<mailto:cz...@bitdefender.com>> wrote:


This patch moves bitfield members for single-step,
software-breakpoint and
guest-request monitor vm-events from the arch-side (struct
arch_domain) to
the common-side (struct domain). Ctrl-reg bits (i.e.
write_ctrlreg_* members)
are left on the arch-side, because control-registers number can
vary across
architectures.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com
<mailto:cz...@bitdefender.com>>


Technically this looks fine, but I do wonder if and what plans you 
have to actually implement these events for ARM.


Currently I've only planned implementations for control-register write 
events & guest-requests.
The other two also seem feasible though, I might give adding those a 
shot sometime after sending the other patches.


I haven't spent too much time looking into it, but I'm not aware of 
equivalent features on ARM to Intel MTF (singlestepping) or to 
software-breakpoint trapping. The only instruction I know that 
functionally comes close to software-breakpoint trapping (INT3) is the 
SMC instruction which can be trapped into the VMM, but I would not 
call that a "breakpoint" in the traditional sense.


Tamas



There's the debugging architecture, hypervisor control of that is 
possible on both 32-bit & 64-bit ARM.
It isn't as easy as for X86 though, where MTF is a hypervisor-internal 
feature and INT3 can be
trapped specifically, whereas on ARM granularity of trap-setting is less 
of a concern apparently.
For this reason, the only issue I see here is the performance penalty 
these traps would cause for
arbitrary software breakpoints (for obvious reasons that doesn't matter 
in the case of single-stepping).


For INT3, the ARM equivalent is be the BKPT/BRK (set HDCR.TDE on 
AArch32/MDCR_EL2.TDE AArch64) instruction.

Trapping on this instruction implies trapping on
- AArch32: some other debug exceptions (looking @ B1.8.9, ARMv7 DDI 0406C.b)
- AArch64: *all software debug exceptions* + *all debug register 
accesses* (this might cause some headaches)
For MTF-like functionality, the debug architecture also provides ways 
for single-stepping.
That would similarly generate software breakpoint exceptions which can 
be routed to the hypervisor.


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

Re: [Xen-devel] [PATCH 2/7] x86: hvm events: merge 2 functions into 1

[Corneliu ZUZU]

On 2/8/2016 8:17 PM, Tamas K Lengyel wrote:



On Mon, Feb 8, 2016 at 10:49 AM, Corneliu ZUZU <cz...@bitdefender.com 
<mailto:cz...@bitdefender.com>> wrote:


On 2/8/2016 7:15 PM, Andrew Cooper wrote:

On 08/02/16 16:57, Corneliu ZUZU wrote:

diff --git a/xen/include/asm-x86/hvm/event.h
b/xen/include/asm-x86/hvm/event.h
index 11eb1fe..7c2252b 100644
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -27,9 +27,8 @@ bool_t hvm_event_cr(unsigned int index,
unsigned long value,
  #define hvm_event_crX(what, new, old) \
  hvm_event_cr(VM_EVENT_X86_##what, new, old)
  void hvm_event_msr(unsigned int msr, uint64_t value);
-/* Called for current VCPU: returns -1 if no listener */
-int hvm_event_int3(unsigned long rip);
-int hvm_event_single_step(unsigned long rip);
+int hvm_event_software_breakpoint(unsigned long rip,
+  bool_t single_step);

Are we liable to ever gain any other type of software
breakpoint?  Might
it be more sense to pass an enum rather than a bool here?

Otherwise, the changes look sensible.

~Andrew


Well, IMHO, no. Besides breakpointing/trapping after each
instruction (i.e. VM_EVENT_REASON_SINGLESTEP)
and on arbitrary instructions
(VM_EVENT_REASON_SOFTWARE_BREAKPOINT) I don't see what other
types of breakpointing one can define (at least from the
hypervisor's point of view), and if in the future
there will be other types, that could also be changed into an enum
then.
But making that param an enum now would also be fine by me.
Since I noticed Tamas also prefers this option, I will make that
change.


It's just about code readability. Functionally it would be the same as 
it is right now, two options in the enum will likely be represented by 
0/1. But when you read the code you don't have to keep that boolean 
state in mind, you explicitly have the path spelled out.


Tamas



That makes sense, and probably that enum value will be handled in a switch.
Will do.

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

Re: [Xen-devel] [PATCH 1/7] arm: move arch/arm/hvm.c to arch/arm/hvm/hvm.c

[Corneliu ZUZU]

On 2/8/2016 7:04 PM, Andrew Cooper wrote:

On 08/02/16 16:57, Corneliu ZUZU wrote:

X86-side hvm.c is @ arch/x86/hvm/hvm.c. To maintain arm<->x86 symmetry,
also move arch/arm/hvm.c to arch/arm/hvm/hvm.c.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>

For future reference, constructing your patches with -M (detect renames)
makes reviews of patches like this far easier.

While you are editing this file, please put a local variable block on
the bottom of the file.  See the final section of CODING_STYLE in the root.

~Andrew

I'm really sorry, I forgot, I was actually *counting* on that option, 
wanted to use it as -M40%

actually. And I really don't get why git malformed the introductory message.
Since the diffs would indeed look *much* better w/ the -M option and I 
should also add
the variable block @ the end of that file (which was originally missing) 
would it be advised to

resend this series?

Corneliu.

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

Re: [Xen-devel] [PATCH 2/7] x86: hvm events: merge 2 functions into 1

[Corneliu ZUZU]

On 2/8/2016 7:15 PM, Andrew Cooper wrote:

On 08/02/16 16:57, Corneliu ZUZU wrote:

diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h
index 11eb1fe..7c2252b 100644
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -27,9 +27,8 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value,
  #define hvm_event_crX(what, new, old) \
  hvm_event_cr(VM_EVENT_X86_##what, new, old)
  void hvm_event_msr(unsigned int msr, uint64_t value);
-/* Called for current VCPU: returns -1 if no listener */
-int hvm_event_int3(unsigned long rip);
-int hvm_event_single_step(unsigned long rip);
+int hvm_event_software_breakpoint(unsigned long rip,
+  bool_t single_step);

Are we liable to ever gain any other type of software breakpoint?  Might
it be more sense to pass an enum rather than a bool here?

Otherwise, the changes look sensible.

~Andrew



Well, IMHO, no. Besides breakpointing/trapping after each instruction 
(i.e. VM_EVENT_REASON_SINGLESTEP)
and on arbitrary instructions (VM_EVENT_REASON_SOFTWARE_BREAKPOINT) I 
don't see what other
types of breakpointing one can define (at least from the hypervisor's 
point of view), and if in the future

there will be other types, that could also be changed into an enum then.
But making that param an enum now would also be fine by me.
Since I noticed Tamas also prefers this option, I will make that change.

Corneliu.

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

Re: [Xen-devel] [PATCH 0/7] Vm-events: move monitor vm-events code to common code.

[Corneliu ZUZU]

On 2/8/2016 6:57 PM, Corneliu ZUZU wrote:

This patch series is an attempt to move (most) of the monitor vm-events code to
the common-side.

Patches summary:

1. Move xen/arch/arm/hvm.c to xen/arch/arm/hvm/hvm.c

2. Merge almost identical functions hvm_event_int3 + hvm_event_single_step ->
hvm_event_software_breakpoint.

3. Add Kconfigs:
HAS_VM_EVENT_WRITE_CTRLREG, HAS_VM_EVENT_SINGLESTEP,
HAS_VM_EVENT_SOFTWARE_BREAKPOINT, HAS_VM_EVENT_GUEST_REQUEST
and move monitor_domctl to common-side. Used the Kconfigs to selectively
activate implemented monitor vm-events code for each architecture.

4. Some file renames. Read (*) below.

5. Move hvm_event_traps, hvm_event_cr, hvm_event_guest_request,
hvm_event_software_breakpoint functions to common-side.
(note: arch_hvm_event_fill_regs on ARM-side will be implemented in a separate
patch)

6. Some file renames, plus some minor fixes. Read (*) below.

7. Move monitor bitfield members from struct arch_domain to struct domain.
Moved bits: single-step, software-breakpoint and guest-request.
(note: write_ctrlreg_* were left on the arch-side, since control-registers
number can vary across architectures)

(*) was only necessary to avoid git seeing a file as being modified, rather than
moved (would have made the diff unnecessarily bulky).
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel



Somehow git send-email managed to malform the introductory message. 
Corrections made, read above instead :)


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

Re: [Xen-devel] [PATCH 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU]

On 2/8/2016 8:15 PM, Tamas K Lengyel wrote:
On Mon, Feb 8, 2016 at 9:57 AM, Corneliu ZUZU <cz...@bitdefender.com 
<mailto:cz...@bitdefender.com>> wrote:


+#if CONFIG_X86


Wouldn't it be safe to include these headers on ARM as well?

+#include /* for VM_EVENT_X86_CR3 */
+#include /* for hvm_update_guest_cr,
... */
+#endif



It wouldn't break anything, but they are only necessary on X86, so why 
include them?



+
+if ( unlikely(mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE &&
+  mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE) )
+{


Please make a switch on mop->op instead where the default case is to 
call arch_monitor_domctl. It would be a bit easier to follow.


+if ( XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES == mop->op )
+{
+mop->event = arch_monitor_get_capabilities(d);
+return 0;
+}
+



Noted, good point.



"proly"->probably?

+/* The monitor op is proly handled on the arch-side. */
+if ( likely(arch_monitor_domctl_op(d, mop, )) )
+return rc;



Yep, will "proly" change that, heh, just kidding. Noted :D.


Empty line not needed.  (*)


Noted.



So I don't particularly like this #if check here. IMHO this should be 
done in arch-specific function that you call from here that is just 
empty for ARM. It could be a static inline function as it's rather small.


+#if CONFIG_X86

+if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index )
+{
+struct vcpu *v;
+/* Latches new CR3 mask through CR0 code. */
+for_each_vcpu ( d, v )
+hvm_update_guest_cr(v, 0);
+}
+#endif



I agree, but I was actually hoping to approach that in a future 
ARM-patch I'm going to send after this one.
On ARM, that part of code (where hypervisor CR trapping is enabled based 
on write_ctrlreg_enabled bits)
is done in another place (on the scheduling tail). I wanted to approach 
removing that #ifdef and finding maybe
a solution to do that similarly for X86. Would it be ok to leave this 
for that discussion? It would be simpler to illustrate

what I have in mind.



Looks good otherwise.

Thanks,
Tamas


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

Re: [Xen-devel] [PATCH 4/7] Rename monitor_x86.c to monitor.c and monitor_arch.h to monitor.h

[Corneliu ZUZU]

On 2/8/2016 8:18 PM, Tamas K Lengyel wrote:



On Mon, Feb 8, 2016 at 9:57 AM, Corneliu ZUZU <cz...@bitdefender.com 
<mailto:cz...@bitdefender.com>> wrote:


(last commit before this one explains why this was necessary)

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com
<mailto:cz...@bitdefender.com>>


I assume this patch will be gone in the next iteration after using -M 
so skipping it now.


Tamas



No, originally I intended to use the -M option, I just forgot.
This is needed even w/ the -M option.
The reason is git seeing file /a.c  as being modified
when moving it to /a.c and at the same time adding
/a.c in place of the old one, *even if* the similarity
between the old /a.c and /a.c would be *100%*
and you'd use the -M option. A google-search led me to a 2009-dated page
that described this as a lack of git diff's algo that would be improved 
upon.

It seems they aren't there yet.

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

Re: [Xen-devel] [PATCH V2] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access

[Corneliu ZUZU]

On 2/3/2016 2:23 PM, Ian Campbell wrote:

On Wed, 2016-02-03 at 13:54 +0200, Corneliu ZUZU wrote:

I thought this mail was not sent properly (didn't find it any longer on
the web (?)) and I resent it just earlier.
I figured it must've been the fact that I forgot to put a "Changed since
v1" section & that I didn't include an
"--in-reply-to" option. Apparently it was actually sent correctly.
Sorry, ignore the last one (which contains a "Changed since v1" section).

OK, please check that what is currently in xen.git#staging is what you
think should be there.

Ian.


Yep, just checked, the changes are there.

Corneliu.

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

[Xen-devel] [PATCH v2] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access

[Corneliu ZUZU]

When __p2m_get_mem_access gets called, the p2m lock is already taken
by either get_page_from_gva or p2m_get_mem_access.
Possible code paths:
1)  -> get_page_from_gva
-> p2m_mem_access_check_and_get_page
-> __p2m_get_mem_access
2)  -> p2m_get_mem_access
-> __p2m_get_mem_access

In both cases if __p2m_get_mem_access subsequently gets to
call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor
hang will occur, since p2m_lookup also spin-locks on the p2m lock.

This bug-fix simply replaces the p2m_lookup call from __p2m_get_mem_access
with a call to __p2m_lookup and also adds an ASSERT to ensure that the p2m lock
is already taken upon __p2m_get_mem_access entry.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>

---
Changed since v1:
  * added p2m-lock ASSERT
---
 xen/arch/arm/p2m.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 2190908..e8e6db4 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -468,6 +468,8 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
 #undef ACCESS
 };
 
+ASSERT(spin_is_locked(>lock));
+
 /* If no setting was ever set, just return rwx. */
 if ( !p2m->mem_access_enabled )
 {
@@ -490,7 +492,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
  * No setting was found in the Radix tree. Check if the
  * entry exists in the page-tables.
  */
-paddr_t maddr = p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
+paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
 if ( INVALID_PADDR == maddr )
 return -ESRCH;
 
-- 
2.5.0


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

Re: [Xen-devel] [PATCH v2] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access

[Corneliu ZUZU]

On 2/3/2016 1:52 PM, Ian Campbell wrote:

On Wed, 2016-02-03 at 13:37 +0200, Corneliu ZUZU wrote:

I just now applied a previous v2 which was already in my queue. Was this
just an accidental resend of v2 or is there some important change and this
is really a v3?


When __p2m_get_mem_access gets called, the p2m lock is already taken
by either get_page_from_gva or p2m_get_mem_access.
Possible code paths:
1)  -> get_page_from_gva
-> p2m_mem_access_check_and_get_page
-> __p2m_get_mem_access
2)  -> p2m_get_mem_access
-> __p2m_get_mem_access

In both cases if __p2m_get_mem_access subsequently gets to
call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor
hang will occur, since p2m_lookup also spin-locks on the p2m lock.

This bug-fix simply replaces the p2m_lookup call from
__p2m_get_mem_access
with a call to __p2m_lookup and also adds an ASSERT to ensure that the
p2m lock
is already taken upon __p2m_get_mem_access entry.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>

---
Changed since v1:
   * added p2m-lock ASSERT
---
  xen/arch/arm/p2m.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 2190908..e8e6db4 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -468,6 +468,8 @@ static int __p2m_get_mem_access(struct domain *d,
gfn_t gfn,
  #undef ACCESS
  };
  
+ASSERT(spin_is_locked(>lock));

+
  /* If no setting was ever set, just return rwx. */
  if ( !p2m->mem_access_enabled )
  {
@@ -490,7 +492,7 @@ static int __p2m_get_mem_access(struct domain *d,
gfn_t gfn,
   * No setting was found in the Radix tree. Check if the
   * entry exists in the page-tables.
   */
-paddr_t maddr = p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
+paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
  if ( INVALID_PADDR == maddr )
  return -ESRCH;
  

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
No, sorry, this is just a duplicate of the 1st v2, I thought the first 
one was not sent properly (after waiting a few days and noticing

I was no longer finding it on the web).
Ignore this one. And thanks.

Corneliu.

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

Re: [Xen-devel] [PATCH V2] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access

[Corneliu ZUZU]

On 2/3/2016 1:48 PM, Ian Campbell wrote:

On Wed, 2016-01-27 at 14:24 +0200, Corneliu ZUZU wrote:

When __p2m_get_mem_access gets called, the p2m lock is already taken
by either get_page_from_gva or p2m_get_mem_access.

Possible code paths:
1)  -> get_page_from_gva
-> p2m_mem_access_check_and_get_page
-> __p2m_get_mem_access
2)  -> p2m_get_mem_access
-> __p2m_get_mem_access

In both cases if __p2m_get_mem_access subsequently gets to
call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor
hang will occur, since p2m_lookup also spin-locks on the p2m lock.

This bug-fix simply replaces the p2m_lookup call from
__p2m_get_mem_access
with a call to __p2m_lookup.

Following Ian's suggestion, we also add an ASSERT to ensure that
the p2m lock is taken upon __p2m_get_mem_access entry.

Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com>

Acked + applied, thanks.


I thought this mail was not sent properly (didn't find it any longer on 
the web (?)) and I resent it just earlier.
I figured it must've been the fact that I forgot to put a "Changed since 
v1" section & that I didn't include an

"--in-reply-to" option. Apparently it was actually sent correctly.
Sorry, ignore the last one (which contains a "Changed since v1" section).

Corneliu.

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