On Tue, May 3, 2016 at 5:31 AM, Julien Grall <julien.gr...@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 <ta...@tklengyel.com>
>> ---
>> Cc: Razvan Cojocaru <rcojoc...@bitdefender.com>
>> Cc: Ian Jackson <ian.jack...@eu.citrix.com>
>> Cc: Stefano Stabellini <stefano.stabell...@eu.citrix.com>
>> Cc: Wei Liu <wei.l...@citrix.com>
>> Cc: Julien Grall <julien.gr...@arm.com>
>> Cc: Keir Fraser <k...@xen.org>
>> Cc: Jan Beulich <jbeul...@suse.com>
>> Cc: Andrew Cooper <andrew.coop...@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 (ta...@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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
