|
[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 6:10 PM, Stefano Stabellini
<sstabellini@xxxxxxxxxx> wrote:
> On Wed, 28 Sep 2016, Tamas K Lengyel wrote:
>> 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.
>
> Fair enough, but don't we have exactly the same check in do_trap_smc?
Oh, if that's what you mean than indeed, we seem to be doing this
check twice which is not necessary.. Nice catch =) I'll send another
revision soon.
>
>> >
>> > 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 |