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

[Julien Grall <julien.grall@arm.com>]

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