Re: CVS commit: src/sys/arch/i386/i386

["Julio M. Merino Vidal" <jmmv84%gmail.com@localhost>]

On 25/07/2007, at 16:17, Juan RP wrote:

> On Wed, 25 Jul 2007 16:04:27 +0200
> "Julio M. Merino Vidal" <jmmv84%gmail.com@localhost> wrote:
>
> > Are you sure that is correct?  The original code was only calling
> > est_init when the cpu_vendor was one of CPUVENDOR_INTEL or
> > CPUVENDOR_IDT.  Now it calls est_init with any vendor.  According to
> > the est_init code, I see:
> >
> >          if (CPUID2FAMILY(curcpu()->ci_signature) == 15)
> >                  bus_clock = p4_get_bus_clock(curcpu());
> >          else if (CPUID2FAMILY(curcpu()->ci_signature) == 6) {
> >                  if (vendor == CPUVENDOR_IDT)
> >                          bus_clock = via_get_bus_clock(curcpu());
> >                  else
> >                          bus_clock = p3_get_bus_clock(curcpu());
> >          }
> >
> > So when cpu_vendor != CPUVENDOR_IDT, it calls p3_get_bus_clock.  And
> > that function only seems to recognize Intel CPUs.  So with your
> > change, this function will get called with any kind of CPU and return
> > invalid values.
>
> Family 15 is amd64 with EM64T and family 3 is i386.

3?  I guess you meant 6 there.

> cpu_vendor will always be CPUVENDOR_INTEL or CPUVENDOR_IDT, and  
> CPUID2_EST
> is only available in those CPUs anyway.

x86/include/cputypes.h defines much more CPUVENDOR_* values.

> What do you see wrong here? I don't understand you.

I was just asking you if you were certain that the change was right,  
because it makes the code behave differently than it did even though  
the commit message did not say why.

The problem is that p3_get_bus_clock may now be called with some  
values it is supposed not to take.  I am not sure if this *can*  
happen or not, but it might.  If that's the case, people with family  
6, non-Intel, non-IDT CPUs will start to see an extra message during  
boot saying that the calculation of the bus clock failed.

--
Julio M. Merino Vidal <jmmv84%gmail.com@localhost>