On Wed, Aug 17, 2022 at 02:28:14PM +1000, Jonathan Gray wrote:
> On Tue, Aug 16, 2022 at 11:53:51AM -0500, Scott Cheloha wrote:
> > On Sun, Aug 14, 2022 at 11:24:37PM -0500, Scott Cheloha wrote:
> > >
> > > In the future when the LAPIC timer is run in oneshot mode there will
> > > be no lapic_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.
> > >
> > > [...]
> > >
> > > Real i386 hardware should be fine. Later models with an ACPI PM timer
> > > will be fine using acpitimer_delay() instead of i8254_delay().
> > >
> > > [...]
> > >
> > > 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
> >
> > Attched is an updated patch. I left the test measurement code in
> > place because I would like to see a test on a real i386 machine, just
> > to make sure it works as expected. I can't imagine why it wouldn't
> > work, but we should never assume anything.
> >
> > Changes from v1:
> >
> > - Actually set delay_func from acpitimerattach() and
> > acpihpet_attach().
> >
> > I think it's safe to assume, on real hardware, that the ACPI PMT is
> > preferable to the i8254 and the HPET is preferable to both of them.
> >
> > This is not *always* true, but it is true on the older machines that
> > can't use tsc_delay(), so the assumption works in practice.
> >
> > Outside of those three timers, the hierarchy gets murky. There are
> > other timers that are better than the HPET, but they aren't always
> > available. If those timers are already providing delay_func this
> > code does not usurp them.
>
> As I understand it, you want lapic to be in one-shot mode for something
> along the lines of tickless.
Yes.
Although "tickless" is a misnomer.
> So you are trying to find MP machines
> where TSC is not useable for delay?
Right. Those are the only machines where it's relevant to consider
the accuracy of acpitimer_delay() or acpihpet_delay()... unless I've
forgotten something.
> TSC is only considered for delay if the invariant and constant flags
> are set.
> invariant:
> "In the Core i7 and future processor generations, the TSC will continue
> to run in the deepest C-states. Therefore, the TSC will run at a
> constant rate in all ACPI P-, C-. and T-states. Support for this feature
> is indicated by CPUID.0x8000_0007.EDX[8]. On processors with invariant
> TSC support, the OS may use the TSC for wall clock timer services
> (instead of ACPI or HPET timers). TSC reads are much more efficient and
> do not incur the overhead associated with a ring transition or access to
> a platform resource."
>
> constant:
> runs at a constant rate across frequency/P state changes
>
> Intel constant
> (family == 0x0f && model >= 0x03) || (family == 0x06 && model >= 0x0e)
>
> family 0x06 model 0x0e is yonah, core solo/duo
> Intel CPUID doc has
> "Intel Core Duo processor, Intel Core Solo processor, model 0Eh.
> All processors are manufactured using the 65 nm process."
>
> family 0x0f model 0x03
> "Pentium 4 processor, Intel Xeon processor, Intel Celeron D processor.
> All processors are model 03h and manufactured using the 90 nm process."
>
> VIA constant
> model >= 0x0f
> model 0x0f is Nano
>
> AMD constant
> CPUIDEDX_ITSC set
>
> invariant
> CPUIDEDX_ITSC set
This matches my understanding of the situation.
> > - Duplicate test measurement code from amd64/lapic.c into i386/lapic.c.
> > Will be removed in the committed version.
> >
> > - Use bus_space_read_8() in acpihpet.c if it's available. The HPET is
> > a 64-bit counter and the spec permits 32-bit or 64-bit aligned access.
> >
> > As one might predict, this cuts the overhead in half because we're
> > doing half as many reads.
> >
> > This part can go into a separate commit, but I thought it was neat
> > so I'm including it here.
>
> This should probably use __LP64__ as if_xge.c does
Ack, I will do it that way in a subsequent patch. It seems a bit
indirect to do it that way though.
> > One remaining question I have:
> >
> > Is there a nice way to test whether ACPI PMT support is compiled into
> > the kernel? We can assume the existence of i8254_delay() because
> > clock.c is required on amd64 and i386. However, acpitimer.c is a
> > optional, so acpitimer_delay() isn't necessarily there.
> >
> > I would rather not introduce a hard requirement on acpitimer.c into
> > acpihpet.c if there's an easy way to check for the latter.
> >
> > Any ideas?
>
> the normal way would be to add "needs-flag"
> then config(8) will create a acpitimer.h with NACPITIMER
> see files.conf(5)
Ah! Perfect, thank you.
> As it stands RAMDISK does not have acpitimer so with your diff those
> kernels do not link.
>
> Index: files.acpi
> ===================================================================
> RCS file: /cvs/src/sys/dev/acpi/files.acpi,v
> retrieving revision 1.64
> diff -u -p -r1.64 files.acpi
> --- files.acpi 29 Dec 2021 18:40:19 -0000 1.64
> +++ files.acpi 17 Aug 2022 02:30:32 -0000
> @@ -13,7 +13,7 @@ file dev/acpi/acpidebug.c acpi & ddb
> # ACPI timer
> device acpitimer
> attach acpitimer at acpi
> -file dev/acpi/acpitimer.c acpitimer
> +file dev/acpi/acpitimer.c acpitimer needs-flag
>
> # AC device
> device acpiac
Updated patch is attached below. The RAMDISK kernel now builds.
> > Anyone have i386 hardware results? If I'm reading the timeline right,
> > most P6 machines and beyond (NetBurst, etc) will have an ACPI PMT. I
> > don't know if any real x86 motherboards shipped with an HPET, but it's
> > possible.
>
> by "real x86" you mean machines that can't run amd64 I gather
Yes, that was my meaning.
> I'm sure i386 releases are built on a machine with HPET
>
> an i386 only machine without HPET is the x40
>
> cpu0: Intel(R) Pentium(R) M processor 1200MHz ("GenuineIntel" -class) 1.20
> GHz, 06-09-05
> cpu0:
> FPU,V86,DE,PSE,TSC,MSR,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,TM,PBE,EST,TM2,PERF,MELTDOWN
> mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
> cpu0: apic clock running at 99MHz
> acpitimer0 at acpi0: 3579545 Hz, 24 bits
> acpitimer_test_delay: expected 0.000001000 actual 0.000001000 error
> 0.000000000
> acpitimer_test_delay: expected 0.000010000 actual 0.000001000 error
> -1.999991000
> acpitimer_test_delay: expected 0.000100000 actual 0.000001000 error
> -1.999901000
> acpitimer_test_delay: expected 0.001000000 actual 0.000001000 error
> -1.999001000
> acpitimer_test_delay: expected 0.010000000 actual 0.000001000 error
> -1.990001000
> acpihpet_test_delay: (no hpet attached)
> acpihpet_test_delay: (no hpet attached)
> acpihpet_test_delay: (no hpet attached)
> acpihpet_test_delay: (no hpet attached)
> acpihpet_test_delay: (no hpet attached)
> i8254_test_delay: expected 0.000001000 actual 0.000001000 error 0.000000000
> i8254_test_delay: expected 0.000010000 actual 0.000001000 error -1.999991000
> i8254_test_delay: expected 0.000100000 actual 0.000001000 error -1.999901000
> i8254_test_delay: expected 0.001000000 actual 0.000001000 error -1.999001000
> i8254_test_delay: expected 0.010000000 actual 0.000001000 error -1.990001000
> kern.timecounter.hardware=acpitimer0
> kern.timecounter.choice=i8254(0) acpitimer0(1000)
Thank you for testing. You have quite a collection.
But those results mean I've misunderstood something. The negative
error values mean a real timecounter is ticking at this point on amd64
but *not* on i386. Or at least, not on that machine.
Spinning for 20,000 microseconds should be enough time for one
hardclock to run if interrupts are enabled and the lapic timer is
running.
Hmmm.
Index: acpitimer.c
===================================================================
RCS file: /cvs/src/sys/dev/acpi/acpitimer.c,v
retrieving revision 1.15
diff -u -p -r1.15 acpitimer.c
--- acpitimer.c 6 Apr 2022 18:59:27 -0000 1.15
+++ acpitimer.c 17 Aug 2022 05:37:14 -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>
@@ -25,10 +26,13 @@
#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(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,
@@ -98,18 +102,45 @@ acpitimerattach(struct device *parent, s
acpi_timecounter.tc_priv = sc;
acpi_timecounter.tc_name = sc->sc_dev.dv_xname;
tc_init(&acpi_timecounter);
+
+#if defined(__amd64__) || defined(__i386__)
+ if (delay_func == i8254_delay)
+ delay_func = acpitimer_delay;
+#endif
#if defined(__amd64__)
extern void cpu_recalibrate_tsc(struct timecounter *);
cpu_recalibrate_tsc(&acpi_timecounter);
#endif
}
+void
+acpitimer_delay(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);
Index: acpihpet.c
===================================================================
RCS file: /cvs/src/sys/dev/acpi/acpihpet.c,v
retrieving revision 1.26
diff -u -p -r1.26 acpihpet.c
--- acpihpet.c 6 Apr 2022 18:59:27 -0000 1.26
+++ acpihpet.c 17 Aug 2022 05:37:14 -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>
@@ -26,12 +27,14 @@
#include <dev/acpi/acpivar.h>
#include <dev/acpi/acpidev.h>
+#include "acpitimer.h"
+
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 acpihpet_delay(int);
u_int acpihpet_gettime(struct timecounter *tc);
uint64_t acpihpet_r(bus_space_tag_t _iot, bus_space_handle_t _ioh,
@@ -262,15 +265,38 @@ 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);
+
+#if defined(__amd64__) || defined(__i386__)
+ if (delay_func == i8254_delay)
+ delay_func = acpihpet_delay;
+#if NACPITIMER > 1
+ extern void acpitimer_delay(int);
+ if (delay_func == acpitimer_delay)
+ delay_func = acpihpet_delay;
+#endif
+#endif /* defined(__amd64__) || defined(__i386__) */
+
#if defined(__amd64__)
extern void cpu_recalibrate_tsc(struct timecounter *);
cpu_recalibrate_tsc(&hpet_timecounter);
#endif
acpihpet_attached++;
+}
+
+void
+acpihpet_delay(int usecs)
+{
+ uint64_t c, s;
+ struct acpihpet_softc *sc = hpet_timecounter.tc_priv;
+
+ s = acpihpet_r(sc->sc_iot, sc->sc_ioh, HPET_MAIN_COUNTER);
+ c = usecs * hpet_timecounter.tc_frequency / 1000000;
+ while (acpihpet_r(sc->sc_iot, sc->sc_ioh, HPET_MAIN_COUNTER) - s < c)
+ CPU_BUSY_CYCLE();
}
u_int
Index: files.acpi
===================================================================
RCS file: /cvs/src/sys/dev/acpi/files.acpi,v
retrieving revision 1.64
diff -u -p -r1.64 files.acpi
--- files.acpi 29 Dec 2021 18:40:19 -0000 1.64
+++ files.acpi 17 Aug 2022 05:37:14 -0000
@@ -13,7 +13,7 @@ file dev/acpi/acpidebug.c acpi & ddb
# ACPI timer
device acpitimer
attach acpitimer at acpi
-file dev/acpi/acpitimer.c acpitimer
+file dev/acpi/acpitimer.c acpitimer needs-flag
# AC device
device acpiac
