Re: [PATCH v2 gnumach] lapic timer: Calibrate via mach timer not PIT

[Samuel Thibault]

Applied, thanks!

Damien Zammit, le lun. 06 mars 2023 07:05:03 +0000, a ecrit:
> Previously the lapic timer was calibrated by one-shot PIT timer2.
> This method can be buggy and generally unused in emulation environments.
> This patch reworks the timer calibration to use a mach timer based
> on regular PIT interrupts to remapped IOAPIC pin.
> This also changes the primary clock source to use PIT timer0 remapped
> to an IOAPIC pin when APIC mode is enabled, instead of a periodic lapic
> timer.
> ---
>  i386/i386/apic.h        |  3 +-
>  i386/i386at/ioapic.c    | 81 +++++++++++++++++++++++------------------
>  i386/i386at/model_dep.c |  8 +++-
>  3 files changed, 54 insertions(+), 38 deletions(-)
> 
> diff --git a/i386/i386/apic.h b/i386/i386/apic.h
> index ac083d26..a79f0ea8 100644
> --- a/i386/i386/apic.h
> +++ b/i386/i386/apic.h
> @@ -243,11 +243,12 @@ void lapic_eoi(void);
>  void ioapic_irq_eoi(int pin);
>  void lapic_enable(void);
>  void lapic_enable_timer(void);
> +void calibrate_lapic_timer(void);
>  void ioapic_mask_irqs(void);
>  void ioapic_toggle(int pin, int mask);
>  void ioapic_configure(void);
> 
> -extern int duplicate_pin;
> +extern int timer_pin;
>  extern void intnull(int unit);
>  extern volatile ApicLocalUnit* lapic;
> 
> diff --git a/i386/i386at/ioapic.c b/i386/i386at/ioapic.c
> index d2ea84ad..c6da35e1 100644
> --- a/i386/i386at/ioapic.c
> +++ b/i386/i386at/ioapic.c
> @@ -28,11 +28,13 @@
>  #include <i386/pio.h>
>  #include <i386/pit.h>
>  #include <i386/pic.h> /* only for macros */
> +#include <i386/smp.h>
>  #include <mach/machine.h>
>  #include <kern/printf.h>
> +#include <kern/timer.h>
> 
>  static int has_irq_specific_eoi = 1; /* FIXME: Assume all machines have this 
> */
> -int duplicate_pin;
> +int timer_pin;
> 
>  uint32_t lapic_timer_val = 0;
>  uint32_t calibrated_ticks = 0;
> @@ -43,7 +45,7 @@ int iunit[NINTR] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 
> 12, 13, 14, 15,
>                      16, 17, 18, 19, 20, 21, 22, 23};
> 
>  interrupt_handler_fn ivect[NINTR] = {
> -    /* 00 */ intnull,        /* install timer later */
> +    /* 00 */ (interrupt_handler_fn)hardclock,
>      /* 01 */ kdintr,         /* kdintr, ... */
>      /* 02 */ intnull,
>      /* 03 */ intnull,        /* lnpoll, comintr, ... */
> @@ -150,32 +152,58 @@ ioapic_toggle_entry(int apic, int pin, int mask)
>      ioapic_write(apic, APIC_IO_REDIR_LOW(pin), entry.lo);
>  }
> 
> +static void timer_expiry_callback(void *arg)
> +{
> +    volatile int *done = arg;
> +    *done = 1;
> +}
> +
>  static uint32_t
> -pit_measure_10x_apic_hz(void)
> +timer_measure_10x_apic_hz(void)
>  {
> -    volatile int i;
> +    volatile int done = 0;
>      uint32_t start = 0xffffffff;
> +    timer_elt_data_t tmp_timer;
> +    tmp_timer.fcn = timer_expiry_callback;
> +    tmp_timer.param = (void *)&done;
> 
> -    /* Prepare accurate delay for 1/hz seconds */
> -    pit_prepare_sleep(hz);
> +    printf("timer calibration...");
> 
>      /* Set APIC timer */
>      lapic->init_count.r = start;
> 
> -    /* zZz */
> -    for (i = 0; i < 10; i++)
> -        pit_sleep();
> +    /* Delay for 10 * 1/hz seconds */
> +    set_timeout(&tmp_timer, hz / 10);
> +    do {
> +        cpu_pause();
> +    } while (!done);
> 
>      /* Stop APIC timer */
>      lapic->lvt_timer.r |= LAPIC_DISABLE;
> 
> +    printf(" done\n");
> +
>      return start - lapic->cur_count.r;
>  }
> 
> -void lapic_update_timer(void)
> +void
> +calibrate_lapic_timer(void)
>  {
> -    /* Timer decrements until zero and then calls this on every interrupt */
> -    lapic_timer_val += calibrated_ticks;
> +    spl_t s;
> +
> +    /* Set one-shot timer */
> +    lapic->divider_config.r = LAPIC_TIMER_DIVIDE_2;
> +    lapic->lvt_timer.r = IOAPIC_INT_BASE;
> +
> +    /* Measure number of APIC timer ticks in 10x 1/hz seconds
> +     * but calibrate the timer to expire at rate of hz
> +     * divide by 10 because we waited 10 times longer than we needed. */
> +    if (!calibrated_ticks) {
> +        s = splhigh();
> +        spl0();
> +        calibrated_ticks = timer_measure_10x_apic_hz() / 10;
> +        splx(s);
> +    }
>  }
> 
>  void
> @@ -306,16 +334,14 @@ ioapic_configure(void)
>              /* Save timer info */
>              timer_gsi = gsi;
>          } else {
> -            /* Disable duplicated timer gsi */
> +            /* Remap timer irq */
>              if (gsi == timer_gsi) {
> -                duplicate_pin = pin;
> -                /* Remap this interrupt pin to GSI base
> -                 * so we don't duplicate vectors */
> +                timer_pin = pin;
> +                /* Remap GSI base to timer pin so ivect[0] is the timer */
>                  entry.both.vector = IOAPIC_INT_BASE;
> -                ioapic_write_entry(apic, duplicate_pin, entry.both);
> -                /* Mask the ioapic pin with deduplicated vector as
> -              * we will never use it, since timer is on another gsi */
> -                mask_irq(duplicate_pin);
> +                ioapic_write_entry(apic, timer_pin, entry.both);
> +                /* Mask the duplicate pin 0 as we will be using timer_pin */
> +                mask_irq(0);
>              }
>          }
>      }
> @@ -337,20 +363,5 @@ ioapic_configure(void)
>      /* Start the IO APIC receiving interrupts */
>      lapic_enable();
> 
> -    /* Set one-shot timer */
> -    lapic->divider_config.r = LAPIC_TIMER_DIVIDE_2;
> -    lapic->lvt_timer.r = IOAPIC_INT_BASE;
> -
> -    /* Measure number of APIC timer ticks in 10x 1/hz seconds
> -     * but calibrate the timer to expire at rate of hz
> -     * divide by 10 because we waited 10 times longer than we needed */
> -    calibrated_ticks = pit_measure_10x_apic_hz() / 10;
> -
> -    /* Set up counter later */
> -    lapic->lvt_timer.r = LAPIC_DISABLE;
> -
> -    /* Install clock interrupt handler on pin 0 */
> -    ivect[0] = (interrupt_handler_fn)hardclock;
> -
>      printf("IOAPIC 0 configured\n");
>  }
> diff --git a/i386/i386at/model_dep.c b/i386/i386at/model_dep.c
> index baff8da1..e8462ba3 100644
> --- a/i386/i386at/model_dep.c
> +++ b/i386/i386at/model_dep.c
> @@ -171,7 +171,7 @@ void machine_init(void)
>  #if defined(APIC)
>       ioapic_configure();
>  #endif
> -     startrtclock();
> +     clkstart();
> 
>  #if defined(APIC)
>  #warning FIXME: Rather unmask them from their respective drivers
> @@ -593,7 +593,11 @@ void
>  startrtclock(void)
>  {
>  #ifdef APIC
> -     lapic_enable_timer();
> +     unmask_irq(timer_pin);
> +     calibrate_lapic_timer();
> +     if (cpu_number() != 0) {
> +             lapic_enable_timer();
> +     }
>  #else
>       clkstart();
>       unmask_irq(0);
> --
> 2.39.0
> 
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.