From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v19 07/14] x86/VPMU: Initialize PMU for PV(H) guests Date: Tue, 24 Mar 2015 11:06:40 -0400 Message-ID: <55117D80.3090900@oracle.com> References: <1426604051-2980-1-git-send-email-boris.ostrovsky@oracle.com> <1426604051-2980-8-git-send-email-boris.ostrovsky@oracle.com> <55117DF9020000780006CFE1@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: <55117DF9020000780006CFE1@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 03/24/2015 10:08 AM, Jan Beulich wrote: >>>> On 17.03.15 at 15:54, wrote: >> @@ -263,14 +264,14 @@ void vpmu_initialise(struct vcpu *v) >> BUILD_BUG_ON(sizeof(struct xen_pmu_intel_ctxt) > XENPMU_CTXT_PAD_SZ); >> BUILD_BUG_ON(sizeof(struct xen_pmu_amd_ctxt) > XENPMU_CTXT_PAD_SZ); >> >> - if ( is_pvh_vcpu(v) ) >> - return; >> - >> ASSERT(!vpmu->flags && !vpmu->context); >> >> - spin_lock(&vpmu_lock); >> - vpmu_count++; /* Prevent vpmu_mode from changing until we are done */ >> - spin_unlock(&vpmu_lock); >> + if ( v->domain != hardware_domain ) >> + { >> + spin_lock(&vpmu_lock); >> + vpmu_count++; /* Prevent vpmu_mode from changing until we are done */ >> + spin_unlock(&vpmu_lock); >> + } > So why is this being made conditional here? A patch this size would > certainly benefit from a slightly more verbose description... I don't want to include dom0's VPMU since the count is used for deciding whether or not we can switch the mode. If other guests have active VPMUs we sometimes can't do the switch. dom0's VPMUs are always there and we should be able to switch the mode with those VPMUs on, which is why there is no reason to count them. I suppose I could check whether the count is smaller than number of dom0's CPUs but that's not particularly convenient since we could have had errors during initialization (so not all dom0's VCPUs have VPMUs), we may have to guard against hotplug events, etc. But yes, an explanation would be useful. I'll add something. > > And then if the conditional is indeed needed - why don't you use > is_hardware_domain()? No specific reason. I'll switch to is_hardware_domain() -boris