From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH V5 07/12] xen: Introduce monitor_op domctl Date: Tue, 17 Feb 2015 23:59:18 +0100 Message-ID: References: <1423845203-18941-1-git-send-email-tamas.lengyel@zentific.com> <1423845203-18941-8-git-send-email-tamas.lengyel@zentific.com> <54E357FE02000078000609D7@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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: Jan Beulich Cc: Tim Deegan , "Tian, Kevin" , "wei.liu2@citrix.com" , Ian Campbell , Steven Maresca , Stefano Stabellini , "Dong, Eddie" , Ian Jackson , "xen-devel@lists.xen.org" , Andres Lagar-Cavilla , Jun Nakajima , "rshriram@cs.ubc.ca" , Keir Fraser , Daniel De Graaf , "yanghy@cn.fujitsu.com" List-Id: xen-devel@lists.xenproject.org On Tue, Feb 17, 2015 at 7:20 PM, Tamas K Lengyel wrote: > On Tue, Feb 17, 2015 at 3:02 PM, Jan Beulich wrote: >>>>> On 13.02.15 at 17:33, wrote: >>> --- a/xen/arch/x86/hvm/emulate.c >>> +++ b/xen/arch/x86/hvm/emulate.c >>> @@ -411,7 +411,8 @@ static int hvmemul_virtual_to_linear( >>> * being triggered for repeated writes to a whole page. >>> */ >>> *reps = min_t(unsigned long, *reps, >>> - unlikely(current->domain->arch.hvm_domain.introspection_enabled) >>> + unlikely(current->domain->arch >>> + .monitor_options.mov_to_msr.extended_capture) >>> ? 1 : 4096); >> >> This makes no sense (especially not to a reader in a year or two): >> There's no connection between mov-to-msr and the repeat count >> capping done here. Please wrap this in a suitably named is_...() or >> has_...() or introspection_enabled() helper, with a comment at its >> definition site making the connection explicit. > > It took me a while to understand what "introspection_enabled" actually > represents and all it really does at the moment is enabling the > interception of an extended set of MSRs. If something, that is a bad > variable name. Since in this series "introspection_enabled" is > removed, here I just updated the variable to the one that holds the > same information. I don't actually know what the code here does as I > didn't touch it. If this indeed has no connection to mov-to-msr, it > should have its own option field with its own name actually describing > what it does. Maybe Razvan has some more information on what is going > on here and if another variable needs to be introduced that was just > latched onto "introspection_enabled". So I looked into this a bit more and this is being used when a mem_event response to a mem_access event has the emulation flag set. So this is an extra option that was latched onto introspection_enabled, thus we will need an extra field to determine if this particular feature is enabled or not. Now I understand why Razvan was wondering about "umbrella" options going forward. IMHO this highlights the problem with umbrella options - it becomes really hard to understand what the option actually does. Especially without proper documentation. Tamas