Re: [Xen-devel] [PATCH 4/8] x86/time: calibrate TSC against platform timer

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.08.16 at 21:25, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 20/06/16 16:19, Jan Beulich wrote:
>>>>> On 20.06.16 at 16:20, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 15/06/16 11:28, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/i8259.c
>>>> +++ b/xen/arch/x86/i8259.c
>>>> @@ -359,12 +359,7 @@ void __init init_IRQ(void)
>>>>  
>>>>      apic_intr_init();
>>>>  
>>>> -    /* Set the clock to HZ Hz */
>>>> -#define CLOCK_TICK_RATE 1193182 /* crystal freq (Hz) */
>>>> -#define LATCH (((CLOCK_TICK_RATE)+(HZ/2))/HZ)
>>>> -    outb_p(0x34, PIT_MODE);        /* binary, mode 2, LSB/MSB, ch 0 */
>>>> -    outb_p(LATCH & 0xff, PIT_CH0); /* LSB */
>>>> -    outb(LATCH >> 8, PIT_CH0);     /* MSB */
>>>> +    preinit_pit();
>>> This highlights the fact that we have two unconditional calls to
>>> preinit_pit() during startup, which is one too many.
>>>
>>> init_IRQ() is called rather earlier than early_time_init(), but I can't
>>> spot anything inbetween the two calls which would require the PIT to be
>>> set up.  AFAICT, it is safe to just drop the preinit_pit() call here.
>> LAPIC initialization makes use of the PIT, and I think that would
>> break when removing it here. And since doing it twice is benign,
>> I'd also like to not drop it from early_time_init().
> 
> Where? LAPIC initialisation is before this point - its higher up in
> init_IRQ() so surely can't depend on this currently working.

Hmm, looks like my earlier flow analysis was wrong (or perhaps
based on the old placement of the call to init_platform_timer() in
init_xen_time()):

__start_xen()
-> [line 1388] init_IRQ()
-> [line 1429] early_time_init()
-> [line 1447] smp_prepare_cpus()
 -> setup_boot_APIC_clock()
  -> calibrate_APIC_clock()
-> [line 1456] init_xen_time();

Let me verify that removing the one here also works in practice.

> As for benign, at the best it is a waste of time, and on reduced
> hardware platforms, wrong.  We shouldn't be propagating problems like these.
> 
>>
>>>> @@ -340,7 +348,8 @@ static struct platform_timesource __init
>>>>      .frequency = CLOCK_TICK_RATE,
>>>>      .read_counter = read_pit_count,
>>>>      .counter_bits = 32,
>>>> -    .init = init_pit
>>>> +    .init = init_pit,
>>>> +    .resume = resume_pit
>>> Please add a redundant comma at the end, to reduce the next diff to
>>> change this block.
>> Well, I'd like the three instance to remain consistent in this regard
>> (yet touching the others doesn't seem warranted). And a change
>> here isn't all that likely.
> 
> This is just like any other bit of style - it should be fixed up while
> editing even if the rest of the file isn't completely up to scratch.

Well, I actually had done that part already.

Jan


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