|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4] vm_event: Implement ARM SMC events
On Wed, Sep 28, 2016 at 5:58 PM, Stefano Stabellini
<sstabellini@xxxxxxxxxx> wrote:
> On Fri, 16 Sep 2016, 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.
>>
>> The intended use-case for this feature is for a monitor application to be
>> able
>> insert tap-points into the domU kernel-code. For this task only unconditional
>> SMC instruction should be used.
>>
>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
>> Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
>> Acked-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>> ---
>> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>>
>> v4: Style fixes
>>
>> Note: previous discussion around this patch proposed filtering SMCs with
>> failed
>> condition checks. As that patch is yet-to-be implemented and the 4.8
>> code-freeze rapidly approaching I would like this patch to get
>> included.
>> In this patch a proper warning is placed in the public header for
>> potential users not to rely on SMCs with failed condition checks being
>> trapped. As the intended use-case for this feature doesn't use
>> conditional SMCs this warning should be sufficient. Hardware that does
>> issue events for SMCs with failed condition checks doesn't pose a
>> problem
>> for a monitor application in any way. The xen-access test tool
>> illustrates
>> how SMCs issued by the guest can be safely handled for all cases.
>>
>> tools/libxc/include/xenctrl.h | 2 +
>> tools/libxc/xc_monitor.c | 14 +++++++
>> tools/tests/xen-access/xen-access.c | 32 ++++++++++++++-
>> xen/arch/arm/Makefile | 1 +
>> xen/arch/arm/monitor.c | 80
>> +++++++++++++++++++++++++++++++++++++
>> xen/arch/arm/traps.c | 16 +++++++-
>> xen/include/asm-arm/domain.h | 5 +++
>> xen/include/asm-arm/monitor.h | 18 +++------
>> xen/include/public/domctl.h | 1 +
>> xen/include/public/vm_event.h | 7 ++++
>> 10 files changed, 160 insertions(+), 16 deletions(-)
>> create mode 100644 xen/arch/arm/monitor.c
>>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index 560ce7b..eb53172 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -2168,6 +2168,8 @@ int xc_monitor_guest_request(xc_interface *xch,
>> domid_t domain_id,
>> int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
>> bool enable, bool sync);
>> int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable);
>> +int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
>> + bool enable);
>> /**
>> * This function enables / disables emulation for each REP for a
>> * REP-compatible instruction.
>> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
>> index 4298813..15a7c32 100644
>> --- a/tools/libxc/xc_monitor.c
>> +++ b/tools/libxc/xc_monitor.c
>> @@ -185,6 +185,20 @@ int xc_monitor_cpuid(xc_interface *xch, domid_t
>> domain_id, bool enable)
>> return do_domctl(xch, &domctl);
>> }
>>
>> +int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
>> + bool enable)
>> +{
>> + DECLARE_DOMCTL;
>> +
>> + domctl.cmd = XEN_DOMCTL_monitor_op;
>> + domctl.domain = domain_id;
>> + domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
>> + : XEN_DOMCTL_MONITOR_OP_DISABLE;
>> + domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL;
>> +
>> + return do_domctl(xch, &domctl);
>> +}
>> +
>> /*
>> * Local variables:
>> * mode: C
>> diff --git a/tools/tests/xen-access/xen-access.c
>> b/tools/tests/xen-access/xen-access.c
>> index ed18c71..cadeae1 100644
>> --- a/tools/tests/xen-access/xen-access.c
>> +++ b/tools/tests/xen-access/xen-access.c
>> @@ -338,6 +338,8 @@ void usage(char* progname)
>> fprintf(stderr, "Usage: %s [-m] <domain_id> write|exec", progname);
>> #if defined(__i386__) || defined(__x86_64__)
>> fprintf(stderr,
>> "|breakpoint|altp2m_write|altp2m_exec|debug|cpuid");
>> +#elif defined(__arm__) || defined(__aarch64__)
>> + fprintf(stderr, "|privcall");
>> #endif
>> fprintf(stderr,
>> "\n"
>> @@ -362,6 +364,7 @@ int main(int argc, char *argv[])
>> int required = 0;
>> int breakpoint = 0;
>> int shutting_down = 0;
>> + int privcall = 0;
>> int altp2m = 0;
>> int debug = 0;
>> int cpuid = 0;
>> @@ -431,6 +434,11 @@ int main(int argc, char *argv[])
>> {
>> cpuid = 1;
>> }
>> +#elif defined(__arm__) || defined(__aarch64__)
>> + else if ( !strcmp(argv[0], "privcall") )
>> + {
>> + privcall = 1;
>> + }
>> #endif
>> else
>> {
>> @@ -563,6 +571,16 @@ int main(int argc, char *argv[])
>> }
>> }
>>
>> + if ( privcall )
>> + {
>> + rc = xc_monitor_privileged_call(xch, domain_id, 1);
>> + if ( rc < 0 )
>> + {
>> + ERROR("Error %d setting privileged call trapping with
>> vm_event\n", rc);
>> + goto exit;
>> + }
>> + }
>> +
>> /* Wait for access */
>> for (;;)
>> {
>> @@ -578,6 +596,9 @@ int main(int argc, char *argv[])
>> if ( cpuid )
>> rc = xc_monitor_cpuid(xch, domain_id, 0);
>>
>> + if ( privcall )
>> + rc = xc_monitor_privileged_call(xch, domain_id, 0);
>> +
>> if ( altp2m )
>> {
>> rc = xc_altp2m_switch_to_view( xch, domain_id, 0 );
>> @@ -678,7 +699,7 @@ int main(int argc, char *argv[])
>> rsp.u.mem_access = req.u.mem_access;
>> break;
>> case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
>> - printf("Breakpoint: rip=%016"PRIx64", gfn=%"PRIx64" (vcpu
>> %d)\n",
>> + printf("Breakpoint: rip=%016"PRIx64" gfn=%"PRIx64" (vcpu
>> %d)\n",
>> req.data.regs.x86.rip,
>> req.u.software_breakpoint.gfn,
>> req.vcpu_id);
>> @@ -695,6 +716,15 @@ int main(int argc, char *argv[])
>> continue;
>> }
>> break;
>> + case VM_EVENT_REASON_PRIVILEGED_CALL:
>> + printf("Privileged call: pc=%"PRIx64" (vcpu %d)\n",
>> + req.data.regs.arm.pc,
>> + req.vcpu_id);
>> +
>> + rsp.data.regs.arm = req.data.regs.arm;
>> + rsp.data.regs.arm.pc += 4;
>> + rsp.flags |= VM_EVENT_FLAG_SET_REGISTERS;
>> + break;
>> case VM_EVENT_REASON_SINGLESTEP:
>> printf("Singlestep: rip=%016"PRIx64", vcpu %d, altp2m %u\n",
>> req.data.regs.x86.rip,
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 64fdf41..b140d7e 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -25,6 +25,7 @@ obj-y += irq.o
>> obj-y += kernel.o
>> obj-$(CONFIG_LIVEPATCH) += livepatch.o
>> obj-y += mm.o
>> +obj-y += monitor.o
>> obj-y += p2m.o
>> obj-y += percpu.o
>> obj-y += platform.o
>> diff --git a/xen/arch/arm/monitor.c b/xen/arch/arm/monitor.c
>> new file mode 100644
>> index 0000000..1e8bdfc
>> --- /dev/null
>> +++ b/xen/arch/arm/monitor.c
>> @@ -0,0 +1,80 @@
>> +/*
>> + * arch/arm/monitor.c
>> + *
>> + * Arch-specific monitor_op domctl handler.
>> + *
>> + * Copyright (c) 2016 Tamas K Lengyel (tamas.lengyel@xxxxxxxxxxxx)
>> + *
>> + * 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/vm_event.h>
>> +#include <xen/monitor.h>
>> +#include <asm/monitor.h>
>> +#include <asm/vm_event.h>
>> +#include <public/vm_event.h>
>> +
>> +int arch_monitor_domctl_event(struct domain *d,
>> + struct xen_domctl_monitor_op *mop)
>> +{
>> + struct arch_domain *ad = &d->arch;
>> + bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
>> +
>> + switch ( mop->event )
>> + {
>> + case XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL:
>> + {
>> + bool_t old_status = ad->monitor.privileged_call_enabled;
>> +
>> + if ( unlikely(old_status == requested_status) )
>> + return -EEXIST;
>> +
>> + domain_pause(d);
>> + ad->monitor.privileged_call_enabled = requested_status;
>> + domain_unpause(d);
>> + break;
>> + }
>> +
>> + default:
>> + /*
>> + * Should not be reached unless arch_monitor_get_capabilities() is
>> + * not properly implemented.
>> + */
>> + ASSERT_UNREACHABLE();
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int monitor_smc(void)
>> +{
>> + struct vcpu *curr = current;
>> + vm_event_request_t req = {
>> + .reason = VM_EVENT_REASON_PRIVILEGED_CALL
>> + };
>> +
>> + if ( !curr->domain->arch.monitor.privileged_call_enabled )
>> + return 0;
>
> Isn't this check redundant? The only caller is do_trap_smc.
No, it's necessary as Xen traps SMCs regardless if we have the monitor
enabled or not. So we have to check if there is a listener to send
this event to when we trap one. If there isn't, this just makes it so
that we fall back to path that's the current default, injecting an
undefined instruction exception.
>
> Aside from that, it is OK for me.
>
>
>> + return 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 39a05fd..3555a28 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"
>> @@ -2527,6 +2528,17 @@ 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;
>> +
>> + if ( current->domain->arch.monitor.privileged_call_enabled )
>> + rc = monitor_smc();
>> +
>> + if ( rc != 1 )
>> + inject_undef_exception(regs, hsr);
>> +}
>> +
>> static void enter_hypervisor_head(struct cpu_user_regs *regs)
>> {
>> if ( guest_mode(regs) )
>> @@ -2602,7 +2614,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));
>> @@ -2635,7 +2647,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/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index 9452fcd..2d6fbb1 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -126,6 +126,11 @@ struct arch_domain
>> paddr_t efi_acpi_gpa;
>> paddr_t efi_acpi_len;
>> #endif
>> +
>> + /* Monitor options */
>> + struct {
>> + uint8_t privileged_call_enabled : 1;
>> + } monitor;
>> } __cacheline_aligned;
>>
>> struct arch_vcpu
>> diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h
>> index 4af707a..1c4fea3 100644
>> --- a/xen/include/asm-arm/monitor.h
>> +++ b/xen/include/asm-arm/monitor.h
>> @@ -32,19 +32,8 @@ int arch_monitor_domctl_op(struct domain *d, struct
>> xen_domctl_monitor_op *mop)
>> return -EOPNOTSUPP;
>> }
>>
>> -static inline
>> int arch_monitor_domctl_event(struct domain *d,
>> - struct xen_domctl_monitor_op *mop)
>> -{
>> - /*
>> - * No arch-specific monitor vm-events on ARM.
>> - *
>> - * Should not be reached unless arch_monitor_get_capabilities() is not
>> - * properly implemented.
>> - */
>> - ASSERT_UNREACHABLE();
>> - return -EOPNOTSUPP;
>> -}
>> + struct xen_domctl_monitor_op *mop);
>>
>> static inline
>> int arch_monitor_init_domain(struct domain *d)
>> @@ -63,9 +52,12 @@ static inline uint32_t
>> arch_monitor_get_capabilities(struct domain *d)
>> {
>> uint32_t capabilities = 0;
>>
>> - capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
>> + capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST |
>> + 1U << XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL);
>>
>> return capabilities;
>> }
>>
>> +int monitor_smc(void);
>> +
>> #endif /* __ASM_ARM_MONITOR_H__ */
>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>> index ddd3de4..177319d 100644
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -1085,6 +1085,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
>> #define XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST 4
>> #define XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION 5
>> #define XEN_DOMCTL_MONITOR_EVENT_CPUID 6
>> +#define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL 7
>>
>> 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 f756126..b251f68 100644
>> --- a/xen/include/public/vm_event.h
>> +++ b/xen/include/public/vm_event.h
>> @@ -124,6 +124,13 @@
>> #define VM_EVENT_REASON_DEBUG_EXCEPTION 9
>> /* CPUID executed */
>> #define VM_EVENT_REASON_CPUID 10
>> +/*
>> + * Privileged call executed (e.g. SMC).
>> + * Note: event may be generated even if SMC condition check fails on some
>> CPUs.
>> + * As this behavior is CPU-specific, users are advised to not rely on
>> it.
>> + * These kinds of events will be filtered out in future versions.
>> + */
>> +#define VM_EVENT_REASON_PRIVILEGED_CALL 11
>
> It is acceptable
>
>> /* Supported values for the vm_event_write_ctrlreg index. */
>> #define VM_EVENT_X86_CR0 0
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |