From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v6 12/19] x86/VPMU: Add support for PMU register handling on PV guests Date: Thu, 22 May 2014 13:16:33 -0400 Message-ID: <537E30F1.1020604@oracle.com> References: <1399996413-1998-1-git-send-email-boris.ostrovsky@oracle.com> <1399996413-1998-13-git-send-email-boris.ostrovsky@oracle.com> <537E2AB90200007800015085@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: <537E2AB90200007800015085@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, keir@xen.org, suravee.suthikulpanit@amd.com, andrew.cooper3@citrix.com, donald.d.dugger@intel.com, xen-devel@lists.xen.org, dietmar.hahn@ts.fujitsu.com, jun.nakajima@intel.com List-Id: xen-devel@lists.xenproject.org On 05/22/2014 10:50 AM, Jan Beulich wrote: > >> @@ -868,8 +869,10 @@ void pv_cpuid(struct cpu_user_regs *regs) >> __clear_bit(X86_FEATURE_TOPOEXT % 32, &c); >> break; >> >> + case 0x0000000a: /* Architectural Performance Monitor Features (Intel) */ >> + break; >> + >> case 0x00000005: /* MONITOR/MWAIT */ >> - case 0x0000000a: /* Architectural Performance Monitor Features */ > Is there actually anything wrong with leaving this as it was, i.e. > clearing a, b, c, and d? Since AMD's PMU-related CPUID is 0x80000001 (and is not used currently anyway as there is no do_cpuid op in AMD's vpmu) I think I'll just move vpmu_do_cpuid() into 0x0000000a case. > >> @@ -885,6 +888,8 @@ void pv_cpuid(struct cpu_user_regs *regs) >> } >> >> out: >> + vpmu_do_cpuid(regs->eax, &a, &b, &c, &d); > This seems incomplete without passing in regs->ecx. And without at > least a brief comment it also looks misplaced at the first glance. vpmu_cpuid() doesn't use indexed CPUIDs (but I can see how ecx could have been added for consistency if I kept the call where it is now.) > >> @@ -2515,7 +2520,14 @@ static int emulate_privileged_op(struct cpu_user_regs *regs) >> if ( v->arch.debugreg[7] & DR7_ACTIVE_MASK ) >> wrmsrl(regs->_ecx, msr_content); >> break; >> - >> + case MSR_P6_PERFCTR0...MSR_P6_PERFCTR1: >> + case MSR_P6_EVNTSEL0...MSR_P6_EVNTSEL1: >> + case MSR_CORE_PERF_FIXED_CTR0...MSR_CORE_PERF_FIXED_CTR2: >> + case MSR_CORE_PERF_FIXED_CTR_CTRL...MSR_CORE_PERF_GLOBAL_OVF_CTRL: >> + case MSR_AMD_FAM15H_EVNTSEL0...MSR_AMD_FAM15H_PERFCTR5: >> + if ( !vpmu_do_wrmsr(regs->ecx, msr_content) ) >> + goto invalid; >> + break; > Can you really handle both Intel and AMD ones as a group here, > without consideration whose CPU you're actually running on? I > think for forward compatibility you should be making the call only > for Intel MSRs on Intel CPUs, and respectively for AMD. The vendor-specific paths are taken in vpmu_do_wrmsr() (and rdmsr). Not sure if splitting this into two cases would be better but if you feel it adds to clarity I can do this. -boris