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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.12.14 at 17:08, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 12/18/2014 09:10 AM, Jan Beulich wrote:
>>>>> On 17.12.14 at 16:38, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> The failure to initialize VPMU may be temporary so we shouldn'd disable VMPU
>>> forever.
>> Reported-by: Jan Beulich <jbeulich@xxxxxxxx>
>> (or Suggested-by if you like that better)
>>
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
>>> ---
>>>   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.

Right - that's what I asked for. The two aspects I disliked are that
you print two messages for a single vCPU in the default case, and
repeatedly the default case message despite the condition being
invariable across vCPU-s.

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

It would have been printk_once() you want here, but with the
change you're planning that's probably moot now anyway.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel