| Message ID | 1455119548-2401-1-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 10.02.16 at 16:52, <czuzu@bitdefender.com> wrote: > xen/arch/x86/Kconfig | 4 + > xen/arch/x86/Makefile | 2 +- > xen/arch/x86/hvm/event.c | 2 +- > xen/arch/x86/hvm/hvm.c | 2 +- > xen/arch/x86/hvm/vmx/vmx.c | 2 +- > xen/arch/x86/monitor_x86.c | 72 ++++++++ > xen/common/Kconfig | 20 +++ > xen/common/Makefile | 1 + > xen/common/domctl.c | 2 +- > xen/{arch/x86 => common}/monitor.c | 195 +++++++++------------- > xen/include/asm-arm/{monitor.h => monitor_arch.h} | 34 +++- > xen/include/asm-x86/monitor_arch.h | 74 ++++++++ > xen/include/{asm-x86 => xen}/monitor.h | 17 +- > 13 files changed, 293 insertions(+), 134 deletions(-) > create mode 100644 xen/arch/x86/monitor_x86.c > rename xen/{arch/x86 => common}/monitor.c (44%) > rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%) > create mode 100644 xen/include/asm-x86/monitor_arch.h > rename xen/include/{asm-x86 => xen}/monitor.h (74%) With percentages as low as 44 I'm not sure all this strange renaming and introduction of oddly named new files is actually a good idea. > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -14,6 +14,10 @@ config X86 > select HAS_MEM_ACCESS > select HAS_MEM_PAGING > select HAS_MEM_SHARING > + select HAS_VM_EVENT_WRITE_CTRLREG > + select HAS_VM_EVENT_SINGLESTEP > + select HAS_VM_EVENT_SOFTWARE_BREAKPOINT > + select HAS_VM_EVENT_GUEST_REQUEST > select HAS_NS16550 > select HAS_PASSTHROUGH > select HAS_PCI Please don't break the alphabetic ordering. > --- a/xen/arch/x86/monitor.c > +++ b/xen/common/monitor.c > @@ -1,9 +1,10 @@ > /* > - * arch/x86/monitor.c > + * xen/common/monitor.c > * > - * Architecture-specific monitor_op domctl handler. > + * Common monitor_op domctl handler. > * > * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) > + * Copyright (c) 2016, Bitdefender S.R.L. > * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public > @@ -18,101 +19,66 @@ > * License along with this program; If not, see <http://www.gnu.org/licenses/>. > */ > > -#include <xen/config.h> > -#include <xen/sched.h> > -#include <xen/mm.h> > -#include <asm/domain.h> > -#include <asm/monitor.h> > -#include <public/domctl.h> > +#include <xen/monitor.h> > +#include <xen/sched.h> /* for domain_pause, ... */ > +#include <xen/config.h> /* for XENLOG_WARNING */ Please simply drop this include, it's being automatically included via compiler command line option. Also please avoid comments like this unless they explain an otherwise unexpected or unreasonable inclusion. Jan
>>> On 10.02.16 at 17:26, <JBeulich@suse.com> wrote: >>>> On 10.02.16 at 16:52, <czuzu@bitdefender.com> wrote: >> xen/arch/x86/Kconfig | 4 + >> xen/arch/x86/Makefile | 2 +- >> xen/arch/x86/hvm/event.c | 2 +- >> xen/arch/x86/hvm/hvm.c | 2 +- >> xen/arch/x86/hvm/vmx/vmx.c | 2 +- >> xen/arch/x86/monitor_x86.c | 72 ++++++++ >> xen/common/Kconfig | 20 +++ >> xen/common/Makefile | 1 + >> xen/common/domctl.c | 2 +- >> xen/{arch/x86 => common}/monitor.c | 195 +++++++++------------- >> xen/include/asm-arm/{monitor.h => monitor_arch.h} | 34 +++- >> xen/include/asm-x86/monitor_arch.h | 74 ++++++++ >> xen/include/{asm-x86 => xen}/monitor.h | 17 +- >> 13 files changed, 293 insertions(+), 134 deletions(-) >> create mode 100644 xen/arch/x86/monitor_x86.c >> rename xen/{arch/x86 => common}/monitor.c (44%) >> rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%) >> create mode 100644 xen/include/asm-x86/monitor_arch.h >> rename xen/include/{asm-x86 => xen}/monitor.h (74%) > > With percentages as low as 44 I'm not sure all this strange > renaming and introduction of oddly named new files is actually a > good idea. Also it looks like this moving around of files needs to be accompanied by adjustments to ./MAINTAINERS. Jan
On Wed, Feb 10, 2016 at 8:52 AM, Corneliu ZUZU <czuzu@bitdefender.com> wrote: > 1. Kconfig: > * Added Kconfigs for common monitor vm-events: > # see files: common/Kconfig, x86/Kconfig > HAS_VM_EVENT_WRITE_CTRLREG > HAS_VM_EVENT_SINGLESTEP > HAS_VM_EVENT_SOFTWARE_BREAKPOINT > HAS_VM_EVENT_GUEST_REQUEST > > 2. Moved monitor_domctl from arch-side to common-side > 2.1. Moved arch/x86/monitor.c to common/monitor.c > # see files: arch/x86/Makefile, xen/common/Makefile, > xen/common/monitor.c > # changes: > - removed status_check (we would have had to duplicate it in X86 > arch_monitor_domctl_event otherwise) > - moved get_capabilities to arch-side > (arch_monitor_get_capabilities) > - moved XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP to arch-side (see > arch_monitor_domctl_op) > - put XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR to x86-side (see > arch_monitor_domctl_event) > - surrounded switch cases w/ CONFIG_HAS_VM_EVENT_* > > 2.2. Moved asm-x86/monitor.h to xen/monitor.h > # see files: arch/x86/hvm/event.c, arch/x86/hvm/hvm.c, > arch/x86/hvm/vmx/vmx.c, xen/common/domctl.c > > 2.3. Removed asm-arm/monitor.h (no longer needed) > > 3. Added x86/monitor_x86.c => will rename in next commit to monitor.c (not > done > in this commit to avoid git seeing this as being the modified old > monitor.c => > keeping the same name would have rendered an unnecessarily bulky diff) > # see files: arch/x86/Makefile > # implements X86-side arch_monitor_domctl_event > > 4. Added asm-x86/monitor_arch.h, asm-arm/monitor_arch.h (renamed to > monitor.h in next commit, reason is the same as @ (3.). > # define/implement: arch_monitor_get_capabilities, > arch_monitor_domctl_op > and arch_monitor_domctl_event > So these commit messages are not very good IMHO. Rather then just summarizing what the patch does you should describe why the patch is needed in the first place. Usually for longer series having a cover page is also helpful to outline how the patches in the series fit together. > > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> > --- > xen/arch/x86/Kconfig | 4 + > xen/arch/x86/Makefile | 2 +- > xen/arch/x86/hvm/event.c | 2 +- > xen/arch/x86/hvm/hvm.c | 2 +- > xen/arch/x86/hvm/vmx/vmx.c | 2 +- > xen/arch/x86/monitor_x86.c | 72 ++++++++ > xen/common/Kconfig | 20 +++ > xen/common/Makefile | 1 + > xen/common/domctl.c | 2 +- > xen/{arch/x86 => common}/monitor.c | 195 > +++++++++------------- > xen/include/asm-arm/{monitor.h => monitor_arch.h} | 34 +++- > xen/include/asm-x86/monitor_arch.h | 74 ++++++++ > xen/include/{asm-x86 => xen}/monitor.h | 17 +- > 13 files changed, 293 insertions(+), 134 deletions(-) > create mode 100644 xen/arch/x86/monitor_x86.c > rename xen/{arch/x86 => common}/monitor.c (44%) > rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%) > create mode 100644 xen/include/asm-x86/monitor_arch.h > rename xen/include/{asm-x86 => xen}/monitor.h (74%) > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > index 3a90f47..e46be1b 100644 > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -14,6 +14,10 @@ config X86 > select HAS_MEM_ACCESS > select HAS_MEM_PAGING > select HAS_MEM_SHARING > + select HAS_VM_EVENT_WRITE_CTRLREG > You mentioned in the previous revision of this series that currently you only have plans to implement write_ctrleg and guest_request events for ARM. I think singlestep and software_breakpoint should not be moved to common without a clear plan to have those implemented. Now, if you were to include the implementation of write_ctrlreg and guest_request in this series and leave the others in x86 as they are now, I don't think there would be any reason to have these Kconfig options present at all. > + select HAS_VM_EVENT_SINGLESTEP > + select HAS_VM_EVENT_SOFTWARE_BREAKPOINT > + select HAS_VM_EVENT_GUEST_REQUEST > select HAS_NS16550 > select HAS_PASSTHROUGH > select HAS_PCI > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile > index 8e6e901..6e80cf0 100644 > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -36,7 +36,7 @@ obj-y += microcode_intel.o > # This must come after the vendor specific files. > obj-y += microcode.o > obj-y += mm.o x86_64/mm.o > -obj-y += monitor.o > +obj-y += monitor_x86.o > obj-y += mpparse.o > obj-y += nmi.o > obj-y += numa.o > diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c > index e3444db..04faa72 100644 > --- a/xen/arch/x86/hvm/event.c > +++ b/xen/arch/x86/hvm/event.c > @@ -23,8 +23,8 @@ > > #include <xen/vm_event.h> > #include <xen/paging.h> > +#include <xen/monitor.h> > #include <asm/hvm/event.h> > -#include <asm/monitor.h> > #include <asm/altp2m.h> > #include <public/vm_event.h> > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 930d0e3..e93a648 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/monitor.h> > #include <xen/cpu.h> > #include <xen/wait.h> > #include <xen/mem_access.h> > @@ -51,7 +52,6 @@ > #include <asm/traps.h> > #include <asm/mc146818rtc.h> > #include <asm/mce.h> > -#include <asm/monitor.h> > #include <asm/hvm/hvm.h> > #include <asm/hvm/vpt.h> > #include <asm/hvm/support.h> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index cf0e642..be67b60 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -25,6 +25,7 @@ > #include <xen/domain_page.h> > #include <xen/hypercall.h> > #include <xen/perfc.h> > +#include <xen/monitor.h> > #include <asm/current.h> > #include <asm/io.h> > #include <asm/iocap.h> > @@ -57,7 +58,6 @@ > #include <asm/hvm/nestedhvm.h> > #include <asm/altp2m.h> > #include <asm/event.h> > -#include <asm/monitor.h> > #include <public/arch-x86/cpuid.h> > > static bool_t __initdata opt_force_ept; > diff --git a/xen/arch/x86/monitor_x86.c b/xen/arch/x86/monitor_x86.c > new file mode 100644 > index 0000000..d19fd15 > --- /dev/null > +++ b/xen/arch/x86/monitor_x86.c > @@ -0,0 +1,72 @@ > +/* > + * arch/x86/monitor_x86.c > + * > + * Arch-specific monitor_op domctl handler. > + * > + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) > + * Copyright (c) 2016, Bitdefender S.R.L. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public > + * License v2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + * > + * You should have received a copy of the GNU General Public > + * License along with this program; If not, see < > http://www.gnu.org/licenses/>. > + */ > + > +#include <asm/monitor_arch.h> > + > +bool_t arch_monitor_domctl_event(struct domain *d, > + struct xen_domctl_monitor_op *mop, > + int *rc) > +{ > + struct arch_domain *ad = &d->arch; > + bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op); > + > + switch ( mop->event ) > + { > + case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR: > + { > + bool_t old_status = ad->monitor.mov_to_msr_enabled; > + > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > This function is defined to return bool_t and yet you are returning non-boolean error codes. I think it would be better if this function just had a single rc instead of two (not passing one rc as a pointer on input). > + > + if ( XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op && > + mop->u.mov_to_msr.extended_capture && > + !hvm_enable_msr_exit_interception(d) ) > + return -EOPNOTSUPP; > + > + domain_pause(d); > + > + if ( XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op && > + mop->u.mov_to_msr.extended_capture ) > + ad->monitor.mov_to_msr_extended = 1; > + else > + ad->monitor.mov_to_msr_extended = 0; > + > + ad->monitor.mov_to_msr_enabled = !old_status; > + domain_unpause(d); > + break; > + } > + > + default: > + return 0; > + } > + > + return 1; > +} > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ >
On 2/10/2016 6:26 PM, Jan Beulich wrote: >>>> On 10.02.16 at 16:52, <czuzu@bitdefender.com> wrote: >> xen/arch/x86/Kconfig | 4 + >> xen/arch/x86/Makefile | 2 +- >> xen/arch/x86/hvm/event.c | 2 +- >> xen/arch/x86/hvm/hvm.c | 2 +- >> xen/arch/x86/hvm/vmx/vmx.c | 2 +- >> xen/arch/x86/monitor_x86.c | 72 ++++++++ >> xen/common/Kconfig | 20 +++ >> xen/common/Makefile | 1 + >> xen/common/domctl.c | 2 +- >> xen/{arch/x86 => common}/monitor.c | 195 +++++++++------------- >> xen/include/asm-arm/{monitor.h => monitor_arch.h} | 34 +++- >> xen/include/asm-x86/monitor_arch.h | 74 ++++++++ >> xen/include/{asm-x86 => xen}/monitor.h | 17 +- >> 13 files changed, 293 insertions(+), 134 deletions(-) >> create mode 100644 xen/arch/x86/monitor_x86.c >> rename xen/{arch/x86 => common}/monitor.c (44%) >> rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%) >> create mode 100644 xen/include/asm-x86/monitor_arch.h >> rename xen/include/{asm-x86 => xen}/monitor.h (74%) > With percentages as low as 44 I'm not sure all this strange > renaming and introduction of oddly named new files is actually a > good idea. The diff would have actually looked a lot worse if I didn't do that, at least IMO. The "strange renaming and ..." was an effort I made to make reviewing these patches easier :). The reason is explained in the introductory message and in this commit message. >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -14,6 +14,10 @@ config X86 >> select HAS_MEM_ACCESS >> select HAS_MEM_PAGING >> select HAS_MEM_SHARING >> + select HAS_VM_EVENT_WRITE_CTRLREG >> + select HAS_VM_EVENT_SINGLESTEP >> + select HAS_VM_EVENT_SOFTWARE_BREAKPOINT >> + select HAS_VM_EVENT_GUEST_REQUEST >> select HAS_NS16550 >> select HAS_PASSTHROUGH >> select HAS_PCI > Please don't break the alphabetic ordering. Noted. Didn't realise they were in alphabetic order. > +#include <xen/config.h> /* for XENLOG_WARNING */ > Please simply drop this include, it's being automatically included via > compiler command line option. Also please avoid comments like this > unless they explain an otherwise unexpected or unreasonable > inclusion. > > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > Noted, noted. That you, Corneliu.
On 2/10/2016 6:30 PM, Jan Beulich wrote: >>>> On 10.02.16 at 17:26, <JBeulich@suse.com> wrote: >>>>> On 10.02.16 at 16:52, <czuzu@bitdefender.com> wrote: >>> xen/arch/x86/Kconfig | 4 + >>> xen/arch/x86/Makefile | 2 +- >>> xen/arch/x86/hvm/event.c | 2 +- >>> xen/arch/x86/hvm/hvm.c | 2 +- >>> xen/arch/x86/hvm/vmx/vmx.c | 2 +- >>> xen/arch/x86/monitor_x86.c | 72 ++++++++ >>> xen/common/Kconfig | 20 +++ >>> xen/common/Makefile | 1 + >>> xen/common/domctl.c | 2 +- >>> xen/{arch/x86 => common}/monitor.c | 195 +++++++++------------- >>> xen/include/asm-arm/{monitor.h => monitor_arch.h} | 34 +++- >>> xen/include/asm-x86/monitor_arch.h | 74 ++++++++ >>> xen/include/{asm-x86 => xen}/monitor.h | 17 +- >>> 13 files changed, 293 insertions(+), 134 deletions(-) >>> create mode 100644 xen/arch/x86/monitor_x86.c >>> rename xen/{arch/x86 => common}/monitor.c (44%) >>> rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%) >>> create mode 100644 xen/include/asm-x86/monitor_arch.h >>> rename xen/include/{asm-x86 => xen}/monitor.h (74%) >> With percentages as low as 44 I'm not sure all this strange >> renaming and introduction of oddly named new files is actually a >> good idea. > Also it looks like this moving around of files needs to be > accompanied by adjustments to ./MAINTAINERS. > > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > Will check that, indeed these might have broken something there. Corneliu.
On 2/10/2016 6:39 PM, Tamas K Lengyel wrote: > > > On Wed, Feb 10, 2016 at 8:52 AM, Corneliu ZUZU <czuzu@bitdefender.com > <mailto:czuzu@bitdefender.com>> wrote: > > 1. Kconfig: > * Added Kconfigs for common monitor vm-events: > # see files: common/Kconfig, x86/Kconfig > HAS_VM_EVENT_WRITE_CTRLREG > HAS_VM_EVENT_SINGLESTEP > HAS_VM_EVENT_SOFTWARE_BREAKPOINT > HAS_VM_EVENT_GUEST_REQUEST > > 2. Moved monitor_domctl from arch-side to common-side > 2.1. Moved arch/x86/monitor.c to common/monitor.c > # see files: arch/x86/Makefile, xen/common/Makefile, > xen/common/monitor.c > # changes: > - removed status_check (we would have had to duplicate it > in X86 > arch_monitor_domctl_event otherwise) > - moved get_capabilities to arch-side > (arch_monitor_get_capabilities) > - moved XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP to > arch-side (see > arch_monitor_domctl_op) > - put XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR to x86-side (see > arch_monitor_domctl_event) > - surrounded switch cases w/ CONFIG_HAS_VM_EVENT_* > > 2.2. Moved asm-x86/monitor.h to xen/monitor.h > # see files: arch/x86/hvm/event.c, arch/x86/hvm/hvm.c, > arch/x86/hvm/vmx/vmx.c, xen/common/domctl.c > > 2.3. Removed asm-arm/monitor.h (no longer needed) > > 3. Added x86/monitor_x86.c => will rename in next commit to > monitor.c (not done > in this commit to avoid git seeing this as being the modified old > monitor.c => > keeping the same name would have rendered an unnecessarily bulky diff) > # see files: arch/x86/Makefile > # implements X86-side arch_monitor_domctl_event > > 4. Added asm-x86/monitor_arch.h, asm-arm/monitor_arch.h (renamed to > monitor.h in next commit, reason is the same as @ (3.). > # define/implement: arch_monitor_get_capabilities, > arch_monitor_domctl_op > and arch_monitor_domctl_event > > > So these commit messages are not very good IMHO. Rather then just > summarizing what the patch does you should describe why the patch is > needed in the first place. Usually for longer series having a cover > page is also helpful to outline how the patches in the series fit > together. The intention was to make review easier (following the changes in parallel w/ the commit message). But indeed I was maybe too focused on that, should have started w/ stating the purpose of these changes rather than jumping directly to detailing them. Will try to compact these commit messages next time (maybe move details to the introductory section instead, as you suggest). > > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com > <mailto:czuzu@bitdefender.com>> > --- > xen/arch/x86/Kconfig | 4 + > xen/arch/x86/Makefile | 2 +- > xen/arch/x86/hvm/event.c | 2 +- > xen/arch/x86/hvm/hvm.c | 2 +- > xen/arch/x86/hvm/vmx/vmx.c | 2 +- > xen/arch/x86/monitor_x86.c | 72 ++++++++ > xen/common/Kconfig | 20 +++ > xen/common/Makefile | 1 + > xen/common/domctl.c | 2 +- > xen/{arch/x86 => common}/monitor.c | 195 > +++++++++------------- > xen/include/asm-arm/{monitor.h => monitor_arch.h} | 34 +++- > xen/include/asm-x86/monitor_arch.h | 74 ++++++++ > xen/include/{asm-x86 => xen}/monitor.h | 17 +- > 13 files changed, 293 insertions(+), 134 deletions(-) > create mode 100644 xen/arch/x86/monitor_x86.c > rename xen/{arch/x86 => common}/monitor.c (44%) > rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%) > create mode 100644 xen/include/asm-x86/monitor_arch.h > rename xen/include/{asm-x86 => xen}/monitor.h (74%) > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > index 3a90f47..e46be1b 100644 > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -14,6 +14,10 @@ config X86 > select HAS_MEM_ACCESS > select HAS_MEM_PAGING > select HAS_MEM_SHARING > + select HAS_VM_EVENT_WRITE_CTRLREG > > > You mentioned in the previous revision of this series that currently > you only have plans to implement write_ctrleg and guest_request events > for ARM. I think singlestep and software_breakpoint should not be > moved to common without a clear plan to have those implemented. Now, > if you were to include the implementation of write_ctrlreg and > guest_request in this series and leave the others in x86 as they are > now, I don't think there would be any reason to have these Kconfig > options present at all. Moving what made sense to be moved to common was a suggestion of Ian's. The purpose of this patch is to --avoid-- having to go through this process again when an implementation of feature X for architecture A != X86 comes into place. IMHO what is common should stay in common and I don't see any issues w/ having them there, only advantages (future implementations of these features will be easier). Maybe Ian can chime in on this. Regarding single-step & software-breakpoint monitor vm-events for ARM, that should be very feasible IMO, as I detailed in an email I sent to you in v1, in which I was pointing how we could use the debugging architecture. > > +bool_t arch_monitor_domctl_event(struct domain *d, > + struct xen_domctl_monitor_op *mop, > + int *rc) > +{ > + struct arch_domain *ad = &d->arch; > + bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == > mop->op); > + > + switch ( mop->event ) > + { > + case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR: > + { > + bool_t old_status = ad->monitor.mov_to_msr_enabled; > + > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > > > This function is defined to return bool_t and yet you are returning > non-boolean error codes. I think it would be better if this function > just had a single rc instead of two (not passing one rc as a pointer > on input). That's a baad mistake, good catch. It should be "*rc = -EEXIST; return 1", will change in v3. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel Thank you, Corneliu.
On Wed, Feb 10, 2016 at 10:34 AM, Corneliu ZUZU <czuzu@bitdefender.com> wrote: > > On 2/10/2016 6:39 PM, Tamas K Lengyel wrote: > > > > On Wed, Feb 10, 2016 at 8:52 AM, Corneliu ZUZU <czuzu@bitdefender.com> > wrote: > >> 1. Kconfig: >> * Added Kconfigs for common monitor vm-events: >> # see files: common/Kconfig, x86/Kconfig >> HAS_VM_EVENT_WRITE_CTRLREG >> HAS_VM_EVENT_SINGLESTEP >> HAS_VM_EVENT_SOFTWARE_BREAKPOINT >> HAS_VM_EVENT_GUEST_REQUEST >> >> 2. Moved monitor_domctl from arch-side to common-side >> 2.1. Moved arch/x86/monitor.c to common/monitor.c >> # see files: arch/x86/Makefile, xen/common/Makefile, >> xen/common/monitor.c >> # changes: >> - removed status_check (we would have had to duplicate it in X86 >> arch_monitor_domctl_event otherwise) >> - moved get_capabilities to arch-side >> (arch_monitor_get_capabilities) >> - moved XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP to arch-side (see >> arch_monitor_domctl_op) >> - put XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR to x86-side (see >> arch_monitor_domctl_event) >> - surrounded switch cases w/ CONFIG_HAS_VM_EVENT_* >> >> 2.2. Moved asm-x86/monitor.h to xen/monitor.h >> # see files: arch/x86/hvm/event.c, arch/x86/hvm/hvm.c, >> arch/x86/hvm/vmx/vmx.c, xen/common/domctl.c >> >> 2.3. Removed asm-arm/monitor.h (no longer needed) >> >> 3. Added x86/monitor_x86.c => will rename in next commit to monitor.c >> (not done >> in this commit to avoid git seeing this as being the modified old >> monitor.c => >> keeping the same name would have rendered an unnecessarily bulky diff) >> # see files: arch/x86/Makefile >> # implements X86-side arch_monitor_domctl_event >> >> 4. Added asm-x86/monitor_arch.h, asm-arm/monitor_arch.h (renamed to >> monitor.h in next commit, reason is the same as @ (3.). >> # define/implement: arch_monitor_get_capabilities, >> arch_monitor_domctl_op >> and arch_monitor_domctl_event >> > > So these commit messages are not very good IMHO. Rather then just > summarizing what the patch does you should describe why the patch is needed > in the first place. Usually for longer series having a cover page is also > helpful to outline how the patches in the series fit together. > > > The intention was to make review easier (following the changes in parallel > w/ > the commit message). But indeed I was maybe too focused on that, should > have > started w/ stating the purpose of these changes rather than jumping > directly to detailing > them. Will try to compact these commit messages next time (maybe move > details to the > introductory section instead, as you suggest). > > > >> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> >> --- >> xen/arch/x86/Kconfig | 4 + >> xen/arch/x86/Makefile | 2 +- >> xen/arch/x86/hvm/event.c | 2 +- >> xen/arch/x86/hvm/hvm.c | 2 +- >> xen/arch/x86/hvm/vmx/vmx.c | 2 +- >> xen/arch/x86/monitor_x86.c | 72 ++++++++ >> xen/common/Kconfig | 20 +++ >> xen/common/Makefile | 1 + >> xen/common/domctl.c | 2 +- >> xen/{arch/x86 => common}/monitor.c | 195 >> +++++++++------------- >> xen/include/asm-arm/{monitor.h => monitor_arch.h} | 34 +++- >> xen/include/asm-x86/monitor_arch.h | 74 ++++++++ >> xen/include/{asm-x86 => xen}/monitor.h | 17 +- >> 13 files changed, 293 insertions(+), 134 deletions(-) >> create mode 100644 xen/arch/x86/monitor_x86.c >> rename xen/{arch/x86 => common}/monitor.c (44%) >> rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%) >> create mode 100644 xen/include/asm-x86/monitor_arch.h >> rename xen/include/{asm-x86 => xen}/monitor.h (74%) >> >> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig >> index 3a90f47..e46be1b 100644 >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -14,6 +14,10 @@ config X86 >> select HAS_MEM_ACCESS >> select HAS_MEM_PAGING >> select HAS_MEM_SHARING >> + select HAS_VM_EVENT_WRITE_CTRLREG >> > > You mentioned in the previous revision of this series that currently you > only have plans to implement write_ctrleg and guest_request events for ARM. > I think singlestep and software_breakpoint should not be moved to common > without a clear plan to have those implemented. Now, if you were to include > the implementation of write_ctrlreg and guest_request in this series and > leave the others in x86 as they are now, I don't think there would be any > reason to have these Kconfig options present at all. > > > Moving what made sense to be moved to common was a suggestion of Ian's. > The purpose of this patch is to --avoid-- having to go through this > process again > when an implementation of feature X for architecture A != X86 comes into > place. > IMHO what is common should stay in common and I don't see any issues w/ > having them there, only advantages (future implementations of these > features will > be easier). > Maybe Ian can chime in on this. > That's the upside. The downside is that in the interim, while those features are not implemented, we need to add a bunch of Kconfig variables to decide under what build they are available. If it was moved to common only when the feature is available for all architectures, we wouldn't need that many ifdefs and the code would be clearer. So I do see why it would be beneficial having these moved to common now, but I still rather have it happen when it's necessary and without the Kconfig settings. Tamas
On 2/10/2016 6:39 PM, Tamas K Lengyel wrote: > I think it would be better if this function just had a single rc > instead of two (not passing one rc as a pointer on input). Good point. Would it be ok if: * I remove the rc param * make return type int * rc = 0 if arch-side didn't handle the event, but no errors occurred * rc = 1 if arch-side handled the event and no errors occurred * rc < 0 if errors occurred * return rc ? Didn't cross my mind why error rcs are < 0, I only now realize that probably just these kind of situations are the reason for that. Corneliu.
On 2/10/2016 7:56 PM, Tamas K Lengyel wrote: > > > On Wed, Feb 10, 2016 at 10:34 AM, Corneliu ZUZU <czuzu@bitdefender.com > <mailto:czuzu@bitdefender.com>> wrote: > > > On 2/10/2016 6:39 PM, Tamas K Lengyel wrote: >> >> >> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig >> index 3a90f47..e46be1b 100644 >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -14,6 +14,10 @@ config X86 >> select HAS_MEM_ACCESS >> select HAS_MEM_PAGING >> select HAS_MEM_SHARING >> + select HAS_VM_EVENT_WRITE_CTRLREG >> >> >> You mentioned in the previous revision of this series that >> currently you only have plans to implement write_ctrleg and >> guest_request events for ARM. I think singlestep and >> software_breakpoint should not be moved to common without a clear >> plan to have those implemented. Now, if you were to include the >> implementation of write_ctrlreg and guest_request in this series >> and leave the others in x86 as they are now, I don't think there >> would be any reason to have these Kconfig options present at all. > > Moving what made sense to be moved to common was a suggestion of > Ian's. > The purpose of this patch is to --avoid-- having to go through > this process again > when an implementation of feature X for architecture A != X86 > comes into place. > IMHO what is common should stay in common and I don't see any > issues w/ > having them there, only advantages (future implementations of > these features will > be easier). > Maybe Ian can chime in on this. > > > That's the upside. The downside is that in the interim, while those > features are not implemented, we need to add a bunch of Kconfig > variables to decide under what build they are available. Technically, you don't need to add anything unless you implement that feature. E.g. the ARM Kconfig currently contains no mention of these options, since they're not implemented there @ all. And when implemented they're only added once and it's one line "select HAS_....", it's not like you have to specify a command-line parameter when building Xen or something like that, so IMO they don't add considerable complexity. And after all, these kind of situations (i.e. activating/deactivating code based on architecture) are why arch-specific Kconfigs exist, right? Why not use them? :) > If it was moved to common only when the feature is available for all > architectures, we wouldn't need that many ifdefs and the code would be > clearer. So I do see why it would be beneficial having these moved to > common now, but I still rather have it happen when it's necessary and > without the Kconfig settings. What if a 3rd architecture comes into place, you'd have to move them back from common to the arch-side and get back to the code duplication we just got rid of? And if you then also implement it for the 3rd architecture, you move them back to common from the arch-side? It seems uncomfortable having to keep track of all architectures when wanting to add such a feature implementation for just one of them. I don't know what & if such plans exist for Xen, but why make that future process of adding in support for other architectures unnecessarily complicated, even if it won't happen soon? Also, IMO the code is clearer like this: * you know what parts interest you when implementing parts of these features/when debugging/when simply looking @ the code * the #ifdefs make it possible for that code to be put in common => that makes it *clear* that those code parts are NOT architecture specific and their implementation can be safely used for all architectures. * code duplication is avoided => avoid having to reflect a modification when it happens in more places than it would be necessary There are disadvantages, everything has a downside but IMHO they are minor compared to the alternative. Ian, any comments on this? :) Thank you, Corneliu.
> > * the #ifdefs make it possible for that code to be put in common => that > makes it *clear* that those code parts are NOT > architecture specific and their implementation can be safely used for all > architectures. > The current practice has been to put empty static inline functions into architecture-specific headers if the part is not handled/implemented for the specific architecture. This has already been the case for monitor_domctl (there is already separate asm-arm/monitor.h and asm-x86/monitor.h) so it should be followed as more of the code moves into common. Tamas
On 2/11/2016 5:44 PM, Tamas K Lengyel wrote: > > * the #ifdefs make it possible for that code to be put in common > => that makes it *clear* that those code parts are NOT > architecture specific and their implementation can be safely used > for all architectures. > > > The current practice has been to put empty static inline functions > into architecture-specific headers if the part is not > handled/implemented for the specific architecture. This has already > been the case for monitor_domctl (there is already separate > asm-arm/monitor.h and asm-x86/monitor.h) so it should be followed as > more of the code moves into common. > > Tamas Point is, they *are* implemented, because that's *common* code, it doesn't make sense to be moved to the arch-side when you know that their implementation will be *the same* from arch-to-arch. Not *everything* needs to stay on the arch-side, just what is architecture-specific - that's why e.g. arch_hvm_event_fill_regs, arch_hvm_event_gfn_of_ip are not in common and are static inline functions as you say, because they have *different* implementations *depending on the architecture*. Finally, if Ian or any other ARM maintainer feels the same with moving code that can be moved and has been moved to common back on the arch-side (effectively undoing 50% of my efforts), I will do so. Corneliu.
On 2/12/2016 8:05 AM, Corneliu ZUZU wrote: > On 2/11/2016 5:44 PM, Tamas K Lengyel wrote: >> >> * the #ifdefs make it possible for that code to be put in common >> => that makes it *clear* that those code parts are NOT >> architecture specific and their implementation can be safely used >> for all architectures. >> >> >> The current practice has been to put empty static inline functions >> into architecture-specific headers if the part is not >> handled/implemented for the specific architecture. This has already >> been the case for monitor_domctl (there is already separate >> asm-arm/monitor.h and asm-x86/monitor.h) so it should be followed as >> more of the code moves into common. >> >> Tamas > > Point is, they *are* implemented, because that's *common* code, it > doesn't make sense to be moved to the arch-side > when you know that their implementation will be *the same* from > arch-to-arch. > Not *everything* needs to stay on the arch-side, just what is > architecture-specific - that's why e.g. arch_hvm_event_fill_regs, > arch_hvm_event_gfn_of_ip are not in common and are static inline > functions as you say, because they have *different* > implementations *depending on the architecture*. > > Finally, if Ian or any other ARM maintainer feels the same with moving > code that can be moved and has been moved to common > back on the arch-side (effectively undoing 50% of my efforts), I will > do so. > > Corneliu. > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel Actually, noted, will move single-step and software-breakpoint back on the arch-side as you suggest in v3. If someone else disagrees, I suppose it will be discussed then. Corneliu.
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 3a90f47..e46be1b 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -14,6 +14,10 @@ config X86 select HAS_MEM_ACCESS select HAS_MEM_PAGING select HAS_MEM_SHARING + select HAS_VM_EVENT_WRITE_CTRLREG + select HAS_VM_EVENT_SINGLESTEP + select HAS_VM_EVENT_SOFTWARE_BREAKPOINT + select HAS_VM_EVENT_GUEST_REQUEST select HAS_NS16550 select HAS_PASSTHROUGH select HAS_PCI diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 8e6e901..6e80cf0 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -36,7 +36,7 @@ obj-y += microcode_intel.o # This must come after the vendor specific files. obj-y += microcode.o obj-y += mm.o x86_64/mm.o -obj-y += monitor.o +obj-y += monitor_x86.o obj-y += mpparse.o obj-y += nmi.o obj-y += numa.o diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c index e3444db..04faa72 100644 --- a/xen/arch/x86/hvm/event.c +++ b/xen/arch/x86/hvm/event.c @@ -23,8 +23,8 @@ #include <xen/vm_event.h> #include <xen/paging.h> +#include <xen/monitor.h> #include <asm/hvm/event.h> -#include <asm/monitor.h> #include <asm/altp2m.h> #include <public/vm_event.h> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 930d0e3..e93a648 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/monitor.h> #include <xen/cpu.h> #include <xen/wait.h> #include <xen/mem_access.h> @@ -51,7 +52,6 @@ #include <asm/traps.h> #include <asm/mc146818rtc.h> #include <asm/mce.h> -#include <asm/monitor.h> #include <asm/hvm/hvm.h> #include <asm/hvm/vpt.h> #include <asm/hvm/support.h> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index cf0e642..be67b60 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -25,6 +25,7 @@ #include <xen/domain_page.h> #include <xen/hypercall.h> #include <xen/perfc.h> +#include <xen/monitor.h> #include <asm/current.h> #include <asm/io.h> #include <asm/iocap.h> @@ -57,7 +58,6 @@ #include <asm/hvm/nestedhvm.h> #include <asm/altp2m.h> #include <asm/event.h> -#include <asm/monitor.h> #include <public/arch-x86/cpuid.h> static bool_t __initdata opt_force_ept; diff --git a/xen/arch/x86/monitor_x86.c b/xen/arch/x86/monitor_x86.c new file mode 100644 index 0000000..d19fd15 --- /dev/null +++ b/xen/arch/x86/monitor_x86.c @@ -0,0 +1,72 @@ +/* + * arch/x86/monitor_x86.c + * + * Arch-specific monitor_op domctl handler. + * + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) + * Copyright (c) 2016, Bitdefender S.R.L. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public + * License v2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public + * License along with this program; If not, see <http://www.gnu.org/licenses/>. + */ + +#include <asm/monitor_arch.h> + +bool_t arch_monitor_domctl_event(struct domain *d, + struct xen_domctl_monitor_op *mop, + int *rc) +{ + struct arch_domain *ad = &d->arch; + bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op); + + switch ( mop->event ) + { + case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR: + { + bool_t old_status = ad->monitor.mov_to_msr_enabled; + + if ( unlikely(old_status == requested_status) ) + return -EEXIST; + + if ( XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op && + mop->u.mov_to_msr.extended_capture && + !hvm_enable_msr_exit_interception(d) ) + return -EOPNOTSUPP; + + domain_pause(d); + + if ( XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op && + mop->u.mov_to_msr.extended_capture ) + ad->monitor.mov_to_msr_extended = 1; + else + ad->monitor.mov_to_msr_extended = 0; + + ad->monitor.mov_to_msr_enabled = !old_status; + domain_unpause(d); + break; + } + + default: + return 0; + } + + return 1; +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 6f404b4..172da13 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -36,6 +36,26 @@ config HAS_MEM_PAGING config HAS_MEM_SHARING bool +config HAS_VM_EVENT_WRITE_CTRLREG + bool + ---help--- + Select if ctrl-reg write monitor vm-events are supported + +config HAS_VM_EVENT_SINGLESTEP + bool + ---help--- + Select if single-step monitor vm-events are supported + +config HAS_VM_EVENT_SOFTWARE_BREAKPOINT + bool + ---help--- + Select if software-breakpoint monitor vm-events are supported + +config HAS_VM_EVENT_GUEST_REQUEST + bool + ---help--- + Select if guest-request monitor vm-events are supported + # Select HAS_PDX if PDX is supported config HAS_PDX bool diff --git a/xen/common/Makefile b/xen/common/Makefile index 6e82b33..0d76efe 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -20,6 +20,7 @@ obj-y += lib.o obj-y += lzo.o obj-$(CONFIG_HAS_MEM_ACCESS) += mem_access.o obj-y += memory.o +obj-y += monitor.o obj-y += multicall.o obj-y += notifier.o obj-y += page_alloc.o diff --git a/xen/common/domctl.c b/xen/common/domctl.c index 121a34a..4b1dec1 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -25,11 +25,11 @@ #include <xen/paging.h> #include <xen/hypercall.h> #include <xen/vm_event.h> +#include <xen/monitor.h> #include <asm/current.h> #include <asm/irq.h> #include <asm/page.h> #include <asm/p2m.h> -#include <asm/monitor.h> #include <public/domctl.h> #include <xsm/xsm.h> diff --git a/xen/arch/x86/monitor.c b/xen/common/monitor.c similarity index 44% rename from xen/arch/x86/monitor.c rename to xen/common/monitor.c index 1d43880..a4899c3 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/common/monitor.c @@ -1,9 +1,10 @@ /* - * arch/x86/monitor.c + * xen/common/monitor.c * - * Architecture-specific monitor_op domctl handler. + * Common monitor_op domctl handler. * * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) + * Copyright (c) 2016, Bitdefender S.R.L. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public @@ -18,101 +19,66 @@ * License along with this program; If not, see <http://www.gnu.org/licenses/>. */ -#include <xen/config.h> -#include <xen/sched.h> -#include <xen/mm.h> -#include <asm/domain.h> -#include <asm/monitor.h> -#include <public/domctl.h> +#include <xen/monitor.h> +#include <xen/sched.h> /* for domain_pause, ... */ +#include <xen/config.h> /* for XENLOG_WARNING */ #include <xsm/xsm.h> +#include <public/domctl.h> -/* - * Sanity check whether option is already enabled/disabled - */ -static inline -int status_check(struct xen_domctl_monitor_op *mop, bool_t status) -{ - bool_t requested_status = (mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE); - - if ( status == requested_status ) - return -EEXIST; - - return 0; -} - -static inline uint32_t get_capabilities(struct domain *d) -{ - uint32_t capabilities = 0; - - /* - * 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; - - capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) | - (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) | - (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) | - (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST); - - /* Since we know this is on VMX, we can just call the hvm func */ - if ( hvm_is_singlestep_supported() ) - capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP); - - return capabilities; -} +#include <asm/monitor_arch.h> /* for monitor_arch_# */ +#if CONFIG_X86 +#include <public/vm_event.h> /* for VM_EVENT_X86_CR3 */ +#include <asm/hvm/hvm.h> /* for hvm_update_guest_cr, ... */ +#endif int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) { int rc; - struct arch_domain *ad = &d->arch; - uint32_t capabilities = get_capabilities(d); + bool_t requested_status = 0; - if ( current->domain == d ) /* no domain_pause() */ + if ( unlikely(current->domain == d) ) /* no domain_pause() */ return -EPERM; rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event); - if ( rc ) + if ( unlikely(rc) ) return rc; switch ( mop->op ) { - case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES: - mop->event = capabilities; - return 0; + case XEN_DOMCTL_MONITOR_OP_ENABLE: + requested_status = 1; + /* fallthrough */ + case XEN_DOMCTL_MONITOR_OP_DISABLE: + /* Check if event type is available. */ + if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << mop->event))) ) + return -EOPNOTSUPP; + break; - case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: - domain_pause(d); - ad->mem_access_emulate_each_rep = !!mop->event; - domain_unpause(d); + case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES: + mop->event = arch_monitor_get_capabilities(d); return 0; - } - - /* - * Sanity check - */ - if ( mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE && - mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE ) - return -EOPNOTSUPP; - /* Check if event type is available. */ - if ( !(capabilities & (1 << mop->event)) ) + default: + /* The monitor op is probably handled on the arch-side. */ + if ( likely(arch_monitor_domctl_op(d, mop, &rc)) ) + return rc; + /* unrecognized op */ return -EOPNOTSUPP; + } switch ( mop->event ) { +#if CONFIG_HAS_VM_EVENT_WRITE_CTRLREG case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG: { + struct arch_domain *ad = &d->arch; unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index); - bool_t status = + bool_t old_status = !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask); - struct vcpu *v; - rc = status_check(mop, status); - if ( rc ) - return rc; + if ( unlikely(old_status == requested_status) ) + return -EEXIST; domain_pause(d); @@ -126,93 +92,92 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) else ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask; - if ( !status ) + if ( !old_status ) ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask; else ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask; - if ( mop->u.mov_to_cr.index == VM_EVENT_X86_CR3 ) - /* Latches new CR3 mask through CR0 code */ +#if CONFIG_X86 + if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index ) + { + struct vcpu *v; + /* Latches new CR3 mask through CR0 code. */ for_each_vcpu ( d, v ) hvm_update_guest_cr(v, 0); + } +#endif domain_unpause(d); break; } +#endif // HAS_VM_EVENT_WRITE_CTRLREG - case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR: - { - bool_t status = ad->monitor.mov_to_msr_enabled; - - rc = status_check(mop, status); - if ( rc ) - return rc; - - if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE && - mop->u.mov_to_msr.extended_capture && - !hvm_enable_msr_exit_interception(d) ) - return -EOPNOTSUPP; - - domain_pause(d); - - if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE && - mop->u.mov_to_msr.extended_capture ) - ad->monitor.mov_to_msr_extended = 1; - else - ad->monitor.mov_to_msr_extended = 0; - - ad->monitor.mov_to_msr_enabled = !status; - domain_unpause(d); - break; - } - +#if CONFIG_HAS_VM_EVENT_SINGLESTEP case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP: { - bool_t status = ad->monitor.singlestep_enabled; + struct arch_domain *ad = &d->arch; + bool_t old_status = ad->monitor.singlestep_enabled; - rc = status_check(mop, status); - if ( rc ) - return rc; + if ( unlikely(old_status == requested_status) ) + return -EEXIST; domain_pause(d); - ad->monitor.singlestep_enabled = !status; + ad->monitor.singlestep_enabled = !old_status; domain_unpause(d); break; } +#endif // HAS_VM_EVENT_SINGLESTEP +#if CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT: { - bool_t status = ad->monitor.software_breakpoint_enabled; + struct arch_domain *ad = &d->arch; + bool_t old_status = ad->monitor.software_breakpoint_enabled; - rc = status_check(mop, status); - if ( rc ) - return rc; + if ( unlikely(old_status == requested_status) ) + return -EEXIST; domain_pause(d); - ad->monitor.software_breakpoint_enabled = !status; + ad->monitor.software_breakpoint_enabled = !old_status; domain_unpause(d); break; } +#endif // HAS_VM_EVENT_SOFTWARE_BREAKPOINT +#if CONFIG_HAS_VM_EVENT_GUEST_REQUEST case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST: { - bool_t status = ad->monitor.guest_request_enabled; + struct arch_domain *ad = &d->arch; + bool_t old_status = ad->monitor.guest_request_enabled; - rc = status_check(mop, status); - if ( rc ) - return rc; + 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 = !status; + ad->monitor.guest_request_enabled = !old_status; domain_unpause(d); break; } +#endif // HAS_VM_EVENT_GUEST_REQUEST default: - return -EOPNOTSUPP; + /* Give arch-side the chance to handle this event */ + if ( likely(arch_monitor_domctl_event(d, mop, &rc)) ) + return rc; + + /* + * Should not be reached unless arch_monitor_get_capabilities() is not + * properly implemented. In that case, since reaching this point does + * not really break anything, don't crash the hypervisor, issue a + * warning instead of BUG(). + */ + printk(XENLOG_WARNING + "WARNING, BUG: arch_monitor_get_capabilities() not implemented" + "properly.\n"); + return -EOPNOTSUPP; }; return 0; diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor_arch.h similarity index 46% rename from xen/include/asm-arm/monitor.h rename to xen/include/asm-arm/monitor_arch.h index a3a9703..d0df66c 100644 --- a/xen/include/asm-arm/monitor.h +++ b/xen/include/asm-arm/monitor_arch.h @@ -1,9 +1,10 @@ /* - * include/asm-arm/monitor.h + * include/asm-arm/monitor_arch.h * - * Architecture-specific monitor_op domctl handler. + * Arch-specific monitor_op domctl handler. * * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) + * Copyright (c) 2016, Bitdefender S.R.L. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public @@ -18,16 +19,35 @@ * License along with this program; If not, see <http://www.gnu.org/licenses/>. */ -#ifndef __ASM_ARM_MONITOR_H__ -#define __ASM_ARM_MONITOR_H__ +#ifndef __ASM_ARM_MONITOR_ARCH_H__ +#define __ASM_ARM_MONITOR_ARCH_H__ #include <xen/sched.h> #include <public/domctl.h> static inline -int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op) +uint32_t arch_monitor_get_capabilities(struct domain *d) { - return -ENOSYS; + /* No monitor vm-events implemented on ARM. */ + return 0; } -#endif /* __ASM_X86_MONITOR_H__ */ +static inline +bool_t arch_monitor_domctl_op(struct domain *d, + struct xen_domctl_monitor_op *mop, + int *rc) +{ + /* No arch-specific monitor ops on ARM. */ + return 0; +} + +static inline +bool_t arch_monitor_domctl_event(struct domain *d, + struct xen_domctl_monitor_op *mop, + int *rc) +{ + /* No arch-specific monitor vm-events on ARM. */ + return 0; +} + +#endif /* __ASM_ARM_MONITOR_ARCH_H__ */ diff --git a/xen/include/asm-x86/monitor_arch.h b/xen/include/asm-x86/monitor_arch.h new file mode 100644 index 0000000..d9daf65 --- /dev/null +++ b/xen/include/asm-x86/monitor_arch.h @@ -0,0 +1,74 @@ +/* + * include/asm-x86/monitor_arch.h + * + * Arch-specific monitor_op domctl handler. + * + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) + * Copyright (c) 2016, Bitdefender S.R.L. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public + * License v2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public + * License along with this program; If not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef __ASM_X86_MONITOR_ARCH_H__ +#define __ASM_X86_MONITOR_ARCH_H__ + +#include <xen/sched.h> /* for struct domain, is_hvm_domain, ... */ +#include <public/domctl.h> /* for XEN_DOMCTL_MONITOR_#, ... */ +#include <asm/cpufeature.h> /* for cpu_has_vmx */ +#include <asm/hvm/hvm.h> /* for hvm_is_singlestep_supported */ + +static inline +uint32_t arch_monitor_get_capabilities(struct domain *d) +{ + uint32_t capabilities = 0; + + /* + * 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; + + capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) | + (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) | + (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) | + (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST); + + /* Since we know this is on VMX, we can just call the hvm func */ + if ( hvm_is_singlestep_supported() ) + capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP); + + return capabilities; +} + +static inline +bool_t arch_monitor_domctl_op(struct domain *d, + struct xen_domctl_monitor_op *mop, + int *rc) +{ + if( likely(XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP == mop->op) ) + { + domain_pause(d); + d->arch.mem_access_emulate_each_rep = !!mop->event; + domain_unpause(d); + *rc = 0; + return 1; + } + return 0; +} + +bool_t arch_monitor_domctl_event(struct domain *d, + struct xen_domctl_monitor_op *mop, + int *rc); + +#endif /* __ASM_X86_MONITOR_ARCH_H__ */ diff --git a/xen/include/asm-x86/monitor.h b/xen/include/xen/monitor.h similarity index 74% rename from xen/include/asm-x86/monitor.h rename to xen/include/xen/monitor.h index 7c8280b..edeff78 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/xen/monitor.h @@ -1,9 +1,10 @@ /* - * include/asm-x86/monitor.h + * include/xen/monitor.h * - * Architecture-specific monitor_op domctl handler. + * Common monitor_op domctl handler. * * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com) + * Copyright (c) 2016, Bitdefender S.R.L. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public @@ -18,14 +19,16 @@ * License along with this program; If not, see <http://www.gnu.org/licenses/>. */ -#ifndef __ASM_X86_MONITOR_H__ -#define __ASM_X86_MONITOR_H__ +#ifndef __MONITOR_H__ +#define __MONITOR_H__ -struct domain; -struct xen_domctl_monitor_op; +#include <xen/sched.h> +#include <public/domctl.h> +#if CONFIG_HAS_VM_EVENT_WRITE_CTRLREG #define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index)) +#endif int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); -#endif /* __ASM_X86_MONITOR_H__ */ +#endif /* __MONITOR_H__ */
1. Kconfig: * Added Kconfigs for common monitor vm-events: # see files: common/Kconfig, x86/Kconfig HAS_VM_EVENT_WRITE_CTRLREG HAS_VM_EVENT_SINGLESTEP HAS_VM_EVENT_SOFTWARE_BREAKPOINT HAS_VM_EVENT_GUEST_REQUEST 2. Moved monitor_domctl from arch-side to common-side 2.1. Moved arch/x86/monitor.c to common/monitor.c # see files: arch/x86/Makefile, xen/common/Makefile, xen/common/monitor.c # changes: - removed status_check (we would have had to duplicate it in X86 arch_monitor_domctl_event otherwise) - moved get_capabilities to arch-side (arch_monitor_get_capabilities) - moved XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP to arch-side (see arch_monitor_domctl_op) - put XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR to x86-side (see arch_monitor_domctl_event) - surrounded switch cases w/ CONFIG_HAS_VM_EVENT_* 2.2. Moved asm-x86/monitor.h to xen/monitor.h # see files: arch/x86/hvm/event.c, arch/x86/hvm/hvm.c, arch/x86/hvm/vmx/vmx.c, xen/common/domctl.c 2.3. Removed asm-arm/monitor.h (no longer needed) 3. Added x86/monitor_x86.c => will rename in next commit to monitor.c (not done in this commit to avoid git seeing this as being the modified old monitor.c => keeping the same name would have rendered an unnecessarily bulky diff) # see files: arch/x86/Makefile # implements X86-side arch_monitor_domctl_event 4. Added asm-x86/monitor_arch.h, asm-arm/monitor_arch.h (renamed to monitor.h in next commit, reason is the same as @ (3.). # define/implement: arch_monitor_get_capabilities, arch_monitor_domctl_op and arch_monitor_domctl_event Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> --- xen/arch/x86/Kconfig | 4 + xen/arch/x86/Makefile | 2 +- xen/arch/x86/hvm/event.c | 2 +- xen/arch/x86/hvm/hvm.c | 2 +- xen/arch/x86/hvm/vmx/vmx.c | 2 +- xen/arch/x86/monitor_x86.c | 72 ++++++++ xen/common/Kconfig | 20 +++ xen/common/Makefile | 1 + xen/common/domctl.c | 2 +- xen/{arch/x86 => common}/monitor.c | 195 +++++++++------------- xen/include/asm-arm/{monitor.h => monitor_arch.h} | 34 +++- xen/include/asm-x86/monitor_arch.h | 74 ++++++++ xen/include/{asm-x86 => xen}/monitor.h | 17 +- 13 files changed, 293 insertions(+), 134 deletions(-) create mode 100644 xen/arch/x86/monitor_x86.c rename xen/{arch/x86 => common}/monitor.c (44%) rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%) create mode 100644 xen/include/asm-x86/monitor_arch.h rename xen/include/{asm-x86 => xen}/monitor.h (74%)