Re: [PATCH v8 1/5] x86: add simple udelay calibration

[Dou Liyang]

Hi, Lu

At 05/05/2017 08:50 PM, Boris Ostrovsky wrote:
> On 05/05/2017 01:41 AM, Lu Baolu wrote:
> > Hi,
> >
> > On 05/03/2017 06:38 AM, Boris Ostrovsky wrote:
> > > On 03/21/2017 04:01 AM, Lu Baolu wrote:
> > > > Add a simple udelay calibration in x86 architecture-specific
> > > > boot-time initializations. This will get a workable estimate
> > > > for loops_per_jiffy. Hence, udelay() could be used after this
> > > > initialization.
> > > This breaks Xen PV guests since at this point, and until
> > > x86_init.paging.pagetable_init() which is when pvclock_vcpu_time_info is
> > > mapped, they cannot access pvclock.
> > >
> > > Is it reasonable to do this before tsc_init() is called? (The failure
> > > has nothing to do with tsc_init(), really --- it's just that it is
> > > called late enough that Xen PV guests get properly initialized.) If it
> > > is, would it be possible to move simple_udelay_calibration() after
> > > x86_init.paging.pagetable_init()?
> > This is currently only used for bare metal. How about by-pass it
> > for Xen PV guests?
>
> It is fixed this for Xen PV guests now (in the sense that we don't crash
> anymore) but my question is still whether this is not too early. Besides
> tsc_init() (which might not be important here), at the time when
> simple_udelay_calibration() is invoked we haven't yet called:
> * kvmclock_init(), which sets calibration routines for KVM
> * init_hypervisor_platform(), which sets calibration routines for vmware
> and Xen HVM
> * x86_init.paging.pagetable_init(), which sets calibration routines for
> Xen PV

I guess these may have been missed.

Do you have any comments about these?

> -boris
>
>
> > Best regards,
> > Lu Baolu
> >
> > > -boris
> > >
> > >
> > > > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > > > Cc: x86@xxxxxxxxxx
> > > > Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > > > ---
> > > >  arch/x86/kernel/setup.c | 22 ++++++++++++++++++++++
> > > >  1 file changed, 22 insertions(+)
> > > >
> > > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > > > index 4bf0c89..e70204e 100644
> > > > --- a/arch/x86/kernel/setup.c
> > > > +++ b/arch/x86/kernel/setup.c
> > > > @@ -837,6 +837,26 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p)
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static void __init simple_udelay_calibration(void)
> > > > +{
> > > > +	unsigned int tsc_khz, cpu_khz;
> > > > +	unsigned long lpj;
> > > > +
> > > > +	if (!boot_cpu_has(X86_FEATURE_TSC))
> > > > +		return;


if it returns here,  can we use udelay() correctly like before?

Thanks,

	dou.

> > > > +
> > > > +	cpu_khz = x86_platform.calibrate_cpu();
> > > > +	tsc_khz = x86_platform.calibrate_tsc();
> > > > +
> > > > +	tsc_khz = tsc_khz ? : cpu_khz;
> > > > +	if (!tsc_khz)
> > > > +		return;
> > > > +
> > > > +	lpj = tsc_khz * 1000;
> > > > +	do_div(lpj, HZ);
> > > > +	loops_per_jiffy = lpj;
> > > > +}
> > > > +
> > > >  /*
> > > >   * Determine if we were loaded by an EFI loader.  If so, then we have also been
> > > >   * passed the efi memmap, systab, etc., so we should use these data structures
> > > > @@ -985,6 +1005,8 @@ void __init setup_arch(char **cmdline_p)
> > > >  	 */
> > > >  	x86_configure_nx();
> > > >
> > > > +	simple_udelay_calibration();
> > > > +
> > > >  	parse_early_param();
> > > >
> > > >  #ifdef CONFIG_MEMORY_HOTPLUG