Re: [PATCH v15 11/21] x86/VPMU: Interface for setting PMU mode and flags

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

On 12/04/2014 03:51 AM, Jan Beulich wrote:
>>>> On 03.12.14 at 21:13, <boris.ostrovsky@oracle.com> wrote:
>> On 11/27/2014 03:57 AM, Jan Beulich wrote:
>>>>>> On 26.11.14 at 15:32, <boris.ostrovsky@oracle.com> wrote:
>>>> On 11/25/2014 08:49 AM, Jan Beulich wrote:
>>>>>>>> On 17.11.14 at 00:07, <boris.ostrovsky@oracle.com> wrote:
>>>>>> @@ -244,19 +256,19 @@ void vpmu_initialise(struct vcpu *v)
>>>>>>         switch ( vendor )
>>>>>>         {
>>>>>>         case X86_VENDOR_AMD:
>>>>>> -        if ( svm_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
>>>>>> -            opt_vpmu_enabled = 0;
>>>>>> +        if ( svm_vpmu_initialise(v) != 0 )
>>>>>> +            vpmu_mode = XENPMU_MODE_OFF;
>>>>>>             break;
>>>>>>     
>>>>>>         case X86_VENDOR_INTEL:
>>>>>> -        if ( vmx_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
>>>>>> -            opt_vpmu_enabled = 0;
>>>>>> +        if ( vmx_vpmu_initialise(v) != 0 )
>>>>>> +            vpmu_mode = XENPMU_MODE_OFF;
>>>>>>             break;
>>>>> So this turns off the vPMU globally upon failure of initializing
>>>>> some random vCPU. Why is that? I see this was the case even
>>>>> before your entire series, but shouldn't this be fixed _before_
>>>>> enhancing the whole thing to support PV/PVH?
>>>> Yes, that's probably too strong. Do you want to fix this as an early
>>>> patch (before PV(H)) support gets in? I'd rather do it in the patch that
>>>> moves things into initcalls.
>>> Yes, I think this should be fixed in a prereq patch, thus allowing it
>>> to be easily backported if so desired.
>> I started to make this change and then I realized that the next patch
>> (12/21) is actually already taking care of this problem: most of the
>> *_vpmu_initialise() will be executed as initcalls during host boot and
>> if any of those fail then we do want to disable VPMU globally (those
>> failures would not be VCPU-specific).
> Funny that you say that - it was actually while reviewing that next
> patch when I made that observation: svm_vpmu_initialise() get a
> -ENOMEM return _added_ there for example, which is contrary to
> what I asked for above.


Oh, *that* initialization code (I was looking at vpmu_init()). Yes, VPMU 
should not be turned off there.

-boris