On Sun, Aug 14, 2022 at 11:24:37PM -0500, Scott Cheloha wrote:
> Hi,
>
> In the future when the LAPIC timer is run in oneshot mode there will
> be no lapic_delay().
>
> This is fine if you have a constant TSC, because we have tsc_delay().
>
> This is *very* bad for older amd64 machines, because you are left with
> i8254_delay().
>
> I would like to offer a less awful delay(9) implementation for this
> class of hardware. Otherwise we may trip over bizarre phantom bugs on
> MP kernels because only one CPU can read the i8254 at a time.
>
> I think patrick@ was struggling with some version of that problem last
> year, but in a VM.
>
> Real i386 hardware should be fine. Later models with an ACPI PM timer
> will be fine using acpitimer_delay() instead of i8254_delay().
>
> If this seems reasonable to people I will come back with a cleaned up
> patch for testing.
>
> Thoughts? Preferences?
>
Seems reasonable to me. Would be interested to see the revised diff once you're
done.
-ml
> -Scott
>
> Here are the sample measurements from my 2017 laptop (kaby lake
> refresh) running the attached patch. It takes longer than a
> microsecond to read either of the ACPI timers. The PM timer is better
> than the HPET. The HPET is a bit better than the i8254. I hope the
> numbers are a little better on older hardware.
>
> acpitimer_test_delay: expected 0.000001000 actual 0.000010638 error
> 0.000009638
> acpitimer_test_delay: expected 0.000010000 actual 0.000015464 error
> 0.000005464
> acpitimer_test_delay: expected 0.000100000 actual 0.000107619 error
> 0.000007619
> acpitimer_test_delay: expected 0.001000000 actual 0.001007275 error
> 0.000007275
> acpitimer_test_delay: expected 0.010000000 actual 0.010007891 error
> 0.000007891
>
> acpihpet_test_delay: expected 0.000001000 actual 0.000022208 error
> 0.000021208
> acpihpet_test_delay: expected 0.000010000 actual 0.000031690 error
> 0.000021690
> acpihpet_test_delay: expected 0.000100000 actual 0.000112647 error
> 0.000012647
> acpihpet_test_delay: expected 0.001000000 actual 0.001021480 error
> 0.000021480
> acpihpet_test_delay: expected 0.010000000 actual 0.010013736 error
> 0.000013736
>
> i8254_test_delay: expected 0.000001000 actual 0.000040110 error
> 0.000039110
> i8254_test_delay: expected 0.000010000 actual 0.000039471 error
> 0.000029471
> i8254_test_delay: expected 0.000100000 actual 0.000128031 error
> 0.000028031
> i8254_test_delay: expected 0.001000000 actual 0.001024586 error
> 0.000024586
> i8254_test_delay: expected 0.010000000 actual 0.010021859 error
> 0.000021859
>
> Index: dev/acpi/acpihpet.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/acpi/acpihpet.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 acpihpet.c
> --- dev/acpi/acpihpet.c 6 Apr 2022 18:59:27 -0000 1.26
> +++ dev/acpi/acpihpet.c 15 Aug 2022 04:21:58 -0000
> @@ -18,6 +18,7 @@
> #include <sys/param.h>
> #include <sys/systm.h>
> #include <sys/device.h>
> +#include <sys/stdint.h>
> #include <sys/timetc.h>
>
> #include <machine/bus.h>
> @@ -31,8 +32,9 @@ int acpihpet_attached;
> int acpihpet_match(struct device *, void *, void *);
> void acpihpet_attach(struct device *, struct device *, void *);
> int acpihpet_activate(struct device *, int);
> -
> +void acpiphet_delay(u_int);
> u_int acpihpet_gettime(struct timecounter *tc);
> +void acpihpet_test_delay(u_int);
>
> uint64_t acpihpet_r(bus_space_tag_t _iot, bus_space_handle_t _ioh,
> bus_size_t _ioa);
> @@ -262,7 +264,7 @@ acpihpet_attach(struct device *parent, s
> freq = 1000000000000000ull / period;
> printf(": %lld Hz\n", freq);
>
> - hpet_timecounter.tc_frequency = (uint32_t)freq;
> + hpet_timecounter.tc_frequency = freq;
> hpet_timecounter.tc_priv = sc;
> hpet_timecounter.tc_name = sc->sc_dev.dv_xname;
> tc_init(&hpet_timecounter);
> @@ -273,10 +275,43 @@ acpihpet_attach(struct device *parent, s
> acpihpet_attached++;
> }
>
> +void
> +acpihpet_delay(u_int usecs)
> +{
> + uint64_t d, s;
> + struct acpihpet_softc *sc = hpet_timecounter.tc_priv;
> +
> + d = usecs * hpet_timecounter.tc_frequency / 1000000;
> + s = acpihpet_r(sc->sc_iot, sc->sc_ioh, HPET_MAIN_COUNTER);
> + while (acpihpet_r(sc->sc_iot, sc->sc_ioh, HPET_MAIN_COUNTER) - s < d)
> + CPU_BUSY_CYCLE();
> +}
> +
> u_int
> acpihpet_gettime(struct timecounter *tc)
> {
> struct acpihpet_softc *sc = tc->tc_priv;
>
> return (bus_space_read_4(sc->sc_iot, sc->sc_ioh, HPET_MAIN_COUNTER));
> +}
> +
> +void
> +acpihpet_test_delay(u_int usecs)
> +{
> + struct timespec ac, er, ex, t0, t1;
> +
> + if (!acpihpet_attached) {
> + printf("%s: (no hpet attached)\n", __func__);
> + return;
> + }
> +
> + nanouptime(&t0);
> + acpihpet_delay(usecs);
> + nanouptime(&t1);
> + timespecsub(&t1, &t0, &ac);
> + NSEC_TO_TIMESPEC(usecs * 1000ULL, &ex);
> + timespecsub(&ac, &ex, &er);
> + printf("%s: expected %lld.%09ld actual %lld.%09ld error %lld.%09ld\n",
> + __func__, ex.tv_sec, ex.tv_nsec, ac.tv_sec, ac.tv_nsec,
> + er.tv_sec, er.tv_nsec);
> }
> Index: dev/acpi/acpitimer.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/acpi/acpitimer.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 acpitimer.c
> --- dev/acpi/acpitimer.c 6 Apr 2022 18:59:27 -0000 1.15
> +++ dev/acpi/acpitimer.c 15 Aug 2022 04:21:58 -0000
> @@ -25,10 +25,14 @@
> #include <dev/acpi/acpireg.h>
> #include <dev/acpi/acpivar.h>
>
> +struct acpitimer_softc;
> +
> int acpitimermatch(struct device *, void *, void *);
> void acpitimerattach(struct device *, struct device *, void *);
> -
> +void acpitimer_delay(u_int);
> +void acpitimer_test_delay(u_int);
> u_int acpi_get_timecount(struct timecounter *tc);
> +uint32_t acpitimer_read(struct acpitimer_softc *);
>
> static struct timecounter acpi_timecounter = {
> .tc_get_timecount = acpi_get_timecount,
> @@ -104,12 +108,34 @@ acpitimerattach(struct device *parent, s
> #endif
> }
>
> +void
> +acpitimer_delay(u_int usecs)
> +{
> + uint64_t count = 0, cycles;
> + struct acpitimer_softc *sc = acpi_timecounter.tc_priv;
> + uint32_t mask = acpi_timecounter.tc_counter_mask;
> + uint32_t val1, val2;
> +
> + val2 = acpitimer_read(sc);
> + cycles = usecs * acpi_timecounter.tc_frequency / 1000000;
> + while (count < cycles) {
> + CPU_BUSY_CYCLE();
> + val1 = val2;
> + val2 = acpitimer_read(sc);
> + count += (val2 - val1) & mask;
> + }
> +}
>
> u_int
> acpi_get_timecount(struct timecounter *tc)
> {
> - struct acpitimer_softc *sc = tc->tc_priv;
> - u_int u1, u2, u3;
> + return acpitimer_read(tc->tc_priv);
> +}
> +
> +uint32_t
> +acpitimer_read(struct acpitimer_softc *sc)
> +{
> + uint32_t u1, u2, u3;
>
> u2 = bus_space_read_4(sc->sc_iot, sc->sc_ioh, 0);
> u3 = bus_space_read_4(sc->sc_iot, sc->sc_ioh, 0);
> @@ -120,4 +146,20 @@ acpi_get_timecount(struct timecounter *t
> } while (u1 > u2 || u2 > u3);
>
> return (u2);
> +}
> +
> +void
> +acpitimer_test_delay(u_int usecs)
> +{
> + struct timespec ac, er, ex, t0, t1;
> +
> + nanouptime(&t0);
> + acpitimer_delay(usecs);
> + nanouptime(&t1);
> + timespecsub(&t1, &t0, &ac);
> + NSEC_TO_TIMESPEC(usecs * 1000ULL, &ex);
> + timespecsub(&ac, &ex, &er);
> + printf("%s: expected %lld.%09ld actual %lld.%09ld error %lld.%09ld\n",
> + __func__, ex.tv_sec, ex.tv_nsec, ac.tv_sec, ac.tv_nsec,
> + er.tv_sec, er.tv_nsec);
> }
> Index: arch/amd64/amd64/lapic.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/lapic.c,v
> retrieving revision 1.59
> diff -u -p -r1.59 lapic.c
> --- arch/amd64/amd64/lapic.c 31 Aug 2021 15:53:36 -0000 1.59
> +++ arch/amd64/amd64/lapic.c 15 Aug 2022 04:21:58 -0000
> @@ -468,6 +468,19 @@ lapic_initclocks(void)
> lapic_startclock();
>
> i8254_inittimecounter_simple();
> +
> + extern void acpitimer_test_delay(u_int);
> + extern void acpihpet_test_delay(u_int);
> + extern void i8254_test_delay(u_int);
> + u_int usec[] = { 1, 10, 100, 1000, 10000 };
> + size_t i;
> + delay(20000); /* wait for real timecounter to activate */
> + for (i = 0; i < nitems(usec); i++)
> + acpitimer_test_delay(usec[i]);
> + for (i = 0; i < nitems(usec); i++)
> + acpihpet_test_delay(usec[i]);
> + for (i = 0; i < nitems(usec); i++)
> + i8254_test_delay(usec[i]);
> }
>
>
> Index: arch/amd64/isa/clock.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/isa/clock.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 clock.c
> --- arch/amd64/isa/clock.c 13 Feb 2022 19:15:09 -0000 1.36
> +++ arch/amd64/isa/clock.c 15 Aug 2022 04:21:59 -0000
> @@ -266,6 +266,22 @@ i8254_delay(int n)
> }
>
> void
> +i8254_test_delay(u_int usecs)
> +{
> + struct timespec ac, er, ex, t0, t1;
> +
> + nanouptime(&t0);
> + i8254_delay(usecs);
> + nanouptime(&t1);
> + timespecsub(&t1, &t0, &ac);
> + NSEC_TO_TIMESPEC(usecs * 1000ULL, &ex);
> + timespecsub(&ac, &ex, &er);
> + printf("%s: expected %lld.%09ld actual %lld.%09ld error %lld.%09ld\n",
> + __func__, ex.tv_sec, ex.tv_nsec, ac.tv_sec, ac.tv_nsec,
> + er.tv_sec, er.tv_nsec);
> +}
> +
> +void
> rtcdrain(void *v)
> {
> struct timeout *to = (struct timeout *)v;
>
