On Wed, Feb 10, 2016 at 8:52 AM, Corneliu ZUZU <cz...@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 <cz...@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 (ta...@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:
> + */
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
