Re: [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 12/18/2015 10:19 AM, Jan Beulich wrote:
>>>> On 18.12.15 at 16:12, <boris.ostrovsky@oracle.com> 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, <boris.ostrovsky@oracle.com> wrote:
>>>>>> On 12/17/2015 09:18 AM, Jan Beulich wrote:
>>>>>>>>>> On 17.12.15 at 15:12, <boris.ostrovsky@oracle.com> 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