Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

2016-02-12 Thread Corneliu ZUZU 

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.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

2016-02-11 Thread Corneliu ZUZU 

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

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

2016-02-11 Thread Tamas K Lengyel 
>
> * 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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

2016-02-10 Thread Corneliu ZUZU 

On 2/10/2016 6:26 PM, Jan Beulich wrote:

On 10.02.16 at 16:52,  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  /* 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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

2016-02-10 Thread Corneliu ZUZU 

On 2/10/2016 6:39 PM, Tamas K Lengyel wrote:



On Wed, Feb 10, 2016 at 8:52 AM, Corneliu ZUZU > 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 >
---
 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

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

2016-02-10 Thread Jan Beulich 
>>> On 10.02.16 at 16:52,  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 (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
> @@ -18,101 +19,66 @@
>   * License along with this program; If not, see 
> .
>   */
>  
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> +#include 
> +#include   /* for domain_pause, ... */
> +#include  /* 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

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

2016-02-10 Thread Jan Beulich 
>>> On 10.02.16 at 17:26,  wrote:
 On 10.02.16 at 16:52,  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

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

2016-02-10 Thread Tamas K Lengyel 
On Wed, Feb 10, 2016 at 8:52 AM, Corneliu ZUZU 
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 
> ---
>  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
> @@

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

2016-02-10 Thread Corneliu ZUZU 

On 2/10/2016 6:30 PM, Jan Beulich wrote:

On 10.02.16 at 17:26,  wrote:

On 10.02.16 at 16:52,  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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

2016-02-10 Thread Tamas K Lengyel 
On Wed, Feb 10, 2016 at 10:34 AM, Corneliu ZUZU 
wrote:

>
> On 2/10/2016 6:39 PM, Tamas K Lengyel wrote:
>
>
>
> On Wed, Feb 10, 2016 at 8:52 AM, Corneliu ZUZU 
> 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 
>> ---
>>  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
>

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

2016-02-10 Thread Corneliu ZUZU 

On 2/10/2016 7:56 PM, Tamas K Lengyel wrote:



On Wed, Feb 10, 2016 at 10:34 AM, Corneliu ZUZU > 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.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

2016-02-10 Thread Corneliu ZUZU 

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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel