From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH v4 3/3] x86/monitor: don't use hvm_funcs directly Date: Thu, 9 Jul 2015 09:15:01 +0300 Message-ID: <559E1165.7080002@bitdefender.com> References: <1436395978-11048-1-git-send-email-tlengyel@novetta.com> <1436395978-11048-3-git-send-email-tlengyel@novetta.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1436395978-11048-3-git-send-email-tlengyel@novetta.com> 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 , xen-devel@lists.xen.org Cc: kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, eddie.dong@intel.com, jbeulich@suse.com, jun.nakajima@intel.com, andrew.cooper3@citrix.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org On 07/09/2015 01:52 AM, Tamas K Lengyel wrote: > A couple spots in x86/monitor used hvm_funcs directly. This patch adds an extra > wrapper for enable_msr_exit_interception and changes monitor.c to use only the > wrappers. > > Signed-off-by: Tamas K Lengyel > --- > xen/arch/x86/monitor.c | 7 ++----- > xen/include/asm-x86/hvm/hvm.h | 11 +++++++++++ > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c > index 590a45d..44efcd0 100644 > --- a/xen/arch/x86/monitor.c > +++ b/xen/arch/x86/monitor.c > @@ -130,7 +130,7 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) > if ( mop->u.mov_to_cr.index == VM_EVENT_X86_CR3 ) > /* Latches new CR3 mask through CR0 code */ > for_each_vcpu ( d, v ) > - hvm_funcs.update_guest_cr(v, 0); > + hvm_update_guest_cr(v, 0); > > break; > } > @@ -146,11 +146,8 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) > if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE && > mop->u.mov_to_msr.extended_capture ) > { > - if ( hvm_funcs.enable_msr_exit_interception ) > - { > + if ( hvm_enable_msr_exit_interception(d) == 0 ) > ad->monitor.mov_to_msr_extended = 1; > - hvm_funcs.enable_msr_exit_interception(d); I don't feel very strongly about it, so if you really prefer you can keep the code as it is, however this looks somewhat counterintuitive to me, especially when you compare the new condition to the old one, because ... > - } > else > return -EOPNOTSUPP; > } else > diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h > index aa8ab8d..9128dff 100644 > --- a/xen/include/asm-x86/hvm/hvm.h > +++ b/xen/include/asm-x86/hvm/hvm.h > @@ -514,6 +514,17 @@ static inline enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v) > return hvm_funcs.nhvm_intr_blocked(v); > } > > +static inline int hvm_enable_msr_exit_interception(struct domain *d) > +{ > + if ( hvm_funcs.enable_msr_exit_interception ) > + { > + hvm_funcs.enable_msr_exit_interception(d); > + return 0; > + } > + > + return -ENOSYS; > +} > + ... I can see no reason why this function should not return bool_t, and return 1 where it now returns 0, and 0 where it now returns -ENOSYS. But perhaps you're anticipating future uses I don't see now, or somebody else has a different opinion. Cheers, Razvan