Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems

[Ravikiran G Thirumalai <kiran@scalex86.org>]

On Fri, Mar 21, 2008 at 10:15:42AM +0100, Ingo Molnar wrote:
>
>* Ravikiran G Thirumalai <kiran@scalex86.org> wrote:
>
>> >>  -       if (!is_vsmp_box() && (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
>> >>  +       if ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) && !is_vsmp_box())
>> >>                 return 0;
>
>> Yes.  The first call is to state that TSC's are synced if it is an AMD 
>> box and if it is _not_ a vSMPowered box (for the apic id lifting case 
>> on Sun boxes), the second call is for _any_ vSMPowered system with 
>> more than one board -- TSC's are not guaranteed to be synched in that 
>> case.  Both these calls are needed.
>
>i suspect there are two questions here: firstly, your change only flips 
>the condition around which should not have any effect _normally_. But 
>the thing is that is_vsmp_box() has side-effects: it reads the PCI 
>config space and sets a flag. It would be cleaner if there was a 
>separate, explicit detect_vsmp_box() call early during bootup which did 
>the PCI config space access, and is_vsmp_box() would only return the 
>current state of the vsmp flag. Then your above change would be 
>unnecessary.

The above change -- the flipping of conditions -- is not _really_ necessary.
The condition and call is needed however.  The flip was done while
experimenting with the patch, since most vSMPowered machines today are mostly
intel, the call to is_vsmp_box() could be avoided here on intel boxes.
However this is only a trivial micro-optimization, as, we call
is_vsmp_box() again even for intel boxes now.

As for the observation about probing the pci space early during the bootup,
we call vsmp_init() much earlier during the bootup, which calls is_vsmp_box(),
does the pci probing and caches the result in the flag,  as you suggest.
So the call in the above diff context does not access the pci config space
as is.

Thanks,
Kiran