Hi,

In lapic_calibrate_timer() we only conditionally decide to use the
lapic timer as our interrupt clock.  That is, lapic timer calibration
can fail and the system will boot anyway.

If after measuring the lapic timer frequency we somehow come up with
zero hertz, we do *not* set initclock_func to lapic_initclocks().
Here's the relevant bits from amd64/lapic.c:

   554  skip_calibration:
   555          printf("%s: apic clock running at %dMHz\n",
   556              ci->ci_dev->dv_xname, lapic_per_second / (1000 * 1000));
   557  
   558          if (lapic_per_second != 0) {

  [...]                 /* (skip ahead a bit...) */

   588                  /*
   589                   * Now that the timer's calibrated, use the apic timer 
routines
   590                   * for all our timing needs..
   591                   */
   592                  delay_init(lapic_delay, 3000);
   593                  initclock_func = lapic_initclocks;
   594          }
   595  }

Line 558.  The corresponding code is identical in i386/lapic.c.

I went ahead and tried it on amd64.  If you force lapic_per_second to
zero the system still boots, but the secondary CPUs just sit idle.
lapic_tval is zero, so when they call lapic_startclock() from
cpu_hatch(), nothing happens.  The i8254 still sends clock interrupts
to CPU0, though, so the system runs in a oddball state where one
processor is doing all the work.

I don't think that this is the intended behavior.  I think this is
just an oversight left over from some older code.  It would be a lot
more sensible to just panic if lapic_per_second is zero here.  Patch
attached.

If a bunch of you prefer to develop a more elaborate fallback scheme
where we don't hatch the secondary CPUs in the event that lapic timer
calibration fails, we could explore that later.  But for now I would
prefer to panic and try to spotlight the problem if it ever occurs in
the wild.

If this change is too risky -- maybe I am breaking someone's weird
setup? -- I can wait until after release.

Thoughts?  Preferences?

Index: amd64/amd64/lapic.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/lapic.c,v
retrieving revision 1.63
diff -u -p -r1.63 lapic.c
--- amd64/amd64/lapic.c 10 Sep 2022 01:30:14 -0000      1.63
+++ amd64/amd64/lapic.c 10 Sep 2022 01:59:52 -0000
@@ -555,43 +555,44 @@ skip_calibration:
        printf("%s: apic clock running at %dMHz\n",
            ci->ci_dev->dv_xname, lapic_per_second / (1000 * 1000));
 
-       if (lapic_per_second != 0) {
-               /*
-                * reprogram the apic timer to run in periodic mode.
-                * XXX need to program timer on other cpu's, too.
-                */
-               lapic_tval = (lapic_per_second * 2) / hz;
-               lapic_tval = (lapic_tval / 2) + (lapic_tval & 0x1);
-
-               lapic_timer_periodic(LAPIC_LVTT_M, lapic_tval);
-
-               /*
-                * Compute fixed-point ratios between cycles and
-                * microseconds to avoid having to do any division
-                * in lapic_delay.
-                */
-
-               tmp = (1000000 * (u_int64_t)1 << 32) / lapic_per_second;
-               lapic_frac_usec_per_cycle = tmp;
-
-               tmp = (lapic_per_second * (u_int64_t)1 << 32) / 1000000;
-
-               lapic_frac_cycle_per_usec = tmp;
-
-               /*
-                * Compute delay in cycles for likely short delays in usec.
-                */
-               for (i = 0; i < 26; i++)
-                       lapic_delaytab[i] = (lapic_frac_cycle_per_usec * i) >>
-                           32;
-
-               /*
-                * Now that the timer's calibrated, use the apic timer routines
-                * for all our timing needs..
-                */
-               delay_init(lapic_delay, 3000);
-               initclock_func = lapic_initclocks;
-       }
+       if (lapic_per_second == 0)
+               panic("%s: apic timer calibration failed", __func__);
+
+       /*
+        * reprogram the apic timer to run in periodic mode.
+        * XXX need to program timer on other cpu's, too.
+        */
+       lapic_tval = (lapic_per_second * 2) / hz;
+       lapic_tval = (lapic_tval / 2) + (lapic_tval & 0x1);
+
+       lapic_timer_periodic(LAPIC_LVTT_M, lapic_tval);
+
+       /*
+        * Compute fixed-point ratios between cycles and
+        * microseconds to avoid having to do any division
+        * in lapic_delay.
+        */
+
+       tmp = (1000000 * (u_int64_t)1 << 32) / lapic_per_second;
+       lapic_frac_usec_per_cycle = tmp;
+
+       tmp = (lapic_per_second * (u_int64_t)1 << 32) / 1000000;
+
+       lapic_frac_cycle_per_usec = tmp;
+
+       /*
+        * Compute delay in cycles for likely short delays in usec.
+        */
+       for (i = 0; i < 26; i++)
+               lapic_delaytab[i] = (lapic_frac_cycle_per_usec * i) >>
+                   32;
+
+       /*
+        * Now that the timer's calibrated, use the apic timer routines
+        * for all our timing needs..
+        */
+       delay_init(lapic_delay, 3000);
+       initclock_func = lapic_initclocks;
 }
 
 /*
Index: i386/i386/lapic.c
===================================================================
RCS file: /cvs/src/sys/arch/i386/i386/lapic.c,v
retrieving revision 1.52
diff -u -p -r1.52 lapic.c
--- i386/i386/lapic.c   10 Sep 2022 01:30:14 -0000      1.52
+++ i386/i386/lapic.c   10 Sep 2022 01:59:52 -0000
@@ -385,43 +385,44 @@ lapic_calibrate_timer(struct cpu_info *c
        printf("%s: apic clock running at %dMHz\n",
            ci->ci_dev->dv_xname, lapic_per_second / (1000 * 1000));
 
-       if (lapic_per_second != 0) {
-               /*
-                * reprogram the apic timer to run in periodic mode.
-                * XXX need to program timer on other cpu's, too.
-                */
-               lapic_tval = (lapic_per_second * 2) / hz;
-               lapic_tval = (lapic_tval / 2) + (lapic_tval & 0x1);
-
-               lapic_timer_periodic(LAPIC_LVTT_M, lapic_tval);
-
-               /*
-                * Compute fixed-point ratios between cycles and
-                * microseconds to avoid having to do any division
-                * in lapic_delay.
-                */
-
-               tmp = (1000000 * (u_int64_t)1 << 32) / lapic_per_second;
-               lapic_frac_usec_per_cycle = tmp;
-
-               tmp = (lapic_per_second * (u_int64_t)1 << 32) / 1000000;
-
-               lapic_frac_cycle_per_usec = tmp;
-
-               /*
-                * Compute delay in cycles for likely short delays in usec.
-                */
-               for (i = 0; i < 26; i++)
-                       lapic_delaytab[i] = (lapic_frac_cycle_per_usec * i) >>
-                           32;
-
-               /*
-                * Now that the timer's calibrated, use the apic timer routines
-                * for all our timing needs..
-                */
-               delay_init(lapic_delay, 3000);
-               initclock_func = lapic_initclocks;
-       }
+       if (lapic_per_second == 0)
+               panic("%s: apic timer calibration failed", __func__);
+
+       /*
+        * reprogram the apic timer to run in periodic mode.
+        * XXX need to program timer on other cpu's, too.
+        */
+       lapic_tval = (lapic_per_second * 2) / hz;
+       lapic_tval = (lapic_tval / 2) + (lapic_tval & 0x1);
+
+       lapic_timer_periodic(LAPIC_LVTT_M, lapic_tval);
+
+       /*
+        * Compute fixed-point ratios between cycles and
+        * microseconds to avoid having to do any division
+        * in lapic_delay.
+        */
+
+       tmp = (1000000 * (u_int64_t)1 << 32) / lapic_per_second;
+       lapic_frac_usec_per_cycle = tmp;
+
+       tmp = (lapic_per_second * (u_int64_t)1 << 32) / 1000000;
+
+       lapic_frac_cycle_per_usec = tmp;
+
+       /*
+        * Compute delay in cycles for likely short delays in usec.
+        */
+       for (i = 0; i < 26; i++)
+               lapic_delaytab[i] = (lapic_frac_cycle_per_usec * i) >>
+                   32;
+
+       /*
+        * Now that the timer's calibrated, use the apic timer routines
+        * for all our timing needs..
+        */
+       delay_init(lapic_delay, 3000);
+       initclock_func = lapic_initclocks;
 }
 
 /*

Reply via email to