From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads Date: Fri, 18 Dec 2015 10:32:10 -0500 Message-ID: <567426FA.9030508@oracle.com> References: <5672CE4002000078000C0C7D@prv-mh.provo.novell.com> <5672C2C6.9020808@oracle.com> <5672D24802000078000C0CB7@prv-mh.provo.novell.com> <5672C60E.3010303@oracle.com> <5672D4DB02000078000C0CD5@prv-mh.provo.novell.com> <5672C9B5.8090508@oracle.com> <56742254.9010903@oracle.com> <5674320E02000078000C1567@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1a9x0u-0007mz-6p for xen-devel@lists.xenproject.org; Fri, 18 Dec 2015 15:32:12 +0000 In-Reply-To: <5674320E02000078000C1567@prv-mh.provo.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 , Kevin Tian Cc: Andrew Cooper , Keir Fraser , Jun Nakajima , xen-devel List-Id: xen-devel@lists.xenproject.org On 12/18/2015 10:19 AM, Jan Beulich wrote: >>>> On 18.12.15 at 16:12, wrote: >> On 12/18/2015 01:21 AM, Tian, Kevin wrote: >>>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com] >>>> Sent: Thursday, December 17, 2015 10:42 PM >>>> >>>> On 12/17/2015 09:29 AM, Jan Beulich wrote: >>>>>>>> On 17.12.15 at 15:26, wrote: >>>>>> On 12/17/2015 09:18 AM, Jan Beulich wrote: >>>>>>>>>> On 17.12.15 at 15:12, wrote: >>>>>>>> On 12/17/2015 09:01 AM, Jan Beulich wrote: >>>>>>>>> @@ -415,8 +416,10 @@ static int core2_vpmu_verify(struct vcpu >>>>>>>>> enabled_cntrs |= (1ULL << i); >>>>>>>>> } >>>>>>>>> >>>>>>>>> - if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) && >>>>>>>>> - !is_canonical_address(core2_vpmu_cxt->ds_area) ) >>>>>>>>> + if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) && >>>>>>>>> + !(has_hvm_container_vcpu(v) >>>>>>>>> + ? is_canonical_address(core2_vpmu_cxt->ds_area) >>>>>>>>> + : __addr_ok(core2_vpmu_cxt->ds_area)) ) >>>>>>>> Should we instead of (or in addition to) this also make the same change >>>>>>>> in core2_vpmu_do_wrmsr()? >>>>>>> Currently there's no need for this since - afaict - PV guests can't >>>>>>> write this MSR directly (it's not among the white listed set in >>>>>>> traps.c). >>>>>> Then we probably shouldn't set VPMU_CPU_HAS_DS for PV guests. >>>>> Or add the MSR to the permitted set. You know better than I >>>>> what the best route here is. >>>> I vaguely recall a conversation where we weren't sure whether BTS (which >>>> needs DS area) will work for PV. Something to do with DS address being >>>> in the right context (guest or host). I'd need to find that conversation >>>> (or test BTS on PV). >>>> >>> I guess I don't need to review current patch until you have a conclusion, >> right? :-) >> >> All I can say that is BTS does not work on PV (at least as far as perf >> is concerned, which is the only tool I know that could use it). Which is >> not surprising given that we can't access DS_AREA MSR. > But in the context of this patch (and what direction a v2 might > need to go) we'd need to assume that MSR can be accessed. In which case I think we need to add the same test to core2_vpmu_do_wrmsr() as well. My earlier statement that we perform this test there *instead of* doing in core2_vpmu_verify() was incorrect --- we need to do it in both places since the value can come from the guest in the vpmu_context that we are loading thus bypassing core2_vpmu_do_wrmsr() path. -boris