On Tue, Aug 23, 2022 at 12:20:39PM -0500, Scott Cheloha wrote:
> > Hyper-V generation 1 VMs are bios boot with emulation of the usual
> > devices.  32-bit and 64-bit guests.
> > 
> > Hyper-V generation 2 VMs are 64-bit uefi with paravirtualised devices.
> > 64-bit guests only.
> > 
> > There is no 8254 in generation 2.
> > No HPET in either generation.
> > 
> > hv_delay uses the "Partition Reference Counter MSR" described in
> > https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/timers
> > It seems it is available in both generations and could be used from i386?
> > 
> > From reading that page hv_delay() should be preferred over lapic_delay()
> 
> Alright, I have nudged hv_delay's quality up over lapic_delay's
> quality.

Before these changes, tsc is probed before pvbus.  Do the tsc sanity
checks result in it not being considered an option on hyper-v?  I think
the tsc_delay and hv_delay numbers should be swapped in a later commit.
It is unclear if that would change the final delay_func setting.

It would be a good idea to have different commits for the places new
delay callbacks are introduced.

- add delay_init()
- use delay_init() in lapic, tsc, hv_delay
- commit acpihpet
- commit acpitimer
- swap tsc and hv_delay numbers

> 
> How are we looking now?

some minor suggestions inline

have you built a release with this?

> 
> Index: sys/arch/amd64/amd64/lapic.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/lapic.c,v
> retrieving revision 1.60
> diff -u -p -r1.60 lapic.c
> --- sys/arch/amd64/amd64/lapic.c      15 Aug 2022 04:17:50 -0000      1.60
> +++ sys/arch/amd64/amd64/lapic.c      23 Aug 2022 17:18:30 -0000
> @@ -486,8 +486,6 @@ wait_next_cycle(void)
>       }
>  }
>  
> -extern void tsc_delay(int);
> -

this cleanup is unrelated and should be a different diff/commit

>  /*
>   * Calibrate the local apic count-down timer (which is running at
>   * bus-clock speed) vs. the i8254 counter/timer (which is running at
> @@ -592,8 +590,7 @@ skip_calibration:
>                * Now that the timer's calibrated, use the apic timer routines
>                * for all our timing needs..
>                */
> -             if (delay_func == i8254_delay)
> -                     delay_func = lapic_delay;
> +             delay_init(lapic_delay, 3000);
>               initclock_func = lapic_initclocks;
>       }
>  }
> Index: sys/arch/amd64/amd64/machdep.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/machdep.c,v
> retrieving revision 1.279
> diff -u -p -r1.279 machdep.c
> --- sys/arch/amd64/amd64/machdep.c    7 Aug 2022 23:56:06 -0000       1.279
> +++ sys/arch/amd64/amd64/machdep.c    23 Aug 2022 17:18:31 -0000
> @@ -2069,3 +2069,13 @@ check_context(const struct reg *regs, st
>  
>       return 0;
>  }
> +
> +void
> +delay_init(void(*fn)(int), int fn_quality)
> +{
> +     static int cur_quality = 0;
> +     if (fn_quality > cur_quality) {
> +             delay_func = fn;
> +             cur_quality = fn_quality;
> +     }
> +}
> Index: sys/arch/amd64/amd64/tsc.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/tsc.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 tsc.c
> --- sys/arch/amd64/amd64/tsc.c        12 Aug 2022 02:20:36 -0000      1.25
> +++ sys/arch/amd64/amd64/tsc.c        23 Aug 2022 17:18:31 -0000
> @@ -109,7 +109,7 @@ tsc_identify(struct cpu_info *ci)
>  
>       tsc_frequency = tsc_freq_cpuid(ci);
>       if (tsc_frequency > 0)
> -             delay_func = tsc_delay;
> +             delay_init(tsc_delay, 5000);
>  }
>  
>  static inline int
> Index: sys/arch/amd64/include/cpu.h
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/include/cpu.h,v
> retrieving revision 1.148
> diff -u -p -r1.148 cpu.h
> --- sys/arch/amd64/include/cpu.h      22 Aug 2022 08:57:54 -0000      1.148
> +++ sys/arch/amd64/include/cpu.h      23 Aug 2022 17:18:31 -0000
> @@ -359,6 +359,7 @@ void signotify(struct proc *);
>   * We need a machine-independent name for this.
>   */
>  extern void (*delay_func)(int);
> +void delay_init(void (*)(int), int);
>  struct timeval;
>  
>  #define DELAY(x)             (*delay_func)(x)
> Index: sys/arch/i386/i386/lapic.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/i386/i386/lapic.c,v
> retrieving revision 1.49
> diff -u -p -r1.49 lapic.c
> --- sys/arch/i386/i386/lapic.c        15 Aug 2022 04:17:50 -0000      1.49
> +++ sys/arch/i386/i386/lapic.c        23 Aug 2022 17:18:31 -0000
> @@ -395,7 +395,7 @@ lapic_calibrate_timer(struct cpu_info *c
>                * Now that the timer's calibrated, use the apic timer routines
>                * for all our timing needs..
>                */
> -             delay_func = lapic_delay;
> +             delay_init(lapic_delay, 3000);
>               initclock_func = lapic_initclocks;
>       }
>  }
> Index: sys/arch/i386/i386/machdep.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/i386/i386/machdep.c,v
> retrieving revision 1.655
> diff -u -p -r1.655 machdep.c
> --- sys/arch/i386/i386/machdep.c      22 Aug 2022 08:53:55 -0000      1.655
> +++ sys/arch/i386/i386/machdep.c      23 Aug 2022 17:18:33 -0000
> @@ -3974,3 +3974,13 @@ cpu_rnd_messybits(void)
>       nanotime(&ts);
>       return (ts.tv_nsec ^ (ts.tv_sec << 20));
>  }
> +
> +void
> +delay_init(void(*fn)(int), int fn_quality)
> +{
> +     static int cur_quality = 0;
> +     if (fn_quality > cur_quality) {
> +             delay_func = fn;
> +             cur_quality = fn_quality;
> +     }
> +}
> Index: sys/arch/i386/include/cpu.h
> ===================================================================
> RCS file: /cvs/src/sys/arch/i386/include/cpu.h,v
> retrieving revision 1.177
> diff -u -p -r1.177 cpu.h
> --- sys/arch/i386/include/cpu.h       22 Aug 2022 08:53:55 -0000      1.177
> +++ sys/arch/i386/include/cpu.h       23 Aug 2022 17:18:33 -0000
> @@ -302,6 +302,7 @@ void signotify(struct proc *);
>   * We need a machine-independent name for this.
>   */
>  extern void (*delay_func)(int);
> +void delay_init(void(*)(int), int);
>  struct timeval;
>  
>  #define      DELAY(x)                (*delay_func)(x)
> Index: sys/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
> --- sys/dev/acpi/acpitimer.c  6 Apr 2022 18:59:27 -0000       1.15
> +++ sys/dev/acpi/acpitimer.c  23 Aug 2022 17:18:33 -0000
> @@ -18,17 +18,22 @@
>  #include <sys/param.h>
>  #include <sys/systm.h>
>  #include <sys/device.h>
> +#include <sys/stdint.h>
>  #include <sys/timetc.h>
>  
>  #include <machine/bus.h>
> +#include <machine/cpu.h>
>  
>  #include <dev/acpi/acpireg.h>
>  #include <dev/acpi/acpivar.h>
>  
> +struct acpitimer_softc;

move acpitimer_softc here instead

> +
>  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 +103,43 @@ acpitimerattach(struct device *parent, s
>       acpi_timecounter.tc_priv = sc;
>       acpi_timecounter.tc_name = sc->sc_dev.dv_xname;
>       tc_init(&acpi_timecounter);
> +
> +     delay_init(acpitimer_delay, 1000);
> +
>  #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: sys/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
> --- sys/dev/acpi/acpihpet.c   6 Apr 2022 18:59:27 -0000       1.26
> +++ sys/dev/acpi/acpihpet.c   23 Aug 2022 17:18:33 -0000
> @@ -18,9 +18,11 @@
>  #include <sys/param.h>
>  #include <sys/systm.h>
>  #include <sys/device.h>
> +#include <sys/stdint.h>
>  #include <sys/timetc.h>
>  
>  #include <machine/bus.h>
> +#include <machine/cpu.h>
>  
>  #include <dev/acpi/acpireg.h>
>  #include <dev/acpi/acpivar.h>
> @@ -31,7 +33,7 @@ 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 +264,30 @@ 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;

this is unrelated and should be a different diff/commit

>       hpet_timecounter.tc_priv = sc;
>       hpet_timecounter.tc_name = sc->sc_dev.dv_xname;
>       tc_init(&hpet_timecounter);
> +
> +     delay_init(acpihpet_delay, 2000);
> +
>  #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: sys/dev/pv/pvbus.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pv/pvbus.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 pvbus.c
> --- sys/dev/pv/pvbus.c        5 Nov 2021 11:38:29 -0000       1.24
> +++ sys/dev/pv/pvbus.c        23 Aug 2022 17:18:33 -0000
> @@ -319,9 +319,8 @@ pvbus_hyperv(struct pvbus_hv *hv)
>           HYPERV_VERSION_EBX_MINOR_S;
>  
>  #if NHYPERV > 0
> -     if (hv->hv_features & CPUID_HV_MSR_TIME_REFCNT &&
> -         delay_func == i8254_delay)
> -             delay_func = hv_delay;
> +     if (hv->hv_features & CPUID_HV_MSR_TIME_REFCNT)
> +             delay_init(hv_delay, 4000);
>  #endif
>  }
>  
> 
> 

Reply via email to