From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v18 07/16] x86/VPMU: Initialize PMU for PV(H) guests Date: Fri, 20 Feb 2015 11:15:48 -0500 Message-ID: <54E75DB4.3090609@oracle.com> References: <1424125619-10851-1-git-send-email-boris.ostrovsky@oracle.com> <1424125619-10851-8-git-send-email-boris.ostrovsky@oracle.com> <54E754280200007800062148@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: <54E754280200007800062148@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 02/20/2015 09:35 AM, Jan Beulich wrote: >>>> On 16.02.15 at 23:26, wrote: >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -437,6 +437,8 @@ int vcpu_initialise(struct vcpu *v) >> vmce_init_vcpu(v); >> } >> >> + spin_lock_init(&v->arch.vpmu.vpmu_lock); > This would rather seem to belong into vpmu_initialize(). vpmu_initialize() is called under this lock so we can't do this. > >> @@ -503,6 +509,16 @@ int __init amd_vpmu_init(void) >> return -EINVAL; >> } >> >> + if ( sizeof(struct xen_pmu_data) + >> + 2 * sizeof(uint64_t) * num_counters > PAGE_SIZE ) >> + { >> + printk(XENLOG_WARNING >> + "VPMU: Register bank does not fit into VPMU shared page\n"); >> + counters = ctrls = NULL; >> + num_counters = 0; >> + return -ENOSPC; >> + } > Wouldn't it be more reasonable to simply lower num_counters in > that case? AMD processors in particular have fixed number of counters per family so a guest would expect to see all of them. (on Intel we then might get away by updating CPUID leaf 0xa but I am not sure it's worth doing). > >> +static int pvpmu_init(struct domain *d, xen_pmu_params_t *params) >> +{ >> + struct vcpu *v; >> + struct vpmu_struct *vpmu; >> + struct page_info *page; >> + uint64_t gfn = params->val; >> + >> + if ( vpmu_mode == XENPMU_MODE_OFF ) >> + return -EINVAL; >> + >> + if ( (params->vcpu >= d->max_vcpus) || (d->vcpu == NULL) || >> + (d->vcpu[params->vcpu] == NULL) ) >> + return -EINVAL; >> + >> + if ( v->arch.vpmu.xenpmu_data ) >> + return -EINVAL; >> + >> + page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC); >> + if ( !page ) >> + return -EINVAL; >> + >> + if ( !get_page_type(page, PGT_writable_page) ) >> + { >> + put_page(page); >> + return -EINVAL; >> + } >> + >> + v = d->vcpu[params->vcpu]; >> + vpmu = vcpu_vpmu(v); >> + spin_lock(&vpmu->vpmu_lock); >> + >> + v->arch.vpmu.xenpmu_data = __map_domain_page_global(page); >> + if ( !v->arch.vpmu.xenpmu_data ) >> + { >> + put_page_and_type(page); >> + spin_unlock(&vpmu->vpmu_lock); >> + return -EINVAL; >> + } >> + >> + vpmu_initialise(v); >> + >> + spin_unlock(&vpmu->vpmu_lock); > So what is this lock guarding against here? Certainly not overwriting > of a non-NULL v->arch.vpmu.xenpmu_data (and hence leaking a > page reference)... This is trying to protect a race with pvmu_finish() that could clear xenpmu_data. (I actually think you were the one who suggested it). -boris > >> @@ -504,7 +590,7 @@ long do_xenpmu_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg) >> >> if ( copy_to_guest(arg, &pmu_params, 1) ) >> return -EFAULT; >> - break; >> + break; > ??? > > Jan >