|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3] vm_event: Implement ARM SMC events
On Fri, Sep 16, 2016 at 7:54 AM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Tamas,
>
> On 15/09/2016 20:24, 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@xxxxxxxxxxxxx>
>> Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
>> ---
>> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>> Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>>
>> v3: Rebase on latest master
>>
>> 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.
>
>
> It is still in my todo list. But as I said last time, it is towards the
> bottom of it. I would have expected a bit of help from your side here rather
> than putting the fault on me.
Not putting a fault on anyone. I did take a look at it but there had
been enough unknowns for me that I would rather have you do it when
you get to it. I think this is just a minor issue that should not be a
roadblock for this patch. If it gets filtered at some point in the
future it will have no impact on applications that this interface is
intended for. For all else, a warning is in place not to rely on this
behavior being a "feature".
>
>> 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.
>
>
> This is nice, but how about passing the immediate to the event monitor? The
> SMC call is not meant to be a breakpoint instruction but a way to call the
> supervisor monitor (and potentially be emulated by the hypervisor).
I understand that was the original intention behind the instruction.
So when the emulation use-case becomes actual the immediate can be
included and passed along too, we would just bump the
VM_EVENT_INTERFACE_VERSION. As that was not my usecase it is beyond
scope for this patch. It can certainly be implemented at some later
time.
>
>
>> ---
>> 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 | 79
>> +++++++++++++++++++++++++++++++++++++
>> xen/arch/arm/traps.c | 15 ++++++-
>> 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, 158 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..6eefe0c 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=%"PRIx64" gfn=%"PRIx64" (vcpu
>> %d)\n",
>
>
> This change looks spurious.
>
>
>> 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..db05453
>> --- /dev/null
>> +++ b/xen/arch/arm/monitor.c
>> @@ -0,0 +1,79 @@
>> +/*
>> + * arch/x86/monitor.c
>
>
> This should be arch/arm/monitor.c
Yeap.
>
>
>> + *
>> + * 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) {
>
>
> Coding style.
Ack.
>
>
>> + 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;
>> +
>> + 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..cf58fd5 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,16 @@ 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;
>
>
> Missing blank line.
Ack.
>
>> + if ( current->domain->arch.monitor.privileged_call_enabled )
>> + rc = monitor_smc();
>> +
>> + if ( rc != 1 )
>> + inject_undef_exception(regs, hsr);
>
>
> It would be worth mentioning somewhere that you expect the monitor app
> skipping the instruction.
>
>
>> +}
>> +
>> static void enter_hypervisor_head(struct cpu_user_regs *regs)
>> {
>> if ( guest_mode(regs) )
>> @@ -2602,7 +2613,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 +2646,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..dc48e4c 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.
>
>
> Missing comma after CPU-specific.
Ack.
>
>> + * Such events are planned to be forcibly filtered out in future
>> versions.
>> + */
>> +#define VM_EVENT_REASON_PRIVILEGED_CALL 11
>>
>> /* Supported values for the vm_event_write_ctrlreg index. */
>> #define VM_EVENT_X86_CR0 0
>>
>
> Regards,
>
> --
> Julien Grall
Thanks!
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |