From mboxrd@z Thu Jan 1 00:00:00 1970 From: Corneliu ZUZU Subject: Re: [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side. Date: Wed, 10 Feb 2016 19:34:48 +0200 Message-ID: <56BB74B8.1020207@bitdefender.com> References: <1455119259-2161-1-git-send-email-czuzu@bitdefender.com> <1455119548-2401-1-git-send-email-czuzu@bitdefender.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5485685844762245149==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel Cc: Kevin Tian , Keir Fraser , Ian Campbell , Razvan Cojocaru , Andrew Cooper , Xen-devel , Jan Beulich , Stefano Stabellini , Jun Nakajima List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --===============5485685844762245149== Content-Type: multipart/alternative; boundary="------------020108010103080500030409" This is a multi-part message in MIME format. --------------020108010103080500030409 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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 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. --------------020108010103080500030409 Content-Type: text/html; charset=windows-1252 Content-Length: 12059 Content-Transfer-Encoding: quoted-printable
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:
=A0 * Added Kconfigs for common monitor vm-events:
=A0 # see files: common/Kconfig, x86/Kconfig
=A0 =A0 HAS_VM_EVENT_WRITE_CTRLREG
=A0 =A0 HAS_VM_EVENT_SINGLESTEP
=A0 =A0 HAS_VM_EVENT_SOFTWARE_BREAKPOINT
=A0 =A0 HAS_VM_EVENT_GUEST_REQUEST

2. Moved monitor_domctl from arch-side to common-side
=A0 2.1. Moved arch/x86/monitor.c to common/monitor.c
=A0 =A0 # see files: arch/x86/Makefile, xen/common/Makefile, xen/common/monitor.c
=A0 =A0 # changes:
=A0 =A0 =A0 =A0 - removed status_check (we would have had to duplicate it in X86
=A0 =A0 =A0 =A0 =A0 =A0 arch_monitor_domctl_event otherwise)
=A0 =A0 =A0 =A0 - moved get_capabilities to arch-side (arch_monitor_get_capabilities)
=A0 =A0 =A0 =A0 - moved XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP to arch-side (see
=A0 =A0 =A0 =A0 =A0 =A0 arch_monitor_domctl_op)
=A0 =A0 =A0 =A0 - put XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR to x86-side (see
=A0 =A0 =A0 =A0 =A0 =A0 arch_monitor_domctl_event)
=A0 =A0 =A0 =A0 - surrounded switch cases w/ CONFIG_HAS_VM_EVENT_*

=A0 2.2. Moved asm-x86/monitor.h to xen/monitor.h
=A0 =A0 # see files: arch/x86/hvm/event.c, arch/x86/hvm/hvm.c,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0arch/x86/hvm/vmx/vmx.c, xen/common/domctl.c

=A0 2.3. Removed asm-arm/monitor.h (no longer needed)

3. Added x86/monitor_x86.c =3D> 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 =3D>
keeping the same name would have rendered an unnecessarily bulky diff)
=A0 =A0 # see files: arch/x86/Makefile
=A0 =A0 # 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.).
=A0 =A0 # define/implement: arch_monitor_get_capabilities, arch_monitor_domctl_op
=A0 =A0 =A0 =A0 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>
---
=A0xen/arch/x86/Kconfig=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 |=A0 =A04 +
=A0xen/arch/x86/Makefile=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0|=A0 =A02 +-
=A0xen/arch/x86/hvm/event.c=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 |=A0 =A02 +-
=A0xen/arch/x86/hvm/hvm.c=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 |=A0 =A02 +-
=A0xen/arch/x86/hvm/vmx/vmx.c=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 |=A0 =A02 +-
=A0xen/arch/x86/monitor_x86.c=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 |=A0 72 ++++++++
=A0xen/common/Kconfig=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 |=A0 20 +++
=A0xen/common/Makefile=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0|=A0 =A01 +
=A0xen/common/domctl.c=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0|=A0 =A02 +-
=A0xen/{arch/x86 =3D> common}/monitor.c=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | 195 +++++++++-------------
=A0xen/include/asm-arm/{monitor.h =3D> monitor_arch.h} |=A0 34 +++-
=A0xen/include/asm-x86/monitor_arch.h=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 |=A0 74 ++++++++
=A0xen/include/{asm-x86 =3D> xen}/monitor.h=A0 =A0 =A0 =A0 =A0 =A0 |=A0 17 +-
=A013 files changed, 293 insertions(+), 134 deletions(-)
=A0create mode 100644 xen/arch/x86/monitor_x86.c
=A0rename xen/{arch/x86 =3D> common}/monitor.c (44%)
=A0rename xen/include/asm-arm/{monitor.h =3D> monitor_arch.h} (46%)
=A0create mode 100644 xen/include/asm-x86/monitor_arch.h
=A0rename xen/include/{asm-x86 =3D> 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
=A0 =A0 =A0 =A0 select HAS_MEM_ACCESS
=A0 =A0 =A0 =A0 select HAS_MEM_PAGING
=A0 =A0 =A0 =A0 select HAS_MEM_SHARING
+=A0 =A0 =A0 =A0select 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 !=3D 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.

=A0
+bool_t arch_monitor_domctl_event(struct domain *d,
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct xen_domctl_monitor_op *mop,
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int *rc)
+{
+=A0 =A0 struct arch_domain *ad =3D &d->arch;
+=A0 =A0 bool_t requested_status =3D (XEN_DOMCTL_MONITOR_OP_ENABLE =3D=3D mop->op);
+
+=A0 =A0 switch ( mop->event )
+=A0 =A0 {
+=A0 =A0 =A0 =A0 case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
+=A0 =A0 =A0 =A0 {
+=A0 =A0 =A0 =A0 =A0 =A0 bool_t old_status =3D ad->monitor.mov_to_msr_enabled;
+
+=A0 =A0 =A0 =A0 =A0 =A0 if ( unlikely(old_status =3D=3D requested_status) )
+=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 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).
=A0

That's a baad mistake, good catch. It should be "*rc =3D -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.
--------------020108010103080500030409-- --===============5485685844762245149== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============5485685844762245149==--