From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v16 02/23] x86/VPMU: Don't globally disable VPMU if initialization fails. Date: Thu, 18 Dec 2014 11:08:49 -0500 Message-ID: <5492FC11.8010305@oracle.com> References: <1418830734-1558-1-git-send-email-boris.ostrovsky@oracle.com> <1418830734-1558-3-git-send-email-boris.ostrovsky@oracle.com> <5492EE5C0200007800050AD9@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: <5492EE5C0200007800050AD9@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@intel.com, 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@intel.com, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org On 12/18/2014 09:10 AM, Jan Beulich wrote: >>>> On 17.12.14 at 16:38, wrote: >> The failure to initialize VPMU may be temporary so we shouldn'd disable VMPU >> forever. > Reported-by: Jan Beulich > (or Suggested-by if you like that better) > >> Signed-off-by: Boris Ostrovsky >> --- >> xen/arch/x86/hvm/vpmu.c | 15 ++++++++------- >> 1 file changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c >> index 1df74c2..b43ea80 100644 >> --- a/xen/arch/x86/hvm/vpmu.c >> +++ b/xen/arch/x86/hvm/vpmu.c >> @@ -218,6 +218,7 @@ void vpmu_initialise(struct vcpu *v) >> { >> struct vpmu_struct *vpmu = vcpu_vpmu(v); >> uint8_t vendor = current_cpu_data.x86_vendor; >> + int ret; >> >> if ( is_pvh_vcpu(v) ) >> return; >> @@ -230,21 +231,21 @@ void vpmu_initialise(struct vcpu *v) >> switch ( vendor ) >> { >> case X86_VENDOR_AMD: >> - if ( svm_vpmu_initialise(v, opt_vpmu_enabled) != 0 ) >> - opt_vpmu_enabled = 0; >> + ret = svm_vpmu_initialise(v, opt_vpmu_enabled); >> break; >> >> case X86_VENDOR_INTEL: >> - if ( vmx_vpmu_initialise(v, opt_vpmu_enabled) != 0 ) >> - opt_vpmu_enabled = 0; >> + ret = vmx_vpmu_initialise(v, opt_vpmu_enabled); >> break; >> >> default: >> - printk("VPMU: Initialization failed. " >> - "Unknown CPU vendor %d\n", vendor); >> - opt_vpmu_enabled = 0; >> + ret = -EINVAL; >> + printk(XENLOG_G_WARNING "VPMU: Unknown CPU vendor %d\n", vendor); >> break; >> } >> + >> + if ( ret ) >> + printk(XENLOG_G_WARNING "VPMU: Initialization failed for %pv\n", v); > Please avoid issuing two messages for the same problem. And the > one in the default case probably doesn't make sense to be issued > more than once (and then perhaps at boot time, if that doesn't > happen already). I see not doing this for default case but arch-specific inits may (at least in principle) fail for some VCPUs and not for others so I think I should keep warnings. What I perhaps should do for default case (and in fact I do this in patch 14 where I moved some of this stuff into __initcalls) is to disable VPMU globally. As for printing only once --- I often wondered whether we should have something similar to Linux' WARN_ONCE(). -boris