Re: [PATCH] vm_event: Implement ARM SMC events

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Wei Liu <wei.liu2@citrix.com>,
	Razvan Cojocaru <rcojocaru@bitdefender.com>,
	Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	Jan Beulich <jbeulich@suse.com>,
	xen-devel@lists.xenproject.org, Keir Fraser <keir@xen.org>,
	Tamas K Lengyel <tklengyel@sec.in.tum.de>
Subject: Re: [PATCH] vm_event: Implement ARM SMC events
Date: Tue, 12 Apr 2016 10:55:21 -0400	[thread overview]
Message-ID: <20160412145521.GC7140@char.us.oracle.com> (raw)
In-Reply-To: <1460404042-31179-1-git-send-email-tamas@tklengyel.com>

On Mon, Apr 11, 2016 at 01:47:22PM -0600, Tamas K Lengyel wrote:
> From: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> 
> 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.
> 
> This patch will likely needs to be broken up into several smaller patches.
> Right now what this patch adds (and could be broken into smaller patches
> accordingly):
>     - Implement monitor_op domctl handler for SOFTWARE_BREAKPOINTs on ARM
>     - Implement vm_event register fill/set routines for ARM. This required
>         removing the function from common as the function prototype now
>         differs on the two archs.
>     - Sending notification as SOFTWARE_BREAKPOINT vm_event from the SMC trap
>         handlers.
>     - Extend the xen-access test tool to receive SMC notification and step
>         the PC manually in the reply.
> 
> I'm sending it as an RFC to gather feedback on what has been overlooked in this
> revision. This patch has been tested on a Cubietruck board and works fine,
> but would probably not work on 64-bit boards.

I only have some small nitpicking.
> +++ b/xen/arch/arm/traps.c
> @@ -41,6 +41,7 @@
>  #include <asm/mmio.h>
>  #include <asm/cpufeature.h>
>  #include <asm/flushtlb.h>
> +#include <asm/vm_event.h>
>  
>  #include "decode.h"
>  #include "vtimer.h"
> @@ -2449,6 +2450,21 @@ bad_data_abort:
>      inject_dabt_exception(regs, info.gva, hsr.len);
>  }
>  
> +static void do_trap_smc(struct cpu_user_regs *regs)
> +{
> +    int rc = 0;

Newline here
> +    if ( current->domain->arch.monitor.software_breakpoint_enabled )
> +    {
> +        rc = vm_event_smc(regs);
> +    }
> +
> +    if ( rc != 1 )
> +    {
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));

This differs a bit from below. Should there be an comment explaining why
we expect it be only in 32-bit mode?

> +        inject_undef32_exception(regs);

Below you do inject_undef64_exception?

Perhaps there should be an check if it is 32 or 64-bit?

> +    }
> +}
> +
>  static void enter_hypervisor_head(struct cpu_user_regs *regs)
>  {
>      if ( guest_mode(regs) )
> @@ -2524,7 +2540,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);
>          break;
>      case HSR_EC_HVC32:
>          GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> @@ -2557,7 +2573,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);

As in here.. Now it will call inject_undef32_exception. That can't
be healthy?

>          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..d997313
> --- /dev/null
> +++ b/xen/arch/arm/vm_event.c
> @@ -0,0 +1,95 @@
> +/*
> + * arch/arm/vm_event.c
> + *
> + * Architecture-specific vm_event handling routines
> + *
> + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)

2016?
Also .. shouldn't the company be attributed as well? I see BitDefender
on some of them so not sure what hte relationship you have with them.

> + *
> + * 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>
> +
> +static inline
> +void vm_event_fill_regs(vm_event_request_t *req,
> +                        const struct cpu_user_regs *regs)
> +{
> +    req->data.regs.arm.r0 = regs->r0;
> +    req->data.regs.arm.r1 = regs->r1;
> +    req->data.regs.arm.r2 = regs->r2;
> +    req->data.regs.arm.r3 = regs->r3;
> +    req->data.regs.arm.r4 = regs->r4;
> +    req->data.regs.arm.r5 = regs->r5;
> +    req->data.regs.arm.r6 = regs->r6;
> +    req->data.regs.arm.r7 = regs->r7;
> +    req->data.regs.arm.r8 = regs->r8;
> +    req->data.regs.arm.r9 = regs->r9;
> +    req->data.regs.arm.r10 = regs->r10;
> +    req->data.regs.arm.r11 = regs->r11;
> +    req->data.regs.arm.r12 = regs->r12;
> +    req->data.regs.arm.sp = regs->sp;
> +    req->data.regs.arm.lr = regs->lr;
> +    req->data.regs.arm.pc = regs->pc;
> +    req->data.regs.arm.cpsr = regs->cpsr;
> +    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)
> +{
> +    v->arch.cpu_info->guest_cpu_user_regs.r0 = rsp->data.regs.arm.r0;
> +    v->arch.cpu_info->guest_cpu_user_regs.r1 = rsp->data.regs.arm.r1;
> +    v->arch.cpu_info->guest_cpu_user_regs.r2 = rsp->data.regs.arm.r2;
> +    v->arch.cpu_info->guest_cpu_user_regs.r3 = rsp->data.regs.arm.r3;
> +    v->arch.cpu_info->guest_cpu_user_regs.r4 = rsp->data.regs.arm.r4;
> +    v->arch.cpu_info->guest_cpu_user_regs.r5 = rsp->data.regs.arm.r5;
> +    v->arch.cpu_info->guest_cpu_user_regs.r6 = rsp->data.regs.arm.r6;
> +    v->arch.cpu_info->guest_cpu_user_regs.r7 = rsp->data.regs.arm.r7;
> +    v->arch.cpu_info->guest_cpu_user_regs.r8 = rsp->data.regs.arm.r8;
> +    v->arch.cpu_info->guest_cpu_user_regs.r9 = rsp->data.regs.arm.r9;
> +    v->arch.cpu_info->guest_cpu_user_regs.r10 = rsp->data.regs.arm.r10;
> +    v->arch.cpu_info->guest_cpu_user_regs.r11 = rsp->data.regs.arm.r11;
> +    v->arch.cpu_info->guest_cpu_user_regs.r12 = rsp->data.regs.arm.r12;
> +    v->arch.cpu_info->guest_cpu_user_regs.sp = rsp->data.regs.arm.sp;
> +    v->arch.cpu_info->guest_cpu_user_regs.lr = rsp->data.regs.arm.lr;
> +    v->arch.cpu_info->guest_cpu_user_regs.pc = rsp->data.regs.arm.pc;
> +    v->arch.cpu_info->guest_cpu_user_regs.cpsr = rsp->data.regs.arm.cpsr;
> +    v->arch.ttbr0 = rsp->data.regs.arm.ttbr0;
> +    v->arch.ttbr1 = rsp->data.regs.arm.ttbr1;
> +}
> +
> +int vm_event_smc(const struct cpu_user_regs *regs) {
> +    struct vcpu *curr = current;
> +    struct arch_domain *ad = &curr->domain->arch;
> +    vm_event_request_t req = { 0 };
> +
> +    if ( !ad->monitor.software_breakpoint_enabled )
> +        return 0;
> +
> +    req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
> +    req.vcpu_id = curr->vcpu_id;
> +
> +    vm_event_fill_regs(&req, regs);
> +
> +    return vm_event_monitor_traps(curr, 1, &req);

Perhaps a comment right past 1 explaining what this mystical
1 value means?

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