Re: [Xen-devel] [PATCH v2 2/5] vm_event: Implement ARM SMC events

2016-05-04 Thread Tamas K Lengyel 
On Wed, May 4, 2016 at 8:03 AM, Julien Grall  wrote:
>
>
> On 04/05/2016 14:30, Razvan Cojocaru wrote:
>>
>> On 05/04/2016 04:26 PM, Julien Grall wrote:
>>>
>>> I may misunderstand some parts of the vm event subsystem. To get/set the
>>> full set of registers, the user will have to use the
>>> DOMCTL_{set,get}vcpucontext, correct? So why does Xen expose a part of
>>> the vCPU context through the vm_event?
>>
>>
>> Because DOMCTL_{set,get}vcpucontext is expensive, and a serious
>> introspection application will receive hundreds or thousands of events
>> per second.
>>
>> Having what's most commonly needed being sent with the vm_event
>> eliminates the need for extra hypercalls and can make the difference
>> between a responsive and an unusable userspace introspection application.
>
>
> Thank you for the explanation. So in this case, we should also make x16
> (AArch64) and r12 (AArch32) available as they will be used for hypercall.
>

That's fine by me. At the moment the only registers I have definite
use for is pc and ttbr0/1, the others are up for grabs.

Tamas

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

Re: [Xen-devel] [PATCH v2 2/5] vm_event: Implement ARM SMC events

2016-05-04 Thread Julien Grall 



On 04/05/2016 14:30, Razvan Cojocaru wrote:

On 05/04/2016 04:26 PM, Julien Grall wrote:

I may misunderstand some parts of the vm event subsystem. To get/set the
full set of registers, the user will have to use the
DOMCTL_{set,get}vcpucontext, correct? So why does Xen expose a part of
the vCPU context through the vm_event?


Because DOMCTL_{set,get}vcpucontext is expensive, and a serious
introspection application will receive hundreds or thousands of events
per second.

Having what's most commonly needed being sent with the vm_event
eliminates the need for extra hypercalls and can make the difference
between a responsive and an unusable userspace introspection application.


Thank you for the explanation. So in this case, we should also make x16 
(AArch64) and r12 (AArch32) available as they will be used for hypercall.


Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v2 2/5] vm_event: Implement ARM SMC events

2016-05-04 Thread Razvan Cojocaru 
On 05/04/2016 04:26 PM, Julien Grall wrote:
> I may misunderstand some parts of the vm event subsystem. To get/set the
> full set of registers, the user will have to use the
> DOMCTL_{set,get}vcpucontext, correct? So why does Xen expose a part of
> the vCPU context through the vm_event?

Because DOMCTL_{set,get}vcpucontext is expensive, and a serious
introspection application will receive hundreds or thousands of events
per second.

Having what's most commonly needed being sent with the vm_event
eliminates the need for extra hypercalls and can make the difference
between a responsive and an unusable userspace introspection application.


Thanks,
Razvan

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

Re: [Xen-devel] [PATCH v2 2/5] vm_event: Implement ARM SMC events

2016-05-04 Thread Julien Grall 

Hi Tamas,

I have just noticed that you use an old email address for Stefano. 
Please check MAINTAINERS file for any update on email address, new 
maintainers.


On 04/05/2016 13:42, Tamas K Lengyel wrote:


 > On 03/05/2016 19:48, Tamas K Lengyel wrote:
 >>
 >>
 >>
 >> On Tue, May 3, 2016 at 5:31 AM, Julien Grall 
 >> >> wrote:
 >> On 29/04/16 19:07, Tamas K Lengyel wrote:
 >>
 >> The ARM SMC instructions are already configured to trap to Xen
 >> by default. In
 >> this patch we allow a user-space process in a privileged domain
 >> to receive
 >> notification of when such event happens through the vm_event
 >> subsystem by
 >> introducing the PRIVILEGED_CALL type.
 >>
 >> Signed-off-by: Tamas K Lengyel 
 >> >>
 >>
 >> ---
 >> Cc: Razvan Cojocaru 
 >> >>
 >> Cc: Ian Jackson 
 >> >>
 >> Cc: Stefano Stabellini 
 >> >>
 >> Cc: Wei Liu  >>
 >> Cc: Julien Grall 
 >> >>
 >> Cc: Keir Fraser 
>>
 >> Cc: Jan Beulich  >>
 >> Cc: Andrew Cooper 
 >> >>
 >>
 >>
 >> v2: introduce VM_EVENT_REASON_PRIVELEGED_CALL
 >>   aarch64 support
 >> ---
 >>MAINTAINERS |   6 +-
 >>tools/libxc/include/xenctrl.h   |   2 +
 >>tools/libxc/xc_monitor.c|  26 +++-
 >>tools/tests/xen-access/xen-access.c |  31 -
 >>xen/arch/arm/Makefile   |   2 +
 >>xen/arch/arm/monitor.c  |  80
+++
 >>xen/arch/arm/traps.c|  20 +-
 >>xen/arch/arm/vm_event.c | 127
 >> 
 >>xen/arch/x86/hvm/event.c|   2 +
 >>xen/common/vm_event.c   |   1 -
 >>xen/include/asm-arm/domain.h|   5 ++
 >>xen/include/asm-arm/monitor.h   |  20 ++
 >>xen/include/asm-arm/vm_event.h  |  16 ++---
 >>xen/include/public/domctl.h |   1 +
 >>xen/include/public/vm_event.h   |  27 
 >>15 files changed, 333 insertions(+), 33 deletions(-)
 >>create mode 100644 xen/arch/arm/monitor.c
 >>create mode 100644 xen/arch/arm/vm_event.c
 >>
 >>
 >> This patch is doing lots of things:
 >>  - Add support for monitoring
 >>  - Add support for vm_event
 >>  - Monitor SMC
 >>  - Move common code to arch specific code
 >>
 >> As far as I can tell, all are distinct from each other. Can you
 >> please split this patch in smaller ones?
 >>
 >>
 >> While I can split off some parts into separate patches, like
 >> getting/setting ARM registers through VM events and the tools patches,
 >> the other components are pretty tightly coupled and don't actually make
 >> sense to split them. For example, enabling a monitor domctl for an event
 >> without the VM event components doesn't make much sense. Vice verse for
 >> the vm_event parts without being able to enable them.
 >
 >
 > Well, the commit message does not mention half of the changes of this
patch. Some changes like moving common code to arch specific code
clearly needs explanation. It is the same for the fact that you only
present a reduce set of registers to vm event for AArch64.

This IMHO is not outstanding, it's the same on x86.


It documents the code and will help future reader to understand why we 
choose to only expose a smaller set of registers.


Note that the x86 structure is documented with: "Using a custom struct 
(no hvm_hw_cpu) so as to not fill the vm_event ring buffer too quickly". 
This is not the case for the

Re: [Xen-devel] [PATCH v2 2/5] vm_event: Implement ARM SMC events

2016-05-04 Thread Tamas K Lengyel 
> On 03/05/2016 19:48, Tamas K Lengyel wrote:
>>
>>
>>
>> On Tue, May 3, 2016 at 5:31 AM, Julien Grall > > wrote:
>> On 29/04/16 19:07, Tamas K Lengyel wrote:
>>
>> The ARM SMC instructions are already configured to trap to Xen
>> by default. In
>> this patch we allow a user-space process in a privileged domain
>> to receive
>> notification of when such event happens through the vm_event
>> subsystem by
>> introducing the PRIVILEGED_CALL type.
>>
>> Signed-off-by: Tamas K Lengyel > >
>>
>> ---
>> Cc: Razvan Cojocaru > >
>> Cc: Ian Jackson > >
>> Cc: Stefano Stabellini > >
>> Cc: Wei Liu >
>> Cc: Julien Grall > >
>> Cc: Keir Fraser >
>> Cc: Jan Beulich >
>> Cc: Andrew Cooper > >
>>
>>
>> v2: introduce VM_EVENT_REASON_PRIVELEGED_CALL
>>   aarch64 support
>> ---
>>MAINTAINERS |   6 +-
>>tools/libxc/include/xenctrl.h   |   2 +
>>tools/libxc/xc_monitor.c|  26 +++-
>>tools/tests/xen-access/xen-access.c |  31 -
>>xen/arch/arm/Makefile   |   2 +
>>xen/arch/arm/monitor.c  |  80
+++
>>xen/arch/arm/traps.c|  20 +-
>>xen/arch/arm/vm_event.c | 127
>> 
>>xen/arch/x86/hvm/event.c|   2 +
>>xen/common/vm_event.c   |   1 -
>>xen/include/asm-arm/domain.h|   5 ++
>>xen/include/asm-arm/monitor.h   |  20 ++
>>xen/include/asm-arm/vm_event.h  |  16 ++---
>>xen/include/public/domctl.h |   1 +
>>xen/include/public/vm_event.h   |  27 
>>15 files changed, 333 insertions(+), 33 deletions(-)
>>create mode 100644 xen/arch/arm/monitor.c
>>create mode 100644 xen/arch/arm/vm_event.c
>>
>>
>> This patch is doing lots of things:
>>  - Add support for monitoring
>>  - Add support for vm_event
>>  - Monitor SMC
>>  - Move common code to arch specific code
>>
>> As far as I can tell, all are distinct from each other. Can you
>> please split this patch in smaller ones?
>>
>>
>> While I can split off some parts into separate patches, like
>> getting/setting ARM registers through VM events and the tools patches,
>> the other components are pretty tightly coupled and don't actually make
>> sense to split them. For example, enabling a monitor domctl for an event
>> without the VM event components doesn't make much sense. Vice verse for
>> the vm_event parts without being able to enable them.
>
>
> Well, the commit message does not mention half of the changes of this
patch. Some changes like moving common code to arch specific code clearly
needs explanation. It is the same for the fact that you only present a
reduce set of registers to vm event for AArch64.

This IMHO is not outstanding, it's the same on x86.

>
> In any case, there is too many logical changes in this patch, which makes
difficult to review it. So please split this patch in smaller chunk.

Sure, I already split the parts I mentioned in the previous message.

>
> [...]
>
>
>> +if ( current->domain->arch.monitor.privileged_call_enabled )
>> +{
>> +rc = monitor_smc(regs);
>> +}
>>
>>
>> The bracket are not necessary.
>>
>>
>> Ack.
>>
>>
>> +
>> +if ( rc != 1 )
>>
>>
>> I think the code would be clearer if you introduce a define for "1".
>>
>>
>> Maybe not a define but calling the variable "handled" as we do on x86
>> would be more descriptive.
>
>
> IHMO, "handled" infers that the variable is boolean. This is not the case
here as you could have negative value.

It may be but thats the convention we have for this on x86 so symmetry is
better then introducing a new define just for the ARM case.

>
>
>>
>>
>> +{
>> +GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
>>
>>
>> This check cannot work for AArch64 guest.
>>
>>
>> For HSR_EC_SMC32 there is already
>> GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); and for HSR_EC_SMC64 there
>>

Re: [Xen-devel] [PATCH v2 2/5] vm_event: Implement ARM SMC events

2016-05-04 Thread Julien Grall 

Hi Tamas,

Can you configure your email client to quote properly? I.e using ">" 
rather than tabulation to show the quoting.


On 03/05/2016 19:48, Tamas K Lengyel wrote:



On Tue, May 3, 2016 at 5:31 AM, Julien Grall > wrote:
On 29/04/16 19:07, Tamas K Lengyel wrote:

The ARM SMC instructions are already configured to trap to Xen
by default. In
this patch we allow a user-space process in a privileged domain
to receive
notification of when such event happens through the vm_event
subsystem by
introducing the PRIVILEGED_CALL type.

Signed-off-by: Tamas K Lengyel >
---
Cc: Razvan Cojocaru >
Cc: Ian Jackson >
Cc: Stefano Stabellini >
Cc: Wei Liu >
Cc: Julien Grall >
Cc: Keir Fraser >
Cc: Jan Beulich >
Cc: Andrew Cooper >

v2: introduce VM_EVENT_REASON_PRIVELEGED_CALL
  aarch64 support
---
   MAINTAINERS |   6 +-
   tools/libxc/include/xenctrl.h   |   2 +
   tools/libxc/xc_monitor.c|  26 +++-
   tools/tests/xen-access/xen-access.c |  31 -
   xen/arch/arm/Makefile   |   2 +
   xen/arch/arm/monitor.c  |  80 +++
   xen/arch/arm/traps.c|  20 +-
   xen/arch/arm/vm_event.c | 127

   xen/arch/x86/hvm/event.c|   2 +
   xen/common/vm_event.c   |   1 -
   xen/include/asm-arm/domain.h|   5 ++
   xen/include/asm-arm/monitor.h   |  20 ++
   xen/include/asm-arm/vm_event.h  |  16 ++---
   xen/include/public/domctl.h |   1 +
   xen/include/public/vm_event.h   |  27 
   15 files changed, 333 insertions(+), 33 deletions(-)
   create mode 100644 xen/arch/arm/monitor.c
   create mode 100644 xen/arch/arm/vm_event.c


This patch is doing lots of things:
 - Add support for monitoring
 - Add support for vm_event
 - Monitor SMC
 - Move common code to arch specific code

As far as I can tell, all are distinct from each other. Can you
please split this patch in smaller ones?


While I can split off some parts into separate patches, like
getting/setting ARM registers through VM events and the tools patches,
the other components are pretty tightly coupled and don't actually make
sense to split them. For example, enabling a monitor domctl for an event
without the VM event components doesn't make much sense. Vice verse for
the vm_event parts without being able to enable them.


Well, the commit message does not mention half of the changes of this 
patch. Some changes like moving common code to arch specific code 
clearly needs explanation. It is the same for the fact that you only 
present a reduce set of registers to vm event for AArch64.


In any case, there is too many logical changes in this patch, which 
makes difficult to review it. So please split this patch in smaller chunk.


[...]


+if ( current->domain->arch.monitor.privileged_call_enabled )
+{
+rc = monitor_smc(regs);
+}


The bracket are not necessary.


Ack.


+
+if ( rc != 1 )


I think the code would be clearer if you introduce a define for "1".


Maybe not a define but calling the variable "handled" as we do on x86
would be more descriptive.


IHMO, "handled" infers that the variable is boolean. This is not the 
case here as you could have negative value.





+{
+GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));


This check cannot work for AArch64 guest.


For HSR_EC_SMC32 there is already
GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); and for HSR_EC_SMC64 there
is GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr)); before calling
do_trap_smc. So are you saying that check is wrong for AArch64 as it is
right now in unstable? Also, is there any reason those checks are
opposite of each other?


No, I am saying that your check is wrong here. psr_mode_is_32bit returns 
true if the guest was running in 32-bit mode, else false when running in 
64-bit mode.


The check are opposites each other because the

Re: [Xen-devel] [PATCH v2 2/5] vm_event: Implement ARM SMC events

2016-05-03 Thread Tamas K Lengyel 
On Tue, May 3, 2016 at 5:31 AM, Julien Grall  wrote:

> Hi Tamas,
>
>
> On 29/04/16 19:07, Tamas K Lengyel wrote:
>
>> The ARM SMC instructions are already configured to trap to Xen by
>> default. In
>> this patch we allow a user-space process in a privileged domain to receive
>> notification of when such event happens through the vm_event subsystem by
>> introducing the PRIVILEGED_CALL type.
>>
>> Signed-off-by: Tamas K Lengyel 
>> ---
>> Cc: Razvan Cojocaru 
>> Cc: Ian Jackson 
>> Cc: Stefano Stabellini 
>> Cc: Wei Liu 
>> Cc: Julien Grall 
>> Cc: Keir Fraser 
>> Cc: Jan Beulich 
>> Cc: Andrew Cooper 
>>
>> v2: introduce VM_EVENT_REASON_PRIVELEGED_CALL
>>  aarch64 support
>> ---
>>   MAINTAINERS |   6 +-
>>   tools/libxc/include/xenctrl.h   |   2 +
>>   tools/libxc/xc_monitor.c|  26 +++-
>>   tools/tests/xen-access/xen-access.c |  31 -
>>   xen/arch/arm/Makefile   |   2 +
>>   xen/arch/arm/monitor.c  |  80 +++
>>   xen/arch/arm/traps.c|  20 +-
>>   xen/arch/arm/vm_event.c | 127
>> 
>>   xen/arch/x86/hvm/event.c|   2 +
>>   xen/common/vm_event.c   |   1 -
>>   xen/include/asm-arm/domain.h|   5 ++
>>   xen/include/asm-arm/monitor.h   |  20 ++
>>   xen/include/asm-arm/vm_event.h  |  16 ++---
>>   xen/include/public/domctl.h |   1 +
>>   xen/include/public/vm_event.h   |  27 
>>   15 files changed, 333 insertions(+), 33 deletions(-)
>>   create mode 100644 xen/arch/arm/monitor.c
>>   create mode 100644 xen/arch/arm/vm_event.c
>>
>
> This patch is doing lots of things:
> - Add support for monitoring
> - Add support for vm_event
> - Monitor SMC
> - Move common code to arch specific code
>
> As far as I can tell, all are distinct from each other. Can you please
> split this patch in smaller ones?
>

While I can split off some parts into separate patches, like
getting/setting ARM registers through VM events and the tools patches, the
other components are pretty tightly coupled and don't actually make sense
to split them. For example, enabling a monitor domctl for an event without
the VM event components doesn't make much sense. Vice verse for the
vm_event parts without being able to enable them.


>
> [...]
>
> diff --git a/xen/arch/arm/monitor.c b/xen/arch/arm/monitor.c
>> new file mode 100644
>> index 000..e845f28
>> --- /dev/null
>> +++ b/xen/arch/arm/monitor.c
>>
>
> [...]
>
> +int monitor_smc(const struct cpu_user_regs *regs) {
>>
>
> The { should be on a separate line.


Ack.


>
>
> +struct vcpu *curr = current;
>> +vm_event_request_t req = { 0 };
>> +
>> +if ( !curr->domain->arch.monitor.privileged_call_enabled )
>> +return 0;
>> +
>> +req.reason = VM_EVENT_REASON_PRIVILEGED_CALL;
>> +req.vcpu_id = curr->vcpu_id;
>> +
>> +vm_event_fill_regs(, regs, curr->domain);
>> +
>> +return vm_event_monitor_traps(curr, 1, );
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 9abfc3c..9c8d395 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -41,6 +41,7 @@
>>   #include 
>>   #include 
>>   #include 
>> +#include 
>>
>>   #include "decode.h"
>>   #include "vtimer.h"
>> @@ -2491,6 +2492,21 @@ bad_data_abort:
>>   inject_dabt_exception(regs, info.gva, hsr.len);
>>   }
>>
>> +static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
>> +{
>> +int rc = 0;
>>
>
> Newline here.
>

Ack.


>
> +if ( current->domain->arch.monitor.privileged_call_enabled )
>> +{
>> +rc = monitor_smc(regs);
>> +}
>>
>
> The bracket are not necessary.
>

Ack.


>
> +
>> +if ( rc != 1 )
>>
>
> I think the code would be clearer if you introduce a define for "1".
>

Maybe not a define but calling the variable "handled" as we do on x86 would
be more descriptive.


>
> +{
>> +GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
>>
>
> This check cannot work for AArch64 guest.


For HSR_EC_SMC32 there is already
GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); and for HSR_EC_SMC64 there is
GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr)); before calling do_trap_smc. So
are you saying that check is wrong for AArch64 as it is right now in
unstable? Also, is there any reason those checks are opposite of each other?


>
>
> +inject_undef_exception(regs, hsr);
>> +}
>> +}
>> +
>>   static void enter_hypervisor_head(struct cpu_user_regs *regs)
>>   {
>>   if (

Re: [Xen-devel] [PATCH v2 2/5] vm_event: Implement ARM SMC events

2016-05-03 Thread Julien Grall 

Hi Tamas,

On 29/04/16 19:07, Tamas K Lengyel wrote:

The ARM SMC instructions are already configured to trap to Xen by default. In
this patch we allow a user-space process in a privileged domain to receive
notification of when such event happens through the vm_event subsystem by
introducing the PRIVILEGED_CALL type.

Signed-off-by: Tamas K Lengyel 
---
Cc: Razvan Cojocaru 
Cc: Ian Jackson 
Cc: Stefano Stabellini 
Cc: Wei Liu 
Cc: Julien Grall 
Cc: Keir Fraser 
Cc: Jan Beulich 
Cc: Andrew Cooper 

v2: introduce VM_EVENT_REASON_PRIVELEGED_CALL
 aarch64 support
---
  MAINTAINERS |   6 +-
  tools/libxc/include/xenctrl.h   |   2 +
  tools/libxc/xc_monitor.c|  26 +++-
  tools/tests/xen-access/xen-access.c |  31 -
  xen/arch/arm/Makefile   |   2 +
  xen/arch/arm/monitor.c  |  80 +++
  xen/arch/arm/traps.c|  20 +-
  xen/arch/arm/vm_event.c | 127 
  xen/arch/x86/hvm/event.c|   2 +
  xen/common/vm_event.c   |   1 -
  xen/include/asm-arm/domain.h|   5 ++
  xen/include/asm-arm/monitor.h   |  20 ++
  xen/include/asm-arm/vm_event.h  |  16 ++---
  xen/include/public/domctl.h |   1 +
  xen/include/public/vm_event.h   |  27 
  15 files changed, 333 insertions(+), 33 deletions(-)
  create mode 100644 xen/arch/arm/monitor.c
  create mode 100644 xen/arch/arm/vm_event.c


This patch is doing lots of things:
- Add support for monitoring
- Add support for vm_event
- Monitor SMC
- Move common code to arch specific code

As far as I can tell, all are distinct from each other. Can you please 
split this patch in smaller ones?


[...]


diff --git a/xen/arch/arm/monitor.c b/xen/arch/arm/monitor.c
new file mode 100644
index 000..e845f28
--- /dev/null
+++ b/xen/arch/arm/monitor.c


[...]


+int monitor_smc(const struct cpu_user_regs *regs) {


The { should be on a separate line.


+struct vcpu *curr = current;
+vm_event_request_t req = { 0 };
+
+if ( !curr->domain->arch.monitor.privileged_call_enabled )
+return 0;
+
+req.reason = VM_EVENT_REASON_PRIVILEGED_CALL;
+req.vcpu_id = curr->vcpu_id;
+
+vm_event_fill_regs(, regs, curr->domain);
+
+return vm_event_monitor_traps(curr, 1, );
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 9abfc3c..9c8d395 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -41,6 +41,7 @@
  #include 
  #include 
  #include 
+#include 

  #include "decode.h"
  #include "vtimer.h"
@@ -2491,6 +2492,21 @@ bad_data_abort:
  inject_dabt_exception(regs, info.gva, hsr.len);
  }

+static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
+{
+int rc = 0;


Newline here.


+if ( current->domain->arch.monitor.privileged_call_enabled )
+{
+rc = monitor_smc(regs);
+}


The bracket are not necessary.


+
+if ( rc != 1 )


I think the code would be clearer if you introduce a define for "1".


+{
+GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));


This check cannot work for AArch64 guest.


+inject_undef_exception(regs, hsr);
+}
+}
+
  static void enter_hypervisor_head(struct cpu_user_regs *regs)
  {
  if ( guest_mode(regs) )
@@ -2566,7 +2582,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs 
*regs)
   */
  GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
  perfc_incr(trap_smc32);
-inject_undef32_exception(regs);
+do_trap_smc(regs, hsr);
  break;
  case HSR_EC_HVC32:
  GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
@@ -2599,7 +2615,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs 
*regs)
   */
  GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
  perfc_incr(trap_smc64);
-inject_undef64_exception(regs, hsr.len);
+do_trap_smc(regs, hsr);
  break;
  case HSR_EC_SYSREG:
  GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
diff --git a/xen/arch/arm/vm_event.c b/xen/arch/arm/vm_event.c
new file mode 100644
index 000..3369a96
--- /dev/null
+++ b/xen/arch/arm/vm_event.c
@@ -0,0 +1,127 @@
+/*
+ * arch/arm/vm_event.c
+ *
+ * Architecture-specific vm_event handling routines
+ *
+ * Copyright (c) 2016 Tamas K Lengyel (ta...@tklengyel.com)
+ *
+ * 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

Re: [Xen-devel] [PATCH v2 2/5] vm_event: Implement ARM SMC events

2016-05-02 Thread Wei Liu 
On Fri, Apr 29, 2016 at 12:07:30PM -0600, Tamas K Lengyel wrote:
>  tools/libxc/include/xenctrl.h   |   2 +
>  tools/libxc/xc_monitor.c|  26 +++-

Acked-by: Wei Liu 

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

Re: [Xen-devel] [PATCH v2 2/5] vm_event: Implement ARM SMC events

2016-04-29 Thread Razvan Cojocaru 
On 04/29/16 23:27, Tamas K Lengyel wrote:
> 
>  
> 
> 
> > diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> > index 2906407..a29bda8 100644
> > --- a/xen/common/vm_event.c
> > +++ b/xen/common/vm_event.c
> > @@ -818,7 +818,6 @@ int vm_event_monitor_traps(struct vcpu *v, 
> uint8_t sync,
> >  req->altp2m_idx = altp2m_vcpu_idx(v);
> >  }
> >
> > -vm_event_fill_regs(req);
> >  vm_event_put_request(d, >vm_event->monitor, req);
> >
> >  return 1;
> 
> So now for x86 we only vm_fill_regs() for CR writes and
> breakpoints (and
> EPT faults, but that's in p2m.c which hasn't been touched by this
> patch)? That's a pretty big change, and one that's not explained
> in the
> patch description (which makes no mention of any x86 changes).
> 
> Having that call in vm_event_monitor_traps() made sure that all
> vm_events get a copy of the respective registers. In the x86
> case, that
> includes the guest request and MSR write events, which now no longer
> seem to carry that information, unless I'm missing something.
> 
> That behaviour should not change for x86 events, please.
> 
> 
> Yeap, good catch. It needs to be moved from the common path because
> the inputs to the function will differ on ARM and x86. I'll
> double-check that the x86 paths will remain functionally the same.
> 
> 
> So for mem_access nothing changes in this patch, fill_regs was already
> called from p2m.c. For MSR's I just missed adding the extra call. As for
> vm_event_monitor_guest_request, it will needs to be moved to be
> arch-specific. I think I'll do it as a precursor patch where I move it
> to be in the arch-specific monitor code (where it should be actually).
> Will do these fixes in the next round.

Fair enough, thanks!


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

Re: [Xen-devel] [PATCH v2 2/5] vm_event: Implement ARM SMC events

2016-04-29 Thread Tamas K Lengyel 
>
>
>
>
>>
>> > diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
>> > index 2906407..a29bda8 100644
>> > --- a/xen/common/vm_event.c
>> > +++ b/xen/common/vm_event.c
>> > @@ -818,7 +818,6 @@ int vm_event_monitor_traps(struct vcpu *v, uint8_t
>> sync,
>> >  req->altp2m_idx = altp2m_vcpu_idx(v);
>> >  }
>> >
>> > -vm_event_fill_regs(req);
>> >  vm_event_put_request(d, >vm_event->monitor, req);
>> >
>> >  return 1;
>>
>> So now for x86 we only vm_fill_regs() for CR writes and breakpoints (and
>> EPT faults, but that's in p2m.c which hasn't been touched by this
>> patch)? That's a pretty big change, and one that's not explained in the
>> patch description (which makes no mention of any x86 changes).
>>
>> Having that call in vm_event_monitor_traps() made sure that all
>> vm_events get a copy of the respective registers. In the x86 case, that
>> includes the guest request and MSR write events, which now no longer
>> seem to carry that information, unless I'm missing something.
>>
>> That behaviour should not change for x86 events, please.
>>
>
> Yeap, good catch. It needs to be moved from the common path because the
> inputs to the function will differ on ARM and x86. I'll double-check that
> the x86 paths will remain functionally the same.
>

So for mem_access nothing changes in this patch, fill_regs was already
called from p2m.c. For MSR's I just missed adding the extra call. As for
vm_event_monitor_guest_request, it will needs to be moved to be
arch-specific. I think I'll do it as a precursor patch where I move it to
be in the arch-specific monitor code (where it should be actually). Will do
these fixes in the next round.

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

Re: [Xen-devel] [PATCH v2 2/5] vm_event: Implement ARM SMC events

2016-04-29 Thread Tamas K Lengyel 
> @@ -2491,6 +2492,21 @@ bad_data_abort:
> >  inject_dabt_exception(regs, info.gva, hsr.len);
> >  }
> >
> > +static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
> > +{
> > +int rc = 0;
> > +if ( current->domain->arch.monitor.privileged_call_enabled )
> > +{
> > +rc = monitor_smc(regs);
> > +}
>
>
> If you need to increment the patch version, maybe remove the curly
> braces here?
>

Sure.


>
> > diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> > index 2906407..a29bda8 100644
> > --- a/xen/common/vm_event.c
> > +++ b/xen/common/vm_event.c
> > @@ -818,7 +818,6 @@ int vm_event_monitor_traps(struct vcpu *v, uint8_t
> sync,
> >  req->altp2m_idx = altp2m_vcpu_idx(v);
> >  }
> >
> > -vm_event_fill_regs(req);
> >  vm_event_put_request(d, >vm_event->monitor, req);
> >
> >  return 1;
>
> So now for x86 we only vm_fill_regs() for CR writes and breakpoints (and
> EPT faults, but that's in p2m.c which hasn't been touched by this
> patch)? That's a pretty big change, and one that's not explained in the
> patch description (which makes no mention of any x86 changes).
>
> Having that call in vm_event_monitor_traps() made sure that all
> vm_events get a copy of the respective registers. In the x86 case, that
> includes the guest request and MSR write events, which now no longer
> seem to carry that information, unless I'm missing something.
>
> That behaviour should not change for x86 events, please.
>

Yeap, good catch. It needs to be moved from the common path because the
inputs to the function will differ on ARM and x86. I'll double-check that
the x86 paths will remain functionally the same.

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

Re: [Xen-devel] [PATCH v2 2/5] vm_event: Implement ARM SMC events

2016-04-29 Thread Razvan Cojocaru 
On 04/29/16 21:07, Tamas K Lengyel wrote:
> The ARM SMC instructions are already configured to trap to Xen by default. In
> this patch we allow a user-space process in a privileged domain to receive
> notification of when such event happens through the vm_event subsystem by
> introducing the PRIVILEGED_CALL type.
> 
> Signed-off-by: Tamas K Lengyel 
> ---
> Cc: Razvan Cojocaru 
> Cc: Ian Jackson 
> Cc: Stefano Stabellini 
> Cc: Wei Liu 
> Cc: Julien Grall 
> Cc: Keir Fraser 
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> 
> v2: introduce VM_EVENT_REASON_PRIVELEGED_CALL
> aarch64 support
> ---
>  MAINTAINERS |   6 +-
>  tools/libxc/include/xenctrl.h   |   2 +
>  tools/libxc/xc_monitor.c|  26 +++-
>  tools/tests/xen-access/xen-access.c |  31 -
>  xen/arch/arm/Makefile   |   2 +
>  xen/arch/arm/monitor.c  |  80 +++
>  xen/arch/arm/traps.c|  20 +-
>  xen/arch/arm/vm_event.c | 127 
> 
>  xen/arch/x86/hvm/event.c|   2 +
>  xen/common/vm_event.c   |   1 -
>  xen/include/asm-arm/domain.h|   5 ++
>  xen/include/asm-arm/monitor.h   |  20 ++
>  xen/include/asm-arm/vm_event.h  |  16 ++---
>  xen/include/public/domctl.h |   1 +
>  xen/include/public/vm_event.h   |  27 
>  15 files changed, 333 insertions(+), 33 deletions(-)
>  create mode 100644 xen/arch/arm/monitor.c
>  create mode 100644 xen/arch/arm/vm_event.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5af7a0c..36d8591 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -355,12 +355,10 @@ VM EVENT AND MEM ACCESS
>  M:   Razvan Cojocaru 
>  M:   Tamas K Lengyel 
>  S:   Supported
> -F:   xen/common/vm_event.c
> +F:   xen/*/vm_event.c
> +F:   xen/*/monitor.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
>  F:   tools/tests/xen-access
>  
>  VTPM

This patch touches MAINTANERS, but so does the last patch in the series
(which does nothing else but touch MAINTAINERS). I wouldn't block this
patch on this account, but would it be problematic for all changes to
MAINTAINERS to occur in that patch?

> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 42f201b..4b75ae4 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2160,6 +2160,8 @@ int xc_monitor_software_breakpoint(xc_interface *xch, 
> domid_t domain_id,
> bool enable);
>  int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
>   bool enable, bool sync);
> +int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
> +   bool enable);
>  
>  /**
>   * This function enables / disables emulation for each REP for a
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index b1705dd..072df70 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -4,7 +4,7 @@
>   *
>   * Interface to VM event monitor
>   *
> - * Copyright (c) 2015 Tamas K Lengyel (ta...@tklengyel.com)
> + * Copyright (c) 2015-2016 Tamas K Lengyel (ta...@tklengyel.com)
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -156,3 +156,27 @@ int xc_monitor_emulate_each_rep(xc_interface *xch, 
> domid_t domain_id,
>  
>  return do_domctl(xch, );
>  }
> +
> +int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
> +   bool enable)
> +{
> +DECLARE_DOMCTL;
> +
> +domctl.cmd = XEN_DOMCTL_monitor_op;
> +domctl.domain = domain_id;
> +domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
> +: XEN_DOMCTL_MONITOR_OP_DISABLE;
> +domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL;
> +
> +return do_domctl(xch, );
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/tests/xen-access/xen-access.c 
> b/tools/tests/xen-access/xen-access.c
> index f26e723..33e8044 100644
> --- a/tools/tests/xen-access/xen-access.c
> +++ b/tools/tests/xen-access/xen-access.c
> @@ -334,6 +334,8 @@ void usage(char* progname)
>  fprintf(stderr, "Usage: %s [-m]  write|exec", progname);
>  #if defined(__i386__) || defined(__x86_64__)
>  fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec");
> +#elif defined(__arm__) ||

[Xen-devel] [PATCH v2 2/5] vm_event: Implement ARM SMC events

2016-04-29 Thread Tamas K Lengyel 
The ARM SMC instructions are already configured to trap to Xen by default. In
this patch we allow a user-space process in a privileged domain to receive
notification of when such event happens through the vm_event subsystem by
introducing the PRIVILEGED_CALL type.

Signed-off-by: Tamas K Lengyel 
---
Cc: Razvan Cojocaru 
Cc: Ian Jackson 
Cc: Stefano Stabellini 
Cc: Wei Liu 
Cc: Julien Grall 
Cc: Keir Fraser 
Cc: Jan Beulich 
Cc: Andrew Cooper 

v2: introduce VM_EVENT_REASON_PRIVELEGED_CALL
aarch64 support
---
 MAINTAINERS |   6 +-
 tools/libxc/include/xenctrl.h   |   2 +
 tools/libxc/xc_monitor.c|  26 +++-
 tools/tests/xen-access/xen-access.c |  31 -
 xen/arch/arm/Makefile   |   2 +
 xen/arch/arm/monitor.c  |  80 +++
 xen/arch/arm/traps.c|  20 +-
 xen/arch/arm/vm_event.c | 127 
 xen/arch/x86/hvm/event.c|   2 +
 xen/common/vm_event.c   |   1 -
 xen/include/asm-arm/domain.h|   5 ++
 xen/include/asm-arm/monitor.h   |  20 ++
 xen/include/asm-arm/vm_event.h  |  16 ++---
 xen/include/public/domctl.h |   1 +
 xen/include/public/vm_event.h   |  27 
 15 files changed, 333 insertions(+), 33 deletions(-)
 create mode 100644 xen/arch/arm/monitor.c
 create mode 100644 xen/arch/arm/vm_event.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5af7a0c..36d8591 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -355,12 +355,10 @@ VM EVENT AND MEM ACCESS
 M: Razvan Cojocaru 
 M: Tamas K Lengyel 
 S: Supported
-F: xen/common/vm_event.c
+F: xen/*/vm_event.c
+F: xen/*/monitor.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
 F: tools/tests/xen-access
 
 VTPM
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 42f201b..4b75ae4 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2160,6 +2160,8 @@ int xc_monitor_software_breakpoint(xc_interface *xch, 
domid_t domain_id,
bool enable);
 int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
  bool enable, bool sync);
+int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
+   bool enable);
 
 /**
  * This function enables / disables emulation for each REP for a
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index b1705dd..072df70 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -4,7 +4,7 @@
  *
  * Interface to VM event monitor
  *
- * Copyright (c) 2015 Tamas K Lengyel (ta...@tklengyel.com)
+ * Copyright (c) 2015-2016 Tamas K Lengyel (ta...@tklengyel.com)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -156,3 +156,27 @@ int xc_monitor_emulate_each_rep(xc_interface *xch, domid_t 
domain_id,
 
 return do_domctl(xch, );
 }
+
+int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
+   bool enable)
+{
+DECLARE_DOMCTL;
+
+domctl.cmd = XEN_DOMCTL_monitor_op;
+domctl.domain = domain_id;
+domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
+: XEN_DOMCTL_MONITOR_OP_DISABLE;
+domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL;
+
+return do_domctl(xch, );
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/tests/xen-access/xen-access.c 
b/tools/tests/xen-access/xen-access.c
index f26e723..33e8044 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -334,6 +334,8 @@ void usage(char* progname)
 fprintf(stderr, "Usage: %s [-m]  write|exec", progname);
 #if defined(__i386__) || defined(__x86_64__)
 fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec");
+#elif defined(__arm__) || defined(__aarch64__)
+fprintf(stderr, "|privcall");
 #endif
 fprintf(stderr,
 "\n"
@@ -357,6 +359,7 @@ int main(int argc, char *argv[])
 int required = 0;
 int breakpoint = 0;
 int shutting_down = 0;
+int privcall = 0;
 int altp2m = 0;
 uint16_t altp2m_view_id = 0;
 
@@ -412,6 +415,11 @@ int main(int argc, char *argv[])
 default_access = XENMEM_access_rw;
 altp2m = 1;
 }
+#elif defined(__arm__) || defined(__aarch64__)
+else if (