Re: [PATCH v16 02/23] x86/VPMU: Don't globally disable VPMU if initialization fails.

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

On 12/18/2014 09:10 AM, Jan Beulich wrote:
>>>> On 17.12.14 at 16:38, <boris.ostrovsky@oracle.com> wrote:
>> The failure to initialize VPMU may be temporary so we shouldn'd disable VMPU
>> forever.
> Reported-by: Jan Beulich <jbeulich@suse.com>
> (or Suggested-by if you like that better)
>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>   xen/arch/x86/hvm/vpmu.c | 15 ++++++++-------
>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
>> index 1df74c2..b43ea80 100644
>> --- a/xen/arch/x86/hvm/vpmu.c
>> +++ b/xen/arch/x86/hvm/vpmu.c
>> @@ -218,6 +218,7 @@ void vpmu_initialise(struct vcpu *v)
>>   {
>>       struct vpmu_struct *vpmu = vcpu_vpmu(v);
>>       uint8_t vendor = current_cpu_data.x86_vendor;
>> +    int ret;
>>   
>>       if ( is_pvh_vcpu(v) )
>>           return;
>> @@ -230,21 +231,21 @@ 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;
>> +        ret = svm_vpmu_initialise(v, opt_vpmu_enabled);
>>           break;
>>   
>>       case X86_VENDOR_INTEL:
>> -        if ( vmx_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
>> -            opt_vpmu_enabled = 0;
>> +        ret = vmx_vpmu_initialise(v, opt_vpmu_enabled);
>>           break;
>>   
>>       default:
>> -        printk("VPMU: Initialization failed. "
>> -               "Unknown CPU vendor %d\n", vendor);
>> -        opt_vpmu_enabled = 0;
>> +        ret = -EINVAL;
>> +        printk(XENLOG_G_WARNING "VPMU: Unknown CPU vendor %d\n", vendor);
>>           break;
>>       }
>> +
>> +    if ( ret )
>> +        printk(XENLOG_G_WARNING "VPMU: Initialization failed for %pv\n", v);
> Please avoid issuing two messages for the same problem. And the
> one in the default case probably doesn't make sense to be issued
> more than once (and then perhaps at boot time, if that doesn't
> happen already).

I see not doing this for default case but arch-specific inits may (at 
least in principle) fail for some VCPUs and not for others so I think I 
should keep warnings.

What I perhaps should do for default case (and in fact I do this in 
patch 14 where I moved some of this stuff into __initcalls) is to 
disable VPMU globally.

As for printing only once --- I often wondered whether we should have 
something similar to Linux' WARN_ONCE().

-boris