On Mon, Aug 02, 2010 at 06:16:16PM +0200, Arjan Koers wrote:
> On 2010-08-02 16:43, 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
>
> Maybe you could add a sanity check in pvclock_clocksource_read
> after 'do { ... } while (version != src->version)' that
> returns last_value if offset is extremely large?
I am not against adding a check, but only if the resulting action is
warn-only. Otherwise we can paper over this, and forget the real bugs.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
