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

[Tamas K Lengyel <tamas@tklengyel.com>]

[-- 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