From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v24 04/15] x86/VPMU: Interface for setting PMU mode and flags Date: Thu, 11 Jun 2015 11:14:16 -0400 Message-ID: <5579A5C8.9090608@oracle.com> References: <1433948671-29504-1-git-send-email-boris.ostrovsky@oracle.com> <1433948671-29504-5-git-send-email-boris.ostrovsky@oracle.com> <5579A125.1060106@oracle.com> <5579BFB70200007800083BEC@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5579BFB70200007800083BEC@mail.emea.novell.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: Jan Beulich Cc: Kevin Tian , "suravee.suthikulpanit@amd.com" , "andrew.cooper3@citrix.com" , "tim@xen.org" , "dietmar.hahn@ts.fujitsu.com" , "xen-devel@lists.xen.org" , "Aravind.Gopalakrishnan@amd.com" , Jun Nakajima , "dgdegra@tycho.nsa.gov" List-Id: xen-devel@lists.xenproject.org On 06/11/2015 11:04 AM, Jan Beulich wrote: >>>> On 11.06.15 at 16:54, wrote: >> On 06/11/2015 04:17 AM, Tian, Kevin wrote: >>>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com] >>>> switch ( vendor ) >>>> { >>>> case X86_VENDOR_AMD: >>>> - ret = svm_vpmu_initialise(v, opt_vpmu_enabled); >>>> + ret = svm_vpmu_initialise(v); >>>> break; >>>> >>>> case X86_VENDOR_INTEL: >>>> - ret = vmx_vpmu_initialise(v, opt_vpmu_enabled); >>>> + ret = vmx_vpmu_initialise(v); >>>> break; >>>> >>>> default: >>>> - if ( opt_vpmu_enabled ) >>>> + if ( vpmu_mode != XENPMU_MODE_OFF ) >>>> { >>>> printk(XENLOG_G_WARNING "VPMU: Unknown CPU vendor %d. " >>>> "Disabling VPMU\n", vendor); >>>> opt_vpmu_enabled = 0; >>>> + vpmu_mode = XENPMU_MODE_OFF; >>>> } >>>> - return; >>>> + return; /* Don't bother restoring vpmu_count, VPMU is off forever >> */ >>> why not restoring vpmu_count here? There could be a race condition >>> regarding to the mode control path: >>> >>> + if ( (vpmu_count == 0) || (vpmu_mode == pmu_params.val) || >>> + ((vpmu_mode ^ pmu_params.val) == >>> + (XENPMU_MODE_SELF | XENPMU_MODE_HV)) ) >>> + vpmu_mode = pmu_params.val; >>> >>> It's possible "vpmu_mode=pmu_params.val" happens later than >>> "vpmu_mode = XENPMU_MODE_OFF"... >>> >>> It might not be a big problem since opt_vpmu_enabled is 0 then, but >>> then there's pointless to reset vpmu_mode further if the behavior >>> is not guaranteed. >> >> It is somewhat pointless to reset it, but mostly because if we ever get >> into the default case above we have much bigger problems than VPMU: the >> only way that I can see when this can happen (vendor not being AMD or >> Intel) is that current_cpu_data.x86_vendor got overwritten by something >> else, which means memory corruption. > Or you're running on a Cyrix CPU (unless there's code earlier on > preventing that switch() from being reached). Can't happen: we are getting into vpmu_initialise() from * VMX or SVM code --- so it's AMD or Intel. * pvpmu_init() but it is gated by 'vpmu_mode == XENPMU_MODE_OFF'. And the only way vpmu_mode can be anything but XENPMU_MODE_OFF is if vpmu_init() set it in the first place. And that routine does check for boot CPU's vendor. As a side question, will Xen boot on Cyrix (or are they VIA)? -boris