* [PATCH v2 1/5] monitor: Rename vm_event_monitor_get_capabilities
@ 2016-04-29 18:07 Tamas K Lengyel
2016-04-29 18:07 ` [PATCH v2 2/5] vm_event: Implement ARM SMC events Tamas K Lengyel
` (3 more replies)
0 siblings, 4 replies; 28+ messages in thread
From: Tamas K Lengyel @ 2016-04-29 18:07 UTC (permalink / raw)
To: xen-devel
Cc: Tamas K Lengyel, Keir Fraser, Razvan Cojocaru, Andrew Cooper,
Julien Grall, Stefano Stabellini, Jan Beulich
The monitor_get_capabilities check actually belongs to the monitor subsystem so
relocating and renaming it to sanitize the code's name and location. Mechanical
patch, no code changes introduced.
Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
---
xen/arch/x86/monitor.c | 2 +-
xen/common/monitor.c | 5 ++---
xen/include/asm-arm/monitor.h | 11 ++++++++++-
xen/include/asm-arm/vm_event.h | 9 ---------
xen/include/asm-x86/monitor.h | 23 +++++++++++++++++++++++
xen/include/asm-x86/vm_event.h | 23 -----------------------
6 files changed, 36 insertions(+), 37 deletions(-)
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 1fec412..621f91a 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -126,7 +126,7 @@ int arch_monitor_domctl_event(struct domain *d,
default:
/*
- * Should not be reached unless vm_event_monitor_get_capabilities() is
+ * Should not be reached unless arch_monitor_get_capabilities() is
* not properly implemented.
*/
ASSERT_UNREACHABLE();
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index d950a7c..7c3d1c8 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -24,7 +24,6 @@
#include <xsm/xsm.h>
#include <public/domctl.h>
#include <asm/monitor.h>
-#include <asm/vm_event.h>
int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
{
@@ -48,12 +47,12 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
if ( unlikely(mop->event > 31) )
return -EINVAL;
/* Check if event type is available. */
- if ( unlikely(!(vm_event_monitor_get_capabilities(d) & (1U << mop->event))) )
+ if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) )
return -EOPNOTSUPP;
break;
case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
- mop->event = vm_event_monitor_get_capabilities(d);
+ mop->event = arch_monitor_get_capabilities(d);
return 0;
default:
diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h
index 6e36e99..3fd3c9d 100644
--- a/xen/include/asm-arm/monitor.h
+++ b/xen/include/asm-arm/monitor.h
@@ -39,11 +39,20 @@ int arch_monitor_domctl_event(struct domain *d,
/*
* No arch-specific monitor vm-events on ARM.
*
- * Should not be reached unless vm_event_monitor_get_capabilities() is not
+ * Should not be reached unless arch_monitor_get_capabilities() is not
* properly implemented.
*/
ASSERT_UNREACHABLE();
return -EOPNOTSUPP;
}
+static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
+{
+ uint32_t capabilities = 0;
+
+ capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
+
+ return capabilities;
+}
+
#endif /* __ASM_ARM_MONITOR_H__ */
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index 014d9ba..a3fc4ce 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -59,13 +59,4 @@ static inline void vm_event_fill_regs(vm_event_request_t *req)
/* Not supported on ARM. */
}
-static inline uint32_t vm_event_monitor_get_capabilities(struct domain *d)
-{
- uint32_t capabilities = 0;
-
- capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
-
- return capabilities;
-}
-
#endif /* __ASM_ARM_VM_EVENT_H__ */
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 0954b59..446b2af 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -50,4 +50,27 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
int arch_monitor_domctl_event(struct domain *d,
struct xen_domctl_monitor_op *mop);
+static inline uint32_t arch_monitor_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 = (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 |= (1U << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
+
+ return capabilities;
+}
+
#endif /* __ASM_X86_MONITOR_H__ */
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 0470240..026f42e 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -44,27 +44,4 @@ void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp);
void vm_event_fill_regs(vm_event_request_t *req);
-static inline uint32_t vm_event_monitor_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 = (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 |= (1U << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
-
- return capabilities;
-}
-
#endif /* __ASM_X86_VM_EVENT_H__ */
--
2.8.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 2/5] vm_event: Implement ARM SMC events
2016-04-29 18:07 [PATCH v2 1/5] monitor: Rename vm_event_monitor_get_capabilities Tamas K Lengyel
@ 2016-04-29 18:07 ` Tamas K Lengyel
2016-04-29 20:07 ` Razvan Cojocaru
` (2 more replies)
2016-04-29 18:07 ` [PATCH v2 3/5] x86/hvm: Rename hvm/event to hvm/monitor Tamas K Lengyel
` (2 subsequent siblings)
3 siblings, 3 replies; 28+ messages in thread
From: Tamas K Lengyel @ 2016-04-29 18:07 UTC (permalink / raw)
To: xen-devel
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, Stefano Stabellini,
Andrew Cooper, Ian Jackson, Julien Grall, Jan Beulich,
Keir Fraser
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 <tamas@tklengyel.com>
---
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
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 <rcojocaru@bitdefender.com>
M: Tamas K Lengyel <tamas@tklengyel.com>
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 (tamas@tklengyel.com)
+ * Copyright (c) 2015-2016 Tamas K Lengyel (tamas@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, &domctl);
}
+
+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, &domctl);
+}
+
+/*
+ * 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] <domain_id> 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 ( !strcmp(argv[0], "privcall") )
+ {
+ privcall = 1;
+ }
#endif
else
{
@@ -524,6 +532,16 @@ int main(int argc, char *argv[])
}
}
+ if ( privcall )
+ {
+ rc = xc_monitor_privileged_call(xch, domain_id, 1);
+ if ( rc < 0 )
+ {
+ ERROR("Error %d setting privileged call trapping with vm_event\n", rc);
+ goto exit;
+ }
+ }
+
/* Wait for access */
for (;;)
{
@@ -535,6 +553,9 @@ int main(int argc, char *argv[])
if ( breakpoint )
rc = xc_monitor_software_breakpoint(xch, domain_id, 0);
+ if ( privcall )
+ rc = xc_monitor_privileged_call(xch, domain_id, 0);
+
if ( altp2m )
{
rc = xc_altp2m_switch_to_view( xch, domain_id, 0 );
@@ -635,7 +656,7 @@ int main(int argc, char *argv[])
rsp.u.mem_access = req.u.mem_access;
break;
case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
- printf("Breakpoint: rip=%016"PRIx64", gfn=%"PRIx64" (vcpu %d)\n",
+ printf("Breakpoint: rip=%"PRIx64" gfn=%"PRIx64" (vcpu %d)\n",
req.data.regs.x86.rip,
req.u.software_breakpoint.gfn,
req.vcpu_id);
@@ -650,7 +671,15 @@ int main(int argc, char *argv[])
interrupted = -1;
continue;
}
+ break;
+ case VM_EVENT_REASON_PRIVILEGED_CALL:
+ printf("Privileged call: pc=%"PRIx64" (vcpu %d)\n",
+ req.data.regs.arm.pc,
+ req.vcpu_id);
+ rsp.data.regs.arm = req.data.regs.arm;
+ rsp.data.regs.arm.pc += 4;
+ rsp.flags |= VM_EVENT_FLAG_SET_REGISTERS;
break;
case VM_EVENT_REASON_SINGLESTEP:
printf("Singlestep: rip=%016"PRIx64", vcpu %d, altp2m %u\n",
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 0328b50..118be99 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -40,6 +40,8 @@ obj-y += device.o
obj-y += decode.o
obj-y += processor.o
obj-y += smc.o
+obj-y += monitor.o
+obj-y += vm_event.o
#obj-bin-y += ....o
diff --git a/xen/arch/arm/monitor.c b/xen/arch/arm/monitor.c
new file mode 100644
index 0000000..e845f28
--- /dev/null
+++ b/xen/arch/arm/monitor.c
@@ -0,0 +1,80 @@
+/*
+ * arch/arm/monitor.c
+ *
+ * Arch-specific monitor_op domctl handler.
+ *
+ * Copyright (c) 2015-2016 Tamas K Lengyel (tamas@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 <asm/vm_event.h>
+#include <public/vm_event.h>
+
+int arch_monitor_domctl_event(struct domain *d,
+ struct xen_domctl_monitor_op *mop)
+{
+ struct arch_domain *ad = &d->arch;
+ bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
+
+ switch ( mop->event )
+ {
+ case XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL:
+ {
+ bool_t old_status = ad->monitor.privileged_call_enabled;
+
+ if ( unlikely(old_status == requested_status) )
+ return -EEXIST;
+
+ domain_pause(d);
+ ad->monitor.privileged_call_enabled = requested_status;
+ domain_unpause(d);
+ break;
+ }
+
+ default:
+ /*
+ * Should not be reached unless arch_monitor_get_capabilities() is
+ * not properly implemented.
+ */
+ ASSERT_UNREACHABLE();
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+int monitor_smc(const struct cpu_user_regs *regs) {
+ 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(&req, regs, curr->domain);
+
+ return vm_event_monitor_traps(curr, 1, &req);
+}
+
+/*
+ * 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 <asm/mmio.h>
#include <asm/cpufeature.h>
#include <asm/flushtlb.h>
+#include <asm/monitor.h>
#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;
+ if ( current->domain->arch.monitor.privileged_call_enabled )
+ {
+ rc = monitor_smc(regs);
+ }
+
+ if ( rc != 1 )
+ {
+ GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+ 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 0000000..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 (tamas@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 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 <xen/sched.h>
+#include <asm/vm_event.h>
+
+void vm_event_fill_regs(vm_event_request_t *req,
+ const struct cpu_user_regs *regs,
+ struct domain *d)
+{
+ if ( is_32bit_domain(d) )
+ {
+ req->data.regs.arm.x0 = regs->r0;
+ req->data.regs.arm.x1 = regs->r1;
+ req->data.regs.arm.x2 = regs->r2;
+ req->data.regs.arm.x3 = regs->r3;
+ req->data.regs.arm.x4 = regs->r4;
+ req->data.regs.arm.x5 = regs->r5;
+ req->data.regs.arm.x6 = regs->r6;
+ req->data.regs.arm.x7 = regs->r7;
+ req->data.regs.arm.x8 = regs->r8;
+ req->data.regs.arm.x9 = regs->r9;
+ req->data.regs.arm.x10 = regs->r10;
+ req->data.regs.arm.pc = regs->pc32;
+ req->data.regs.arm.sp_el0 = regs->sp_usr;
+ req->data.regs.arm.sp_el1 = regs->sp_svc;
+ }
+#ifdef CONFIG_ARM_64
+ else
+ {
+ req->data.regs.arm.x0 = regs->x0;
+ req->data.regs.arm.x1 = regs->x1;
+ req->data.regs.arm.x2 = regs->x2;
+ req->data.regs.arm.x3 = regs->x3;
+ req->data.regs.arm.x4 = regs->x4;
+ req->data.regs.arm.x5 = regs->x5;
+ req->data.regs.arm.x6 = regs->x6;
+ req->data.regs.arm.x7 = regs->x7;
+ req->data.regs.arm.x8 = regs->x8;
+ req->data.regs.arm.x9 = regs->x9;
+ req->data.regs.arm.x10 = regs->x10;
+ req->data.regs.arm.pc = regs->pc;
+ req->data.regs.arm.sp_el0 = regs->sp_el0;
+ req->data.regs.arm.sp_el1 = regs->sp_el1;
+ }
+#endif
+ req->data.regs.arm.fp = regs->fp;
+ req->data.regs.arm.lr = regs->lr;
+ req->data.regs.arm.cpsr = regs->cpsr;
+ req->data.regs.arm.spsr_el1 = regs->spsr_svc;
+ req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
+ req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
+}
+
+void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
+{
+ struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs;
+
+ if ( is_32bit_domain(v->domain) )
+ {
+ regs->r0 = rsp->data.regs.arm.x0;
+ regs->r1 = rsp->data.regs.arm.x1;
+ regs->r2 = rsp->data.regs.arm.x2;
+ regs->r3 = rsp->data.regs.arm.x3;
+ regs->r4 = rsp->data.regs.arm.x4;
+ regs->r5 = rsp->data.regs.arm.x5;
+ regs->r6 = rsp->data.regs.arm.x6;
+ regs->r7 = rsp->data.regs.arm.x7;
+ regs->r8 = rsp->data.regs.arm.x8;
+ regs->r9 = rsp->data.regs.arm.x9;
+ regs->r10 = rsp->data.regs.arm.x10;
+ regs->pc32 = rsp->data.regs.arm.pc;
+ regs->sp_usr = rsp->data.regs.arm.sp_el0;
+ regs->sp_svc = rsp->data.regs.arm.sp_el1;
+ }
+#ifdef CONFIG_ARM_64
+ else
+ {
+ regs->x0 = rsp->data.regs.arm.x0;
+ regs->x1 = rsp->data.regs.arm.x1;
+ regs->x2 = rsp->data.regs.arm.x2;
+ regs->x3 = rsp->data.regs.arm.x3;
+ regs->x4 = rsp->data.regs.arm.x4;
+ regs->x5 = rsp->data.regs.arm.x5;
+ regs->x6 = rsp->data.regs.arm.x6;
+ regs->x7 = rsp->data.regs.arm.x7;
+ regs->x8 = rsp->data.regs.arm.x8;
+ regs->x9 = rsp->data.regs.arm.x9;
+ regs->x10 = rsp->data.regs.arm.x10;
+ regs->pc = rsp->data.regs.arm.pc;
+ regs->sp_el0 = rsp->data.regs.arm.sp_el0;
+ regs->sp_el1 = rsp->data.regs.arm.sp_el1;
+ }
+#endif
+
+ regs->fp = rsp->data.regs.arm.fp;
+ regs->lr = rsp->data.regs.arm.lr;
+ regs->cpsr = rsp->data.regs.arm.cpsr;
+ v->arch.ttbr0 = rsp->data.regs.arm.ttbr0;
+ v->arch.ttbr1 = rsp->data.regs.arm.ttbr1;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index 56c5514..f7d1418 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -47,6 +47,7 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
.u.write_ctrlreg.old_value = old
};
+ vm_event_fill_regs(&req);
vm_event_monitor_traps(curr, sync, &req);
return 1;
}
@@ -115,6 +116,7 @@ int hvm_event_breakpoint(unsigned long rip,
}
req.vcpu_id = curr->vcpu_id;
+ vm_event_fill_regs(&req);
return vm_event_monitor_traps(curr, 1, &req);
}
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, &d->vm_event->monitor, req);
return 1;
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index c35ed40..87c7d7d 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -129,6 +129,11 @@ struct arch_domain
paddr_t efi_acpi_gpa;
paddr_t efi_acpi_len;
#endif
+
+ /* Monitor options */
+ struct {
+ uint8_t privileged_call_enabled : 1;
+ } monitor;
} __cacheline_aligned;
struct arch_vcpu
diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h
index 3fd3c9d..114237a 100644
--- a/xen/include/asm-arm/monitor.h
+++ b/xen/include/asm-arm/monitor.h
@@ -3,7 +3,7 @@
*
* Arch-specific monitor_op domctl handler.
*
- * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2015-2016 Tamas K Lengyel (tamas@tklengyel.com)
* Copyright (c) 2016, Bitdefender S.R.L.
*
* This program is free software; you can redistribute it and/or
@@ -32,27 +32,19 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
return -EOPNOTSUPP;
}
-static inline
int arch_monitor_domctl_event(struct domain *d,
- struct xen_domctl_monitor_op *mop)
-{
- /*
- * No arch-specific monitor vm-events on ARM.
- *
- * Should not be reached unless arch_monitor_get_capabilities() is not
- * properly implemented.
- */
- ASSERT_UNREACHABLE();
- return -EOPNOTSUPP;
-}
+ struct xen_domctl_monitor_op *mop);
static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
{
uint32_t capabilities = 0;
- capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
+ capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
+ (1U << XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL);
return capabilities;
}
+int monitor_smc(const struct cpu_user_regs *regs);
+
#endif /* __ASM_ARM_MONITOR_H__ */
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index a3fc4ce..432a790 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -1,7 +1,7 @@
/*
* vm_event.h: architecture specific vm_event handling routines
*
- * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2015-2016 Tamas K Lengyel (tamas@tklengyel.com)
*
* This program is free software; you can redistribute it and/or modify it
* under the terms and conditions of the GNU General Public License,
@@ -19,6 +19,7 @@
#ifndef __ASM_ARM_VM_EVENT_H__
#define __ASM_ARM_VM_EVENT_H__
+#include <xen/stdbool.h>
#include <xen/sched.h>
#include <xen/vm_event.h>
#include <public/domctl.h>
@@ -48,15 +49,10 @@ void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
/* Not supported on ARM. */
}
-static inline
-void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
-{
- /* Not supported on ARM. */
-}
+void vm_event_fill_regs(vm_event_request_t *req,
+ const struct cpu_user_regs *regs,
+ struct domain *d);
-static inline void vm_event_fill_regs(vm_event_request_t *req)
-{
- /* Not supported on ARM. */
-}
+void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp);
#endif /* __ASM_ARM_VM_EVENT_H__ */
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 2457698..35adce2 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1080,6 +1080,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
#define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP 2
#define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT 3
#define XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST 4
+#define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL 5
struct xen_domctl_monitor_op {
uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 9270d52..f039207 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -119,6 +119,8 @@
#define VM_EVENT_REASON_SINGLESTEP 7
/* An event has been requested via HVMOP_guest_request_vm_event. */
#define VM_EVENT_REASON_GUEST_REQUEST 8
+/* Privileged call executed (e.g. SMC) */
+#define VM_EVENT_REASON_PRIVILEGED_CALL 9
/* Supported values for the vm_event_write_ctrlreg index. */
#define VM_EVENT_X86_CR0 0
@@ -166,6 +168,30 @@ struct vm_event_regs_x86 {
uint32_t _pad;
};
+struct vm_event_regs_arm {
+ /* Aarch64 Aarch32 */
+ uint64_t x0; /* r0_usr */
+ uint64_t x1; /* r1_usr */
+ uint64_t x2; /* r2_usr */
+ uint64_t x3; /* r3_usr */
+ uint64_t x4; /* r4_usr */
+ uint64_t x5; /* r5_usr */
+ uint64_t x6; /* r6_usr */
+ uint64_t x7; /* r7_usr */
+ uint64_t x8; /* r8_usr */
+ uint64_t x9; /* r9_usr */
+ uint64_t x10; /* r10_usr */
+ uint64_t lr; /* lr_usr */
+ uint64_t sp_el0; /* sp_usr */
+ uint64_t sp_el1; /* sp_svc */
+ uint32_t spsr_el1; /* spsr_svc */
+ uint64_t fp;
+ uint64_t pc;
+ uint32_t cpsr;
+ uint64_t ttbr0;
+ uint64_t ttbr1;
+};
+
/*
* mem_access flag definitions
*
@@ -254,6 +280,7 @@ typedef struct vm_event_st {
union {
union {
struct vm_event_regs_x86 x86;
+ struct vm_event_regs_arm arm;
} regs;
struct vm_event_emul_read_data emul_read_data;
--
2.8.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 3/5] x86/hvm: Rename hvm/event to hvm/monitor
2016-04-29 18:07 [PATCH v2 1/5] monitor: Rename vm_event_monitor_get_capabilities Tamas K Lengyel
2016-04-29 18:07 ` [PATCH v2 2/5] vm_event: Implement ARM SMC events Tamas K Lengyel
@ 2016-04-29 18:07 ` Tamas K Lengyel
2016-05-04 1:13 ` Tian, Kevin
2016-04-29 18:07 ` [PATCH v2 4/5] x86/hvm: Add debug exception vm_events Tamas K Lengyel
2016-04-29 18:07 ` [PATCH v2 5/5] MAINTAINERS: Update monitor/vm_event covered code Tamas K Lengyel
3 siblings, 1 reply; 28+ messages in thread
From: Tamas K Lengyel @ 2016-04-29 18:07 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Tamas K Lengyel, Jan Beulich, Razvan Cojocaru,
Andrew Cooper, Jun Nakajima
Mechanical renaming to better describe that the code in hvm/event is part of
the monitor subsystem.
Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
---
MAINTAINERS | 1 -
xen/arch/x86/hvm/Makefile | 2 +-
xen/arch/x86/hvm/hvm.c | 12 +++++------
xen/arch/x86/hvm/{event.c => monitor.c} | 17 ++++++++-------
xen/arch/x86/hvm/vmx/vmx.c | 13 +++++------
xen/include/asm-x86/hvm/{event.h => monitor.h} | 30 +++++++++++++-------------
6 files changed, 38 insertions(+), 37 deletions(-)
rename xen/arch/x86/hvm/{event.c => monitor.c} (88%)
rename xen/include/asm-x86/hvm/{event.h => monitor.h} (60%)
diff --git a/MAINTAINERS b/MAINTAINERS
index 36d8591..0c050a5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -358,7 +358,6 @@ S: Supported
F: xen/*/vm_event.c
F: xen/*/monitor.c
F: xen/common/mem_access.c
-F: xen/arch/x86/hvm/event.c
F: tools/tests/xen-access
VTPM
diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 8bc55a9..f750d13 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -3,7 +3,6 @@ subdir-y += vmx
obj-y += asid.o
obj-y += emulate.o
-obj-y += event.o
obj-y += hpet.o
obj-y += hvm.o
obj-y += i8254.o
@@ -11,6 +10,7 @@ obj-y += intercept.o
obj-y += io.o
obj-y += ioreq.o
obj-y += irq.o
+obj-y += monitor.o
obj-y += mtrr.o
obj-y += nestedhvm.o
obj-y += pmtimer.o
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8cb6e9e..08fd25f 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -59,7 +59,7 @@
#include <asm/hvm/cacheattr.h>
#include <asm/hvm/trace.h>
#include <asm/hvm/nestedhvm.h>
-#include <asm/hvm/event.h>
+#include <asm/hvm/monitor.h>
#include <asm/hvm/ioreq.h>
#include <asm/hvm/vmx/vmx.h>
#include <asm/altp2m.h>
@@ -1960,7 +1960,7 @@ int hvm_handle_xsetbv(u32 index, u64 new_bv)
{
int rc;
- hvm_event_crX(XCR0, new_bv, current->arch.xcr0);
+ hvm_monitor_crX(XCR0, new_bv, current->arch.xcr0);
rc = handle_xsetbv(index, new_bv);
if ( rc )
@@ -2196,7 +2196,7 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer)
{
ASSERT(v->arch.vm_event);
- if ( hvm_event_crX(CR0, value, old_value) )
+ if ( hvm_monitor_crX(CR0, value, old_value) )
{
/* The actual write will occur in hvm_do_resume(), if permitted. */
v->arch.vm_event->write_data.do_write.cr0 = 1;
@@ -2298,7 +2298,7 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
{
ASSERT(v->arch.vm_event);
- if ( hvm_event_crX(CR3, value, old) )
+ if ( hvm_monitor_crX(CR3, value, old) )
{
/* The actual write will occur in hvm_do_resume(), if permitted. */
v->arch.vm_event->write_data.do_write.cr3 = 1;
@@ -2378,7 +2378,7 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer)
{
ASSERT(v->arch.vm_event);
- if ( hvm_event_crX(CR4, value, old_cr) )
+ if ( hvm_monitor_crX(CR4, value, old_cr) )
{
/* The actual write will occur in hvm_do_resume(), if permitted. */
v->arch.vm_event->write_data.do_write.cr4 = 1;
@@ -3711,7 +3711,7 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
v->arch.vm_event->write_data.msr = msr;
v->arch.vm_event->write_data.value = msr_content;
- hvm_event_msr(msr, msr_content);
+ hvm_monitor_msr(msr, msr_content);
return X86EMUL_OKAY;
}
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/monitor.c
similarity index 88%
rename from xen/arch/x86/hvm/event.c
rename to xen/arch/x86/hvm/monitor.c
index f7d1418..76ed811 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -1,5 +1,5 @@
/*
- * arch/x86/hvm/event.c
+ * arch/x86/hvm/monitor.c
*
* Arch-specific hardware virtual machine event abstractions.
*
@@ -7,6 +7,7 @@
* Copyright (c) 2005, International Business Machines Corporation.
* Copyright (c) 2008, Citrix Systems, Inc.
* Copyright (c) 2016, Bitdefender S.R.L.
+ * Copyright (c) 2016, Tamas K Lengyel (tamas@tklengyel.com)
*
* This program is free software; you can redistribute it and/or modify it
* under the terms and conditions of the GNU General Public License,
@@ -22,12 +23,12 @@
*/
#include <xen/vm_event.h>
-#include <asm/hvm/event.h>
+#include <asm/hvm/monitor.h>
#include <asm/monitor.h>
#include <asm/vm_event.h>
#include <public/vm_event.h>
-bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
+bool_t hvm_monitor_cr(unsigned int index, unsigned long value, unsigned long old)
{
struct vcpu *curr = current;
struct arch_domain *ad = &curr->domain->arch;
@@ -55,7 +56,7 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
return 0;
}
-void hvm_event_msr(unsigned int msr, uint64_t value)
+void hvm_monitor_msr(unsigned int msr, uint64_t value)
{
struct vcpu *curr = current;
struct arch_domain *ad = &curr->domain->arch;
@@ -88,8 +89,8 @@ static inline unsigned long gfn_of_rip(unsigned long rip)
return paging_gva_to_gfn(curr, sreg.base + rip, &pfec);
}
-int hvm_event_breakpoint(unsigned long rip,
- enum hvm_event_breakpoint_type type)
+int hvm_monitor_breakpoint(unsigned long rip,
+ enum hvm_monitor_breakpoint_type type)
{
struct vcpu *curr = current;
struct arch_domain *ad = &curr->domain->arch;
@@ -97,14 +98,14 @@ int hvm_event_breakpoint(unsigned long rip,
switch ( type )
{
- case HVM_EVENT_SOFTWARE_BREAKPOINT:
+ case HVM_MONITOR_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;
- case HVM_EVENT_SINGLESTEP_BREAKPOINT:
+ case HVM_MONITOR_SINGLESTEP_BREAKPOINT:
if ( !ad->monitor.singlestep_enabled )
return 0;
req.reason = VM_EVENT_REASON_SINGLESTEP;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index bc4410f..476a606 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -50,7 +50,7 @@
#include <asm/hvm/vpt.h>
#include <public/hvm/save.h>
#include <asm/hvm/trace.h>
-#include <asm/hvm/event.h>
+#include <asm/hvm/monitor.h>
#include <asm/xenoprof.h>
#include <asm/debugger.h>
#include <asm/apic.h>
@@ -2473,10 +2473,10 @@ static int vmx_cr_access(unsigned long exit_qualification)
/*
* Special case unlikely to be interesting to a
- * VM_EVENT_FLAG_DENY-capable application, so the hvm_event_crX()
+ * VM_EVENT_FLAG_DENY-capable application, so the hvm_monitor_crX()
* return value is ignored for now.
*/
- hvm_event_crX(CR0, value, old);
+ hvm_monitor_crX(CR0, value, old);
curr->arch.hvm_vcpu.guest_cr[0] = value;
vmx_update_guest_cr(curr, 0);
HVMTRACE_0D(CLTS);
@@ -3389,8 +3389,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
}
else {
int handled =
- hvm_event_breakpoint(regs->eip,
- HVM_EVENT_SOFTWARE_BREAKPOINT);
+ hvm_monitor_breakpoint(regs->eip,
+ HVM_MONITOR_SOFTWARE_BREAKPOINT);
if ( handled < 0 )
{
@@ -3717,7 +3717,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
vmx_update_cpu_exec_control(v);
if ( v->arch.hvm_vcpu.single_step )
{
- hvm_event_breakpoint(regs->eip, HVM_EVENT_SINGLESTEP_BREAKPOINT);
+ hvm_monitor_breakpoint(regs->eip,
+ HVM_MONITOR_SINGLESTEP_BREAKPOINT);
if ( v->domain->debugger_attached )
domain_pause_for_debugger();
}
diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/monitor.h
similarity index 60%
rename from xen/include/asm-x86/hvm/event.h
rename to xen/include/asm-x86/hvm/monitor.h
index 03f7fee..1d3c668 100644
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/monitor.h
@@ -1,7 +1,7 @@
/*
- * include/asm-x86/hvm/event.h
+ * include/asm-x86/hvm/monitor.h
*
- * Arch-specific hardware virtual machine event abstractions.
+ * Arch-specific hardware virtual machine monitor 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,
@@ -16,17 +16,17 @@
* this program; If not, see <http://www.gnu.org/licenses/>.
*/
-#ifndef __ASM_X86_HVM_EVENT_H__
-#define __ASM_X86_HVM_EVENT_H__
+#ifndef __ASM_X86_HVM_MONITOR_H__
+#define __ASM_X86_HVM_MONITOR_H__
#include <xen/sched.h>
#include <xen/paging.h>
#include <public/vm_event.h>
-enum hvm_event_breakpoint_type
+enum hvm_monitor_breakpoint_type
{
- HVM_EVENT_SOFTWARE_BREAKPOINT,
- HVM_EVENT_SINGLESTEP_BREAKPOINT,
+ HVM_MONITOR_SOFTWARE_BREAKPOINT,
+ HVM_MONITOR_SINGLESTEP_BREAKPOINT,
};
/*
@@ -34,15 +34,15 @@ enum hvm_event_breakpoint_type
* The event might not fire if the client has subscribed to it in onchangeonly
* mode, hence the bool_t return type for control register write events.
*/
-bool_t hvm_event_cr(unsigned int index, unsigned long value,
- unsigned long old);
-#define hvm_event_crX(cr, new, old) \
- hvm_event_cr(VM_EVENT_X86_##cr, new, old)
-void hvm_event_msr(unsigned int msr, uint64_t value);
-int hvm_event_breakpoint(unsigned long rip,
- enum hvm_event_breakpoint_type type);
+bool_t hvm_monitor_cr(unsigned int index, unsigned long value,
+ unsigned long old);
+#define hvm_monitor_crX(cr, new, old) \
+ hvm_monitor_cr(VM_EVENT_X86_##cr, new, old)
+void hvm_monitor_msr(unsigned int msr, uint64_t value);
+int hvm_monitor_breakpoint(unsigned long rip,
+ enum hvm_monitor_breakpoint_type type);
-#endif /* __ASM_X86_HVM_EVENT_H__ */
+#endif /* __ASM_X86_HVM_MONITOR_H__ */
/*
* Local variables:
--
2.8.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 4/5] x86/hvm: Add debug exception vm_events
2016-04-29 18:07 [PATCH v2 1/5] monitor: Rename vm_event_monitor_get_capabilities Tamas K Lengyel
2016-04-29 18:07 ` [PATCH v2 2/5] vm_event: Implement ARM SMC events Tamas K Lengyel
2016-04-29 18:07 ` [PATCH v2 3/5] x86/hvm: Rename hvm/event to hvm/monitor Tamas K Lengyel
@ 2016-04-29 18:07 ` Tamas K Lengyel
2016-05-02 13:12 ` Jan Beulich
2016-04-29 18:07 ` [PATCH v2 5/5] MAINTAINERS: Update monitor/vm_event covered code Tamas K Lengyel
3 siblings, 1 reply; 28+ messages in thread
From: Tamas K Lengyel @ 2016-04-29 18:07 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Tamas K Lengyel, Keir Fraser, Jan Beulich,
Razvan Cojocaru, Andrew Cooper, Jun Nakajima
Since in-guest debug exceptions are now unconditionally trapped to Xen, adding
a hook for vm_event subscribers to tap into this new always-on guest event. We
rename along the way hvm_event_breakpoint_type to hvm_event_type to better
match the various events that can be passed with it. We also introduce the
necessary monitor_op domctl's to enable subscribing to the events.
Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
---
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
v2: Rename hvm_monitor_event to hvm_monitor_debug
---
tools/libxc/include/xenctrl.h | 3 +-
tools/libxc/xc_monitor.c | 15 +++++++++
tools/tests/xen-access/xen-access.c | 61 ++++++++++++++++++++++++++++++++-----
xen/arch/x86/hvm/monitor.c | 26 ++++++++++++++--
xen/arch/x86/hvm/vmx/vmx.c | 26 +++++++++++++---
xen/arch/x86/monitor.c | 16 ++++++++++
xen/include/asm-x86/domain.h | 2 ++
xen/include/asm-x86/hvm/monitor.h | 7 +++--
xen/include/asm-x86/monitor.h | 3 +-
xen/include/public/domctl.h | 6 ++++
xen/include/public/vm_event.h | 12 +++++++-
11 files changed, 156 insertions(+), 21 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 4b75ae4..ff3ba9e 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2162,7 +2162,8 @@ 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);
-
+int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
+ bool enable, bool sync);
/**
* This function enables / disables emulation for each REP for a
* REP-compatible instruction.
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 072df70..e9b0343 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -171,6 +171,21 @@ int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
return do_domctl(xch, &domctl);
}
+int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
+ bool enable, bool sync)
+{
+ 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_DEBUG_EXCEPTION;
+ domctl.u.monitor_op.u.debug_exception.sync = sync;
+
+ return do_domctl(xch, &domctl);
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index 33e8044..ae235e2 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -53,6 +53,10 @@
#define ERROR(a, b...) fprintf(stderr, a "\n", ## b)
#define PERROR(a, b...) fprintf(stderr, a ": %s\n", ## b, strerror(errno))
+/* From xen/include/asm-x86/processor.h */
+#define X86_TRAP_DEBUG 1
+#define X86_TRAP_INT3 3
+
typedef struct vm_event {
domid_t domain_id;
xenevtchn_handle *xce_handle;
@@ -333,7 +337,7 @@ void usage(char* progname)
{
fprintf(stderr, "Usage: %s [-m] <domain_id> write|exec", progname);
#if defined(__i386__) || defined(__x86_64__)
- fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec");
+ fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec|debug");
#elif defined(__arm__) || defined(__aarch64__)
fprintf(stderr, "|privcall");
#endif
@@ -356,11 +360,13 @@ int main(int argc, char *argv[])
xc_interface *xch;
xenmem_access_t default_access = XENMEM_access_rwx;
xenmem_access_t after_first_access = XENMEM_access_rwx;
+ int memaccess = 0;
int required = 0;
int breakpoint = 0;
int shutting_down = 0;
int privcall = 0;
int altp2m = 0;
+ int debug = 0;
uint16_t altp2m_view_id = 0;
char* progname = argv[0];
@@ -394,11 +400,13 @@ int main(int argc, char *argv[])
{
default_access = XENMEM_access_rx;
after_first_access = XENMEM_access_rwx;
+ memaccess = 1;
}
else if ( !strcmp(argv[0], "exec") )
{
default_access = XENMEM_access_rw;
after_first_access = XENMEM_access_rwx;
+ memaccess = 1;
}
#if defined(__i386__) || defined(__x86_64__)
else if ( !strcmp(argv[0], "breakpoint") )
@@ -409,11 +417,17 @@ int main(int argc, char *argv[])
{
default_access = XENMEM_access_rx;
altp2m = 1;
+ memaccess = 1;
}
else if ( !strcmp(argv[0], "altp2m_exec") )
{
default_access = XENMEM_access_rw;
altp2m = 1;
+ memaccess = 1;
+ }
+ else if ( !strcmp(argv[0], "debug") )
+ {
+ debug = 1;
}
#elif defined(__arm__) || defined(__aarch64__)
else if ( !strcmp(argv[0], "privcall") )
@@ -454,7 +468,7 @@ int main(int argc, char *argv[])
}
/* With altp2m we just create a new, restricted view of the memory */
- if ( altp2m )
+ if ( memaccess && altp2m )
{
xen_pfn_t gfn = 0;
unsigned long perm_set = 0;
@@ -501,7 +515,7 @@ int main(int argc, char *argv[])
}
}
- if ( !altp2m )
+ if ( memaccess && !altp2m )
{
/* Set the default access type and convert all pages to it */
rc = xc_set_mem_access(xch, domain_id, default_access, ~0ull, 0);
@@ -542,6 +556,16 @@ int main(int argc, char *argv[])
}
}
+ if ( debug )
+ {
+ rc = xc_monitor_debug_exceptions(xch, domain_id, 1, 1);
+ if ( rc < 0 )
+ {
+ ERROR("Error %d setting debug exception listener with vm_event\n", rc);
+ goto exit;
+ }
+ }
+
/* Wait for access */
for (;;)
{
@@ -552,9 +576,10 @@ int main(int argc, char *argv[])
if ( breakpoint )
rc = xc_monitor_software_breakpoint(xch, domain_id, 0);
-
if ( privcall )
rc = xc_monitor_privileged_call(xch, domain_id, 0);
+ if ( debug )
+ rc = xc_monitor_debug_exceptions(xch, domain_id, 0, 0);
if ( altp2m )
{
@@ -662,9 +687,10 @@ int main(int argc, char *argv[])
req.vcpu_id);
/* Reinject */
- rc = xc_hvm_inject_trap(
- xch, domain_id, req.vcpu_id, 3,
- HVMOP_TRAP_sw_exc, -1, 0, 0);
+ rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id,
+ X86_TRAP_INT3,
+ req.u.software_breakpoint.type, -1,
+ req.u.software_breakpoint.insn_length, 0);
if (rc < 0)
{
ERROR("Error %d injecting breakpoint\n", rc);
@@ -698,6 +724,27 @@ int main(int argc, char *argv[])
rsp.flags |= VM_EVENT_FLAG_TOGGLE_SINGLESTEP;
break;
+ case VM_EVENT_REASON_DEBUG_EXCEPTION:
+ printf("Debug exception: rip=%016"PRIx64", vcpu %d. Type: %u. Length: %u\n",
+ req.data.regs.x86.rip,
+ req.vcpu_id,
+ req.u.debug_exception.type,
+ req.u.debug_exception.insn_length);
+
+ /* Reinject */
+ rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id,
+ X86_TRAP_DEBUG,
+ req.u.debug_exception.type, -1,
+ req.u.debug_exception.insn_length,
+ req.data.regs.x86.cr2);
+ if (rc < 0)
+ {
+ ERROR("Error %d injecting breakpoint\n", rc);
+ interrupted = -1;
+ continue;
+ }
+
+ break;
default:
fprintf(stderr, "UNKNOWN REASON CODE %d\n", req.reason);
}
diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index 76ed811..f91c00b 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -89,12 +89,14 @@ static inline unsigned long gfn_of_rip(unsigned long rip)
return paging_gva_to_gfn(curr, sreg.base + rip, &pfec);
}
-int hvm_monitor_breakpoint(unsigned long rip,
- enum hvm_monitor_breakpoint_type type)
+int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type,
+ unsigned long trap_type, unsigned long insn_length)
{
struct vcpu *curr = current;
struct arch_domain *ad = &curr->domain->arch;
vm_event_request_t req = {};
+ bool_t sync;
+ int rc;
switch ( type )
{
@@ -103,6 +105,9 @@ int hvm_monitor_breakpoint(unsigned long rip,
return 0;
req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
req.u.software_breakpoint.gfn = gfn_of_rip(rip);
+ req.u.software_breakpoint.type = trap_type;
+ req.u.software_breakpoint.insn_length = insn_length;
+ sync = 1;
break;
case HVM_MONITOR_SINGLESTEP_BREAKPOINT:
@@ -110,6 +115,17 @@ int hvm_monitor_breakpoint(unsigned long rip,
return 0;
req.reason = VM_EVENT_REASON_SINGLESTEP;
req.u.singlestep.gfn = gfn_of_rip(rip);
+ sync = 1;
+ break;
+
+ case HVM_MONITOR_DEBUG_EXCEPTION:
+ if ( !ad->monitor.debug_exception_enabled )
+ return 0;
+ req.reason = VM_EVENT_REASON_DEBUG_EXCEPTION;
+ req.u.debug_exception.gfn = gfn_of_rip(rip);
+ req.u.debug_exception.type = trap_type;
+ req.u.debug_exception.insn_length = insn_length;
+ sync = !!ad->monitor.debug_exception_sync;
break;
default:
@@ -119,7 +135,11 @@ int hvm_monitor_breakpoint(unsigned long rip,
req.vcpu_id = curr->vcpu_id;
vm_event_fill_regs(&req);
- return vm_event_monitor_traps(curr, 1, &req);
+ rc = vm_event_monitor_traps(curr, sync, &req);
+ if ( type == HVM_MONITOR_DEBUG_EXCEPTION && rc == 1 && !sync )
+ rc = 0;
+
+ return rc;
}
/*
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 476a606..ac546ab 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3373,9 +3373,25 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
if ( !v->domain->debugger_attached )
- vmx_propagate_intr(intr_info);
+ {
+ unsigned long insn_length = 0;
+ int handled;
+ unsigned long trap_type = MASK_EXTR(intr_info,
+ INTR_INFO_INTR_TYPE_MASK);
+
+ if( trap_type >= X86_EVENTTYPE_SW_INTERRUPT )
+ __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_length);
+
+ handled = hvm_monitor_debug(regs->eip,
+ HVM_MONITOR_DEBUG_EXCEPTION,
+ trap_type, insn_length);
+ if ( handled <= 0 )
+ vmx_propagate_intr(intr_info);
+
+ }
else
domain_pause_for_debugger();
+
break;
case TRAP_int3:
{
@@ -3389,8 +3405,9 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
}
else {
int handled =
- hvm_monitor_breakpoint(regs->eip,
- HVM_MONITOR_SOFTWARE_BREAKPOINT);
+ hvm_monitor_debug(regs->eip,
+ HVM_MONITOR_SOFTWARE_BREAKPOINT,
+ X86_EVENTTYPE_SW_EXCEPTION, 1);
if ( handled < 0 )
{
@@ -3717,8 +3734,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
vmx_update_cpu_exec_control(v);
if ( v->arch.hvm_vcpu.single_step )
{
- hvm_monitor_breakpoint(regs->eip,
- HVM_MONITOR_SINGLESTEP_BREAKPOINT);
+ hvm_monitor_debug(regs->eip, HVM_MONITOR_SINGLESTEP_BREAKPOINT, 0, 0);
if ( v->domain->debugger_attached )
domain_pause_for_debugger();
}
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 621f91a..e8b79f6 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -124,6 +124,22 @@ int arch_monitor_domctl_event(struct domain *d,
break;
}
+ case XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION:
+ {
+ bool_t old_status = ad->monitor.debug_exception_enabled;
+
+ if ( unlikely(old_status == requested_status) )
+ return -EEXIST;
+
+ domain_pause(d);
+ ad->monitor.debug_exception_enabled = requested_status;
+ ad->monitor.debug_exception_sync = requested_status ?
+ mop->u.debug_exception.sync :
+ 0;
+ domain_unpause(d);
+ break;
+ }
+
default:
/*
* Should not be reached unless arch_monitor_get_capabilities() is
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 165e533..1cf97c3 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -405,6 +405,8 @@ struct arch_domain
unsigned int mov_to_msr_extended : 1;
unsigned int singlestep_enabled : 1;
unsigned int software_breakpoint_enabled : 1;
+ unsigned int debug_exception_enabled : 1;
+ unsigned int debug_exception_sync : 1;
} monitor;
/* Mem_access emulation control */
diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h
index 1d3c668..422bc11 100644
--- a/xen/include/asm-x86/hvm/monitor.h
+++ b/xen/include/asm-x86/hvm/monitor.h
@@ -23,10 +23,11 @@
#include <xen/paging.h>
#include <public/vm_event.h>
-enum hvm_monitor_breakpoint_type
+enum hvm_monitor_debug_type
{
HVM_MONITOR_SOFTWARE_BREAKPOINT,
HVM_MONITOR_SINGLESTEP_BREAKPOINT,
+ HVM_MONITOR_DEBUG_EXCEPTION,
};
/*
@@ -39,8 +40,8 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long value,
#define hvm_monitor_crX(cr, new, old) \
hvm_monitor_cr(VM_EVENT_X86_##cr, new, old)
void hvm_monitor_msr(unsigned int msr, uint64_t value);
-int hvm_monitor_breakpoint(unsigned long rip,
- enum hvm_monitor_breakpoint_type type);
+int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type,
+ unsigned long trap_type, unsigned long insn_length);
#endif /* __ASM_X86_HVM_MONITOR_H__ */
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 446b2af..240a94e 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -64,7 +64,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
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);
+ (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
+ (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION);
/* Since we know this is on VMX, we can just call the hvm func */
if ( hvm_is_singlestep_supported() )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 35adce2..b69b1bb 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1081,6 +1081,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
#define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT 3
#define XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST 4
#define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL 5
+#define XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION 6
struct xen_domctl_monitor_op {
uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
@@ -1116,6 +1117,11 @@ struct xen_domctl_monitor_op {
/* Pause vCPU until response */
uint8_t sync;
} guest_request;
+
+ struct {
+ /* Pause vCPU until response */
+ uint8_t sync;
+ } debug_exception;
} u;
};
typedef struct xen_domctl_monitor_op xen_domctl_monitor_op_t;
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index f039207..6c85532 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -121,6 +121,8 @@
#define VM_EVENT_REASON_GUEST_REQUEST 8
/* Privileged call executed (e.g. SMC) */
#define VM_EVENT_REASON_PRIVILEGED_CALL 9
+/* A debug exception was caught */
+#define VM_EVENT_REASON_DEBUG_EXCEPTION 10
/* Supported values for the vm_event_write_ctrlreg index. */
#define VM_EVENT_X86_CR0 0
@@ -229,8 +231,15 @@ struct vm_event_write_ctrlreg {
uint64_t old_value;
};
+struct vm_event_singlestep {
+ uint64_t gfn;
+};
+
struct vm_event_debug {
uint64_t gfn;
+ uint8_t type; /* HVMOP_TRAP_* */
+ uint8_t insn_length;
+ uint8_t _pad[6];
};
struct vm_event_mov_to_msr {
@@ -274,7 +283,8 @@ typedef struct vm_event_st {
struct vm_event_write_ctrlreg write_ctrlreg;
struct vm_event_mov_to_msr mov_to_msr;
struct vm_event_debug software_breakpoint;
- struct vm_event_debug singlestep;
+ struct vm_event_singlestep singlestep;
+ struct vm_event_debug debug_exception;
} u;
union {
--
2.8.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 5/5] MAINTAINERS: Update monitor/vm_event covered code
2016-04-29 18:07 [PATCH v2 1/5] monitor: Rename vm_event_monitor_get_capabilities Tamas K Lengyel
` (2 preceding siblings ...)
2016-04-29 18:07 ` [PATCH v2 4/5] x86/hvm: Add debug exception vm_events Tamas K Lengyel
@ 2016-04-29 18:07 ` Tamas K Lengyel
2016-05-02 13:00 ` Jan Beulich
2016-05-03 8:18 ` Jan Beulich
3 siblings, 2 replies; 28+ messages in thread
From: Tamas K Lengyel @ 2016-04-29 18:07 UTC (permalink / raw)
To: xen-devel
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Stefano Stabellini,
Jan Beulich
Add headers to the covered list.
Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
---
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
MAINTAINERS | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 0c050a5..cbf2234 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -351,13 +351,16 @@ S: Supported
T: hg http://xenbits.xen.org/linux-2.6.18-xen.hg
F: drivers/xen/usb*/
-VM EVENT AND MEM ACCESS
+VM EVENT, MEM ACCESS and MONITOR
M: Razvan Cojocaru <rcojocaru@bitdefender.com>
M: Tamas K Lengyel <tamas@tklengyel.com>
S: Supported
F: xen/*/vm_event.c
F: xen/*/monitor.c
F: xen/common/mem_access.c
+F: xen/include/*/mem_access.h
+F: xen/include/*/monitor.h
+F: xen/include/*/vm_event.h
F: tools/tests/xen-access
VTPM
--
2.8.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] vm_event: Implement ARM SMC events
2016-04-29 18:07 ` [PATCH v2 2/5] vm_event: Implement ARM SMC events Tamas K Lengyel
@ 2016-04-29 20:07 ` Razvan Cojocaru
2016-04-29 20:12 ` Tamas K Lengyel
2016-05-02 10:35 ` Wei Liu
2016-05-03 11:31 ` Julien Grall
2 siblings, 1 reply; 28+ messages in thread
From: Razvan Cojocaru @ 2016-04-29 20:07 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: Wei Liu, Stefano Stabellini, Andrew Cooper, Ian Jackson,
Julien Grall, Jan Beulich, Keir Fraser
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 <tamas@tklengyel.com>
> ---
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>
> 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 <rcojocaru@bitdefender.com>
> M: Tamas K Lengyel <tamas@tklengyel.com>
> 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 (tamas@tklengyel.com)
> + * Copyright (c) 2015-2016 Tamas K Lengyel (tamas@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, &domctl);
> }
> +
> +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, &domctl);
> +}
> +
> +/*
> + * 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] <domain_id> 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 ( !strcmp(argv[0], "privcall") )
> + {
> + privcall = 1;
> + }
> #endif
> else
> {
> @@ -524,6 +532,16 @@ int main(int argc, char *argv[])
> }
> }
>
> + if ( privcall )
> + {
> + rc = xc_monitor_privileged_call(xch, domain_id, 1);
> + if ( rc < 0 )
> + {
> + ERROR("Error %d setting privileged call trapping with vm_event\n", rc);
> + goto exit;
> + }
> + }
> +
> /* Wait for access */
> for (;;)
> {
> @@ -535,6 +553,9 @@ int main(int argc, char *argv[])
> if ( breakpoint )
> rc = xc_monitor_software_breakpoint(xch, domain_id, 0);
>
> + if ( privcall )
> + rc = xc_monitor_privileged_call(xch, domain_id, 0);
> +
> if ( altp2m )
> {
> rc = xc_altp2m_switch_to_view( xch, domain_id, 0 );
> @@ -635,7 +656,7 @@ int main(int argc, char *argv[])
> rsp.u.mem_access = req.u.mem_access;
> break;
> case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
> - printf("Breakpoint: rip=%016"PRIx64", gfn=%"PRIx64" (vcpu %d)\n",
> + printf("Breakpoint: rip=%"PRIx64" gfn=%"PRIx64" (vcpu %d)\n",
> req.data.regs.x86.rip,
> req.u.software_breakpoint.gfn,
> req.vcpu_id);
> @@ -650,7 +671,15 @@ int main(int argc, char *argv[])
> interrupted = -1;
> continue;
> }
> + break;
> + case VM_EVENT_REASON_PRIVILEGED_CALL:
> + printf("Privileged call: pc=%"PRIx64" (vcpu %d)\n",
> + req.data.regs.arm.pc,
> + req.vcpu_id);
>
> + rsp.data.regs.arm = req.data.regs.arm;
> + rsp.data.regs.arm.pc += 4;
> + rsp.flags |= VM_EVENT_FLAG_SET_REGISTERS;
> break;
> case VM_EVENT_REASON_SINGLESTEP:
> printf("Singlestep: rip=%016"PRIx64", vcpu %d, altp2m %u\n",
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 0328b50..118be99 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -40,6 +40,8 @@ obj-y += device.o
> obj-y += decode.o
> obj-y += processor.o
> obj-y += smc.o
> +obj-y += monitor.o
> +obj-y += vm_event.o
>
> #obj-bin-y += ....o
>
> diff --git a/xen/arch/arm/monitor.c b/xen/arch/arm/monitor.c
> new file mode 100644
> index 0000000..e845f28
> --- /dev/null
> +++ b/xen/arch/arm/monitor.c
> @@ -0,0 +1,80 @@
> +/*
> + * arch/arm/monitor.c
> + *
> + * Arch-specific monitor_op domctl handler.
> + *
> + * Copyright (c) 2015-2016 Tamas K Lengyel (tamas@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 <asm/vm_event.h>
> +#include <public/vm_event.h>
> +
> +int arch_monitor_domctl_event(struct domain *d,
> + struct xen_domctl_monitor_op *mop)
> +{
> + struct arch_domain *ad = &d->arch;
> + bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
> +
> + switch ( mop->event )
> + {
> + case XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL:
> + {
> + bool_t old_status = ad->monitor.privileged_call_enabled;
> +
> + if ( unlikely(old_status == requested_status) )
> + return -EEXIST;
> +
> + domain_pause(d);
> + ad->monitor.privileged_call_enabled = requested_status;
> + domain_unpause(d);
> + break;
> + }
> +
> + default:
> + /*
> + * Should not be reached unless arch_monitor_get_capabilities() is
> + * not properly implemented.
> + */
> + ASSERT_UNREACHABLE();
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +int monitor_smc(const struct cpu_user_regs *regs) {
> + 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(&req, regs, curr->domain);
> +
> + return vm_event_monitor_traps(curr, 1, &req);
> +}
> +
> +/*
> + * 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 <asm/mmio.h>
> #include <asm/cpufeature.h>
> #include <asm/flushtlb.h>
> +#include <asm/monitor.h>
>
> #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;
> + 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?
> +
> + if ( rc != 1 )
> + {
> + GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> + 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 0000000..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 (tamas@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 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 <xen/sched.h>
> +#include <asm/vm_event.h>
> +
> +void vm_event_fill_regs(vm_event_request_t *req,
> + const struct cpu_user_regs *regs,
> + struct domain *d)
> +{
> + if ( is_32bit_domain(d) )
> + {
> + req->data.regs.arm.x0 = regs->r0;
> + req->data.regs.arm.x1 = regs->r1;
> + req->data.regs.arm.x2 = regs->r2;
> + req->data.regs.arm.x3 = regs->r3;
> + req->data.regs.arm.x4 = regs->r4;
> + req->data.regs.arm.x5 = regs->r5;
> + req->data.regs.arm.x6 = regs->r6;
> + req->data.regs.arm.x7 = regs->r7;
> + req->data.regs.arm.x8 = regs->r8;
> + req->data.regs.arm.x9 = regs->r9;
> + req->data.regs.arm.x10 = regs->r10;
> + req->data.regs.arm.pc = regs->pc32;
> + req->data.regs.arm.sp_el0 = regs->sp_usr;
> + req->data.regs.arm.sp_el1 = regs->sp_svc;
> + }
> +#ifdef CONFIG_ARM_64
> + else
> + {
> + req->data.regs.arm.x0 = regs->x0;
> + req->data.regs.arm.x1 = regs->x1;
> + req->data.regs.arm.x2 = regs->x2;
> + req->data.regs.arm.x3 = regs->x3;
> + req->data.regs.arm.x4 = regs->x4;
> + req->data.regs.arm.x5 = regs->x5;
> + req->data.regs.arm.x6 = regs->x6;
> + req->data.regs.arm.x7 = regs->x7;
> + req->data.regs.arm.x8 = regs->x8;
> + req->data.regs.arm.x9 = regs->x9;
> + req->data.regs.arm.x10 = regs->x10;
> + req->data.regs.arm.pc = regs->pc;
> + req->data.regs.arm.sp_el0 = regs->sp_el0;
> + req->data.regs.arm.sp_el1 = regs->sp_el1;
> + }
> +#endif
> + req->data.regs.arm.fp = regs->fp;
> + req->data.regs.arm.lr = regs->lr;
> + req->data.regs.arm.cpsr = regs->cpsr;
> + req->data.regs.arm.spsr_el1 = regs->spsr_svc;
> + req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
> + req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
> +}
> +
> +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
> +{
> + struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs;
> +
> + if ( is_32bit_domain(v->domain) )
> + {
> + regs->r0 = rsp->data.regs.arm.x0;
> + regs->r1 = rsp->data.regs.arm.x1;
> + regs->r2 = rsp->data.regs.arm.x2;
> + regs->r3 = rsp->data.regs.arm.x3;
> + regs->r4 = rsp->data.regs.arm.x4;
> + regs->r5 = rsp->data.regs.arm.x5;
> + regs->r6 = rsp->data.regs.arm.x6;
> + regs->r7 = rsp->data.regs.arm.x7;
> + regs->r8 = rsp->data.regs.arm.x8;
> + regs->r9 = rsp->data.regs.arm.x9;
> + regs->r10 = rsp->data.regs.arm.x10;
> + regs->pc32 = rsp->data.regs.arm.pc;
> + regs->sp_usr = rsp->data.regs.arm.sp_el0;
> + regs->sp_svc = rsp->data.regs.arm.sp_el1;
> + }
> +#ifdef CONFIG_ARM_64
> + else
> + {
> + regs->x0 = rsp->data.regs.arm.x0;
> + regs->x1 = rsp->data.regs.arm.x1;
> + regs->x2 = rsp->data.regs.arm.x2;
> + regs->x3 = rsp->data.regs.arm.x3;
> + regs->x4 = rsp->data.regs.arm.x4;
> + regs->x5 = rsp->data.regs.arm.x5;
> + regs->x6 = rsp->data.regs.arm.x6;
> + regs->x7 = rsp->data.regs.arm.x7;
> + regs->x8 = rsp->data.regs.arm.x8;
> + regs->x9 = rsp->data.regs.arm.x9;
> + regs->x10 = rsp->data.regs.arm.x10;
> + regs->pc = rsp->data.regs.arm.pc;
> + regs->sp_el0 = rsp->data.regs.arm.sp_el0;
> + regs->sp_el1 = rsp->data.regs.arm.sp_el1;
> + }
> +#endif
> +
> + regs->fp = rsp->data.regs.arm.fp;
> + regs->lr = rsp->data.regs.arm.lr;
> + regs->cpsr = rsp->data.regs.arm.cpsr;
> + v->arch.ttbr0 = rsp->data.regs.arm.ttbr0;
> + v->arch.ttbr1 = rsp->data.regs.arm.ttbr1;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
> index 56c5514..f7d1418 100644
> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -47,6 +47,7 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
> .u.write_ctrlreg.old_value = old
> };
>
> + vm_event_fill_regs(&req);
> vm_event_monitor_traps(curr, sync, &req);
> return 1;
> }
> @@ -115,6 +116,7 @@ int hvm_event_breakpoint(unsigned long rip,
> }
>
> req.vcpu_id = curr->vcpu_id;
> + vm_event_fill_regs(&req);
>
> return vm_event_monitor_traps(curr, 1, &req);
> }
> 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, &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.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] vm_event: Implement ARM SMC events
2016-04-29 20:07 ` Razvan Cojocaru
@ 2016-04-29 20:12 ` Tamas K Lengyel
2016-04-29 20:27 ` Tamas K Lengyel
0 siblings, 1 reply; 28+ messages in thread
From: Tamas K Lengyel @ 2016-04-29 20:12 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Wei Liu, Keir Fraser, Stefano Stabellini, Andrew Cooper,
Ian Jackson, Julien Grall, Jan Beulich, Xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 1701 bytes --]
> @@ -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, &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
[-- Attachment #1.2: Type: text/html, Size: 2473 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] vm_event: Implement ARM SMC events
2016-04-29 20:12 ` Tamas K Lengyel
@ 2016-04-29 20:27 ` Tamas K Lengyel
2016-04-29 20:32 ` Razvan Cojocaru
0 siblings, 1 reply; 28+ messages in thread
From: Tamas K Lengyel @ 2016-04-29 20:27 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Wei Liu, Keir Fraser, Stefano Stabellini, Andrew Cooper,
Ian Jackson, Julien Grall, Jan Beulich, Xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 1700 bytes --]
>
>
>
>
>>
>> > 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, &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
[-- Attachment #1.2: Type: text/html, Size: 2492 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] vm_event: Implement ARM SMC events
2016-04-29 20:27 ` Tamas K Lengyel
@ 2016-04-29 20:32 ` Razvan Cojocaru
0 siblings, 0 replies; 28+ messages in thread
From: Razvan Cojocaru @ 2016-04-29 20:32 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Wei Liu, Keir Fraser, Stefano Stabellini, Andrew Cooper,
Ian Jackson, Julien Grall, Jan Beulich, Xen-devel
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, &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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] vm_event: Implement ARM SMC events
2016-04-29 18:07 ` [PATCH v2 2/5] vm_event: Implement ARM SMC events Tamas K Lengyel
2016-04-29 20:07 ` Razvan Cojocaru
@ 2016-05-02 10:35 ` Wei Liu
2016-05-03 11:31 ` Julien Grall
2 siblings, 0 replies; 28+ messages in thread
From: Wei Liu @ 2016-05-02 10:35 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Wei Liu, Razvan Cojocaru, Stefano Stabellini, Andrew Cooper,
Ian Jackson, Julien Grall, Jan Beulich, xen-devel, Keir Fraser
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 <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/5] MAINTAINERS: Update monitor/vm_event covered code
2016-04-29 18:07 ` [PATCH v2 5/5] MAINTAINERS: Update monitor/vm_event covered code Tamas K Lengyel
@ 2016-05-02 13:00 ` Jan Beulich
2016-05-03 8:18 ` Jan Beulich
1 sibling, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2016-05-02 13:00 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Stefano Stabellini, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel
>>> On 29.04.16 at 20:07, <tamas@tklengyel.com> wrote:
> Add headers to the covered list.
>
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] x86/hvm: Add debug exception vm_events
2016-04-29 18:07 ` [PATCH v2 4/5] x86/hvm: Add debug exception vm_events Tamas K Lengyel
@ 2016-05-02 13:12 ` Jan Beulich
2016-05-02 15:35 ` Tamas K Lengyel
0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2016-05-02 13:12 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Kevin Tian, Keir Fraser, Razvan Cojocaru, Andrew Cooper,
Jun Nakajima, xen-devel
>>> On 29.04.16 at 20:07, <tamas@tklengyel.com> wrote:
> @@ -229,8 +231,15 @@ struct vm_event_write_ctrlreg {
> uint64_t old_value;
> };
>
> +struct vm_event_singlestep {
> + uint64_t gfn;
> +};
> +
> struct vm_event_debug {
> uint64_t gfn;
> + uint8_t type; /* HVMOP_TRAP_* */
> + uint8_t insn_length;
> + uint8_t _pad[6];
> };
This being an incompatible change - didn't you mean to increment some
version number?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] x86/hvm: Add debug exception vm_events
2016-05-02 13:12 ` Jan Beulich
@ 2016-05-02 15:35 ` Tamas K Lengyel
2016-05-02 15:40 ` Jan Beulich
0 siblings, 1 reply; 28+ messages in thread
From: Tamas K Lengyel @ 2016-05-02 15:35 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Keir Fraser, Razvan Cojocaru, Andrew Cooper,
Jun Nakajima, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 842 bytes --]
On May 2, 2016 07:12, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 29.04.16 at 20:07, <tamas@tklengyel.com> wrote:
> > @@ -229,8 +231,15 @@ struct vm_event_write_ctrlreg {
> > uint64_t old_value;
> > };
> >
> > +struct vm_event_singlestep {
> > + uint64_t gfn;
> > +};
> > +
> > struct vm_event_debug {
> > uint64_t gfn;
> > + uint8_t type; /* HVMOP_TRAP_* */
> > + uint8_t insn_length;
> > + uint8_t _pad[6];
> > };
>
> This being an incompatible change - didn't you mean to increment some
> version number?
>
> Jan
I'm not sure. It would still work with clients compiled with the older
version of the header as the layout of the debug struct didnt change, was
just appended. The size of the request/response struct didn't change either
so technically this would still be backwards compatible.
Tamas
[-- Attachment #1.2: Type: text/html, Size: 1260 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] x86/hvm: Add debug exception vm_events
2016-05-02 15:35 ` Tamas K Lengyel
@ 2016-05-02 15:40 ` Jan Beulich
2016-05-02 15:45 ` Tamas K Lengyel
0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2016-05-02 15:40 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Kevin Tian, Keir Fraser, Razvan Cojocaru, Andrew Cooper,
Jun Nakajima, xen-devel
>>> On 02.05.16 at 17:35, <tamas@tklengyel.com> wrote:
> On May 2, 2016 07:12, "Jan Beulich" <JBeulich@suse.com> wrote:
>>
>> >>> On 29.04.16 at 20:07, <tamas@tklengyel.com> wrote:
>> > @@ -229,8 +231,15 @@ struct vm_event_write_ctrlreg {
>> > uint64_t old_value;
>> > };
>> >
>> > +struct vm_event_singlestep {
>> > + uint64_t gfn;
>> > +};
>> > +
>> > struct vm_event_debug {
>> > uint64_t gfn;
>> > + uint8_t type; /* HVMOP_TRAP_* */
>> > + uint8_t insn_length;
>> > + uint8_t _pad[6];
>> > };
>>
>> This being an incompatible change - didn't you mean to increment some
>> version number?
>
> I'm not sure. It would still work with clients compiled with the older
> version of the header as the layout of the debug struct didnt change, was
> just appended. The size of the request/response struct didn't change either
> so technically this would still be backwards compatible.
But you also need to consider the other direction: Code compiled
against the new variant, but running on an older hypervisor would
expect the new fields to be valid, yet they can't be, and the caller
has no way to know.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] x86/hvm: Add debug exception vm_events
2016-05-02 15:40 ` Jan Beulich
@ 2016-05-02 15:45 ` Tamas K Lengyel
0 siblings, 0 replies; 28+ messages in thread
From: Tamas K Lengyel @ 2016-05-02 15:45 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Keir Fraser, Razvan Cojocaru, Andrew Cooper,
Jun Nakajima, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 1315 bytes --]
On May 2, 2016 09:40, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 02.05.16 at 17:35, <tamas@tklengyel.com> wrote:
> > On May 2, 2016 07:12, "Jan Beulich" <JBeulich@suse.com> wrote:
> >>
> >> >>> On 29.04.16 at 20:07, <tamas@tklengyel.com> wrote:
> >> > @@ -229,8 +231,15 @@ struct vm_event_write_ctrlreg {
> >> > uint64_t old_value;
> >> > };
> >> >
> >> > +struct vm_event_singlestep {
> >> > + uint64_t gfn;
> >> > +};
> >> > +
> >> > struct vm_event_debug {
> >> > uint64_t gfn;
> >> > + uint8_t type; /* HVMOP_TRAP_* */
> >> > + uint8_t insn_length;
> >> > + uint8_t _pad[6];
> >> > };
> >>
> >> This being an incompatible change - didn't you mean to increment some
> >> version number?
> >
> > I'm not sure. It would still work with clients compiled with the older
> > version of the header as the layout of the debug struct didnt change,
was
> > just appended. The size of the request/response struct didn't change
either
> > so technically this would still be backwards compatible.
>
> But you also need to consider the other direction: Code compiled
> against the new variant, but running on an older hypervisor would
> expect the new fields to be valid, yet they can't be, and the caller
> has no way to know.
Fair point, will incrementnthe version.
Thanks!
Tamas
[-- Attachment #1.2: Type: text/html, Size: 2073 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/5] MAINTAINERS: Update monitor/vm_event covered code
2016-04-29 18:07 ` [PATCH v2 5/5] MAINTAINERS: Update monitor/vm_event covered code Tamas K Lengyel
2016-05-02 13:00 ` Jan Beulich
@ 2016-05-03 8:18 ` Jan Beulich
2016-05-03 8:20 ` Razvan Cojocaru
2016-05-03 9:09 ` Wei Liu
1 sibling, 2 replies; 28+ messages in thread
From: Jan Beulich @ 2016-05-03 8:18 UTC (permalink / raw)
To: Wei Liu, Tamas K Lengyel
Cc: Stefano Stabellini, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel
>>> On 29.04.16 at 20:07, <tamas@tklengyel.com> wrote:
> Add headers to the covered list.
>
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
While the rest of this series doesn't look like it wants to go in for 4.7,
I think this one can and should. Thoughts?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/5] MAINTAINERS: Update monitor/vm_event covered code
2016-05-03 8:18 ` Jan Beulich
@ 2016-05-03 8:20 ` Razvan Cojocaru
2016-05-03 8:26 ` Jan Beulich
2016-05-03 9:09 ` Wei Liu
1 sibling, 1 reply; 28+ messages in thread
From: Razvan Cojocaru @ 2016-05-03 8:20 UTC (permalink / raw)
To: Jan Beulich, Wei Liu, Tamas K Lengyel
Cc: Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, xen-devel
On 05/03/2016 11:18 AM, Jan Beulich wrote:
>>>> On 29.04.16 at 20:07, <tamas@tklengyel.com> wrote:
>> Add headers to the covered list.
>>
>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
>
> While the rest of this series doesn't look like it wants to go in for 4.7,
> I think this one can and should. Thoughts?
FWIW, I agree, and it's also appropriate to add:
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/5] MAINTAINERS: Update monitor/vm_event covered code
2016-05-03 8:20 ` Razvan Cojocaru
@ 2016-05-03 8:26 ` Jan Beulich
0 siblings, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2016-05-03 8:26 UTC (permalink / raw)
To: Razvan Cojocaru, Tamas K Lengyel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, xen-devel
>>> On 03.05.16 at 10:20, <rcojocaru@bitdefender.com> wrote:
> On 05/03/2016 11:18 AM, Jan Beulich wrote:
>>>>> On 29.04.16 at 20:07, <tamas@tklengyel.com> wrote:
>>> Add headers to the covered list.
>>>
>>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
>>
>> While the rest of this series doesn't look like it wants to go in for 4.7,
>> I think this one can and should. Thoughts?
>
> FWIW, I agree, and it's also appropriate to add:
>
> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Albeit it needs re-submission anyway, to apply without the changes
done by patch 2, or with those changes folded in (as I think you
had already suggested).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/5] MAINTAINERS: Update monitor/vm_event covered code
2016-05-03 8:18 ` Jan Beulich
2016-05-03 8:20 ` Razvan Cojocaru
@ 2016-05-03 9:09 ` Wei Liu
1 sibling, 0 replies; 28+ messages in thread
From: Wei Liu @ 2016-05-03 9:09 UTC (permalink / raw)
To: Jan Beulich
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Stefano Stabellini,
xen-devel
On Tue, May 03, 2016 at 02:18:35AM -0600, Jan Beulich wrote:
> >>> On 29.04.16 at 20:07, <tamas@tklengyel.com> wrote:
> > Add headers to the covered list.
> >
> > Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
>
> While the rest of this series doesn't look like it wants to go in for 4.7,
> I think this one can and should. Thoughts?
>
Sure, this one can go in.
Wei.
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] vm_event: Implement ARM SMC events
2016-04-29 18:07 ` [PATCH v2 2/5] vm_event: Implement ARM SMC events Tamas K Lengyel
2016-04-29 20:07 ` Razvan Cojocaru
2016-05-02 10:35 ` Wei Liu
@ 2016-05-03 11:31 ` Julien Grall
2016-05-03 18:48 ` Tamas K Lengyel
2 siblings, 1 reply; 28+ messages in thread
From: Julien Grall @ 2016-05-03 11:31 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: Wei Liu, Razvan Cojocaru, Stefano Stabellini, Andrew Cooper,
Ian Jackson, Jan Beulich, Keir Fraser
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 <tamas@tklengyel.com>
> ---
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>
> 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 0000000..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(&req, regs, curr->domain);
> +
> + return vm_event_monitor_traps(curr, 1, &req);
> +}
> +
> +/*
> + * 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 <asm/mmio.h>
> #include <asm/cpufeature.h>
> #include <asm/flushtlb.h>
> +#include <asm/monitor.h>
>
> #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 0000000..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 (tamas@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 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 <xen/sched.h>
> +#include <asm/vm_event.h>
> +
> +void vm_event_fill_regs(vm_event_request_t *req,
> + const struct cpu_user_regs *regs,
> + struct domain *d)
> +{
> + if ( is_32bit_domain(d) )
> + {
> + req->data.regs.arm.x0 = regs->r0;
> + req->data.regs.arm.x1 = regs->r1;
> + req->data.regs.arm.x2 = regs->r2;
> + req->data.regs.arm.x3 = regs->r3;
> + req->data.regs.arm.x4 = regs->r4;
> + req->data.regs.arm.x5 = regs->r5;
> + req->data.regs.arm.x6 = regs->r6;
> + req->data.regs.arm.x7 = regs->r7;
> + req->data.regs.arm.x8 = regs->r8;
> + req->data.regs.arm.x9 = regs->r9;
> + req->data.regs.arm.x10 = regs->r10;
> + req->data.regs.arm.pc = regs->pc32;
> + req->data.regs.arm.sp_el0 = regs->sp_usr;
> + req->data.regs.arm.sp_el1 = regs->sp_svc;
> + }
> +#ifdef CONFIG_ARM_64
Why
> + else
> + {
> + req->data.regs.arm.x0 = regs->x0;
> + req->data.regs.arm.x1 = regs->x1;
> + req->data.regs.arm.x2 = regs->x2;
> + req->data.regs.arm.x3 = regs->x3;
> + req->data.regs.arm.x4 = regs->x4;
> + req->data.regs.arm.x5 = regs->x5;
> + req->data.regs.arm.x6 = regs->x6;
> + req->data.regs.arm.x7 = regs->x7;
> + req->data.regs.arm.x8 = regs->x8;
> + req->data.regs.arm.x9 = regs->x9;
> + req->data.regs.arm.x10 = regs->x10;
AArch64 provides 31 generate-purpose registers. Although, x29 and x30
are respectively used for fp and lr.
> + req->data.regs.arm.pc = regs->pc;
> + req->data.regs.arm.sp_el0 = regs->sp_el0;
> + req->data.regs.arm.sp_el1 = regs->sp_el1;
> + }
> +#endif
> + req->data.regs.arm.fp = regs->fp;
> + req->data.regs.arm.lr = regs->lr;
> + req->data.regs.arm.cpsr = regs->cpsr;
> + req->data.regs.arm.spsr_el1 = regs->spsr_svc;
> + req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
> + req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
> +}
> +
> +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
> +{
> + struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs;
> +
> + if ( is_32bit_domain(v->domain) )
> + {
> + regs->r0 = rsp->data.regs.arm.x0;
> + regs->r1 = rsp->data.regs.arm.x1;
> + regs->r2 = rsp->data.regs.arm.x2;
> + regs->r3 = rsp->data.regs.arm.x3;
> + regs->r4 = rsp->data.regs.arm.x4;
> + regs->r5 = rsp->data.regs.arm.x5;
> + regs->r6 = rsp->data.regs.arm.x6;
> + regs->r7 = rsp->data.regs.arm.x7;
> + regs->r8 = rsp->data.regs.arm.x8;
> + regs->r9 = rsp->data.regs.arm.x9;
> + regs->r10 = rsp->data.regs.arm.x10;
> + regs->pc32 = rsp->data.regs.arm.pc;
> + regs->sp_usr = rsp->data.regs.arm.sp_el0;
> + regs->sp_svc = rsp->data.regs.arm.sp_el1;
> + }
> +#ifdef CONFIG_ARM_64
> + else
> + {
> + regs->x0 = rsp->data.regs.arm.x0;
> + regs->x1 = rsp->data.regs.arm.x1;
> + regs->x2 = rsp->data.regs.arm.x2;
> + regs->x3 = rsp->data.regs.arm.x3;
> + regs->x4 = rsp->data.regs.arm.x4;
> + regs->x5 = rsp->data.regs.arm.x5;
> + regs->x6 = rsp->data.regs.arm.x6;
> + regs->x7 = rsp->data.regs.arm.x7;
> + regs->x8 = rsp->data.regs.arm.x8;
> + regs->x9 = rsp->data.regs.arm.x9;
> + regs->x10 = rsp->data.regs.arm.x10;
> + regs->pc = rsp->data.regs.arm.pc;
> + regs->sp_el0 = rsp->data.regs.arm.sp_el0;
> + regs->sp_el1 = rsp->data.regs.arm.sp_el1;
> + }
> +#endif
> +
> + regs->fp = rsp->data.regs.arm.fp;
> + regs->lr = rsp->data.regs.arm.lr;
> + regs->cpsr = rsp->data.regs.arm.cpsr;
> + v->arch.ttbr0 = rsp->data.regs.arm.ttbr0;
> + v->arch.ttbr1 = rsp->data.regs.arm.ttbr1;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
[...]
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 2457698..35adce2 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1080,6 +1080,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
> #define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP 2
> #define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT 3
> #define XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST 4
> +#define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL 5
>
> struct xen_domctl_monitor_op {
> uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
> index 9270d52..f039207 100644
> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -119,6 +119,8 @@
> #define VM_EVENT_REASON_SINGLESTEP 7
> /* An event has been requested via HVMOP_guest_request_vm_event. */
> #define VM_EVENT_REASON_GUEST_REQUEST 8
> +/* Privileged call executed (e.g. SMC) */
> +#define VM_EVENT_REASON_PRIVILEGED_CALL 9
>
> /* Supported values for the vm_event_write_ctrlreg index. */
> #define VM_EVENT_X86_CR0 0
> @@ -166,6 +168,30 @@ struct vm_event_regs_x86 {
> uint32_t _pad;
> };
>
> +struct vm_event_regs_arm {
> + /* Aarch64 Aarch32 */
> + uint64_t x0; /* r0_usr */
> + uint64_t x1; /* r1_usr */
> + uint64_t x2; /* r2_usr */
> + uint64_t x3; /* r3_usr */
> + uint64_t x4; /* r4_usr */
> + uint64_t x5; /* r5_usr */
> + uint64_t x6; /* r6_usr */
> + uint64_t x7; /* r7_usr */
> + uint64_t x8; /* r8_usr */
> + uint64_t x9; /* r9_usr */
> + uint64_t x10; /* r10_usr */
I would introduce an union to let the choice to the userspace to deal
only with AArch32 registers. See vcpu_guest_core_regs.
> + uint64_t lr; /* lr_usr */
> + uint64_t sp_el0; /* sp_usr */
> + uint64_t sp_el1; /* sp_svc */
> + uint32_t spsr_el1; /* spsr_svc */
> + uint64_t fp;
> + uint64_t pc;
> + uint32_t cpsr;
> + uint64_t ttbr0;
> + uint64_t ttbr1;
> +};
> +
> /*
> * mem_access flag definitions
> *
> @@ -254,6 +280,7 @@ typedef struct vm_event_st {
> union {
> union {
> struct vm_event_regs_x86 x86;
> + struct vm_event_regs_arm arm;
> } regs;
>
> struct vm_event_emul_read_data emul_read_data;
>
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] vm_event: Implement ARM SMC events
2016-05-03 11:31 ` Julien Grall
@ 2016-05-03 18:48 ` Tamas K Lengyel
2016-05-04 10:31 ` Julien Grall
0 siblings, 1 reply; 28+ messages in thread
From: Tamas K Lengyel @ 2016-05-03 18:48 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, Keir Fraser, Razvan Cojocaru, Stefano Stabellini,
Andrew Cooper, Ian Jackson, Jan Beulich, Xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 14130 bytes --]
On Tue, May 3, 2016 at 5:31 AM, Julien Grall <julien.grall@arm.com> 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 <tamas@tklengyel.com>
>> ---
>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> 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 0000000..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(&req, regs, curr->domain);
>> +
>> + return vm_event_monitor_traps(curr, 1, &req);
>> +}
>> +
>> +/*
>> + * 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 <asm/mmio.h>
>> #include <asm/cpufeature.h>
>> #include <asm/flushtlb.h>
>> +#include <asm/monitor.h>
>>
>> #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 ( 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 0000000..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 (tamas@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 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 <xen/sched.h>
>> +#include <asm/vm_event.h>
>> +
>> +void vm_event_fill_regs(vm_event_request_t *req,
>> + const struct cpu_user_regs *regs,
>> + struct domain *d)
>> +{
>> + if ( is_32bit_domain(d) )
>> + {
>> + req->data.regs.arm.x0 = regs->r0;
>> + req->data.regs.arm.x1 = regs->r1;
>> + req->data.regs.arm.x2 = regs->r2;
>> + req->data.regs.arm.x3 = regs->r3;
>> + req->data.regs.arm.x4 = regs->r4;
>> + req->data.regs.arm.x5 = regs->r5;
>> + req->data.regs.arm.x6 = regs->r6;
>> + req->data.regs.arm.x7 = regs->r7;
>> + req->data.regs.arm.x8 = regs->r8;
>> + req->data.regs.arm.x9 = regs->r9;
>> + req->data.regs.arm.x10 = regs->r10;
>> + req->data.regs.arm.pc = regs->pc32;
>> + req->data.regs.arm.sp_el0 = regs->sp_usr;
>> + req->data.regs.arm.sp_el1 = regs->sp_svc;
>> + }
>> +#ifdef CONFIG_ARM_64
>>
> Why
>
>> + else
>> + {
>> + req->data.regs.arm.x0 = regs->x0;
>> + req->data.regs.arm.x1 = regs->x1;
>> + req->data.regs.arm.x2 = regs->x2;
>> + req->data.regs.arm.x3 = regs->x3;
>> + req->data.regs.arm.x4 = regs->x4;
>> + req->data.regs.arm.x5 = regs->x5;
>> + req->data.regs.arm.x6 = regs->x6;
>> + req->data.regs.arm.x7 = regs->x7;
>> + req->data.regs.arm.x8 = regs->x8;
>> + req->data.regs.arm.x9 = regs->x9;
>> + req->data.regs.arm.x10 = regs->x10;
>>
>
> AArch64 provides 31 generate-purpose registers. Although, x29 and x30 are
> respectively used for fp and lr.
For vm_event it's not necessary to get all registers, rather it's just a
handful of selection that may be especially "useful" for introspection.
It's also important not to fill up the vm_event monitor ring with huge
request/response structs so even on x86 we only have a subset of the
registers. As right now there are no applications for aarch64, it's only a
guess of what registers would be "useful" and may be adjusted in future
versions as we start to have applications using this.
>
>
> + req->data.regs.arm.pc = regs->pc;
>> + req->data.regs.arm.sp_el0 = regs->sp_el0;
>> + req->data.regs.arm.sp_el1 = regs->sp_el1;
>> + }
>> +#endif
>> + req->data.regs.arm.fp = regs->fp;
>> + req->data.regs.arm.lr = regs->lr;
>> + req->data.regs.arm.cpsr = regs->cpsr;
>> + req->data.regs.arm.spsr_el1 = regs->spsr_svc;
>> + req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
>> + req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
>> +}
>> +
>> +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
>> +{
>> + struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs;
>> +
>> + if ( is_32bit_domain(v->domain) )
>> + {
>> + regs->r0 = rsp->data.regs.arm.x0;
>> + regs->r1 = rsp->data.regs.arm.x1;
>> + regs->r2 = rsp->data.regs.arm.x2;
>> + regs->r3 = rsp->data.regs.arm.x3;
>> + regs->r4 = rsp->data.regs.arm.x4;
>> + regs->r5 = rsp->data.regs.arm.x5;
>> + regs->r6 = rsp->data.regs.arm.x6;
>> + regs->r7 = rsp->data.regs.arm.x7;
>> + regs->r8 = rsp->data.regs.arm.x8;
>> + regs->r9 = rsp->data.regs.arm.x9;
>> + regs->r10 = rsp->data.regs.arm.x10;
>> + regs->pc32 = rsp->data.regs.arm.pc;
>> + regs->sp_usr = rsp->data.regs.arm.sp_el0;
>> + regs->sp_svc = rsp->data.regs.arm.sp_el1;
>> + }
>> +#ifdef CONFIG_ARM_64
>> + else
>> + {
>> + regs->x0 = rsp->data.regs.arm.x0;
>> + regs->x1 = rsp->data.regs.arm.x1;
>> + regs->x2 = rsp->data.regs.arm.x2;
>> + regs->x3 = rsp->data.regs.arm.x3;
>> + regs->x4 = rsp->data.regs.arm.x4;
>> + regs->x5 = rsp->data.regs.arm.x5;
>> + regs->x6 = rsp->data.regs.arm.x6;
>> + regs->x7 = rsp->data.regs.arm.x7;
>> + regs->x8 = rsp->data.regs.arm.x8;
>> + regs->x9 = rsp->data.regs.arm.x9;
>> + regs->x10 = rsp->data.regs.arm.x10;
>> + regs->pc = rsp->data.regs.arm.pc;
>> + regs->sp_el0 = rsp->data.regs.arm.sp_el0;
>> + regs->sp_el1 = rsp->data.regs.arm.sp_el1;
>> + }
>> +#endif
>> +
>> + regs->fp = rsp->data.regs.arm.fp;
>> + regs->lr = rsp->data.regs.arm.lr;
>> + regs->cpsr = rsp->data.regs.arm.cpsr;
>> + v->arch.ttbr0 = rsp->data.regs.arm.ttbr0;
>> + v->arch.ttbr1 = rsp->data.regs.arm.ttbr1;
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>>
>
> [...]
>
>
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>> index 2457698..35adce2 100644
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -1080,6 +1080,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
>> #define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP 2
>> #define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT 3
>> #define XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST 4
>> +#define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL 5
>>
>> struct xen_domctl_monitor_op {
>> uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
>> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
>> index 9270d52..f039207 100644
>> --- a/xen/include/public/vm_event.h
>> +++ b/xen/include/public/vm_event.h
>> @@ -119,6 +119,8 @@
>> #define VM_EVENT_REASON_SINGLESTEP 7
>> /* An event has been requested via HVMOP_guest_request_vm_event. */
>> #define VM_EVENT_REASON_GUEST_REQUEST 8
>> +/* Privileged call executed (e.g. SMC) */
>> +#define VM_EVENT_REASON_PRIVILEGED_CALL 9
>>
>> /* Supported values for the vm_event_write_ctrlreg index. */
>> #define VM_EVENT_X86_CR0 0
>> @@ -166,6 +168,30 @@ struct vm_event_regs_x86 {
>> uint32_t _pad;
>> };
>>
>> +struct vm_event_regs_arm {
>> + /* Aarch64 Aarch32 */
>> + uint64_t x0; /* r0_usr */
>> + uint64_t x1; /* r1_usr */
>> + uint64_t x2; /* r2_usr */
>> + uint64_t x3; /* r3_usr */
>> + uint64_t x4; /* r4_usr */
>> + uint64_t x5; /* r5_usr */
>> + uint64_t x6; /* r6_usr */
>> + uint64_t x7; /* r7_usr */
>> + uint64_t x8; /* r8_usr */
>> + uint64_t x9; /* r9_usr */
>> + uint64_t x10; /* r10_usr */
>>
>
> I would introduce an union to let the choice to the userspace to deal only
> with AArch32 registers. See vcpu_guest_core_regs.
>
Sure.
>
> + uint64_t lr; /* lr_usr */
>> + uint64_t sp_el0; /* sp_usr */
>> + uint64_t sp_el1; /* sp_svc */
>> + uint32_t spsr_el1; /* spsr_svc */
>> + uint64_t fp;
>> + uint64_t pc;
>> + uint32_t cpsr;
>> + uint64_t ttbr0;
>> + uint64_t ttbr1;
>> +};
>> +
>> /*
>> * mem_access flag definitions
>> *
>> @@ -254,6 +280,7 @@ typedef struct vm_event_st {
>> union {
>> union {
>> struct vm_event_regs_x86 x86;
>> + struct vm_event_regs_arm arm;
>> } regs;
>>
>> struct vm_event_emul_read_data emul_read_data;
>>
>>
> Regards,
>
> --
> Julien Grall
>
Thanks,
Tamas
[-- Attachment #1.2: Type: text/html, Size: 20932 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/5] x86/hvm: Rename hvm/event to hvm/monitor
2016-04-29 18:07 ` [PATCH v2 3/5] x86/hvm: Rename hvm/event to hvm/monitor Tamas K Lengyel
@ 2016-05-04 1:13 ` Tian, Kevin
0 siblings, 0 replies; 28+ messages in thread
From: Tian, Kevin @ 2016-05-04 1:13 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: Nakajima, Jun, Andrew Cooper, Razvan Cojocaru, Jan Beulich
> From: Tamas K Lengyel [mailto:tamas@tklengyel.com]
> Sent: Saturday, April 30, 2016 2:08 AM
>
> Mechanical renaming to better describe that the code in hvm/event is part of
> the monitor subsystem.
>
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] vm_event: Implement ARM SMC events
2016-05-03 18:48 ` Tamas K Lengyel
@ 2016-05-04 10:31 ` Julien Grall
[not found] ` <CABfawhmwVQyYeGYMxKTb1zMUkyOSutMwm4bkTxoeNaFTmGuyXA@mail.gmail.com>
0 siblings, 1 reply; 28+ messages in thread
From: Julien Grall @ 2016-05-04 10:31 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Wei Liu, Keir Fraser, Razvan Cojocaru, Stefano Stabellini,
Andrew Cooper, Ian Jackson, Jan Beulich, Xen-devel
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 <julien.grall@arm.com
> <mailto:julien.grall@arm.com>> 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 <tamas@tklengyel.com
> <mailto:tamas@tklengyel.com>>
> ---
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com
> <mailto:rcojocaru@bitdefender.com>>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com
> <mailto:ian.jackson@eu.citrix.com>>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com
> <mailto:stefano.stabellini@eu.citrix.com>>
> Cc: Wei Liu <wei.liu2@citrix.com <mailto:wei.liu2@citrix.com>>
> Cc: Julien Grall <julien.grall@arm.com
> <mailto:julien.grall@arm.com>>
> Cc: Keir Fraser <keir@xen.org <mailto:keir@xen.org>>
> Cc: Jan Beulich <jbeulich@suse.com <mailto:jbeulich@suse.com>>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com
> <mailto:andrew.cooper3@citrix.com>>
>
> 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 exception SMC64 can only
be taken from an AArch64 state, and SMC32 from an AArch32 State.
[...]
> AArch64 provides 31 generate-purpose registers. Although, x29 and
> x30 are respectively used for fp and lr.
>
>
> For vm_event it's not necessary to get all registers, rather it's just a
> handful of selection that may be especially "useful" for introspection.
How did you decide that only the first to xN registers are useful? It
would be valid to have an SMC call using x20 for an arguments.
Similarly, the hypercall convention for AArch64 makes use of x16 which
is not exposed to the vm event subsystem.
> It's also important not to fill up the vm_event monitor ring with huge
> request/response structs so even on x86 we only have a subset of the
> registers. As right now there are no applications for aarch64, it's only
> a guess of what registers would be "useful" and may be adjusted in
> future versions as we start to have applications using this.
Guessing the set of useful registers is usually not a good idea (see why
above).
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] vm_event: Implement ARM SMC events
[not found] ` <CABfawhmVUyqcuoxitKrXxcg93yLY4e8H7S1A_GEFbX3+Vb-hrA@mail.gmail.com>
@ 2016-05-04 12:42 ` Tamas K Lengyel
2016-05-04 13:26 ` Julien Grall
0 siblings, 1 reply; 28+ messages in thread
From: Tamas K Lengyel @ 2016-05-04 12:42 UTC (permalink / raw)
To: Julien Grall
Cc: Keir Fraser, Razvan Cojocaru, Stefano Stabellini, Andrew Cooper,
Ian Jackson, Jan Beulich, Xen-devel, wei.liu2
[-- Attachment #1.1: Type: text/plain, Size: 7161 bytes --]
> On 03/05/2016 19:48, Tamas K Lengyel wrote:
>>
>>
>>
>> On Tue, May 3, 2016 at 5:31 AM, Julien Grall <julien.grall@arm.com
>> <mailto:julien.grall@arm.com>> 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 <tamas@tklengyel.com
>> <mailto:tamas@tklengyel.com>>
>>
>> ---
>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com
>> <mailto:rcojocaru@bitdefender.com>>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com
>> <mailto:ian.jackson@eu.citrix.com>>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com
>> <mailto:stefano.stabellini@eu.citrix.com>>
>> Cc: Wei Liu <wei.liu2@citrix.com <mailto:wei.liu2@citrix.com>>
>> Cc: Julien Grall <julien.grall@arm.com
>> <mailto:julien.grall@arm.com>>
>> Cc: Keir Fraser <keir@xen.org <mailto:keir@xen.org>>
>> Cc: Jan Beulich <jbeulich@suse.com <mailto:jbeulich@suse.com>>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com
>> <mailto:andrew.cooper3@citrix.com>>
>>
>>
>> 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
>> 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 exception SMC64 can only
be taken from an AArch64 state, and SMC32 from an AArch32 State.
Ok, got it.
>
> [...]
>
>
>> AArch64 provides 31 generate-purpose registers. Although, x29 and
>> x30 are respectively used for fp and lr.
>>
>>
>> For vm_event it's not necessary to get all registers, rather it's just a
>> handful of selection that may be especially "useful" for introspection.
>
>
> How did you decide that only the first to xN registers are useful? It
would be valid to have an SMC call using x20 for an arguments.
>
> Similarly, the hypercall convention for AArch64 makes use of x16 which is
not exposed to the vm event subsystem.
Certainly, as I said, if a future application needs other registers to be
sent here and can justify it, this can be adjusted.
>
>
>> It's also important not to fill up the vm_event monitor ring with huge
>> request/response structs so even on x86 we only have a subset of the
>> registers. As right now there are no applications for aarch64, it's only
>> a guess of what registers would be "useful" and may be adjusted in
>> future versions as we start to have applications using this.
>
>
> Guessing the set of useful registers is usually not a good idea (see why
above).
>
Remember, the subscriber can always get/set the full set of registers when
it needs to, so completeness here is not critical. You are missing the
point that the space on the ring is limited and it can fill up fast when
the full vCPU context is pushed on it for all events. Right now the x86
side is still larger so we have some room for additional registers to be
sent when users of this system have a better view into what they find
important. Which is IMHO not now.
Thanks,
Tamas
[-- Attachment #1.2: Type: text/html, Size: 10479 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] vm_event: Implement ARM SMC events
2016-05-04 12:42 ` Tamas K Lengyel
@ 2016-05-04 13:26 ` Julien Grall
2016-05-04 13:30 ` Razvan Cojocaru
0 siblings, 1 reply; 28+ messages in thread
From: Julien Grall @ 2016-05-04 13:26 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Stefano Stabellini, Keir Fraser, Razvan Cojocaru,
Stefano Stabellini, Andrew Cooper, Ian Jackson, Jan Beulich,
Xen-devel, wei.liu2
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 <julien.grall@arm.com
> <mailto:julien.grall@arm.com>
> >> <mailto:julien.grall@arm.com <mailto:julien.grall@arm.com>>> 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 <tamas@tklengyel.com
> <mailto:tamas@tklengyel.com>
> >> <mailto:tamas@tklengyel.com <mailto:tamas@tklengyel.com>>>
> >>
> >> ---
> >> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com
> <mailto:rcojocaru@bitdefender.com>
> >> <mailto:rcojocaru@bitdefender.com
> <mailto:rcojocaru@bitdefender.com>>>
> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com
> <mailto:ian.jackson@eu.citrix.com>
> >> <mailto:ian.jackson@eu.citrix.com
> <mailto:ian.jackson@eu.citrix.com>>>
> >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com
> <mailto:stefano.stabellini@eu.citrix.com>
> >> <mailto:stefano.stabellini@eu.citrix.com
> <mailto:stefano.stabellini@eu.citrix.com>>>
> >> Cc: Wei Liu <wei.liu2@citrix.com
> <mailto:wei.liu2@citrix.com> <mailto:wei.liu2@citrix.com
> <mailto:wei.liu2@citrix.com>>>
> >> Cc: Julien Grall <julien.grall@arm.com
> <mailto:julien.grall@arm.com>
> >> <mailto:julien.grall@arm.com <mailto:julien.grall@arm.com>>>
> >> Cc: Keir Fraser <keir@xen.org <mailto:keir@xen.org>
> <mailto:keir@xen.org <mailto:keir@xen.org>>>
> >> Cc: Jan Beulich <jbeulich@suse.com
> <mailto:jbeulich@suse.com> <mailto:jbeulich@suse.com
> <mailto:jbeulich@suse.com>>>
> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com
> <mailto:andrew.cooper3@citrix.com>
> >> <mailto:andrew.cooper3@citrix.com
> <mailto:andrew.cooper3@citrix.com>>>
> >>
> >>
> >> 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 ARM structure today.
> >
> > [...]
> >
> >
> >> + 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.
Whilst I agree that we should use the same convention across all the
architectures, nothing prevents you to update the x86 code and use the
new define.
It does not make much sense to introduce code on ARM that may not be
clearer just because x86 did it.
Anyway, Stefano may have a different view on this.
[...]
> >
> > [...]
> >
> >
> >> AArch64 provides 31 generate-purpose registers. Although, x29 and
> >> x30 are respectively used for fp and lr.
> >>
> >>
> >> For vm_event it's not necessary to get all registers, rather it's just a
> >> handful of selection that may be especially "useful" for introspection.
> >
> >
> > How did you decide that only the first to xN registers are useful? It
> would be valid to have an SMC call using x20 for an arguments.
> >
> > Similarly, the hypercall convention for AArch64 makes use of x16
> which is not exposed to the vm event subsystem.
>
> Certainly, as I said, if a future application needs other registers to
> be sent here and can justify it, this can be adjusted.
>
> >
> >
> >> It's also important not to fill up the vm_event monitor ring with huge
> >> request/response structs so even on x86 we only have a subset of the
> >> registers. As right now there are no applications for aarch64, it's only
> >> a guess of what registers would be "useful" and may be adjusted in
> >> future versions as we start to have applications using this.
> >
> >
> > Guessing the set of useful registers is usually not a good idea (see
> why above).
> >
>
> Remember, the subscriber can always get/set the full set of registers
> when it needs to, so completeness here is not critical. You are missing
> the point that the space on the ring is limited and it can fill up fast
> when the full vCPU context is pushed on it for all events. Right now the
> x86 side is still larger so we have some room for additional registers
> to be sent when users of this system have a better view into what they
> find important. Which is IMHO not now.
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?
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] vm_event: Implement ARM SMC events
2016-05-04 13:26 ` Julien Grall
@ 2016-05-04 13:30 ` Razvan Cojocaru
2016-05-04 14:03 ` Julien Grall
0 siblings, 1 reply; 28+ messages in thread
From: Razvan Cojocaru @ 2016-05-04 13:30 UTC (permalink / raw)
To: Julien Grall, Tamas K Lengyel
Cc: Stefano Stabellini, Keir Fraser, Stefano Stabellini,
Andrew Cooper, Ian Jackson, Jan Beulich, Xen-devel, wei.liu2
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] vm_event: Implement ARM SMC events
2016-05-04 13:30 ` Razvan Cojocaru
@ 2016-05-04 14:03 ` Julien Grall
2016-05-04 14:08 ` Tamas K Lengyel
0 siblings, 1 reply; 28+ messages in thread
From: Julien Grall @ 2016-05-04 14:03 UTC (permalink / raw)
To: Razvan Cojocaru, Tamas K Lengyel
Cc: Stefano Stabellini, Keir Fraser, Stefano Stabellini,
Andrew Cooper, Ian Jackson, Jan Beulich, Xen-devel, wei.liu2
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/5] vm_event: Implement ARM SMC events
2016-05-04 14:03 ` Julien Grall
@ 2016-05-04 14:08 ` Tamas K Lengyel
0 siblings, 0 replies; 28+ messages in thread
From: Tamas K Lengyel @ 2016-05-04 14:08 UTC (permalink / raw)
To: Julien Grall
Cc: wei.liu2, Stefano Stabellini, Keir Fraser, Razvan Cojocaru,
Stefano Stabellini, Andrew Cooper, Ian Jackson, Jan Beulich,
Xen-devel
On Wed, May 4, 2016 at 8:03 AM, Julien Grall <julien.grall@arm.com> 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
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2016-05-04 14:08 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-29 18:07 [PATCH v2 1/5] monitor: Rename vm_event_monitor_get_capabilities Tamas K Lengyel
2016-04-29 18:07 ` [PATCH v2 2/5] vm_event: Implement ARM SMC events Tamas K Lengyel
2016-04-29 20:07 ` Razvan Cojocaru
2016-04-29 20:12 ` Tamas K Lengyel
2016-04-29 20:27 ` Tamas K Lengyel
2016-04-29 20:32 ` Razvan Cojocaru
2016-05-02 10:35 ` Wei Liu
2016-05-03 11:31 ` Julien Grall
2016-05-03 18:48 ` Tamas K Lengyel
2016-05-04 10:31 ` Julien Grall
[not found] ` <CABfawhmwVQyYeGYMxKTb1zMUkyOSutMwm4bkTxoeNaFTmGuyXA@mail.gmail.com>
[not found] ` <CABfawhmVUyqcuoxitKrXxcg93yLY4e8H7S1A_GEFbX3+Vb-hrA@mail.gmail.com>
2016-05-04 12:42 ` Tamas K Lengyel
2016-05-04 13:26 ` Julien Grall
2016-05-04 13:30 ` Razvan Cojocaru
2016-05-04 14:03 ` Julien Grall
2016-05-04 14:08 ` Tamas K Lengyel
2016-04-29 18:07 ` [PATCH v2 3/5] x86/hvm: Rename hvm/event to hvm/monitor Tamas K Lengyel
2016-05-04 1:13 ` Tian, Kevin
2016-04-29 18:07 ` [PATCH v2 4/5] x86/hvm: Add debug exception vm_events Tamas K Lengyel
2016-05-02 13:12 ` Jan Beulich
2016-05-02 15:35 ` Tamas K Lengyel
2016-05-02 15:40 ` Jan Beulich
2016-05-02 15:45 ` Tamas K Lengyel
2016-04-29 18:07 ` [PATCH v2 5/5] MAINTAINERS: Update monitor/vm_event covered code Tamas K Lengyel
2016-05-02 13:00 ` Jan Beulich
2016-05-03 8:18 ` Jan Beulich
2016-05-03 8:20 ` Razvan Cojocaru
2016-05-03 8:26 ` Jan Beulich
2016-05-03 9:09 ` Wei Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).