Xen project Mailing List

On Tue, May 3, 2016 at 5:31 AM, Julien Grall <julien.grall@xxxxxxx> 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@xxxxxxxxxxxxx>
---
Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxx>
Cc: Keir Fraser <keir@xxxxxxx>
Cc: Jan Beulich <jbeulich@xxxxxxxx>
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

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@xxxxxxxxxxxxx)
+ *
+ * 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

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