| Message ID | 1453979874-20037-1-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2016-01-28 at 13:17 +0200, Corneliu ZUZU wrote: > This patch implements ARM support for guest-request vm-events. > The code has been ported from x86 side w/ minor adjustments. I've not looked at the patch yet, but if it only involves minor adjustments from the x86 side can some amount of it not be refactored into common code? Ian.
On 1/28/2016 1:23 PM, Ian Campbell wrote: > On Thu, 2016-01-28 at 13:17 +0200, Corneliu ZUZU wrote: >> This patch implements ARM support for guest-request vm-events. >> The code has been ported from x86 side w/ minor adjustments. > I've not looked at the patch yet, but if it only involves minor adjustments > from the x86 side can some amount of it not be refactored into common code? > > Ian. > At a first glance it seems to me that parts of monitor vm-events code could be moved to common. But it also seems that it would require a bit of effort and I'm not sure yet if the end result won't actually complicate implementation of monitor vm-events for other architectures in the future. Some of the monitor vm-events implemented are strictly architecture specific, e.g. VM_EVENT_REASON_MOV_TO_MSR will always be an x86-only vm-event, unless it is somehow generalized (maybe somehow merged w/ VM_EVENT_REASON_WRITE_CTRLREG?). But *most* of them indeed don't directly have this kind of specificity, so it would make sense to make most of the code common, if possible. To me personally this seems like a good idea and I'd be willing to give it a try, but as I said, it might require some other changes of the code, including x86 changes. I was about to release another patch after this one to implement control-register writes vm-events for ARM, but I anticipate that doing this move first will actually benefit my effort in that direction as well (I think the patch code will get to be cleaner). So, shall I try it? Thanks, Corneliu.
On Thu, 2016-01-28 at 14:36 +0200, CORNELIU ZUZU wrote: > On 1/28/2016 1:23 PM, Ian Campbell wrote: > > On Thu, 2016-01-28 at 13:17 +0200, Corneliu ZUZU wrote: > > > This patch implements ARM support for guest-request vm-events. > > > The code has been ported from x86 side w/ minor adjustments. > > I've not looked at the patch yet, but if it only involves minor > > adjustments > > from the x86 side can some amount of it not be refactored into common > > code? > > > > Ian. > > > > At a first glance it seems to me that parts of monitor vm-events code > could be moved to common. > But it also seems that it would require a bit of effort and I'm not sure > yet if the end result > won't actually complicate implementation of monitor vm-events for other > architectures in the future. > Some of the monitor vm-events implemented are strictly architecture > specific, > e.g. VM_EVENT_REASON_MOV_TO_MSR will always be an x86-only vm-event, unless > it is somehow generalized (maybe somehow merged w/ > VM_EVENT_REASON_WRITE_CTRLREG?). > But *most* of them indeed don't directly have this kind of specificity, > so it would make sense to make > most of the code common, if possible. That the sort of thing we would usually handle by having the common code call into a arch_foo() to decode any non-common options. > To me personally this seems like a good idea and I'd be willing to give > it a try, but as I said, > it might require some other changes of the code, including x86 changes. > I was about to release another patch after this one to implement > control-register writes > vm-events for ARM, but I anticipate that doing this move first will > actually benefit my effort in that > direction as well (I think the patch code will get to be cleaner). > > So, shall I try it? From my POV as an ARM maintainer I think this is the way to go. I've also copied the VM EVENT maintainers in case they object for some reason. I'd recommend always CCing Razvan and Tamas on this series, and considering adding any new ARM files to the entry in MAINTAINERS (assuming they don't object of course) Ian.
On 01/28/2016 02:45 PM, Ian Campbell wrote: > On Thu, 2016-01-28 at 14:36 +0200, CORNELIU ZUZU wrote: >> On 1/28/2016 1:23 PM, Ian Campbell wrote: >>> On Thu, 2016-01-28 at 13:17 +0200, Corneliu ZUZU wrote: >>>> This patch implements ARM support for guest-request vm-events. >>>> The code has been ported from x86 side w/ minor adjustments. >>> I've not looked at the patch yet, but if it only involves minor >>> adjustments >>> from the x86 side can some amount of it not be refactored into common >>> code? >>> >>> Ian. >>> >> >> At a first glance it seems to me that parts of monitor vm-events code >> could be moved to common. >> But it also seems that it would require a bit of effort and I'm not sure >> yet if the end result >> won't actually complicate implementation of monitor vm-events for other >> architectures in the future. >> Some of the monitor vm-events implemented are strictly architecture >> specific, >> e.g. VM_EVENT_REASON_MOV_TO_MSR will always be an x86-only vm-event, unless >> it is somehow generalized (maybe somehow merged w/ >> VM_EVENT_REASON_WRITE_CTRLREG?). >> But *most* of them indeed don't directly have this kind of specificity, >> so it would make sense to make >> most of the code common, if possible. > > That the sort of thing we would usually handle by having the common code > call into a arch_foo() to decode any non-common options. > >> To me personally this seems like a good idea and I'd be willing to give >> it a try, but as I said, >> it might require some other changes of the code, including x86 changes. >> I was about to release another patch after this one to implement >> control-register writes >> vm-events for ARM, but I anticipate that doing this move first will >> actually benefit my effort in that >> direction as well (I think the patch code will get to be cleaner). >> >> So, shall I try it? > > From my POV as an ARM maintainer I think this is the way to go. I've also > copied the VM EVENT maintainers in case they object for some reason. > > I'd recommend always CCing Razvan and Tamas on this series, and considering > adding any new ARM files to the entry in MAINTAINERS (assuming they don't > object of course) No objection from me on either factoring out the common code (where, and if possible) or adding the new files to MAINTAINERS. Thanks, Razvan
On 1/28/2016 2:45 PM, Ian Campbell wrote: > On Thu, 2016-01-28 at 14:36 +0200, CORNELIU ZUZU wrote: >> On 1/28/2016 1:23 PM, Ian Campbell wrote: >>> On Thu, 2016-01-28 at 13:17 +0200, Corneliu ZUZU wrote: >>>> This patch implements ARM support for guest-request vm-events. >>>> The code has been ported from x86 side w/ minor adjustments. >>> I've not looked at the patch yet, but if it only involves minor >>> adjustments >>> from the x86 side can some amount of it not be refactored into common >>> code? >>> >>> Ian. >>> >> At a first glance it seems to me that parts of monitor vm-events code >> could be moved to common. >> But it also seems that it would require a bit of effort and I'm not sure >> yet if the end result >> won't actually complicate implementation of monitor vm-events for other >> architectures in the future. >> Some of the monitor vm-events implemented are strictly architecture >> specific, >> e.g. VM_EVENT_REASON_MOV_TO_MSR will always be an x86-only vm-event, unless >> it is somehow generalized (maybe somehow merged w/ >> VM_EVENT_REASON_WRITE_CTRLREG?). >> But *most* of them indeed don't directly have this kind of specificity, >> so it would make sense to make >> most of the code common, if possible. > That the sort of thing we would usually handle by having the common code > call into a arch_foo() to decode any non-common options. > >> To me personally this seems like a good idea and I'd be willing to give >> it a try, but as I said, >> it might require some other changes of the code, including x86 changes. >> I was about to release another patch after this one to implement >> control-register writes >> vm-events for ARM, but I anticipate that doing this move first will >> actually benefit my effort in that >> direction as well (I think the patch code will get to be cleaner). >> >> So, shall I try it? > >From my POV as an ARM maintainer I think this is the way to go. I've also > copied the VM EVENT maintainers in case they object for some reason. > > I'd recommend always CCing Razvan and Tamas on this series, and considering > adding any new ARM files to the entry in MAINTAINERS (assuming they don't > object of course) > > Ian. > Ok, great. I'll try it tonight and post a patch series ASAP. Concerning the CC list, Razvan actually noticed me earlier that he and Tamas should have been on it for this patch, since it is vm-events related. He said the problem is w/ the MAINTAINERS file and he will update it. Corneliu.
On Thu, 2016-01-28 at 14:57 +0200, Corneliu ZUZU wrote: > > > Ok, great. I'll try it tonight and post a patch series ASAP. > Concerning the CC list, Razvan actually noticed me earlier that he and > Tamas should have > been on it for this patch, since it is vm-events related. He said the > problem is w/ the > MAINTAINERS file and he will update it. It's just that you touch all new files I think. I wonder if listing xen/arch/*/vm_event.c would do what we want. Ian.
On 01/28/2016 03:03 PM, Ian Campbell wrote: > On Thu, 2016-01-28 at 14:57 +0200, Corneliu ZUZU wrote: >> >> >> Ok, great. I'll try it tonight and post a patch series ASAP. >> Concerning the CC list, Razvan actually noticed me earlier that he and >> Tamas should have >> been on it for this patch, since it is vm-events related. He said the >> problem is w/ the >> MAINTAINERS file and he will update it. > > It's just that you touch all new files I think. > > I wonder if listing xen/arch/*/vm_event.c would do what we want. It worked for me, so if Tamas is onboard with this, then we can make this change. Thanks, Razvan
On Thu, Jan 28, 2016 at 7:10 AM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote: > On 01/28/2016 03:03 PM, Ian Campbell wrote: > > On Thu, 2016-01-28 at 14:57 +0200, Corneliu ZUZU wrote: > >> > >> > >> Ok, great. I'll try it tonight and post a patch series ASAP. > >> Concerning the CC list, Razvan actually noticed me earlier that he and > >> Tamas should have > >> been on it for this patch, since it is vm-events related. He said the > >> problem is w/ the > >> MAINTAINERS file and he will update it. > > > > It's just that you touch all new files I think. > > > > I wonder if listing xen/arch/*/vm_event.c would do what we want. > > It worked for me, so if Tamas is onboard with this, then we can make > this change. > > > Thanks, > Razvan > Sure, that works. Tamas
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 2f050f5..d3d8d7b 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -31,6 +31,8 @@ obj-y += smpboot.o obj-y += smp.o obj-y += shutdown.o obj-y += traps.o +obj-y += event.o +obj-y += monitor.o obj-y += vgic.o vgic-v2.o obj-$(CONFIG_ARM_64) += vgic-v3.o obj-y += vtimer.o diff --git a/xen/arch/arm/event.c b/xen/arch/arm/event.c new file mode 100644 index 0000000..e4bc25a --- /dev/null +++ b/xen/arch/arm/event.c @@ -0,0 +1,86 @@ +/* +* arch/arm/event.c +* +* Common hardware virtual machine event abstractions. +* Copy-pasted & adjusted from x86 counterpart. +* +* Copyright (c) 2004, Intel Corporation. +* Copyright (c) 2005, International Business Machines Corporation. +* Copyright (c) 2008, Citrix Systems, Inc. +* Copyright (c) 2016, Bitdefender S.R.L. +* +* This program is free software; you can redistribute it and/or modify it +* under the terms and conditions of the GNU General Public License, +* version 2, as published by the Free Software Foundation. +* +* This program is distributed in the hope 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/paging.h> +#include <asm/hvm/event.h> +#include <public/vm_event.h> + +static int hvm_event_traps(uint8_t sync, vm_event_request_t *req) +{ + int rc; + struct vcpu *curr = current; + struct domain *currd = curr->domain; + + rc = vm_event_claim_slot(currd, &currd->vm_event->monitor); + switch ( rc ) + { + case 0: + break; + case -ENOSYS: + /* + * If there was no ring to handle the event, then + * simply continue executing normally. + */ + return 1; + default: + return rc; + }; + + if ( sync ) + { + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; + vm_event_vcpu_pause(curr); + } + + vm_event_put_request(currd, &currd->vm_event->monitor, req); + + return 1; +} + +void hvm_event_guest_request(void) +{ + struct vcpu *curr = current; + struct arch_domain *currad = &curr->domain->arch; + + if ( currad->monitor.guest_request_enabled ) + { + vm_event_request_t req = { + .reason = VM_EVENT_REASON_GUEST_REQUEST, + .vcpu_id = curr->vcpu_id, + }; + + hvm_event_traps(currad->monitor.guest_request_sync, &req); + } +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c index 5fd0753..f314bd7 100644 --- a/xen/arch/arm/hvm.c +++ b/xen/arch/arm/hvm.c @@ -11,10 +11,10 @@ #include <public/hvm/params.h> #include <public/hvm/hvm_op.h> +#include <asm/hvm/event.h> #include <asm/hypercall.h> long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) - { long rc = 0; @@ -55,6 +55,13 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) break; } + case HVMOP_guest_request_vm_event: + if ( guest_handle_is_null(arg) ) + hvm_event_guest_request(); + else + rc = -EINVAL; + break; + default: { gdprintk(XENLOG_DEBUG, "HVMOP op=%lu: not implemented\n", op); diff --git a/xen/arch/arm/monitor.c b/xen/arch/arm/monitor.c new file mode 100644 index 0000000..22455f7 --- /dev/null +++ b/xen/arch/arm/monitor.c @@ -0,0 +1,123 @@ +/* + * arch/arm/monitor.c + * + * Architecture-specific monitor_op domctl handler. + * Copy-pasted & adjusted from x86 counterpart. + * + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) + * Copyright (c) 2016 Bitdefender S.R.L. + * + * 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, write to the + * Free Software Foundation, Inc., 59 Temple Place - Suite 330, + * Boston, MA 021110-1307, USA. + */ + +#include <xen/sched.h> +#include <asm/domain.h> +#include <asm/monitor.h> +#include <public/domctl.h> +#include <xsm/xsm.h> + +/* + * Sanity check whether option is already enabled/disabled + */ +static inline +int status_check(struct xen_domctl_monitor_op *mop, bool_t old_status) +{ + bool_t requested_status = (mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE); + + if ( old_status == requested_status ) + return -EEXIST; + + return 0; +} + +static inline uint32_t get_capabilities(struct domain *d) +{ + uint32_t capabilities = 0; + + capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST); + + return capabilities; +} + +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) +{ + int rc; + struct arch_domain *ad = &d->arch; + uint32_t capabilities = get_capabilities(d); + + rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event); + if ( rc ) + return rc; + + if ( mop->op == XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES ) + { + mop->event = capabilities; + return 0; + } + + /* + * Sanity check + */ + if ( mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE && + mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE ) + return -EOPNOTSUPP; + + /* Check if event type is available. */ + if ( !(capabilities & (1 << mop->event)) ) + return -EOPNOTSUPP; + + switch ( mop->event ) + { + case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST: + { + bool_t old_status = ad->monitor.guest_request_enabled; + + rc = status_check(mop, old_status); + if ( rc ) + return rc; + + ad->monitor.guest_request_sync = mop->u.guest_request.sync; + + domain_pause(d); + ad->monitor.guest_request_enabled = !old_status; + domain_unpause(d); + break; + } + + default: + /* + * Should not be reached unless get_capabilities() is not properly + * implemented. In that case, since reaching this point does not + * really break anything, don't crash the hypervisor, issue a + * warning instead of BUG(). + */ + printk(XENLOG_WARNING + "WARNING, BUG: get_capabilities() not properly" + "implemented.\n"); + + return -EOPNOTSUPP; + }; + + return 0; +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index aa7f283..cd5d285 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -124,6 +124,14 @@ struct arch_domain } vuart; unsigned int evtchn_irq; + + /* Monitor options */ + struct { + uint8_t guest_request_enabled : 1; + uint8_t guest_request_sync : 1; + uint8_t : 6; + } monitor; + } __cacheline_aligned; struct arch_vcpu diff --git a/xen/include/asm-arm/hvm/event.h b/xen/include/asm-arm/hvm/event.h new file mode 100644 index 0000000..246dfb8 --- /dev/null +++ b/xen/include/asm-arm/hvm/event.h @@ -0,0 +1,36 @@ +/* + * include/asm-arm/hvm/event.h: + * + * Copy-pasted & adjusted from x86 counterpart. + * + * Copyright (c) 2016, Bitdefender S.R.L. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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/>. + */ + +#ifndef __ASM_ARM_HVM_EVENT_H__ +#define __ASM_ARM_HVM_EVENT_H__ + +void hvm_event_guest_request(void); + +#endif /* __ASM_ARM_HVM_EVENT_H__ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h index a3a9703..9d1a530 100644 --- a/xen/include/asm-arm/monitor.h +++ b/xen/include/asm-arm/monitor.h @@ -24,10 +24,6 @@ #include <xen/sched.h> #include <public/domctl.h> -static inline -int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op) -{ - return -ENOSYS; -} +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); -#endif /* __ASM_X86_MONITOR_H__ */ +#endif /* __ASM_ARM_MONITOR_H__ */ diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h index 4d0fbf7..b468b89 100644 --- a/xen/include/asm-arm/vm_event.h +++ b/xen/include/asm-arm/vm_event.h @@ -25,14 +25,14 @@ static inline int vm_event_init_domain(struct domain *d) { - /* Not supported on ARM. */ + /* Nothing to do */ return 0; } static inline void vm_event_cleanup_domain(struct domain *d) { - /* Not supported on ARM. */ + memset(&d->arch.monitor, 0, sizeof(d->arch.monitor)); } static inline
This patch implements ARM support for guest-request vm-events. The code has been ported from x86 side w/ minor adjustments. Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> --- xen/arch/arm/Makefile | 2 + xen/arch/arm/event.c | 86 ++++++++++++++++++++++++++++ xen/arch/arm/hvm.c | 9 ++- xen/arch/arm/monitor.c | 123 ++++++++++++++++++++++++++++++++++++++++ xen/include/asm-arm/domain.h | 8 +++ xen/include/asm-arm/hvm/event.h | 36 ++++++++++++ xen/include/asm-arm/monitor.h | 8 +-- xen/include/asm-arm/vm_event.h | 4 +- 8 files changed, 267 insertions(+), 9 deletions(-) create mode 100644 xen/arch/arm/event.c create mode 100644 xen/arch/arm/monitor.c create mode 100644 xen/include/asm-arm/hvm/event.h