| Message ID | 1455824116-13783-1-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/18/2016 09:35 PM, Corneliu ZUZU wrote: > This patch adds ARM support for guest-request monitor vm-events. > > Summary of changes: > == Moved to common-side: > * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86 > arch_monitor_domctl_event to common monitor_domctl) > * hvm_event_guest_request, hvm_event_traps (also added target vcpu as param) > * guest-request bits from X86 'struct arch_domain' (to common 'struct domain') > == ARM implementations: > * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls > hvm_event_guest_request (as on X86) > * arch_monitor_get_capabilities: updated to reflect support for > XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST > * vm_event_init_domain (does nothing), vm_event_cleanup_domain > == Misc: > * hvm_event_fill_regs renamed to arch_hvm_event_fill_regs, no longer > X86-specific. ARM-side implementation of this function currently does > nothing, that will be added in a separate patch. We should probably take into account what happens with Tamas' "vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs" patch here. That patch already affects hvm_event_fill_regs(). Tamas'll probably chime in with more. Thanks, Razvan
On Thu, Feb 18, 2016 at 1:08 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote: > On 02/18/2016 09:35 PM, Corneliu ZUZU wrote: > > This patch adds ARM support for guest-request monitor vm-events. > > > > Summary of changes: > > == Moved to common-side: > > * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86 > > arch_monitor_domctl_event to common monitor_domctl) > > * hvm_event_guest_request, hvm_event_traps (also added target vcpu as > param) > > * guest-request bits from X86 'struct arch_domain' (to common 'struct > domain') > > == ARM implementations: > > * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls > > hvm_event_guest_request (as on X86) > > * arch_monitor_get_capabilities: updated to reflect support for > > XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST > > * vm_event_init_domain (does nothing), vm_event_cleanup_domain > > == Misc: > > * hvm_event_fill_regs renamed to arch_hvm_event_fill_regs, no longer > > X86-specific. ARM-side implementation of this function currently > does > > nothing, that will be added in a separate patch. > > We should probably take into account what happens with Tamas' "vm_event: > consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs" patch here. > That patch already affects hvm_event_fill_regs(). > Well it seems one of us will have to rebase depending which patch gets accepted & merged first. The conflict is minimal so it's not a major issue. If my patch gets merged first then just have to introduce the empty function in the ARM header with the new name. Tamas
On 2/18/2016 11:25 PM, Tamas K Lengyel wrote: > > > On Thu, Feb 18, 2016 at 1:08 PM, Razvan Cojocaru > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote: > > On 02/18/2016 09:35 PM, Corneliu ZUZU wrote: > > This patch adds ARM support for guest-request monitor vm-events. > > > > Summary of changes: > > == Moved to common-side: > > * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86 > > arch_monitor_domctl_event to common monitor_domctl) > > * hvm_event_guest_request, hvm_event_traps (also added target > vcpu as param) > > * guest-request bits from X86 'struct arch_domain' (to common > 'struct domain') > > == ARM implementations: > > * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls > > hvm_event_guest_request (as on X86) > > * arch_monitor_get_capabilities: updated to reflect support for > > XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST > > * vm_event_init_domain (does nothing), vm_event_cleanup_domain > > == Misc: > > * hvm_event_fill_regs renamed to arch_hvm_event_fill_regs, no > longer > > X86-specific. ARM-side implementation of this function > currently does > > nothing, that will be added in a separate patch. > > We should probably take into account what happens with Tamas' > "vm_event: > consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs" patch > here. > That patch already affects hvm_event_fill_regs(). > > > Well it seems one of us will have to rebase depending which patch gets > accepted & merged first. The conflict is minimal so it's not a major > issue. If my patch gets merged first then just have to introduce the > empty function in the ARM header with the new name. > > Tamas > Okay then, for me it's fine either way. Corneliu.
On Thu, 18 Feb 2016, Corneliu ZUZU wrote: > diff --git a/MAINTAINERS b/MAINTAINERS > index cd4da04..768ad32 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -356,6 +356,7 @@ M: Tamas K Lengyel <tamas@tklengyel.com> > S: Supported > F: xen/common/vm_event.c > F: xen/common/mem_access.c > +F: xen/common/hvm/event.c > F: xen/common/monitor.c > F: xen/arch/x86/hvm/event.c > F: xen/arch/x86/monitor.c This needs an Ack from Tamas or Razvan > diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c > index 056db1a..767edd1 100644 > --- a/xen/arch/arm/hvm.c > +++ b/xen/arch/arm/hvm.c > @@ -22,6 +22,7 @@ > #include <xen/errno.h> > #include <xen/guest_access.h> > #include <xen/sched.h> > +#include <xen/hvm/event.h> > > #include <xsm/xsm.h> > > @@ -72,6 +73,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/common/Makefile b/xen/common/Makefile > index 0d76efe..703faa5 100644 > --- a/xen/common/Makefile > +++ b/xen/common/Makefile > @@ -67,7 +67,7 @@ obj-$(xenoprof) += xenoprof.o > > obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o kernel.o memory.o multicall.o tmem_xen.o xlat.o) > > -subdir-$(CONFIG_X86) += hvm > +subdir-y += hvm > > subdir-$(coverage) += gcov > > diff --git a/xen/common/hvm/Makefile b/xen/common/hvm/Makefile > index a464a57..11e109d 100644 > --- a/xen/common/hvm/Makefile > +++ b/xen/common/hvm/Makefile > @@ -1 +1,2 @@ > -obj-y += save.o > +obj-$(CONFIG_X86) += save.o > +obj-y += event.o > diff --git a/xen/common/hvm/event.c b/xen/common/hvm/event.c > new file mode 100644 > index 0000000..28ceadc > --- /dev/null > +++ b/xen/common/hvm/event.c > @@ -0,0 +1,96 @@ > +/* > + * xen/common/hvm/event.c > + * > + * Common hardware virtual machine event abstractions. > + * > + * 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/hvm/event.h> > +#include <xen/vm_event.h> > +#include <asm/hvm/event.h> > +#if CONFIG_X86 > +#include <asm/altp2m.h> > +#endif > + > +int hvm_event_traps(struct vcpu *v, uint8_t sync, vm_event_request_t *req) > +{ > + int rc; > + struct domain *d = v->domain; > + > + rc = vm_event_claim_slot(d, &d->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(v); > + } > + > +#if CONFIG_X86 > + if ( altp2m_active(d) ) I would rather #define altp2m_active(d) (0) on ARM, removing the two ifdefs in this file. > + { > + req->flags |= VM_EVENT_FLAG_ALTERNATE_P2M; > + req->altp2m_idx = vcpu_altp2m(v).p2midx; > + } > +#endif > + > + arch_hvm_event_fill_regs(req); > + > + vm_event_put_request(d, &d->vm_event->monitor, req); > + > + return 1; > +} > + > +void hvm_event_guest_request(void) > +{ > + struct vcpu *curr = current; > + struct domain *d = curr->domain; > + > + if ( d->monitor.guest_request_enabled ) > + { > + vm_event_request_t req = { > + .reason = VM_EVENT_REASON_GUEST_REQUEST, > + .vcpu_id = curr->vcpu_id, > + }; > + > + hvm_event_traps(curr, d->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: > + */
On 02/19/2016 03:49 PM, Stefano Stabellini wrote: > On Thu, 18 Feb 2016, Corneliu ZUZU wrote: >> diff --git a/MAINTAINERS b/MAINTAINERS >> index cd4da04..768ad32 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -356,6 +356,7 @@ M: Tamas K Lengyel <tamas@tklengyel.com> >> S: Supported >> F: xen/common/vm_event.c >> F: xen/common/mem_access.c >> +F: xen/common/hvm/event.c >> F: xen/common/monitor.c >> F: xen/arch/x86/hvm/event.c >> F: xen/arch/x86/monitor.c > > This needs an Ack from Tamas or Razvan The MAINTAINERS change is fine with me. Unless Tamas has an issue with it (which he most likely doesn't), it should be fine. >> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >> index 056db1a..767edd1 100644 >> --- a/xen/arch/arm/hvm.c >> +++ b/xen/arch/arm/hvm.c >> @@ -22,6 +22,7 @@ >> #include <xen/errno.h> >> #include <xen/guest_access.h> >> #include <xen/sched.h> >> +#include <xen/hvm/event.h> >> >> #include <xsm/xsm.h> >> >> @@ -72,6 +73,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/common/Makefile b/xen/common/Makefile >> index 0d76efe..703faa5 100644 >> --- a/xen/common/Makefile >> +++ b/xen/common/Makefile >> @@ -67,7 +67,7 @@ obj-$(xenoprof) += xenoprof.o >> >> obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o kernel.o memory.o multicall.o tmem_xen.o xlat.o) >> >> -subdir-$(CONFIG_X86) += hvm >> +subdir-y += hvm >> >> subdir-$(coverage) += gcov >> >> diff --git a/xen/common/hvm/Makefile b/xen/common/hvm/Makefile >> index a464a57..11e109d 100644 >> --- a/xen/common/hvm/Makefile >> +++ b/xen/common/hvm/Makefile >> @@ -1 +1,2 @@ >> -obj-y += save.o >> +obj-$(CONFIG_X86) += save.o >> +obj-y += event.o >> diff --git a/xen/common/hvm/event.c b/xen/common/hvm/event.c >> new file mode 100644 >> index 0000000..28ceadc >> --- /dev/null >> +++ b/xen/common/hvm/event.c >> @@ -0,0 +1,96 @@ >> +/* >> + * xen/common/hvm/event.c >> + * >> + * Common hardware virtual machine event abstractions. >> + * >> + * 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/hvm/event.h> >> +#include <xen/vm_event.h> >> +#include <asm/hvm/event.h> >> +#if CONFIG_X86 >> +#include <asm/altp2m.h> >> +#endif >> + >> +int hvm_event_traps(struct vcpu *v, uint8_t sync, vm_event_request_t *req) >> +{ >> + int rc; >> + struct domain *d = v->domain; >> + >> + rc = vm_event_claim_slot(d, &d->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(v); >> + } >> + >> +#if CONFIG_X86 >> + if ( altp2m_active(d) ) > > I would rather > > #define altp2m_active(d) (0) > > on ARM, removing the two ifdefs in this file. I second that, not a big fan of the #ifdefs. Thanks, Razvan
>>> On 18.02.16 at 20:35, <czuzu@bitdefender.com> wrote: > --- > MAINTAINERS | 1 + > xen/arch/arm/hvm.c | 8 +++ > xen/arch/x86/hvm/event.c | 116 ++++++---------------------------------- > xen/arch/x86/hvm/hvm.c | 1 + > xen/arch/x86/monitor.c | 14 ----- > xen/arch/x86/vm_event.c | 1 + > xen/common/Makefile | 2 +- > xen/common/hvm/Makefile | 3 +- > xen/common/hvm/event.c | 96 +++++++++++++++++++++++++++++++++ So here you _again_ try to introduce something HVM-ish for ARM. Why? Why can't this code live in common/vm_event.c? > --- a/xen/include/asm-x86/domain.h > +++ b/xen/include/asm-x86/domain.h > @@ -376,17 +376,15 @@ struct arch_domain > unsigned long *pirq_eoi_map; > unsigned long pirq_eoi_map_mfn; > > - /* Monitor options */ > + /* Arch-specific monitor options */ > struct { > - unsigned int write_ctrlreg_enabled : 4; > - unsigned int write_ctrlreg_sync : 4; > - unsigned int write_ctrlreg_onchangeonly : 4; > - unsigned int mov_to_msr_enabled : 1; > - unsigned int mov_to_msr_extended : 1; > - unsigned int singlestep_enabled : 1; > - unsigned int software_breakpoint_enabled : 1; > - unsigned int guest_request_enabled : 1; > - unsigned int guest_request_sync : 1; > + uint16_t write_ctrlreg_enabled : 4; > + uint16_t write_ctrlreg_sync : 4; > + uint16_t write_ctrlreg_onchangeonly : 4; > + uint16_t mov_to_msr_enabled : 1; > + uint16_t mov_to_msr_extended : 1; > + uint16_t singlestep_enabled : 1; > + uint16_t software_breakpoint_enabled : 1; > } monitor; What is this type change supposed to achieve in general, and in particular in the context of this patch? > --- a/xen/include/asm-x86/hvm/event.h > +++ b/xen/include/asm-x86/hvm/event.h > @@ -1,5 +1,9 @@ > /* > - * event.h: Hardware virtual machine assist events. > + * include/asm-x86/hvm/event.h > + * > + * Arch-specific hardware virtual machine event abstractions. > + * > + * Copyright (c) 2016, Bitdefender S.R.L. Is this true? Namely, was _all_ of the code here written by people on your company, including what may have got moved here from elsewhere? Jan
On 2/19/2016 3:49 PM, Stefano Stabellini wrote: > On Thu, 18 Feb 2016, Corneliu ZUZU wrote: >> + >> + if ( sync ) >> + { >> + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; >> + vm_event_vcpu_pause(v); >> + } >> + >> +#if CONFIG_X86 >> + if ( altp2m_active(d) ) > I would rather > > #define altp2m_active(d) (0) > > on ARM, removing the two ifdefs in this file. Yeah, I actually wanted to get rid of that too at some point, the question is, what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm not familiar w/ altp2m design, maybe someone that knows more of the internals of that can give a suggestion. Thanks, Corneliu.
On Fri, 19 Feb 2016, Corneliu ZUZU wrote: > On 2/19/2016 3:49 PM, Stefano Stabellini wrote: > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote: > > > + > > > + if ( sync ) > > > + { > > > + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; > > > + vm_event_vcpu_pause(v); > > > + } > > > + > > > +#if CONFIG_X86 > > > + if ( altp2m_active(d) ) > > I would rather > > > > #define altp2m_active(d) (0) > > > > on ARM, removing the two ifdefs in this file. > > Yeah, I actually wanted to get rid of that too at some point, the question is, > what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm not familiar > w/ altp2m design, maybe someone that knows more of the internals of that can > give a suggestion. If you #define altp2m_active to (0), gcc will automatically avoid the if statement.
On Fri, Feb 19, 2016 at 7:26 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 18.02.16 at 20:35, <czuzu@bitdefender.com> wrote: > > --- > > MAINTAINERS | 1 + > > xen/arch/arm/hvm.c | 8 +++ > > xen/arch/x86/hvm/event.c | 116 > ++++++---------------------------------- > > xen/arch/x86/hvm/hvm.c | 1 + > > xen/arch/x86/monitor.c | 14 ----- > > xen/arch/x86/vm_event.c | 1 + > > xen/common/Makefile | 2 +- > > xen/common/hvm/Makefile | 3 +- > > xen/common/hvm/event.c | 96 +++++++++++++++++++++++++++++++++ > > So here you _again_ try to introduce something HVM-ish for ARM. > Why? Why can't this code live in common/vm_event.c? > I too am wondering if this is the right way to architect this. It would be better to move the guest-requested stuff into the generic vm_event component as it doesn't seem to be HVM specific other then it using an HVMOP hypercall to be triggered. Tamas
On 19/02/16 16:00, Stefano Stabellini wrote: > On Fri, 19 Feb 2016, Corneliu ZUZU wrote: >> On 2/19/2016 3:49 PM, Stefano Stabellini wrote: >>> On Thu, 18 Feb 2016, Corneliu ZUZU wrote: >>>> + >>>> + if ( sync ) >>>> + { >>>> + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; >>>> + vm_event_vcpu_pause(v); >>>> + } >>>> + >>>> +#if CONFIG_X86 >>>> + if ( altp2m_active(d) ) >>> I would rather >>> >>> #define altp2m_active(d) (0) >>> >>> on ARM, removing the two ifdefs in this file. >> Yeah, I actually wanted to get rid of that too at some point, the question is, >> what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm not familiar >> w/ altp2m design, maybe someone that knows more of the internals of that can >> give a suggestion. > If you #define altp2m_active to (0), gcc will automatically avoid the if > statement. You will still get the compile error from ARM's struct vcpu not having altp2m information. ~Andrew
On 2/19/2016 6:05 PM, Andrew Cooper wrote: > On 19/02/16 16:00, Stefano Stabellini wrote: >> On Fri, 19 Feb 2016, Corneliu ZUZU wrote: >>> On 2/19/2016 3:49 PM, Stefano Stabellini wrote: >>>> On Thu, 18 Feb 2016, Corneliu ZUZU wrote: >>>>> + >>>>> + if ( sync ) >>>>> + { >>>>> + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; >>>>> + vm_event_vcpu_pause(v); >>>>> + } >>>>> + >>>>> +#if CONFIG_X86 >>>>> + if ( altp2m_active(d) ) >>>> I would rather >>>> >>>> #define altp2m_active(d) (0) >>>> >>>> on ARM, removing the two ifdefs in this file. >>> Yeah, I actually wanted to get rid of that too at some point, the question is, >>> what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm not familiar >>> w/ altp2m design, maybe someone that knows more of the internals of that can >>> give a suggestion. >> If you #define altp2m_active to (0), gcc will automatically avoid the if >> statement. > You will still get the compile error from ARM's struct vcpu not having > altp2m information. > > ~Andrew > Yep. Corneliu.
On 2/19/2016 4:26 PM, Jan Beulich wrote: >>>> On 18.02.16 at 20:35, <czuzu@bitdefender.com> wrote: >> --- >> MAINTAINERS | 1 + >> xen/arch/arm/hvm.c | 8 +++ >> xen/arch/x86/hvm/event.c | 116 ++++++---------------------------------- >> xen/arch/x86/hvm/hvm.c | 1 + >> xen/arch/x86/monitor.c | 14 ----- >> xen/arch/x86/vm_event.c | 1 + >> xen/common/Makefile | 2 +- >> xen/common/hvm/Makefile | 3 +- >> xen/common/hvm/event.c | 96 +++++++++++++++++++++++++++++++++ > So here you _again_ try to introduce something HVM-ish for ARM. > Why? Why can't this code live in common/vm_event.c? Are you referring to "xen/arch/arm/hvm.c"? If so, I did not introduce that file, it was already there, all I did is add handling of HVMOP_guest_request_vm_event to ARM-side's already existing do_hvm_op :). On the "HVM-ish" note, is there some incompatibility between ARM and the concept of HVM? > >> --- a/xen/include/asm-x86/domain.h >> +++ b/xen/include/asm-x86/domain.h >> @@ -376,17 +376,15 @@ struct arch_domain >> unsigned long *pirq_eoi_map; >> unsigned long pirq_eoi_map_mfn; >> >> - /* Monitor options */ >> + /* Arch-specific monitor options */ >> struct { >> - unsigned int write_ctrlreg_enabled : 4; >> - unsigned int write_ctrlreg_sync : 4; >> - unsigned int write_ctrlreg_onchangeonly : 4; >> - unsigned int mov_to_msr_enabled : 1; >> - unsigned int mov_to_msr_extended : 1; >> - unsigned int singlestep_enabled : 1; >> - unsigned int software_breakpoint_enabled : 1; >> - unsigned int guest_request_enabled : 1; >> - unsigned int guest_request_sync : 1; >> + uint16_t write_ctrlreg_enabled : 4; >> + uint16_t write_ctrlreg_sync : 4; >> + uint16_t write_ctrlreg_onchangeonly : 4; >> + uint16_t mov_to_msr_enabled : 1; >> + uint16_t mov_to_msr_extended : 1; >> + uint16_t singlestep_enabled : 1; >> + uint16_t software_breakpoint_enabled : 1; >> } monitor; > What is this type change supposed to achieve in general, and in > particular in the context of this patch? Some time before this patch there was a discussion I had w/ ARM maintainer Ian Campbell who made the suggestion to try and move parts of the monitor vm-events code to the common-side. (you can find that discussion here: https://www.mail-archive.com/xen-devel@lists.xen.org/msg54139.html). In short, the reason for his suggestion was that that previous attempt of mine to add guest-request support for ARM highlighted code duplication between X86 and ARM if I were to leave the monitor vm-events code as it was (that is, completely arch-specific). In an effort to avoid that, I first submitted a 7 patch-series which tried to move as much as possible to the common-side. "As much as possible" meant guest-request, ctrl-reg write, single-step and software breakpoints, all moved to the common-side. That also meant introducing some Kconfigs to selectively activate some parts of that now-common code, since not all of it was used on ARM at that moment. Although conceptually that code could have remained on the common-side, Tamas pointed out that we could get rid of the Kconfigs (which in his opinion bloated the code) by only moving at the moment what is implemented on both X86 and ARM. As for the rest, he noted that when I add implementations for the other monitor vm-events on ARM as well, I could move that to common as well. The above is a result of that - guest-request is implemented on both X86 and ARM with this patch, hence I also moved it to common. >> --- a/xen/include/asm-x86/hvm/event.h >> +++ b/xen/include/asm-x86/hvm/event.h >> @@ -1,5 +1,9 @@ >> /* >> - * event.h: Hardware virtual machine assist events. >> + * include/asm-x86/hvm/event.h >> + * >> + * Arch-specific hardware virtual machine event abstractions. >> + * >> + * Copyright (c) 2016, Bitdefender S.R.L. > Is this true? Namely, was _all_ of the code here written by people > on your company, including what may have got moved here from > elsewhere? > > Jan > > My bad. Removing that line would be satisfactory, yes? Corneliu.
On 2/19/2016 6:02 PM, Tamas K Lengyel wrote: > > > On Fri, Feb 19, 2016 at 7:26 AM, Jan Beulich <JBeulich@suse.com > <mailto:JBeulich@suse.com>> wrote: > > >>> On 18.02.16 at 20:35, <czuzu@bitdefender.com <mailto:czuzu@bitdefender.com>> wrote: > > --- > > MAINTAINERS | 1 + > > xen/arch/arm/hvm.c | 8 +++ > > xen/arch/x86/hvm/event.c | 116 > ++++++---------------------------------- > > xen/arch/x86/hvm/hvm.c | 1 + > > xen/arch/x86/monitor.c | 14 ----- > > xen/arch/x86/vm_event.c | 1 + > > xen/common/Makefile | 2 +- > > xen/common/hvm/Makefile | 3 +- > > xen/common/hvm/event.c | 96 > +++++++++++++++++++++++++++++++++ > > So here you _again_ try to introduce something HVM-ish for ARM. > Why? Why can't this code live in common/vm_event.c? > > > I too am wondering if this is the right way to architect this. It > would be better to move the guest-requested stuff into the generic > vm_event component as it doesn't seem to be HVM specific other then it > using an HVMOP hypercall to be triggered. > > Tamas > Oh, that. "xen/common/hvm/event.c". I too don't know if it's the right way, but Jan, please at least don't attribute the way the code already is to me, I did not architect it. And it's not human to expect doing everything perfectly in a single shot. If you're of the opinion that it should be in vm_event.c I will gladly try to put it there. Of course, that could also be done in another patch. Corneliu.
>>> On 19.02.16 at 17:25, <czuzu@bitdefender.com> wrote: > On 2/19/2016 4:26 PM, Jan Beulich wrote: >>>>> On 18.02.16 at 20:35, <czuzu@bitdefender.com> wrote: >>> --- >>> MAINTAINERS | 1 + >>> xen/arch/arm/hvm.c | 8 +++ >>> xen/arch/x86/hvm/event.c | 116 ++++++---------------------------------- >>> xen/arch/x86/hvm/hvm.c | 1 + >>> xen/arch/x86/monitor.c | 14 ----- >>> xen/arch/x86/vm_event.c | 1 + >>> xen/common/Makefile | 2 +- >>> xen/common/hvm/Makefile | 3 +- >>> xen/common/hvm/event.c | 96 +++++++++++++++++++++++++++++++++ >> So here you _again_ try to introduce something HVM-ish for ARM. >> Why? Why can't this code live in common/vm_event.c? > > Are you referring to "xen/arch/arm/hvm.c"? If so, I did not introduce > that file, it was already there, all I did is add handling of > HVMOP_guest_request_vm_event to ARM-side's already existing do_hvm_op :). No, I'm referring to your initial attempt to create arch/arm/hvm/... > On the "HVM-ish" note, is there some incompatibility between ARM and the > concept of HVM? ARM guests are neither PV nor HVM right now, but somewhere in the middle (PVHv2 may come closest). >>> --- a/xen/include/asm-x86/domain.h >>> +++ b/xen/include/asm-x86/domain.h >>> @@ -376,17 +376,15 @@ struct arch_domain >>> unsigned long *pirq_eoi_map; >>> unsigned long pirq_eoi_map_mfn; >>> >>> - /* Monitor options */ >>> + /* Arch-specific monitor options */ >>> struct { >>> - unsigned int write_ctrlreg_enabled : 4; >>> - unsigned int write_ctrlreg_sync : 4; >>> - unsigned int write_ctrlreg_onchangeonly : 4; >>> - unsigned int mov_to_msr_enabled : 1; >>> - unsigned int mov_to_msr_extended : 1; >>> - unsigned int singlestep_enabled : 1; >>> - unsigned int software_breakpoint_enabled : 1; >>> - unsigned int guest_request_enabled : 1; >>> - unsigned int guest_request_sync : 1; >>> + uint16_t write_ctrlreg_enabled : 4; >>> + uint16_t write_ctrlreg_sync : 4; >>> + uint16_t write_ctrlreg_onchangeonly : 4; >>> + uint16_t mov_to_msr_enabled : 1; >>> + uint16_t mov_to_msr_extended : 1; >>> + uint16_t singlestep_enabled : 1; >>> + uint16_t software_breakpoint_enabled : 1; >>> } monitor; >> What is this type change supposed to achieve in general, and in >> particular in the context of this patch? > > Some time before this patch there was a discussion I had w/ ARM > maintainer Ian Campbell who made the suggestion to try and move parts of > the monitor vm-events code to the common-side. > (you can find that discussion here: > https://www.mail-archive.com/xen-devel@lists.xen.org/msg54139.html). > In short, the reason for his suggestion was that that previous attempt > of mine to add guest-request support for ARM highlighted code > duplication between X86 and ARM if I were to leave the monitor vm-events > code as it was (that is, completely arch-specific). In an effort to > avoid that, I first submitted a 7 patch-series which tried to move as > much as possible to the common-side. > "As much as possible" meant guest-request, ctrl-reg write, single-step > and software breakpoints, all moved to the common-side. That also meant > introducing some Kconfigs to selectively activate some parts of that > now-common code, since not all of it was used on ARM at that moment. > Although conceptually that code could have remained on the common-side, > Tamas pointed out that we could get rid of the Kconfigs (which in his > opinion bloated the code) by only moving at the moment what is > implemented on both X86 and ARM. As for the rest, he noted that when I > add implementations for the other monitor vm-events on ARM as well, I > could move that to common as well. The above is a result of that - > guest-request is implemented on both X86 and ARM with this patch, hence > I also moved it to common. I agree there are two fields being removed. But the question was why you also change the type of the other fields. >>> --- a/xen/include/asm-x86/hvm/event.h >>> +++ b/xen/include/asm-x86/hvm/event.h >>> @@ -1,5 +1,9 @@ >>> /* >>> - * event.h: Hardware virtual machine assist events. >>> + * include/asm-x86/hvm/event.h >>> + * >>> + * Arch-specific hardware virtual machine event abstractions. >>> + * >>> + * Copyright (c) 2016, Bitdefender S.R.L. >> Is this true? Namely, was _all_ of the code here written by people >> on your company, including what may have got moved here from >> elsewhere? > > My bad. Removing that line would be satisfactory, yes? To me, yes. Jan
On Fri, 19 Feb 2016, Corneliu ZUZU wrote: > On 2/19/2016 6:05 PM, Andrew Cooper wrote: > > On 19/02/16 16:00, Stefano Stabellini wrote: > > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote: > > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote: > > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote: > > > > > > + > > > > > > + if ( sync ) > > > > > > + { > > > > > > + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; > > > > > > + vm_event_vcpu_pause(v); > > > > > > + } > > > > > > + > > > > > > +#if CONFIG_X86 > > > > > > + if ( altp2m_active(d) ) > > > > > I would rather > > > > > > > > > > #define altp2m_active(d) (0) > > > > > > > > > > on ARM, removing the two ifdefs in this file. > > > > Yeah, I actually wanted to get rid of that too at some point, the > > > > question is, > > > > what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm not > > > > familiar > > > > w/ altp2m design, maybe someone that knows more of the internals of that > > > > can > > > > give a suggestion. > > > If you #define altp2m_active to (0), gcc will automatically avoid the if > > > statement. > > You will still get the compile error from ARM's struct vcpu not having > > altp2m information. > > > > ~Andrew > > > > Yep. Yes, you are right, especially given that Xen is compiled -Wall -Werror. How do you plan to introduce altp2m support on ARM? Is there going to be a struct altp2mvcpu on ARM too? It is not nice to access stuff under v->arch from common code. Maybe we need another arch_blah function to set altp2m_idx.
On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini < stefano.stabellini@eu.citrix.com> wrote: > On Fri, 19 Feb 2016, Corneliu ZUZU wrote: > > On 2/19/2016 6:05 PM, Andrew Cooper wrote: > > > On 19/02/16 16:00, Stefano Stabellini wrote: > > > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote: > > > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote: > > > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote: > > > > > > > + > > > > > > > + if ( sync ) > > > > > > > + { > > > > > > > + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; > > > > > > > + vm_event_vcpu_pause(v); > > > > > > > + } > > > > > > > + > > > > > > > +#if CONFIG_X86 > > > > > > > + if ( altp2m_active(d) ) > > > > > > I would rather > > > > > > > > > > > > #define altp2m_active(d) (0) > > > > > > > > > > > > on ARM, removing the two ifdefs in this file. > > > > > Yeah, I actually wanted to get rid of that too at some point, the > > > > > question is, > > > > > what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm > not > > > > > familiar > > > > > w/ altp2m design, maybe someone that knows more of the internals > of that > > > > > can > > > > > give a suggestion. > > > > If you #define altp2m_active to (0), gcc will automatically avoid > the if > > > > statement. > > > You will still get the compile error from ARM's struct vcpu not having > > > altp2m information. > > > > > > ~Andrew > > > > > > > Yep. > > Yes, you are right, especially given that Xen is compiled -Wall -Werror. > > How do you plan to introduce altp2m support on ARM? Is there going to be > a struct altp2mvcpu on ARM too? It is not nice to access stuff under > v->arch from common code. Maybe we need another arch_blah function to > set altp2m_idx. > As altp2m could be implemented for ARM as well it might make sense to start introducing bits and pieces that would make it easier to do that work in the future. But I agree, accessing v->arch directly from common is not a good way to go about it. Tamas
On 2/19/2016 7:15 PM, Jan Beulich wrote: >>>> On 19.02.16 at 17:25, <czuzu@bitdefender.com> wrote: >> On 2/19/2016 4:26 PM, Jan Beulich wrote: >>>>>> On 18.02.16 at 20:35, <czuzu@bitdefender.com> wrote: >>>> --- >>>> MAINTAINERS | 1 + >>>> xen/arch/arm/hvm.c | 8 +++ >>>> xen/arch/x86/hvm/event.c | 116 ++++++---------------------------------- >>>> xen/arch/x86/hvm/hvm.c | 1 + >>>> xen/arch/x86/monitor.c | 14 ----- >>>> xen/arch/x86/vm_event.c | 1 + >>>> xen/common/Makefile | 2 +- >>>> xen/common/hvm/Makefile | 3 +- >>>> xen/common/hvm/event.c | 96 +++++++++++++++++++++++++++++++++ >>> So here you _again_ try to introduce something HVM-ish for ARM. >>> Why? Why can't this code live in common/vm_event.c? >> Are you referring to "xen/arch/arm/hvm.c"? If so, I did not introduce >> that file, it was already there, all I did is add handling of >> HVMOP_guest_request_vm_event to ARM-side's already existing do_hvm_op :). > No, I'm referring to your initial attempt to create arch/arm/hvm/... I don't understand. Have I done that again with this patch? > >> On the "HVM-ish" note, is there some incompatibility between ARM and the >> concept of HVM? > ARM guests are neither PV nor HVM right now, but somewhere in > the middle (PVHv2 may come closest). I did not know that, but the fact that there is already "hvm-like" code written for ARM didn't hint me towards that fact either :) I'm aware that I'm far from familiar with the codebase right now, I'm browsing more of the code these days and taking notes to try and understand in depth at least the parts I'm sending contributions for. I've already got some questions I want to post to the mailing list soon, *including* exactly how the distinction between the guest-types comes into play w/ the vm-events code. Specifically, I'm talking for example about the following piece of code from the X86 arch_monitor_get_capabilities: /* * At the moment only Intel HVM domains are supported. However, event * delivery could be extended to AMD and PV domains. */ if ( !is_hvm_domain(d) || !cpu_has_vmx ) return capabilities; == "However, event delivery could be extended to AMD and PV domains". This comment begs for questions like: * what would be necessary to extend support to PV domains? * can we really do this operation without hardware assisted virtualization whatsoever? If not, how much can we do without that? * what about pvh? Since I have other questions like the above and I'll probably have more while I'm trying to get a better picture of the code, would it be ok if we defer addressing these issues to then? > >>>> --- a/xen/include/asm-x86/domain.h >>>> +++ b/xen/include/asm-x86/domain.h >>>> @@ -376,17 +376,15 @@ struct arch_domain >>>> unsigned long *pirq_eoi_map; >>>> unsigned long pirq_eoi_map_mfn; >>>> >>>> - /* Monitor options */ >>>> + /* Arch-specific monitor options */ >>>> struct { >>>> - unsigned int write_ctrlreg_enabled : 4; >>>> - unsigned int write_ctrlreg_sync : 4; >>>> - unsigned int write_ctrlreg_onchangeonly : 4; >>>> - unsigned int mov_to_msr_enabled : 1; >>>> - unsigned int mov_to_msr_extended : 1; >>>> - unsigned int singlestep_enabled : 1; >>>> - unsigned int software_breakpoint_enabled : 1; >>>> - unsigned int guest_request_enabled : 1; >>>> - unsigned int guest_request_sync : 1; >>>> + uint16_t write_ctrlreg_enabled : 4; >>>> + uint16_t write_ctrlreg_sync : 4; >>>> + uint16_t write_ctrlreg_onchangeonly : 4; >>>> + uint16_t mov_to_msr_enabled : 1; >>>> + uint16_t mov_to_msr_extended : 1; >>>> + uint16_t singlestep_enabled : 1; >>>> + uint16_t software_breakpoint_enabled : 1; >>>> } monitor; >>> What is this type change supposed to achieve in general, and in >>> particular in the context of this patch? >> Some time before this patch there was a discussion I had w/ ARM >> maintainer Ian Campbell who made the suggestion to try and move parts of >> the monitor vm-events code to the common-side. >> (you can find that discussion here: >> https://www.mail-archive.com/xen-devel@lists.xen.org/msg54139.html). >> In short, the reason for his suggestion was that that previous attempt >> of mine to add guest-request support for ARM highlighted code >> duplication between X86 and ARM if I were to leave the monitor vm-events >> code as it was (that is, completely arch-specific). In an effort to >> avoid that, I first submitted a 7 patch-series which tried to move as >> much as possible to the common-side. >> "As much as possible" meant guest-request, ctrl-reg write, single-step >> and software breakpoints, all moved to the common-side. That also meant >> introducing some Kconfigs to selectively activate some parts of that >> now-common code, since not all of it was used on ARM at that moment. >> Although conceptually that code could have remained on the common-side, >> Tamas pointed out that we could get rid of the Kconfigs (which in his >> opinion bloated the code) by only moving at the moment what is >> implemented on both X86 and ARM. As for the rest, he noted that when I >> add implementations for the other monitor vm-events on ARM as well, I >> could move that to common as well. The above is a result of that - >> guest-request is implemented on both X86 and ARM with this patch, hence >> I also moved it to common. > I agree there are two fields being removed. But the question was > why you also change the type of the other fields. It made sense because: * we don't need 32-bits to store that data anymore * as implementations are laid out for ARM also, more of those bits will get moved to common, so even less bits will be needed >>>> --- a/xen/include/asm-x86/hvm/event.h >>>> +++ b/xen/include/asm-x86/hvm/event.h >>>> @@ -1,5 +1,9 @@ >>>> /* >>>> - * event.h: Hardware virtual machine assist events. >>>> + * include/asm-x86/hvm/event.h >>>> + * >>>> + * Arch-specific hardware virtual machine event abstractions. >>>> + * >>>> + * Copyright (c) 2016, Bitdefender S.R.L. >>> Is this true? Namely, was _all_ of the code here written by people >>> on your company, including what may have got moved here from >>> elsewhere? >> My bad. Removing that line would be satisfactory, yes? > To me, yes. > > Jan > Noted. Corneliu.
On 2/19/2016 7:54 PM, Tamas K Lengyel wrote: > > > On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini > <stefano.stabellini@eu.citrix.com > <mailto:stefano.stabellini@eu.citrix.com>> wrote: > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote: > > On 2/19/2016 6:05 PM, Andrew Cooper wrote: > > > On 19/02/16 16:00, Stefano Stabellini wrote: > > > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote: > > > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote: > > > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote: > > > > > > > + > > > > > > > + if ( sync ) > > > > > > > + { > > > > > > > + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; > > > > > > > + vm_event_vcpu_pause(v); > > > > > > > + } > > > > > > > + > > > > > > > +#if CONFIG_X86 > > > > > > > + if ( altp2m_active(d) ) > > > > > > I would rather > > > > > > > > > > > > #define altp2m_active(d) (0) > > > > > > > > > > > > on ARM, removing the two ifdefs in this file. > > > > > Yeah, I actually wanted to get rid of that too at some > point, the > > > > > question is, > > > > > what do I do with "req->altp2m_idx = > vcpu_altp2m(v).p2midx"? I'm not > > > > > familiar > > > > > w/ altp2m design, maybe someone that knows more of the > internals of that > > > > > can > > > > > give a suggestion. > > > > If you #define altp2m_active to (0), gcc will automatically > avoid the if > > > > statement. > > > You will still get the compile error from ARM's struct vcpu > not having > > > altp2m information. > > > > > > ~Andrew > > > > > > > Yep. > > Yes, you are right, especially given that Xen is compiled -Wall > -Werror. > > How do you plan to introduce altp2m support on ARM? Is there going > to be > a struct altp2mvcpu on ARM too? It is not nice to access stuff under > v->arch from common code. Maybe we need another arch_blah function to > set altp2m_idx. > > > As altp2m could be implemented for ARM as well it might make sense to > start introducing bits and pieces that would make it easier to do that > work in the future. But I agree, accessing v->arch directly from > common is not a good way to go about it. > > Tamas I am not at all familiar w/ altp2m at the moment, but I'll try to look into it. Since that doesn't relate so much with the code motion of this changeset and it might not be that straightforward to implement, would it be ok to leave the #ifdef CONFIG_X86 there for now and remove it in a separate patch? Corneliu.
On Fri, Feb 19, 2016 at 11:11 AM, Corneliu ZUZU <czuzu@bitdefender.com> wrote: > On 2/19/2016 7:54 PM, Tamas K Lengyel wrote: > > > > On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini < > <stefano.stabellini@eu.citrix.com>stefano.stabellini@eu.citrix.com> wrote: > >> On Fri, 19 Feb 2016, Corneliu ZUZU wrote: >> > On 2/19/2016 6:05 PM, Andrew Cooper wrote: >> > > On 19/02/16 16:00, Stefano Stabellini wrote: >> > > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote: >> > > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote: >> > > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote: >> > > > > > > + >> > > > > > > + if ( sync ) >> > > > > > > + { >> > > > > > > + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; >> > > > > > > + vm_event_vcpu_pause(v); >> > > > > > > + } >> > > > > > > + >> > > > > > > +#if CONFIG_X86 >> > > > > > > + if ( altp2m_active(d) ) >> > > > > > I would rather >> > > > > > >> > > > > > #define altp2m_active(d) (0) >> > > > > > >> > > > > > on ARM, removing the two ifdefs in this file. >> > > > > Yeah, I actually wanted to get rid of that too at some point, the >> > > > > question is, >> > > > > what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm >> not >> > > > > familiar >> > > > > w/ altp2m design, maybe someone that knows more of the internals >> of that >> > > > > can >> > > > > give a suggestion. >> > > > If you #define altp2m_active to (0), gcc will automatically avoid >> the if >> > > > statement. >> > > You will still get the compile error from ARM's struct vcpu not having >> > > altp2m information. >> > > >> > > ~Andrew >> > > >> > >> > Yep. >> >> Yes, you are right, especially given that Xen is compiled -Wall -Werror. >> >> How do you plan to introduce altp2m support on ARM? Is there going to be >> a struct altp2mvcpu on ARM too? It is not nice to access stuff under >> v->arch from common code. Maybe we need another arch_blah function to >> set altp2m_idx. >> > > As altp2m could be implemented for ARM as well it might make sense to > start introducing bits and pieces that would make it easier to do that work > in the future. But I agree, accessing v->arch directly from common is not a > good way to go about it. > > Tamas > > > I am not at all familiar w/ altp2m at the moment, but I'll try to look > into it. > Since that doesn't relate so much with the code motion of this changeset > and it might not be that straightforward to implement, would it be ok to > leave the #ifdef CONFIG_X86 there for now and remove it in a separate patch? > We are trying to avoid having to do ifdefs where-ever possible. So in this case too introducing arch-specific function(s) that are empty for ARM would be more appropriate. Tamas
On 2/19/2016 8:27 PM, Tamas K Lengyel wrote: > > > On Fri, Feb 19, 2016 at 11:11 AM, Corneliu ZUZU <czuzu@bitdefender.com > <mailto:czuzu@bitdefender.com>> wrote: > > On 2/19/2016 7:54 PM, Tamas K Lengyel wrote: >> >> >> On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini >> <stefano.stabellini@eu.citrix.com >> <mailto:stefano.stabellini@eu.citrix.com>> wrote: >> >> On Fri, 19 Feb 2016, Corneliu ZUZU wrote: >> > On 2/19/2016 6:05 PM, Andrew Cooper wrote: >> > > On 19/02/16 16:00, Stefano Stabellini wrote: >> > > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote: >> > > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote: >> > > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote: >> > > > > > > + >> > > > > > > + if ( sync ) >> > > > > > > + { >> > > > > > > + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; >> > > > > > > + vm_event_vcpu_pause(v); >> > > > > > > + } >> > > > > > > + >> > > > > > > +#if CONFIG_X86 >> > > > > > > + if ( altp2m_active(d) ) >> > > > > > I would rather >> > > > > > >> > > > > > #define altp2m_active(d) (0) >> > > > > > >> > > > > > on ARM, removing the two ifdefs in this file. >> > > > > Yeah, I actually wanted to get rid of that too at >> some point, the >> > > > > question is, >> > > > > what do I do with "req->altp2m_idx = >> vcpu_altp2m(v).p2midx"? I'm not >> > > > > familiar >> > > > > w/ altp2m design, maybe someone that knows more of >> the internals of that >> > > > > can >> > > > > give a suggestion. >> > > > If you #define altp2m_active to (0), gcc will >> automatically avoid the if >> > > > statement. >> > > You will still get the compile error from ARM's struct >> vcpu not having >> > > altp2m information. >> > > >> > > ~Andrew >> > > >> > >> > Yep. >> >> Yes, you are right, especially given that Xen is compiled >> -Wall -Werror. >> >> How do you plan to introduce altp2m support on ARM? Is there >> going to be >> a struct altp2mvcpu on ARM too? It is not nice to access >> stuff under >> v->arch from common code. Maybe we need another arch_blah >> function to >> set altp2m_idx. >> >> >> As altp2m could be implemented for ARM as well it might make >> sense to start introducing bits and pieces that would make it >> easier to do that work in the future. But I agree, accessing >> v->arch directly from common is not a good way to go about it. >> >> Tamas > > I am not at all familiar w/ altp2m at the moment, but I'll try to > look into it. > Since that doesn't relate so much with the code motion of this > changeset and it might not be that straightforward to implement, > would it be ok to leave the #ifdef CONFIG_X86 there for now and > remove it in a separate patch? > > > We are trying to avoid having to do ifdefs where-ever possible. So in > this case too introducing arch-specific function(s) that are empty for > ARM would be more appropriate. > > Tamas > I understand that, I was merely asking if it would be okay to do it in another patch, because it didn't seem that straightforward. More concretely, are you suggesting to: * do the "#define altp2m_active(d) (0)" as Stefano suggested * incorporate "vcpu_altp2m(v).p2midx" into an arch_foo function ? That seems easy enough to do. If so, how should I call this arch_foo function and where would it be appropriate to put it? Thanks, Corneliu.
On Fri, Feb 19, 2016 at 11:33 AM, Corneliu ZUZU <czuzu@bitdefender.com> wrote: > On 2/19/2016 8:27 PM, Tamas K Lengyel wrote: > > > > On Fri, Feb 19, 2016 at 11:11 AM, Corneliu ZUZU <czuzu@bitdefender.com> > wrote: > >> On 2/19/2016 7:54 PM, Tamas K Lengyel wrote: >> >> >> >> On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini < >> stefano.stabellini@eu.citrix.com> wrote: >> >>> On Fri, 19 Feb 2016, Corneliu ZUZU wrote: >>> > On 2/19/2016 6:05 PM, Andrew Cooper wrote: >>> > > On 19/02/16 16:00, Stefano Stabellini wrote: >>> > > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote: >>> > > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote: >>> > > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote: >>> > > > > > > + >>> > > > > > > + if ( sync ) >>> > > > > > > + { >>> > > > > > > + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; >>> > > > > > > + vm_event_vcpu_pause(v); >>> > > > > > > + } >>> > > > > > > + >>> > > > > > > +#if CONFIG_X86 >>> > > > > > > + if ( altp2m_active(d) ) >>> > > > > > I would rather >>> > > > > > >>> > > > > > #define altp2m_active(d) (0) >>> > > > > > >>> > > > > > on ARM, removing the two ifdefs in this file. >>> > > > > Yeah, I actually wanted to get rid of that too at some point, the >>> > > > > question is, >>> > > > > what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm >>> not >>> > > > > familiar >>> > > > > w/ altp2m design, maybe someone that knows more of the internals >>> of that >>> > > > > can >>> > > > > give a suggestion. >>> > > > If you #define altp2m_active to (0), gcc will automatically avoid >>> the if >>> > > > statement. >>> > > You will still get the compile error from ARM's struct vcpu not >>> having >>> > > altp2m information. >>> > > >>> > > ~Andrew >>> > > >>> > >>> > Yep. >>> >>> Yes, you are right, especially given that Xen is compiled -Wall -Werror. >>> >>> How do you plan to introduce altp2m support on ARM? Is there going to be >>> a struct altp2mvcpu on ARM too? It is not nice to access stuff under >>> v->arch from common code. Maybe we need another arch_blah function to >>> set altp2m_idx. >>> >> >> As altp2m could be implemented for ARM as well it might make sense to >> start introducing bits and pieces that would make it easier to do that work >> in the future. But I agree, accessing v->arch directly from common is not a >> good way to go about it. >> >> Tamas >> >> >> I am not at all familiar w/ altp2m at the moment, but I'll try to look >> into it. >> Since that doesn't relate so much with the code motion of this changeset >> and it might not be that straightforward to implement, would it be ok to >> leave the #ifdef CONFIG_X86 there for now and remove it in a separate patch? >> > > We are trying to avoid having to do ifdefs where-ever possible. So in this > case too introducing arch-specific function(s) that are empty for ARM would > be more appropriate. > > Tamas > > > I understand that, I was merely asking if it would be okay to do it in > another patch, because it didn't seem that straightforward. > More concretely, are you suggesting to: > * do the "#define altp2m_active(d) (0)" as Stefano suggested > That is easy enough to implement so go with that. > * incorporate "vcpu_altp2m(v).p2midx" into an arch_foo function > Implement an arch-specific function for this, say p2m_get_vcpu_altp2m_idx(v) which would just return 0 on ARM for now. Tamas
On 2/19/2016 8:42 PM, Tamas K Lengyel wrote: > > > On Fri, Feb 19, 2016 at 11:33 AM, Corneliu ZUZU <czuzu@bitdefender.com > <mailto:czuzu@bitdefender.com>> wrote: > > On 2/19/2016 8:27 PM, Tamas K Lengyel wrote: >> >> >> On Fri, Feb 19, 2016 at 11:11 AM, Corneliu ZUZU >> <czuzu@bitdefender.com <mailto:czuzu@bitdefender.com>> wrote: >> >> On 2/19/2016 7:54 PM, Tamas K Lengyel wrote: >>> >>> >>> On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini >>> <stefano.stabellini@eu.citrix.com >>> <mailto:stefano.stabellini@eu.citrix.com>> wrote: >>> >>> On Fri, 19 Feb 2016, Corneliu ZUZU wrote: >>> > On 2/19/2016 6:05 PM, Andrew Cooper wrote: >>> > > On 19/02/16 16:00, Stefano Stabellini wrote: >>> > > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote: >>> > > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote: >>> > > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote: >>> > > > > > > + >>> > > > > > > + if ( sync ) >>> > > > > > > + { >>> > > > > > > + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; >>> > > > > > > + vm_event_vcpu_pause(v); >>> > > > > > > + } >>> > > > > > > + >>> > > > > > > +#if CONFIG_X86 >>> > > > > > > + if ( altp2m_active(d) ) >>> > > > > > I would rather >>> > > > > > >>> > > > > > #define altp2m_active(d) (0) >>> > > > > > >>> > > > > > on ARM, removing the two ifdefs in this file. >>> > > > > Yeah, I actually wanted to get rid of that too >>> at some point, the >>> > > > > question is, >>> > > > > what do I do with "req->altp2m_idx = >>> vcpu_altp2m(v).p2midx"? I'm not >>> > > > > familiar >>> > > > > w/ altp2m design, maybe someone that knows more >>> of the internals of that >>> > > > > can >>> > > > > give a suggestion. >>> > > > If you #define altp2m_active to (0), gcc will >>> automatically avoid the if >>> > > > statement. >>> > > You will still get the compile error from ARM's >>> struct vcpu not having >>> > > altp2m information. >>> > > >>> > > ~Andrew >>> > > >>> > >>> > Yep. >>> >>> Yes, you are right, especially given that Xen is >>> compiled -Wall -Werror. >>> >>> How do you plan to introduce altp2m support on ARM? Is >>> there going to be >>> a struct altp2mvcpu on ARM too? It is not nice to access >>> stuff under >>> v->arch from common code. Maybe we need another >>> arch_blah function to >>> set altp2m_idx. >>> >>> >>> As altp2m could be implemented for ARM as well it might make >>> sense to start introducing bits and pieces that would make >>> it easier to do that work in the future. But I agree, >>> accessing v->arch directly from common is not a good way to >>> go about it. >>> >>> Tamas >> >> I am not at all familiar w/ altp2m at the moment, but I'll >> try to look into it. >> Since that doesn't relate so much with the code motion of >> this changeset and it might not be that straightforward to >> implement, would it be ok to leave the #ifdef CONFIG_X86 >> there for now and remove it in a separate patch? >> >> >> We are trying to avoid having to do ifdefs where-ever possible. >> So in this case too introducing arch-specific function(s) that >> are empty for ARM would be more appropriate. >> >> Tamas >> > > I understand that, I was merely asking if it would be okay to do > it in another patch, because it didn't seem that straightforward. > More concretely, are you suggesting to: > * do the "#define altp2m_active(d) (0)" as Stefano suggested > > > That is easy enough to implement so go with that. > > * incorporate "vcpu_altp2m(v).p2midx" into an arch_foo function > > > Implement an arch-specific function for this, say > p2m_get_vcpu_altp2m_idx(v) which would just return 0 on ARM for now. > > Tamas > Got it. Corneliu.
>>> On 19.02.16 at 19:01, <czuzu@bitdefender.com> wrote: > On 2/19/2016 7:15 PM, Jan Beulich wrote: >>>>> On 19.02.16 at 17:25, <czuzu@bitdefender.com> wrote: >>> On 2/19/2016 4:26 PM, Jan Beulich wrote: >>>>>>> On 18.02.16 at 20:35, <czuzu@bitdefender.com> wrote: >>>>> --- >>>>> MAINTAINERS | 1 + >>>>> xen/arch/arm/hvm.c | 8 +++ >>>>> xen/arch/x86/hvm/event.c | 116 ++++++---------------------------------- >>>>> xen/arch/x86/hvm/hvm.c | 1 + >>>>> xen/arch/x86/monitor.c | 14 ----- >>>>> xen/arch/x86/vm_event.c | 1 + >>>>> xen/common/Makefile | 2 +- >>>>> xen/common/hvm/Makefile | 3 +- >>>>> xen/common/hvm/event.c | 96 +++++++++++++++++++++++++++++++++ >>>> So here you _again_ try to introduce something HVM-ish for ARM. >>>> Why? Why can't this code live in common/vm_event.c? >>> Are you referring to "xen/arch/arm/hvm.c"? If so, I did not introduce >>> that file, it was already there, all I did is add handling of >>> HVMOP_guest_request_vm_event to ARM-side's already existing do_hvm_op :). >> No, I'm referring to your initial attempt to create arch/arm/hvm/... > > I don't understand. Have I done that again with this patch? Not the exact same thing, but something going along those same lines of thinking. >>> On the "HVM-ish" note, is there some incompatibility between ARM and the >>> concept of HVM? >> ARM guests are neither PV nor HVM right now, but somewhere in >> the middle (PVHv2 may come closest). > > I did not know that, but the fact that there is already "hvm-like" code > written for ARM didn't hint me towards that fact either :) > I'm aware that I'm far from familiar with the codebase right now, I'm > browsing more of the code these days and taking notes to try and > understand in depth at least the parts I'm sending contributions for. > I've already got some questions I want to post to the mailing list soon, > *including* exactly how the distinction between the guest-types comes > into play w/ the vm-events code. > Specifically, I'm talking for example about the following piece of code > from the X86 arch_monitor_get_capabilities: > > /* > * At the moment only Intel HVM domains are supported. However, event > * delivery could be extended to AMD and PV domains. > */ > if ( !is_hvm_domain(d) || !cpu_has_vmx ) > return capabilities; > > == "However, event delivery could be extended to AMD and PV domains". > This comment begs for questions like: > * what would be necessary to extend support to PV domains? > * can we really do this operation without hardware assisted > virtualization whatsoever? If not, how much can we do without that? > * what about pvh? > > Since I have other questions like the above and I'll probably have more > while I'm trying to get a better picture of the code, would it be ok if > we defer addressing these issues to then? Yes, you should definitely not hijack this thread for other, more general inquiries. >>>>> --- a/xen/include/asm-x86/domain.h >>>>> +++ b/xen/include/asm-x86/domain.h >>>>> @@ -376,17 +376,15 @@ struct arch_domain >>>>> unsigned long *pirq_eoi_map; >>>>> unsigned long pirq_eoi_map_mfn; >>>>> >>>>> - /* Monitor options */ >>>>> + /* Arch-specific monitor options */ >>>>> struct { >>>>> - unsigned int write_ctrlreg_enabled : 4; >>>>> - unsigned int write_ctrlreg_sync : 4; >>>>> - unsigned int write_ctrlreg_onchangeonly : 4; >>>>> - unsigned int mov_to_msr_enabled : 1; >>>>> - unsigned int mov_to_msr_extended : 1; >>>>> - unsigned int singlestep_enabled : 1; >>>>> - unsigned int software_breakpoint_enabled : 1; >>>>> - unsigned int guest_request_enabled : 1; >>>>> - unsigned int guest_request_sync : 1; >>>>> + uint16_t write_ctrlreg_enabled : 4; >>>>> + uint16_t write_ctrlreg_sync : 4; >>>>> + uint16_t write_ctrlreg_onchangeonly : 4; >>>>> + uint16_t mov_to_msr_enabled : 1; >>>>> + uint16_t mov_to_msr_extended : 1; >>>>> + uint16_t singlestep_enabled : 1; >>>>> + uint16_t software_breakpoint_enabled : 1; >>>>> } monitor; >>>> What is this type change supposed to achieve in general, and in >>>> particular in the context of this patch? >>> Some time before this patch there was a discussion I had w/ ARM >>> maintainer Ian Campbell who made the suggestion to try and move parts of >>> the monitor vm-events code to the common-side. >>> (you can find that discussion here: >>> https://www.mail-archive.com/xen-devel@lists.xen.org/msg54139.html). >>> In short, the reason for his suggestion was that that previous attempt >>> of mine to add guest-request support for ARM highlighted code >>> duplication between X86 and ARM if I were to leave the monitor vm-events >>> code as it was (that is, completely arch-specific). In an effort to >>> avoid that, I first submitted a 7 patch-series which tried to move as >>> much as possible to the common-side. >>> "As much as possible" meant guest-request, ctrl-reg write, single-step >>> and software breakpoints, all moved to the common-side. That also meant >>> introducing some Kconfigs to selectively activate some parts of that >>> now-common code, since not all of it was used on ARM at that moment. >>> Although conceptually that code could have remained on the common-side, >>> Tamas pointed out that we could get rid of the Kconfigs (which in his >>> opinion bloated the code) by only moving at the moment what is >>> implemented on both X86 and ARM. As for the rest, he noted that when I >>> add implementations for the other monitor vm-events on ARM as well, I >>> could move that to common as well. The above is a result of that - >>> guest-request is implemented on both X86 and ARM with this patch, hence >>> I also moved it to common. >> I agree there are two fields being removed. But the question was >> why you also change the type of the other fields. > > It made sense because: > * we don't need 32-bits to store that data anymore > * as implementations are laid out for ARM also, more of those bits will > get moved to common, so even less bits will be needed Altering bit field types from signed or unsigned int to some other type always need more rational than that, because from a strict language perspective only those two types can be used for bit fields in a compatible manner. Furthermore, gcc had (and maybe still has) some "interesting" (at least to someone having come from a different world years ago) behavior for bit fields of types wider than "int", which implicitly makes it desirable to avoid non- standard types unless absolutely needed in a specific case. (And such then shouldn't be mixed in with other changes, to make both the adjustment and the reasons for them stand out.) Jan
On 2/22/2016 12:14 PM, Jan Beulich wrote: >>>> On 19.02.16 at 19:01, <czuzu@bitdefender.com> wrote: >> On 2/19/2016 7:15 PM, Jan Beulich wrote: >>>>>> On 19.02.16 at 17:25, <czuzu@bitdefender.com> wrote: >>>> On 2/19/2016 4:26 PM, Jan Beulich wrote: >>>>>>>> On 18.02.16 at 20:35, <czuzu@bitdefender.com> wrote: >>>>>> --- >>>>>> MAINTAINERS | 1 + >>>>>> xen/arch/arm/hvm.c | 8 +++ >>>>>> xen/arch/x86/hvm/event.c | 116 ++++++---------------------------------- >>>>>> xen/arch/x86/hvm/hvm.c | 1 + >>>>>> xen/arch/x86/monitor.c | 14 ----- >>>>>> xen/arch/x86/vm_event.c | 1 + >>>>>> xen/common/Makefile | 2 +- >>>>>> xen/common/hvm/Makefile | 3 +- >>>>>> xen/common/hvm/event.c | 96 +++++++++++++++++++++++++++++++++ >>>>> So here you _again_ try to introduce something HVM-ish for ARM. >>>>> Why? Why can't this code live in common/vm_event.c? >>>> Are you referring to "xen/arch/arm/hvm.c"? If so, I did not introduce >>>> that file, it was already there, all I did is add handling of >>>> HVMOP_guest_request_vm_event to ARM-side's already existing do_hvm_op :). >>> No, I'm referring to your initial attempt to create arch/arm/hvm/... >> I don't understand. Have I done that again with this patch? > Not the exact same thing, but something going along those same > lines of thinking. > >>>> On the "HVM-ish" note, is there some incompatibility between ARM and the >>>> concept of HVM? >>> ARM guests are neither PV nor HVM right now, but somewhere in >>> the middle (PVHv2 may come closest). >> I did not know that, but the fact that there is already "hvm-like" code >> written for ARM didn't hint me towards that fact either :) >> I'm aware that I'm far from familiar with the codebase right now, I'm >> browsing more of the code these days and taking notes to try and >> understand in depth at least the parts I'm sending contributions for. >> I've already got some questions I want to post to the mailing list soon, >> *including* exactly how the distinction between the guest-types comes >> into play w/ the vm-events code. >> Specifically, I'm talking for example about the following piece of code >> from the X86 arch_monitor_get_capabilities: >> >> /* >> * At the moment only Intel HVM domains are supported. However, event >> * delivery could be extended to AMD and PV domains. >> */ >> if ( !is_hvm_domain(d) || !cpu_has_vmx ) >> return capabilities; >> >> == "However, event delivery could be extended to AMD and PV domains". >> This comment begs for questions like: >> * what would be necessary to extend support to PV domains? >> * can we really do this operation without hardware assisted >> virtualization whatsoever? If not, how much can we do without that? >> * what about pvh? >> >> Since I have other questions like the above and I'll probably have more >> while I'm trying to get a better picture of the code, would it be ok if >> we defer addressing these issues to then? > Yes, you should definitely not hijack this thread for other, more > general inquiries. Ok then, should I also understand then that for now it's ok to keep the "HVM-ish" hvm_event_traps & hvm_event_guest_request (I suppose you were referring to these 2 functions above) on the common-side event.c until we address these issues? Or I could try to move them to common/vm_event.c as you suggest renamed to vm_event_traps & vm_event_guest_request and also rename arch_hvm_event_fill_regs to arch_vm_event_fill_regs (?). >>>>>> --- a/xen/include/asm-x86/domain.h >>>>>> +++ b/xen/include/asm-x86/domain.h >>>>>> @@ -376,17 +376,15 @@ struct arch_domain >>>>>> unsigned long *pirq_eoi_map; >>>>>> unsigned long pirq_eoi_map_mfn; >>>>>> >>>>>> - /* Monitor options */ >>>>>> + /* Arch-specific monitor options */ >>>>>> struct { >>>>>> - unsigned int write_ctrlreg_enabled : 4; >>>>>> - unsigned int write_ctrlreg_sync : 4; >>>>>> - unsigned int write_ctrlreg_onchangeonly : 4; >>>>>> - unsigned int mov_to_msr_enabled : 1; >>>>>> - unsigned int mov_to_msr_extended : 1; >>>>>> - unsigned int singlestep_enabled : 1; >>>>>> - unsigned int software_breakpoint_enabled : 1; >>>>>> - unsigned int guest_request_enabled : 1; >>>>>> - unsigned int guest_request_sync : 1; >>>>>> + uint16_t write_ctrlreg_enabled : 4; >>>>>> + uint16_t write_ctrlreg_sync : 4; >>>>>> + uint16_t write_ctrlreg_onchangeonly : 4; >>>>>> + uint16_t mov_to_msr_enabled : 1; >>>>>> + uint16_t mov_to_msr_extended : 1; >>>>>> + uint16_t singlestep_enabled : 1; >>>>>> + uint16_t software_breakpoint_enabled : 1; >>>>>> } monitor; >>>>> What is this type change supposed to achieve in general, and in >>>>> particular in the context of this patch? >>>> Some time before this patch there was a discussion I had w/ ARM >>>> maintainer Ian Campbell who made the suggestion to try and move parts of >>>> the monitor vm-events code to the common-side. >>>> (you can find that discussion here: >>>> https://www.mail-archive.com/xen-devel@lists.xen.org/msg54139.html). >>>> In short, the reason for his suggestion was that that previous attempt >>>> of mine to add guest-request support for ARM highlighted code >>>> duplication between X86 and ARM if I were to leave the monitor vm-events >>>> code as it was (that is, completely arch-specific). In an effort to >>>> avoid that, I first submitted a 7 patch-series which tried to move as >>>> much as possible to the common-side. >>>> "As much as possible" meant guest-request, ctrl-reg write, single-step >>>> and software breakpoints, all moved to the common-side. That also meant >>>> introducing some Kconfigs to selectively activate some parts of that >>>> now-common code, since not all of it was used on ARM at that moment. >>>> Although conceptually that code could have remained on the common-side, >>>> Tamas pointed out that we could get rid of the Kconfigs (which in his >>>> opinion bloated the code) by only moving at the moment what is >>>> implemented on both X86 and ARM. As for the rest, he noted that when I >>>> add implementations for the other monitor vm-events on ARM as well, I >>>> could move that to common as well. The above is a result of that - >>>> guest-request is implemented on both X86 and ARM with this patch, hence >>>> I also moved it to common. >>> I agree there are two fields being removed. But the question was >>> why you also change the type of the other fields. >> It made sense because: >> * we don't need 32-bits to store that data anymore >> * as implementations are laid out for ARM also, more of those bits will >> get moved to common, so even less bits will be needed > Altering bit field types from signed or unsigned int to some other > type always need more rational than that, because from a strict > language perspective only those two types can be used for bit > fields in a compatible manner. Furthermore, gcc had (and maybe > still has) some "interesting" (at least to someone having come > from a different world years ago) behavior for bit fields of types > wider than "int", which implicitly makes it desirable to avoid non- > standard types unless absolutely needed in a specific case. (And > such then shouldn't be mixed in with other changes, to make both > the adjustment and the reasons for them stand out.) > > Jan > That's another reason for me to read the C standard someday. (C90, 6.5.2.1) "A bit-field shall have a type that is a qualified or unqualified version of one of int, unsigned int, or signed int" (C99, 6.7.2.1p4) "A bit-field shall have a type that is a qualified or unqualified version of _Bool, signed int, unsigned int, or some other implementation-defined type." I've always used bitfields w/ arbitrary sizes: char, short, int, long long...GCC never has complained about it, I didn't know types other than signed/unsigned int were non-standard extensions. GCC warns me about this if I use "-std=gnu90 -pedantic", w/ gnu99 it doesn't, maybe because of the "or some other implementation-defined type" part (which I don't fully understand). Will change back the uint16_t to plain unsigned int and uint8_t here: + /* Common monitor options */ + struct { + uint8_t guest_request_enabled : 1; + uint8_t guest_request_sync : 1; + } monitor; to unsigned int as well. Thanks, Corneliu.
>>> On 22.02.16 at 12:26, <czuzu@bitdefender.com> wrote: > On 2/22/2016 12:14 PM, Jan Beulich wrote: >>>>> On 19.02.16 at 19:01, <czuzu@bitdefender.com> wrote: >>> On 2/19/2016 7:15 PM, Jan Beulich wrote: >>>>>>> On 19.02.16 at 17:25, <czuzu@bitdefender.com> wrote: >>>>> On 2/19/2016 4:26 PM, Jan Beulich wrote: >>>>>>>>> On 18.02.16 at 20:35, <czuzu@bitdefender.com> wrote: >>>>> On the "HVM-ish" note, is there some incompatibility between ARM and the >>>>> concept of HVM? >>>> ARM guests are neither PV nor HVM right now, but somewhere in >>>> the middle (PVHv2 may come closest). >>> I did not know that, but the fact that there is already "hvm-like" code >>> written for ARM didn't hint me towards that fact either :) >>> I'm aware that I'm far from familiar with the codebase right now, I'm >>> browsing more of the code these days and taking notes to try and >>> understand in depth at least the parts I'm sending contributions for. >>> I've already got some questions I want to post to the mailing list soon, >>> *including* exactly how the distinction between the guest-types comes >>> into play w/ the vm-events code. >>> Specifically, I'm talking for example about the following piece of code >>> from the X86 arch_monitor_get_capabilities: >>> >>> /* >>> * At the moment only Intel HVM domains are supported. However, event >>> * delivery could be extended to AMD and PV domains. >>> */ >>> if ( !is_hvm_domain(d) || !cpu_has_vmx ) >>> return capabilities; >>> >>> == "However, event delivery could be extended to AMD and PV domains". >>> This comment begs for questions like: >>> * what would be necessary to extend support to PV domains? >>> * can we really do this operation without hardware assisted >>> virtualization whatsoever? If not, how much can we do without that? >>> * what about pvh? >>> >>> Since I have other questions like the above and I'll probably have more >>> while I'm trying to get a better picture of the code, would it be ok if >>> we defer addressing these issues to then? >> Yes, you should definitely not hijack this thread for other, more >> general inquiries. > > Ok then, should I also understand then that for now it's ok to keep the > "HVM-ish" hvm_event_traps & hvm_event_guest_request (I suppose you were > referring to these 2 functions above) on the common-side event.c until > we address these issues? > Or I could try to move them to common/vm_event.c as you suggest renamed > to vm_event_traps & vm_event_guest_request and also rename > arch_hvm_event_fill_regs to arch_vm_event_fill_regs (?). I'd say dropping the hvm_ suffixes / infixes would be fine (and even desirable) alongside their movement to common/vm_event.c, but the question really needs to go to the maintainers of that code. Jan
On 02/22/2016 01:38 PM, Jan Beulich wrote: >>>> On 22.02.16 at 12:26, <czuzu@bitdefender.com> wrote: >> On 2/22/2016 12:14 PM, Jan Beulich wrote: >>>>>> On 19.02.16 at 19:01, <czuzu@bitdefender.com> wrote: >>>> On 2/19/2016 7:15 PM, Jan Beulich wrote: >>>>>>>> On 19.02.16 at 17:25, <czuzu@bitdefender.com> wrote: >>>>>> On 2/19/2016 4:26 PM, Jan Beulich wrote: >>>>>>>>>> On 18.02.16 at 20:35, <czuzu@bitdefender.com> wrote: >>>>>> On the "HVM-ish" note, is there some incompatibility between ARM and the >>>>>> concept of HVM? >>>>> ARM guests are neither PV nor HVM right now, but somewhere in >>>>> the middle (PVHv2 may come closest). >>>> I did not know that, but the fact that there is already "hvm-like" code >>>> written for ARM didn't hint me towards that fact either :) >>>> I'm aware that I'm far from familiar with the codebase right now, I'm >>>> browsing more of the code these days and taking notes to try and >>>> understand in depth at least the parts I'm sending contributions for. >>>> I've already got some questions I want to post to the mailing list soon, >>>> *including* exactly how the distinction between the guest-types comes >>>> into play w/ the vm-events code. >>>> Specifically, I'm talking for example about the following piece of code >>>> from the X86 arch_monitor_get_capabilities: >>>> >>>> /* >>>> * At the moment only Intel HVM domains are supported. However, event >>>> * delivery could be extended to AMD and PV domains. >>>> */ >>>> if ( !is_hvm_domain(d) || !cpu_has_vmx ) >>>> return capabilities; >>>> >>>> == "However, event delivery could be extended to AMD and PV domains". >>>> This comment begs for questions like: >>>> * what would be necessary to extend support to PV domains? >>>> * can we really do this operation without hardware assisted >>>> virtualization whatsoever? If not, how much can we do without that? >>>> * what about pvh? >>>> >>>> Since I have other questions like the above and I'll probably have more >>>> while I'm trying to get a better picture of the code, would it be ok if >>>> we defer addressing these issues to then? >>> Yes, you should definitely not hijack this thread for other, more >>> general inquiries. >> >> Ok then, should I also understand then that for now it's ok to keep the >> "HVM-ish" hvm_event_traps & hvm_event_guest_request (I suppose you were >> referring to these 2 functions above) on the common-side event.c until >> we address these issues? >> Or I could try to move them to common/vm_event.c as you suggest renamed >> to vm_event_traps & vm_event_guest_request and also rename >> arch_hvm_event_fill_regs to arch_vm_event_fill_regs (?). > > I'd say dropping the hvm_ suffixes / infixes would be fine (and > even desirable) alongside their movement to common/vm_event.c, > but the question really needs to go to the maintainers of that > code. I agree. Thanks, Razvan
On 2/18/2016 9:35 PM, Corneliu ZUZU wrote: > This patch adds ARM support for guest-request monitor vm-events. > > Summary of changes: > == Moved to common-side: > * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86 > arch_monitor_domctl_event to common monitor_domctl) > * hvm_event_guest_request, hvm_event_traps (also added target vcpu as param) > * guest-request bits from X86 'struct arch_domain' (to common 'struct domain') > == ARM implementations: > * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls > hvm_event_guest_request (as on X86) > * arch_monitor_get_capabilities: updated to reflect support for > XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST > * vm_event_init_domain (does nothing), vm_event_cleanup_domain > == Misc: > * hvm_event_fill_regs renamed to arch_hvm_event_fill_regs, no longer > X86-specific. ARM-side implementation of this function currently does > nothing, that will be added in a separate patch. > > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> Before sending in the next revision of this patch, I have a few questions I'd like to ask. That being the case, I thought it would be ok to also include *all* the changes that will be done in the next revision here for (potentially) additional feedback. Already discussed changes TBD: * Add #define altp2m_active(d) (0) and implement p2m_get_vcpu_altp2m_idx(v) for ARM to remove #ifdef CONFIG_X86 in hvm_event_traps (Stefano, Tamas) * Remove wrong copyright comment (Jan) * Change bitfields members type back to unsigned int (Jan) * Move hvm_event_traps and hvm_event_guest_request to common/vm_event.c and rename them to vm_event_traps and vm_event_guest_request (Jan) Questions: 1) I've noticed the practice in Xen is to prepend the arch_ prefix to functions that usually have a counterpart on the common-side, i.e. there are a lot of arch-specific functions missing this prefix. Would it then be advised to: - rename arch_monitor_get_capabilities to vm_event_monitor_get_capabilities and move it to vm_event.h - rename arch_hvm_event_fill_regs to vm_event_fill_regs and move it to vm_event.h 2) For Tamas: would it be ok if I put p2m_get_vcpu_altp2m_idx in asm/altp2m.h and rename it to altp2m_vcpu_idx(v) (to obey function-naming pattern in that file)? 3) I've noticed that on X86 on a guest-request monitor vm-event the RIP is automatically incremented. On ARM, with the current changeset, that doesn't seem to happen. X86 code path: vmx_vmexit_handler(EXIT_REASON_VMCALL) -> hvm_do_hypercall -> do_hvm_op(HVMOP_guest_request_vm_event) -> hvm_do_hypercall returns HVM_HCALL_completed -> vmx_vmexit_handler calls update_guest_eip(), which increments the RIP. ARM code path, e.g. AArch64: do_trap_hypervisor(HSR_EC_HVC64) -> do_trap_hypercall -> do_hvm_op(HVMOP_guest_request_vm_event). Notice that do_trap_hypercall checks if the PC has changed after doing the hypercall. If it didn't, it poisons the hypercall argument registers, as on X86 (DEADBEEF), which happens on a debug build, which means that the PC isn't updated anywhere. I've also noticed that this seems to hold for other hvm ops such as HVMOP_set_param/HVMOP_get_param. I'm wondering if the logic behind this is ok as it is or if I should call advance_pc after handling HVMOP_guest_request_vm_event in do_hvm_op (?) Thank you, Corneliu.
On 02/23/2016 11:09 AM, Corneliu ZUZU wrote: > On 2/18/2016 9:35 PM, Corneliu ZUZU wrote: >> This patch adds ARM support for guest-request monitor vm-events. >> >> Summary of changes: >> == Moved to common-side: >> * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86 >> arch_monitor_domctl_event to common monitor_domctl) >> * hvm_event_guest_request, hvm_event_traps (also added target vcpu >> as param) >> * guest-request bits from X86 'struct arch_domain' (to common >> 'struct domain') >> == ARM implementations: >> * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls >> hvm_event_guest_request (as on X86) >> * arch_monitor_get_capabilities: updated to reflect support for >> XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST >> * vm_event_init_domain (does nothing), vm_event_cleanup_domain >> == Misc: >> * hvm_event_fill_regs renamed to arch_hvm_event_fill_regs, no longer >> X86-specific. ARM-side implementation of this function >> currently does >> nothing, that will be added in a separate patch. >> >> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> > > Before sending in the next revision of this patch, I have a few > questions I'd like to ask. > That being the case, I thought it would be ok to also include *all* the > changes that will be done in the next revision here for (potentially) > additional feedback. > > Already discussed changes TBD: > * Add #define altp2m_active(d) (0) and implement > p2m_get_vcpu_altp2m_idx(v) for ARM to remove #ifdef CONFIG_X86 in > hvm_event_traps (Stefano, Tamas) > * Remove wrong copyright comment (Jan) > * Change bitfields members type back to unsigned int (Jan) > * Move hvm_event_traps and hvm_event_guest_request to common/vm_event.c > and rename them to vm_event_traps and vm_event_guest_request (Jan) > > Questions: > > 1) I've noticed the practice in Xen is to prepend the arch_ prefix to > functions that usually have a counterpart on the common-side, i.e. there > are a lot of arch-specific functions missing this prefix. Would it then > be advised to: > - rename arch_monitor_get_capabilities to > vm_event_monitor_get_capabilities and move it to vm_event.h > - rename arch_hvm_event_fill_regs to vm_event_fill_regs and move it > to vm_event.h Please see the recent commit adc75eba8b15c7103a010f736fe62e3fb2383964 in staging. Cheers, Razvan
On 2/23/2016 12:54 PM, Razvan Cojocaru wrote: > On 02/23/2016 11:09 AM, Corneliu ZUZU wrote: >> On 2/18/2016 9:35 PM, Corneliu ZUZU wrote: >>> This patch adds ARM support for guest-request monitor vm-events. >>> >>> Summary of changes: >>> == Moved to common-side: >>> * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86 >>> arch_monitor_domctl_event to common monitor_domctl) >>> * hvm_event_guest_request, hvm_event_traps (also added target vcpu >>> as param) >>> * guest-request bits from X86 'struct arch_domain' (to common >>> 'struct domain') >>> == ARM implementations: >>> * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls >>> hvm_event_guest_request (as on X86) >>> * arch_monitor_get_capabilities: updated to reflect support for >>> XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST >>> * vm_event_init_domain (does nothing), vm_event_cleanup_domain >>> == Misc: >>> * hvm_event_fill_regs renamed to arch_hvm_event_fill_regs, no longer >>> X86-specific. ARM-side implementation of this function >>> currently does >>> nothing, that will be added in a separate patch. >>> >>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> >> Before sending in the next revision of this patch, I have a few >> questions I'd like to ask. >> That being the case, I thought it would be ok to also include *all* the >> changes that will be done in the next revision here for (potentially) >> additional feedback. >> >> Already discussed changes TBD: >> * Add #define altp2m_active(d) (0) and implement >> p2m_get_vcpu_altp2m_idx(v) for ARM to remove #ifdef CONFIG_X86 in >> hvm_event_traps (Stefano, Tamas) >> * Remove wrong copyright comment (Jan) >> * Change bitfields members type back to unsigned int (Jan) >> * Move hvm_event_traps and hvm_event_guest_request to common/vm_event.c >> and rename them to vm_event_traps and vm_event_guest_request (Jan) >> >> Questions: >> >> 1) I've noticed the practice in Xen is to prepend the arch_ prefix to >> functions that usually have a counterpart on the common-side, i.e. there >> are a lot of arch-specific functions missing this prefix. Would it then >> be advised to: >> - rename arch_monitor_get_capabilities to >> vm_event_monitor_get_capabilities and move it to vm_event.h >> - rename arch_hvm_event_fill_regs to vm_event_fill_regs and move it >> to vm_event.h > Please see the recent commit adc75eba8b15c7103a010f736fe62e3fb2383964 in > staging. > > > Cheers, > Razvan > Great, that settles arch_hvm_event_fill_regs -> vm_event_fill_regs. Should I then do the same for arch_monitor_get_capabilities (-> vm_event_monitor_get_capabilities)? Thanks, Corneliu.
On 02/23/2016 01:00 PM, Corneliu ZUZU wrote: > On 2/23/2016 12:54 PM, Razvan Cojocaru wrote: >> On 02/23/2016 11:09 AM, Corneliu ZUZU wrote: >>> On 2/18/2016 9:35 PM, Corneliu ZUZU wrote: >>>> This patch adds ARM support for guest-request monitor vm-events. >>>> >>>> Summary of changes: >>>> == Moved to common-side: >>>> * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86 >>>> arch_monitor_domctl_event to common monitor_domctl) >>>> * hvm_event_guest_request, hvm_event_traps (also added target vcpu >>>> as param) >>>> * guest-request bits from X86 'struct arch_domain' (to common >>>> 'struct domain') >>>> == ARM implementations: >>>> * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls >>>> hvm_event_guest_request (as on X86) >>>> * arch_monitor_get_capabilities: updated to reflect support for >>>> XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST >>>> * vm_event_init_domain (does nothing), vm_event_cleanup_domain >>>> == Misc: >>>> * hvm_event_fill_regs renamed to arch_hvm_event_fill_regs, no longer >>>> X86-specific. ARM-side implementation of this function >>>> currently does >>>> nothing, that will be added in a separate patch. >>>> >>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> >>> Before sending in the next revision of this patch, I have a few >>> questions I'd like to ask. >>> That being the case, I thought it would be ok to also include *all* the >>> changes that will be done in the next revision here for (potentially) >>> additional feedback. >>> >>> Already discussed changes TBD: >>> * Add #define altp2m_active(d) (0) and implement >>> p2m_get_vcpu_altp2m_idx(v) for ARM to remove #ifdef CONFIG_X86 in >>> hvm_event_traps (Stefano, Tamas) >>> * Remove wrong copyright comment (Jan) >>> * Change bitfields members type back to unsigned int (Jan) >>> * Move hvm_event_traps and hvm_event_guest_request to common/vm_event.c >>> and rename them to vm_event_traps and vm_event_guest_request (Jan) >>> >>> Questions: >>> >>> 1) I've noticed the practice in Xen is to prepend the arch_ prefix to >>> functions that usually have a counterpart on the common-side, i.e. there >>> are a lot of arch-specific functions missing this prefix. Would it then >>> be advised to: >>> - rename arch_monitor_get_capabilities to >>> vm_event_monitor_get_capabilities and move it to vm_event.h >>> - rename arch_hvm_event_fill_regs to vm_event_fill_regs and move it >>> to vm_event.h >> Please see the recent commit adc75eba8b15c7103a010f736fe62e3fb2383964 in >> staging. >> >> >> Cheers, >> Razvan >> > > Great, that settles arch_hvm_event_fill_regs -> vm_event_fill_regs. > Should I then do the same for arch_monitor_get_capabilities (-> > vm_event_monitor_get_capabilities)? Sounds good to me, so yes, unless Tamas or Jan think otherwise. Thanks, Razvan
> > > > > Great, that settles arch_hvm_event_fill_regs -> vm_event_fill_regs. > > Should I then do the same for arch_monitor_get_capabilities (-> > > vm_event_monitor_get_capabilities)? > > Sounds good to me, so yes, unless Tamas or Jan think otherwise. > SGTM too. Tamas
> > 2) For Tamas: would it be ok if I put p2m_get_vcpu_altp2m_idx in > asm/altp2m.h and rename it to altp2m_vcpu_idx(v) (to obey function-naming > pattern in that file)? > Yes, that works. Tamas
On 2/23/2016 4:30 PM, Tamas K Lengyel wrote: > > > 2) For Tamas: would it be ok if I put p2m_get_vcpu_altp2m_idx in > asm/altp2m.h and rename it to altp2m_vcpu_idx(v) (to obey > function-naming pattern in that file)? > > > Yes, that works. > > Tamas > Thanks. Any thoughts on 3)? Corneliu.
diff --git a/MAINTAINERS b/MAINTAINERS index cd4da04..768ad32 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -356,6 +356,7 @@ M: Tamas K Lengyel <tamas@tklengyel.com> S: Supported F: xen/common/vm_event.c F: xen/common/mem_access.c +F: xen/common/hvm/event.c F: xen/common/monitor.c F: xen/arch/x86/hvm/event.c F: xen/arch/x86/monitor.c diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c index 056db1a..767edd1 100644 --- a/xen/arch/arm/hvm.c +++ b/xen/arch/arm/hvm.c @@ -22,6 +22,7 @@ #include <xen/errno.h> #include <xen/guest_access.h> #include <xen/sched.h> +#include <xen/hvm/event.h> #include <xsm/xsm.h> @@ -72,6 +73,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/x86/hvm/event.c b/xen/arch/x86/hvm/event.c index cb9c152..bbb0dc8 100644 --- a/xen/arch/x86/hvm/event.c +++ b/xen/arch/x86/hvm/event.c @@ -6,6 +6,7 @@ * 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, @@ -20,103 +21,32 @@ * this program; If not, see <http://www.gnu.org/licenses/>. */ -#include <xen/vm_event.h> -#include <xen/paging.h> +#include <xen/hvm/event.h> #include <asm/hvm/event.h> #include <asm/monitor.h> -#include <asm/altp2m.h> #include <public/vm_event.h> -static void hvm_event_fill_regs(vm_event_request_t *req) -{ - const struct cpu_user_regs *regs = guest_cpu_user_regs(); - const struct vcpu *curr = current; - - req->data.regs.x86.rax = regs->eax; - req->data.regs.x86.rcx = regs->ecx; - req->data.regs.x86.rdx = regs->edx; - req->data.regs.x86.rbx = regs->ebx; - req->data.regs.x86.rsp = regs->esp; - req->data.regs.x86.rbp = regs->ebp; - req->data.regs.x86.rsi = regs->esi; - req->data.regs.x86.rdi = regs->edi; - - req->data.regs.x86.r8 = regs->r8; - req->data.regs.x86.r9 = regs->r9; - req->data.regs.x86.r10 = regs->r10; - req->data.regs.x86.r11 = regs->r11; - req->data.regs.x86.r12 = regs->r12; - req->data.regs.x86.r13 = regs->r13; - req->data.regs.x86.r14 = regs->r14; - req->data.regs.x86.r15 = regs->r15; - - req->data.regs.x86.rflags = regs->eflags; - req->data.regs.x86.rip = regs->eip; - - req->data.regs.x86.msr_efer = curr->arch.hvm_vcpu.guest_efer; - req->data.regs.x86.cr0 = curr->arch.hvm_vcpu.guest_cr[0]; - req->data.regs.x86.cr3 = curr->arch.hvm_vcpu.guest_cr[3]; - req->data.regs.x86.cr4 = curr->arch.hvm_vcpu.guest_cr[4]; -} - -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); - } - - if ( altp2m_active(currd) ) - { - req->flags |= VM_EVENT_FLAG_ALTERNATE_P2M; - req->altp2m_idx = vcpu_altp2m(curr).p2midx; - } - - hvm_event_fill_regs(req); - vm_event_put_request(currd, &currd->vm_event->monitor, req); - - return 1; -} - bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old) { - struct arch_domain *currad = ¤t->domain->arch; + struct vcpu *curr = current; + struct arch_domain *ad = &curr->domain->arch; unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index); - if ( (currad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) && - (!(currad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) || + if ( (ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) && + (!(ad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) || value != old) ) { + bool_t sync = !!(ad->monitor.write_ctrlreg_sync & ctrlreg_bitmask); + vm_event_request_t req = { .reason = VM_EVENT_REASON_WRITE_CTRLREG, - .vcpu_id = current->vcpu_id, + .vcpu_id = curr->vcpu_id, .u.write_ctrlreg.index = index, .u.write_ctrlreg.new_value = value, .u.write_ctrlreg.old_value = old }; - hvm_event_traps(currad->monitor.write_ctrlreg_sync & ctrlreg_bitmask, - &req); + hvm_event_traps(curr, sync, &req); return 1; } @@ -126,30 +56,18 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old) void hvm_event_msr(unsigned int msr, uint64_t value) { struct vcpu *curr = current; - vm_event_request_t req = { - .reason = VM_EVENT_REASON_MOV_TO_MSR, - .vcpu_id = curr->vcpu_id, - .u.mov_to_msr.msr = msr, - .u.mov_to_msr.value = value, - }; - - if ( curr->domain->arch.monitor.mov_to_msr_enabled ) - hvm_event_traps(1, &req); -} - -void hvm_event_guest_request(void) -{ - struct vcpu *curr = current; - struct arch_domain *currad = &curr->domain->arch; + struct arch_domain *ad = &curr->domain->arch; - if ( currad->monitor.guest_request_enabled ) + if ( ad->monitor.mov_to_msr_enabled ) { vm_event_request_t req = { - .reason = VM_EVENT_REASON_GUEST_REQUEST, + .reason = VM_EVENT_REASON_MOV_TO_MSR, .vcpu_id = curr->vcpu_id, + .u.mov_to_msr.msr = msr, + .u.mov_to_msr.value = value, }; - hvm_event_traps(currad->monitor.guest_request_sync, &req); + hvm_event_traps(curr, 1, &req); } } @@ -197,7 +115,7 @@ int hvm_event_breakpoint(unsigned long rip, req.vcpu_id = curr->vcpu_id; - return hvm_event_traps(1, &req); + return hvm_event_traps(curr, 1, &req); } /* diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index a29c421..11c2ef6 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -32,6 +32,7 @@ #include <xen/guest_access.h> #include <xen/event.h> #include <xen/paging.h> +#include <xen/hvm/event.h> #include <xen/cpu.h> #include <xen/wait.h> #include <xen/mem_access.h> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index b4bd008..7768cbb 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -124,20 +124,6 @@ int arch_monitor_domctl_event(struct domain *d, break; } - case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST: - { - bool_t old_status = ad->monitor.guest_request_enabled; - - if ( unlikely(old_status == requested_status) ) - return -EEXIST; - - domain_pause(d); - ad->monitor.guest_request_sync = mop->u.guest_request.sync; - ad->monitor.guest_request_enabled = requested_status; - domain_unpause(d); - break; - } - default: /* * Should not be reached unless arch_monitor_get_capabilities() is not diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c index 08d678a..f97dec3 100644 --- a/xen/arch/x86/vm_event.c +++ b/xen/arch/x86/vm_event.c @@ -57,6 +57,7 @@ void vm_event_cleanup_domain(struct domain *d) d->arch.mem_access_emulate_each_rep = 0; memset(&d->arch.monitor, 0, sizeof(d->arch.monitor)); + memset(&d->monitor, 0, sizeof(d->monitor)); } void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v) diff --git a/xen/common/Makefile b/xen/common/Makefile index 0d76efe..703faa5 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -67,7 +67,7 @@ obj-$(xenoprof) += xenoprof.o obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o kernel.o memory.o multicall.o tmem_xen.o xlat.o) -subdir-$(CONFIG_X86) += hvm +subdir-y += hvm subdir-$(coverage) += gcov diff --git a/xen/common/hvm/Makefile b/xen/common/hvm/Makefile index a464a57..11e109d 100644 --- a/xen/common/hvm/Makefile +++ b/xen/common/hvm/Makefile @@ -1 +1,2 @@ -obj-y += save.o +obj-$(CONFIG_X86) += save.o +obj-y += event.o diff --git a/xen/common/hvm/event.c b/xen/common/hvm/event.c new file mode 100644 index 0000000..28ceadc --- /dev/null +++ b/xen/common/hvm/event.c @@ -0,0 +1,96 @@ +/* + * xen/common/hvm/event.c + * + * Common hardware virtual machine event abstractions. + * + * 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/hvm/event.h> +#include <xen/vm_event.h> +#include <asm/hvm/event.h> +#if CONFIG_X86 +#include <asm/altp2m.h> +#endif + +int hvm_event_traps(struct vcpu *v, uint8_t sync, vm_event_request_t *req) +{ + int rc; + struct domain *d = v->domain; + + rc = vm_event_claim_slot(d, &d->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(v); + } + +#if CONFIG_X86 + if ( altp2m_active(d) ) + { + req->flags |= VM_EVENT_FLAG_ALTERNATE_P2M; + req->altp2m_idx = vcpu_altp2m(v).p2midx; + } +#endif + + arch_hvm_event_fill_regs(req); + + vm_event_put_request(d, &d->vm_event->monitor, req); + + return 1; +} + +void hvm_event_guest_request(void) +{ + struct vcpu *curr = current; + struct domain *d = curr->domain; + + if ( d->monitor.guest_request_enabled ) + { + vm_event_request_t req = { + .reason = VM_EVENT_REASON_GUEST_REQUEST, + .vcpu_id = curr->vcpu_id, + }; + + hvm_event_traps(curr, d->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/common/monitor.c b/xen/common/monitor.c index 2a99a04..7c3d1c8 100644 --- a/xen/common/monitor.c +++ b/xen/common/monitor.c @@ -28,6 +28,7 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) { int rc; + bool_t requested_status = 0; if ( unlikely(current->domain == d) ) /* no domain_pause() */ return -EPERM; @@ -39,15 +40,16 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) switch ( mop->op ) { case XEN_DOMCTL_MONITOR_OP_ENABLE: + requested_status = 1; + /* fallthrough */ case XEN_DOMCTL_MONITOR_OP_DISABLE: - /* Check if event type is available. */ /* sanity check: avoid left-shift undefined behavior */ if ( unlikely(mop->event > 31) ) return -EINVAL; + /* Check if event type is available. */ if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) ) return -EOPNOTSUPP; - /* Arch-side handles enable/disable ops. */ - return arch_monitor_domctl_event(d, mop); + break; case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES: mop->event = arch_monitor_get_capabilities(d); @@ -57,6 +59,29 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) /* The monitor op is probably handled on the arch-side. */ return arch_monitor_domctl_op(d, mop); } + + switch ( mop->event ) + { + case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST: + { + bool_t old_status = d->monitor.guest_request_enabled; + + if ( unlikely(old_status == requested_status) ) + return -EEXIST; + + domain_pause(d); + d->monitor.guest_request_sync = mop->u.guest_request.sync; + d->monitor.guest_request_enabled = requested_status; + domain_unpause(d); + break; + } + + default: + /* Give arch-side the chance to handle this event */ + return arch_monitor_domctl_event(d, mop); + } + + return 0; } /* diff --git a/xen/include/asm-arm/hvm/event.h b/xen/include/asm-arm/hvm/event.h new file mode 100644 index 0000000..cd93581 --- /dev/null +++ b/xen/include/asm-arm/hvm/event.h @@ -0,0 +1,41 @@ +/* + * include/asm-arm/hvm/event.h + * + * Arch-specific hardware virtual machine event abstractions. + * + * 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__ + +#include <public/vm_event.h> + +static inline void arch_hvm_event_fill_regs(vm_event_request_t *req) +{ + /* Not supported on ARM. */ +} + +#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 82a4a66..db1e357 100644 --- a/xen/include/asm-arm/monitor.h +++ b/xen/include/asm-arm/monitor.h @@ -27,8 +27,11 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d) { - /* No monitor vm-events implemented on ARM. */ - return 0; + uint32_t capabilities = 0; + + capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST); + + return capabilities; } static inline diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h index 4d0fbf7..017fb6f 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->monitor, 0, sizeof(d->monitor)); } static inline diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h index 4fad638..1de7a9e 100644 --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -376,17 +376,15 @@ struct arch_domain unsigned long *pirq_eoi_map; unsigned long pirq_eoi_map_mfn; - /* Monitor options */ + /* Arch-specific monitor options */ struct { - unsigned int write_ctrlreg_enabled : 4; - unsigned int write_ctrlreg_sync : 4; - unsigned int write_ctrlreg_onchangeonly : 4; - unsigned int mov_to_msr_enabled : 1; - unsigned int mov_to_msr_extended : 1; - unsigned int singlestep_enabled : 1; - unsigned int software_breakpoint_enabled : 1; - unsigned int guest_request_enabled : 1; - unsigned int guest_request_sync : 1; + uint16_t write_ctrlreg_enabled : 4; + uint16_t write_ctrlreg_sync : 4; + uint16_t write_ctrlreg_onchangeonly : 4; + uint16_t mov_to_msr_enabled : 1; + uint16_t mov_to_msr_extended : 1; + uint16_t singlestep_enabled : 1; + uint16_t software_breakpoint_enabled : 1; } monitor; /* Mem_access emulation control */ diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h index 7a83b45..6f70e83 100644 --- a/xen/include/asm-x86/hvm/event.h +++ b/xen/include/asm-x86/hvm/event.h @@ -1,5 +1,9 @@ /* - * event.h: Hardware virtual machine assist events. + * include/asm-x86/hvm/event.h + * + * Arch-specific hardware virtual machine event abstractions. + * + * 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, @@ -17,6 +21,42 @@ #ifndef __ASM_X86_HVM_EVENT_H__ #define __ASM_X86_HVM_EVENT_H__ +#include <xen/sched.h> +#include <xen/paging.h> +#include <public/vm_event.h> + +static inline void arch_hvm_event_fill_regs(vm_event_request_t *req) +{ + const struct cpu_user_regs *regs = guest_cpu_user_regs(); + const struct vcpu *curr = current; + + req->data.regs.x86.rax = regs->eax; + req->data.regs.x86.rcx = regs->ecx; + req->data.regs.x86.rdx = regs->edx; + req->data.regs.x86.rbx = regs->ebx; + req->data.regs.x86.rsp = regs->esp; + req->data.regs.x86.rbp = regs->ebp; + req->data.regs.x86.rsi = regs->esi; + req->data.regs.x86.rdi = regs->edi; + + req->data.regs.x86.r8 = regs->r8; + req->data.regs.x86.r9 = regs->r9; + req->data.regs.x86.r10 = regs->r10; + req->data.regs.x86.r11 = regs->r11; + req->data.regs.x86.r12 = regs->r12; + req->data.regs.x86.r13 = regs->r13; + req->data.regs.x86.r14 = regs->r14; + req->data.regs.x86.r15 = regs->r15; + + req->data.regs.x86.rflags = regs->eflags; + req->data.regs.x86.rip = regs->eip; + + req->data.regs.x86.msr_efer = curr->arch.hvm_vcpu.guest_efer; + req->data.regs.x86.cr0 = curr->arch.hvm_vcpu.guest_cr[0]; + req->data.regs.x86.cr3 = curr->arch.hvm_vcpu.guest_cr[3]; + req->data.regs.x86.cr4 = curr->arch.hvm_vcpu.guest_cr[4]; +} + enum hvm_event_breakpoint_type { HVM_EVENT_SOFTWARE_BREAKPOINT, @@ -35,7 +75,6 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value, void hvm_event_msr(unsigned int msr, uint64_t value); int hvm_event_breakpoint(unsigned long rip, enum hvm_event_breakpoint_type type); -void hvm_event_guest_request(void); #endif /* __ASM_X86_HVM_EVENT_H__ */ diff --git a/xen/include/xen/hvm/event.h b/xen/include/xen/hvm/event.h new file mode 100644 index 0000000..941d611 --- /dev/null +++ b/xen/include/xen/hvm/event.h @@ -0,0 +1,41 @@ +/* + * include/xen/hvm/event.h + * + * Common hardware virtual machine event abstractions. + * + * 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 __XEN_HVM_EVENT_H__ +#define __XEN_HVM_EVENT_H__ + +#include <xen/sched.h> +#include <public/vm_event.h> + +void hvm_event_guest_request(void); + +int hvm_event_traps(struct vcpu *v, uint8_t sync, vm_event_request_t *req); + +#endif /* __XEN_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/xen/sched.h b/xen/include/xen/sched.h index b47a3fe..c2efb49 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -464,6 +464,12 @@ struct domain /* vNUMA topology accesses are protected by rwlock. */ rwlock_t vnuma_rwlock; struct vnuma_info *vnuma; + + /* Common monitor options */ + struct { + uint8_t guest_request_enabled : 1; + uint8_t guest_request_sync : 1; + } monitor; }; /* Protect updates/reads (resp.) of domain_list and domain_hash. */
This patch adds ARM support for guest-request monitor vm-events. Summary of changes: == Moved to common-side: * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86 arch_monitor_domctl_event to common monitor_domctl) * hvm_event_guest_request, hvm_event_traps (also added target vcpu as param) * guest-request bits from X86 'struct arch_domain' (to common 'struct domain') == ARM implementations: * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls hvm_event_guest_request (as on X86) * arch_monitor_get_capabilities: updated to reflect support for XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST * vm_event_init_domain (does nothing), vm_event_cleanup_domain == Misc: * hvm_event_fill_regs renamed to arch_hvm_event_fill_regs, no longer X86-specific. ARM-side implementation of this function currently does nothing, that will be added in a separate patch. Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> --- MAINTAINERS | 1 + xen/arch/arm/hvm.c | 8 +++ xen/arch/x86/hvm/event.c | 116 ++++++---------------------------------- xen/arch/x86/hvm/hvm.c | 1 + xen/arch/x86/monitor.c | 14 ----- xen/arch/x86/vm_event.c | 1 + xen/common/Makefile | 2 +- xen/common/hvm/Makefile | 3 +- xen/common/hvm/event.c | 96 +++++++++++++++++++++++++++++++++ xen/common/monitor.c | 31 +++++++++-- xen/include/asm-arm/hvm/event.h | 41 ++++++++++++++ xen/include/asm-arm/monitor.h | 7 ++- xen/include/asm-arm/vm_event.h | 4 +- xen/include/asm-x86/domain.h | 18 +++---- xen/include/asm-x86/hvm/event.h | 43 ++++++++++++++- xen/include/xen/hvm/event.h | 41 ++++++++++++++ xen/include/xen/sched.h | 6 +++ 17 files changed, 299 insertions(+), 134 deletions(-) create mode 100644 xen/common/hvm/event.c create mode 100644 xen/include/asm-arm/hvm/event.h create mode 100644 xen/include/xen/hvm/event.h