Re: 2.6.35-rc1 regression with pvclock and smp guests

[Zachary Amsden <zamsden@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1991 bytes --]

On 08/02/2010 04:43 AM, Glauber Costa wrote:
> On Sat, Jul 31, 2010 at 01:55:10PM -1000, Zachary Amsden wrote:
>    
>> On 07/31/2010 06:36 AM, Arjan Koers wrote:
>>      
>>> On 2010-07-31 13:53, Arjan Koers wrote:
>>>        
>>>> The kernel boots successfully when CONFIG_PRINTK_TIME is not set.
>>>>
>>>>          
>>> The problem occurs when this message is printed:
>>>
>>> [    0.016000] kvm-clock: cpu 1, msr 0:1511c01, secondary cpu clock
>>>
>>> When I disable that printk, the kernel boots with
>>> CONFIG_PRINTK_TIME=y
>>>
>>> --- a/arch/x86/kernel/kvmclock.c
>>> +++ b/arch/x86/kernel/kvmclock.c
>>> @@ -131,8 +131,8 @@ static int kvm_register_clock(char *txt)
>>>   	int low, high;
>>>   	low = (int)__pa(&per_cpu(hv_clock, cpu)) | 1;
>>>   	high = ((u64)__pa(&per_cpu(hv_clock, cpu))>>   32);
>>> -	printk(KERN_INFO "kvm-clock: cpu %d, msr %x:%x, %s\n",
>>> -	       cpu, high, low, txt);
>>> +	/*printk(KERN_INFO "kvm-clock: cpu %d, msr %x:%x, %s\n",
>>> +	       cpu, high, low, txt);*/
>>>
>>>   	return native_write_msr_safe(msr_kvm_system_time, low, high);
>>>   }
>>>
>>> So the problem appears to be that the clock of the second CPU
>>> is used too soon (or that clock setup should finish earlier).
>>>        
>> That's almost hilarious.  The printk from setting up the kvm clock
>> is invoking the kvm clock before it is setup.
>>
>> There's no reason other printks couldn't do the same thing, however.
>> I think it's safest to keep an initialized flag and check for it
>> before attempting to return a meaningful value.
>>      
> I was on vacations, just got back.
>
> I think it is safe to just patch our own use of it. Before that, all other
> printks will be handled by the main cpu anyway, since it'll be the only one active
> at the moment. The only possible offenders for this are us, and the cpu initialization
> code, which is already fragile in multiple ways anyway.
>
> A flag would only make things more complicated and dirty
>    
Can we just do this?

[-- Attachment #2: zero.patch --]
[-- Type: text/plain, Size: 855 bytes --]

Initialize hv_clock to zero

This stops callers from getting random values if data is accessed before
clock is initialized; instead they will get zeroed clock values (because
computation involves a multiplication by a factor in hv_clock).

Signed-off-by: Zachary Amsden <zamsden@redhat.com>

diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index eb9b76c..e7acd0d 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -40,7 +40,7 @@ static int parse_no_kvmclock(char *arg)
 early_param("no-kvmclock", parse_no_kvmclock);
 
 /* The hypervisor will put information about time periodically here */
-static DEFINE_PER_CPU_SHARED_ALIGNED(struct pvclock_vcpu_time_info, hv_clock);
+static DEFINE_PER_CPU_SHARED_ALIGNED(struct pvclock_vcpu_time_info, hv_clock) = {0};
 static struct pvclock_wall_clock wall_clock;
 
 /*