[v5,09/13] xen: move do_nmi_op and make it x86 only

Message ID	20170626162842.482-10-wei.liu2@citrix.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <xen-devel-bounces@lists.xen.org> From: Wei Liu <wei.liu2@citrix.com> To: Xen-devel <xen-devel@lists.xenproject.org> Date: Mon, 26 Jun 2017 17:28:38 +0100 Message-ID: <20170626162842.482-10-wei.liu2@citrix.com> In-Reply-To: <20170626162842.482-1-wei.liu2@citrix.com> References: <20170626162842.482-1-wei.liu2@citrix.com> MIME-Version: 1.0 Cc: Andrew Cooper <andrew.cooper3@citrix.com>, Julien Grall <julien.grall@arm.com>, Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wei.liu2@citrix.com>, Jan Beulich <jbeulich@suse.com> Subject: [Xen-devel] [PATCH v5 09/13] xen: move do_nmi_op and make it x86 only Precedence: list Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" <xen-devel-bounces@lists.xen.org>

Wei Liu June 26, 2017, 4:28 p.m. UTC

Since ARM doesn't need do_nmi_op, move the hypercall handler from
common/kernel.c to pv/callback.c. Drop the stubs in ARM. Delete the
common and ARM nmi.h and adjust header inclusions in various files.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>

ARM build test passed.
---
 xen/arch/x86/nmi.c              |  2 +-
 xen/arch/x86/oprofile/nmi_int.c |  2 +-
 xen/arch/x86/pv/callback.c      | 49 +++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/traps.c            |  2 +-
 xen/arch/x86/x86_64/traps.c     |  2 +-
 xen/common/compat/kernel.c      |  5 -----
 xen/common/kernel.c             | 26 ----------------------
 xen/include/asm-arm/nmi.h       | 15 -------------
 xen/include/xen/nmi.h           | 14 ------------
 9 files changed, 53 insertions(+), 64 deletions(-)
 delete mode 100644 xen/include/asm-arm/nmi.h
 delete mode 100644 xen/include/xen/nmi.h

Andrew Cooper June 27, 2017, 6:08 p.m. UTC | #1

On 26/06/17 17:28, Wei Liu wrote:
> Since ARM doesn't need do_nmi_op, move the hypercall handler from
> common/kernel.c to pv/callback.c. Drop the stubs in ARM. Delete the
> common and ARM nmi.h and adjust header inclusions in various files.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Jan Beulich June 27, 2017, 6:31 p.m. UTC | #2

>>> Wei Liu <wei.liu2@citrix.com> 06/26/17 6:29 PM >>>
>Since ARM doesn't need do_nmi_op, move the hypercall handler from
>common/kernel.c to pv/callback.c.

There are two handlers you actually move, and while their neither large nor
likely to change, I still somewhat dislike the code duplication you introduce.
But I guess not enough to put under question the R-b you've already got
from Andrew.

Jan

Andrew Cooper June 27, 2017, 6:50 p.m. UTC | #3

On 27/06/17 19:31, Jan Beulich wrote:
>>>> Wei Liu <wei.liu2@citrix.com> 06/26/17 6:29 PM >>>
>> Since ARM doesn't need do_nmi_op, move the hypercall handler from
>> common/kernel.c to pv/callback.c.
> There are two handlers you actually move, and while their neither large nor
> likely to change, I still somewhat dislike the code duplication you introduce.
> But I guess not enough to put under question the R-b you've already got
> from Andrew.

If its not obvious already, I am taking a strong stance against any code
obfuscation, because obfuscation the source code is entirely detrimental
to the project.

Macros such as DO() fall into the obfuscation category, so I will
support all changes which reduce/remove its usage.

~Andrew

Stefano Stabellini June 27, 2017, 8:34 p.m. UTC | #4

On Mon, 26 Jun 2017, Wei Liu wrote:
> Since ARM doesn't need do_nmi_op, move the hypercall handler from
> common/kernel.c to pv/callback.c. Drop the stubs in ARM. Delete the
> common and ARM nmi.h and adjust header inclusions in various files.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> 
> ARM build test passed.
> ---
>  xen/arch/x86/nmi.c              |  2 +-
>  xen/arch/x86/oprofile/nmi_int.c |  2 +-
>  xen/arch/x86/pv/callback.c      | 49 +++++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/traps.c            |  2 +-
>  xen/arch/x86/x86_64/traps.c     |  2 +-
>  xen/common/compat/kernel.c      |  5 -----
>  xen/common/kernel.c             | 26 ----------------------
>  xen/include/asm-arm/nmi.h       | 15 -------------
>  xen/include/xen/nmi.h           | 14 ------------
>  9 files changed, 53 insertions(+), 64 deletions(-)
>  delete mode 100644 xen/include/asm-arm/nmi.h
>  delete mode 100644 xen/include/xen/nmi.h
> 
> diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
> index 410cfa1f94..ced61fd17e 100644
> --- a/xen/arch/x86/nmi.c
> +++ b/xen/arch/x86/nmi.c
> @@ -17,7 +17,6 @@
>  #include <xen/lib.h>
>  #include <xen/mm.h>
>  #include <xen/irq.h>
> -#include <xen/nmi.h>
>  #include <xen/delay.h>
>  #include <xen/time.h>
>  #include <xen/sched.h>
> @@ -29,6 +28,7 @@
>  #include <asm/mc146818rtc.h>
>  #include <asm/msr.h>
>  #include <asm/mpspec.h>
> +#include <asm/nmi.h>
>  #include <asm/debugger.h>
>  #include <asm/div64.h>
>  #include <asm/apic.h>
> diff --git a/xen/arch/x86/oprofile/nmi_int.c b/xen/arch/x86/oprofile/nmi_int.c
> index 13534d4914..126f7a8d9f 100644
> --- a/xen/arch/x86/oprofile/nmi_int.c
> +++ b/xen/arch/x86/oprofile/nmi_int.c
> @@ -15,7 +15,6 @@
>  #include <xen/types.h>
>  #include <xen/errno.h>
>  #include <xen/init.h>
> -#include <xen/nmi.h>
>  #include <xen/string.h>
>  #include <xen/delay.h>
>  #include <xen/xenoprof.h>
> @@ -24,6 +23,7 @@
>  #include <asm/apic.h>
>  #include <asm/regs.h>
>  #include <asm/current.h>
> +#include <asm/nmi.h>
>  
>  #include "op_counter.h"
>  #include "op_x86_model.h"
> diff --git a/xen/arch/x86/pv/callback.c b/xen/arch/x86/pv/callback.c
> index b9dd39bf0e..5317ae8f05 100644
> --- a/xen/arch/x86/pv/callback.c
> +++ b/xen/arch/x86/pv/callback.c
> @@ -22,6 +22,7 @@
>  #include <xen/lib.h>
>  #include <xen/sched.h>
>  #include <compat/callback.h>
> +#include <compat/nmi.h>
>  
>  #include <asm/current.h>
>  #include <asm/nmi.h>
> @@ -411,6 +412,54 @@ int compat_set_trap_table(XEN_GUEST_HANDLE(trap_info_compat_t) traps)
>      return rc;
>  }
>  
> +long do_nmi_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +    struct xennmi_callback cb;
> +    long rc = 0;
> +
> +    switch ( cmd )
> +    {
> +    case XENNMI_register_callback:
> +        rc = -EFAULT;
> +        if ( copy_from_guest(&cb, arg, 1) )
> +            break;
> +        rc = register_guest_nmi_callback(cb.handler_address);
> +        break;
> +    case XENNMI_unregister_callback:
> +        rc = unregister_guest_nmi_callback();
> +        break;
> +    default:
> +        rc = -ENOSYS;
> +        break;
> +    }
> +
> +    return rc;
> +}
> +
> +int compat_nmi_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +    struct compat_nmi_callback cb;
> +    int rc = 0;
> +
> +    switch ( cmd )
> +    {
> +    case XENNMI_register_callback:
> +        rc = -EFAULT;
> +        if ( copy_from_guest(&cb, arg, 1) )
> +            break;
> +        rc = register_guest_nmi_callback(cb.handler_address);
> +        break;
> +    case XENNMI_unregister_callback:
> +        rc = unregister_guest_nmi_callback();
> +        break;
> +    default:
> +        rc = -ENOSYS;
> +        break;
> +    }
> +
> +    return rc;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index d89409ff05..58f52926d9 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -43,7 +43,6 @@
>  #include <xen/domain_page.h>
>  #include <xen/symbols.h>
>  #include <xen/iocap.h>
> -#include <xen/nmi.h>
>  #include <xen/version.h>
>  #include <xen/kexec.h>
>  #include <xen/trace.h>
> @@ -64,6 +63,7 @@
>  #include <asm/xstate.h>
>  #include <asm/debugger.h>
>  #include <asm/msr.h>
> +#include <asm/nmi.h>
>  #include <asm/shared.h>
>  #include <asm/x86_emulate.h>
>  #include <asm/traps.h>
> diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
> index a15231ca0c..41ec78f8fc 100644
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -10,7 +10,6 @@
>  #include <xen/console.h>
>  #include <xen/sched.h>
>  #include <xen/shutdown.h>
> -#include <xen/nmi.h>
>  #include <xen/guest_access.h>
>  #include <xen/watchdog.h>
>  #include <xen/hypercall.h>
> @@ -18,6 +17,7 @@
>  #include <asm/flushtlb.h>
>  #include <asm/traps.h>
>  #include <asm/event.h>
> +#include <asm/nmi.h>
>  #include <asm/msr.h>
>  #include <asm/page.h>
>  #include <asm/shared.h>
> diff --git a/xen/common/compat/kernel.c b/xen/common/compat/kernel.c
> index df93fdd89c..64232669d2 100644
> --- a/xen/common/compat/kernel.c
> +++ b/xen/common/compat/kernel.c
> @@ -9,11 +9,9 @@ asm(".file \"" __FILE__ "\"");
>  #include <xen/errno.h>
>  #include <xen/version.h>
>  #include <xen/sched.h>
> -#include <xen/nmi.h>
>  #include <xen/guest_access.h>
>  #include <asm/current.h>
>  #include <compat/xen.h>
> -#include <compat/nmi.h>
>  #include <compat/version.h>
>  
>  extern xen_commandline_t saved_cmdline;
> @@ -39,9 +37,6 @@ CHECK_TYPE(capabilities_info);
>  
>  CHECK_TYPE(domain_handle);
>  
> -#define xennmi_callback compat_nmi_callback
> -#define xennmi_callback_t compat_nmi_callback_t
> -
>  #ifdef COMPAT_VM_ASSIST_VALID
>  #undef VM_ASSIST_VALID
>  #define VM_ASSIST_VALID COMPAT_VM_ASSIST_VALID
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index e1ebb0b412..ce7cb8adb5 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -10,12 +10,10 @@
>  #include <xen/version.h>
>  #include <xen/sched.h>
>  #include <xen/paging.h>
> -#include <xen/nmi.h>
>  #include <xen/guest_access.h>
>  #include <xen/hypercall.h>
>  #include <xsm/xsm.h>
>  #include <asm/current.h>
> -#include <public/nmi.h>
>  #include <public/version.h>
>  
>  #ifndef COMPAT
> @@ -431,30 +429,6 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>      return -ENOSYS;
>  }
>  
> -DO(nmi_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> -{
> -    struct xennmi_callback cb;
> -    long rc = 0;
> -
> -    switch ( cmd )
> -    {
> -    case XENNMI_register_callback:
> -        rc = -EFAULT;
> -        if ( copy_from_guest(&cb, arg, 1) )
> -            break;
> -        rc = register_guest_nmi_callback(cb.handler_address);
> -        break;
> -    case XENNMI_unregister_callback:
> -        rc = unregister_guest_nmi_callback();
> -        break;
> -    default:
> -        rc = -ENOSYS;
> -        break;
> -    }
> -
> -    return rc;
> -}
> -
>  #ifdef VM_ASSIST_VALID
>  DO(vm_assist)(unsigned int cmd, unsigned int type)
>  {
> diff --git a/xen/include/asm-arm/nmi.h b/xen/include/asm-arm/nmi.h
> deleted file mode 100644
> index a60587e7d1..0000000000
> --- a/xen/include/asm-arm/nmi.h
> +++ /dev/null
> @@ -1,15 +0,0 @@
> -#ifndef ASM_NMI_H
> -#define ASM_NMI_H
> -
> -#define register_guest_nmi_callback(a)  (-ENOSYS)
> -#define unregister_guest_nmi_callback() (-ENOSYS)
> -
> -#endif /* ASM_NMI_H */
> -/*
> - * Local variables:
> - * mode: C
> - * c-file-style: "BSD"
> - * c-basic-offset: 4
> - * indent-tabs-mode: nil
> - * End:
> - */
> diff --git a/xen/include/xen/nmi.h b/xen/include/xen/nmi.h
> deleted file mode 100644
> index e526b6ab6f..0000000000
> --- a/xen/include/xen/nmi.h
> +++ /dev/null
> @@ -1,14 +0,0 @@
> -/******************************************************************************
> - * nmi.h
> - *
> - * Register and unregister NMI callbacks.
> - *
> - * Copyright (c) 2006, Ian Campbell <ian.campbell@xensource.com>
> - */
> -
> -#ifndef __XEN_NMI_H__
> -#define __XEN_NMI_H__
> -
> -#include <asm/nmi.h>
> -
> -#endif /* __XEN_NMI_H__ */
> -- 
> 2.11.0
>

Wei Liu June 28, 2017, 10:12 a.m. UTC | #5

On Tue, Jun 27, 2017 at 12:31:20PM -0600, Jan Beulich wrote:
> >>> Wei Liu <wei.liu2@citrix.com> 06/26/17 6:29 PM >>>
> >Since ARM doesn't need do_nmi_op, move the hypercall handler from
> >common/kernel.c to pv/callback.c.
> 
> There are two handlers you actually move, and while their neither large nor
> likely to change, I still somewhat dislike the code duplication you introduce.
> But I guess not enough to put under question the R-b you've already got
> from Andrew.
> 

I'm with Andrew here. I deliberately unrolled all macros because the
resulting code is cleaner.

[v5,09/13] xen: move do_nmi_op and make it x86 only

Commit Message

Comments

Patch