Re: amd64: lapic: refactor lapic timer programming

2020-07-06 Thread Mike Larkin
On Fri, Jul 03, 2020 at 07:41:45PM -0500, Scott Cheloha wrote:
> Hi,
>
> I want to run the lapic timer in one-shot mode on amd64 as we do with
> other interrupt clocks on other platforms.  I aim to make the clock
> interrupt code MD where possible.
>
> However, nobody is going to test my MD clock interrupt work unless
> amd64 is ready to use it.  amd64 doesn't run in oneshot mode so there
> is preliminary work to do first.
>
> --
>
> Before we can run the lapic timer in one-shot mode we need to simplify
> the process of actually programming it.
>
> This patch refactors all lapic timer programming into a single
> routine.  We don't use any divisor other than 1 so I don't see a need
> to make it a parameter to lapic_timer_arm().  We can add TSC deadline
> support later if someone wants it.
>
> The way we program the timer differs from how e.g. Darwin and FreeBSD
> and Linux do it.  They write:
>
>  - lvtt (mode + vector + (maybe) mask)
>  - dcr
>  - icr
>
> while we do:
>
>  - lvtt (mode + mask)
>  - dcr
>  - icr
>  - (maybe) lvtt (mode + vector)
>
> I don't see a reason to arm the timer with four writes instead of
> three, so in this patch I use the three-write ordering.
>
> Am I missing something?  Do I need to disable interrupts before I
> reprogram the timer?
>

This reads ok to me. I am not aware of any requirements to disable
interrupts while reprogramming the timer.

-ml

> -Scott
>
> Index: lapic.c
> ===
> RCS file: /cvs/src/sys/arch/amd64/amd64/lapic.c,v
> retrieving revision 1.55
> diff -u -p -r1.55 lapic.c
> --- lapic.c   3 Aug 2019 14:57:51 -   1.55
> +++ lapic.c   4 Jul 2020 00:40:26 -
> @@ -413,6 +413,42 @@ u_int32_t lapic_frac_usec_per_cycle;
>  u_int64_t lapic_frac_cycle_per_usec;
>  u_int32_t lapic_delaytab[26];
>
> +void lapic_timer_arm(uint32_t, int, uint32_t);
> +void lapic_timer_arm_once(int, uint32_t);
> +void lapic_timer_arm_period(int, uint32_t);
> +
> +/*
> + * Start the local apic countdown timer.
> + *
> + * First set the mode, vector, and (maybe) the mask.
> + * then set the divisor,
> + * and finally set the cycle count.
> + */
> +void
> +lapic_timer_arm(uint32_t mode, int masked, uint32_t cycles)
> +{
> + uint32_t lvtt;
> +
> + lvtt = mode | LAPIC_TIMER_VECTOR;
> + lvtt |= (masked) ? LAPIC_LVTT_M : 0;
> +
> + lapic_writereg(LAPIC_LVTT, lvtt);
> + lapic_writereg(LAPIC_DCR_TIMER, LAPIC_DCRT_DIV1);
> + lapic_writereg(LAPIC_ICR_TIMER, cycles);
> +}
> +
> +void
> +lapic_timer_arm_once(int masked, uint32_t cycles)
> +{
> + lapic_timer_arm(LAPIC_LVTT_TM_ONESHOT, masked, cycles);
> +}
> +
> +void
> +lapic_timer_arm_period(int masked, uint32_t cycles)
> +{
> + lapic_timer_arm(LAPIC_LVTT_TM_PERIODIC, masked, cycles);
> +}
> +
>  void
>  lapic_clockintr(void *arg, struct intrframe frame)
>  {
> @@ -430,17 +466,7 @@ lapic_clockintr(void *arg, struct intrfr
>  void
>  lapic_startclock(void)
>  {
> - /*
> -  * Start local apic countdown timer running, in repeated mode.
> -  *
> -  * Mask the clock interrupt and set mode,
> -  * then set divisor,
> -  * then unmask and set the vector.
> -  */
> - lapic_writereg(LAPIC_LVTT, LAPIC_LVTT_TM|LAPIC_LVTT_M);
> - lapic_writereg(LAPIC_DCR_TIMER, LAPIC_DCRT_DIV1);
> - lapic_writereg(LAPIC_ICR_TIMER, lapic_tval);
> - lapic_writereg(LAPIC_LVTT, LAPIC_LVTT_TM|LAPIC_TIMER_VECTOR);
> + lapic_timer_arm_period(0, lapic_tval);
>  }
>
>  void
> @@ -498,9 +524,7 @@ lapic_calibrate_timer(struct cpu_info *c
>* Configure timer to one-shot, interrupt masked,
>* large positive number.
>*/
> - lapic_writereg(LAPIC_LVTT, LAPIC_LVTT_M);
> - lapic_writereg(LAPIC_DCR_TIMER, LAPIC_DCRT_DIV1);
> - lapic_writereg(LAPIC_ICR_TIMER, 0x8000);
> + lapic_timer_arm_once(1, 0x8000);
>
>   s = intr_disable();
>
> @@ -540,10 +564,7 @@ skip_calibration:
>   lapic_tval = (lapic_per_second * 2) / hz;
>   lapic_tval = (lapic_tval / 2) + (lapic_tval & 0x1);
>
> - lapic_writereg(LAPIC_LVTT, LAPIC_LVTT_TM | LAPIC_LVTT_M |
> - LAPIC_TIMER_VECTOR);
> - lapic_writereg(LAPIC_DCR_TIMER, LAPIC_DCRT_DIV1);
> - lapic_writereg(LAPIC_ICR_TIMER, lapic_tval);
> + lapic_timer_arm_period(0, lapic_tval);
>
>   /*
>* Compute fixed-point ratios between cycles and
>



amd64: lapic: refactor lapic timer programming

2020-07-03 Thread Scott Cheloha
Hi,

I want to run the lapic timer in one-shot mode on amd64 as we do with
other interrupt clocks on other platforms.  I aim to make the clock
interrupt code MD where possible.

However, nobody is going to test my MD clock interrupt work unless
amd64 is ready to use it.  amd64 doesn't run in oneshot mode so there
is preliminary work to do first.

--

Before we can run the lapic timer in one-shot mode we need to simplify
the process of actually programming it.

This patch refactors all lapic timer programming into a single
routine.  We don't use any divisor other than 1 so I don't see a need
to make it a parameter to lapic_timer_arm().  We can add TSC deadline
support later if someone wants it.

The way we program the timer differs from how e.g. Darwin and FreeBSD
and Linux do it.  They write:

 - lvtt (mode + vector + (maybe) mask)
 - dcr
 - icr

while we do:

 - lvtt (mode + mask)
 - dcr
 - icr
 - (maybe) lvtt (mode + vector)

I don't see a reason to arm the timer with four writes instead of
three, so in this patch I use the three-write ordering.

Am I missing something?  Do I need to disable interrupts before I
reprogram the timer?

-Scott

Index: lapic.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/lapic.c,v
retrieving revision 1.55
diff -u -p -r1.55 lapic.c
--- lapic.c 3 Aug 2019 14:57:51 -   1.55
+++ lapic.c 4 Jul 2020 00:40:26 -
@@ -413,6 +413,42 @@ u_int32_t lapic_frac_usec_per_cycle;
 u_int64_t lapic_frac_cycle_per_usec;
 u_int32_t lapic_delaytab[26];
 
+void lapic_timer_arm(uint32_t, int, uint32_t);
+void lapic_timer_arm_once(int, uint32_t);
+void lapic_timer_arm_period(int, uint32_t);
+
+/*
+ * Start the local apic countdown timer.
+ *
+ * First set the mode, vector, and (maybe) the mask.
+ * then set the divisor,
+ * and finally set the cycle count.
+ */
+void
+lapic_timer_arm(uint32_t mode, int masked, uint32_t cycles)
+{
+   uint32_t lvtt;
+
+   lvtt = mode | LAPIC_TIMER_VECTOR;
+   lvtt |= (masked) ? LAPIC_LVTT_M : 0;
+
+   lapic_writereg(LAPIC_LVTT, lvtt);
+   lapic_writereg(LAPIC_DCR_TIMER, LAPIC_DCRT_DIV1);
+   lapic_writereg(LAPIC_ICR_TIMER, cycles);
+}
+
+void
+lapic_timer_arm_once(int masked, uint32_t cycles)
+{
+   lapic_timer_arm(LAPIC_LVTT_TM_ONESHOT, masked, cycles);
+}
+
+void
+lapic_timer_arm_period(int masked, uint32_t cycles)
+{
+   lapic_timer_arm(LAPIC_LVTT_TM_PERIODIC, masked, cycles);
+}
+
 void
 lapic_clockintr(void *arg, struct intrframe frame)
 {
@@ -430,17 +466,7 @@ lapic_clockintr(void *arg, struct intrfr
 void
 lapic_startclock(void)
 {
-   /*
-* Start local apic countdown timer running, in repeated mode.
-*
-* Mask the clock interrupt and set mode,
-* then set divisor,
-* then unmask and set the vector.
-*/
-   lapic_writereg(LAPIC_LVTT, LAPIC_LVTT_TM|LAPIC_LVTT_M);
-   lapic_writereg(LAPIC_DCR_TIMER, LAPIC_DCRT_DIV1);
-   lapic_writereg(LAPIC_ICR_TIMER, lapic_tval);
-   lapic_writereg(LAPIC_LVTT, LAPIC_LVTT_TM|LAPIC_TIMER_VECTOR);
+   lapic_timer_arm_period(0, lapic_tval);
 }
 
 void
@@ -498,9 +524,7 @@ lapic_calibrate_timer(struct cpu_info *c
 * Configure timer to one-shot, interrupt masked,
 * large positive number.
 */
-   lapic_writereg(LAPIC_LVTT, LAPIC_LVTT_M);
-   lapic_writereg(LAPIC_DCR_TIMER, LAPIC_DCRT_DIV1);
-   lapic_writereg(LAPIC_ICR_TIMER, 0x8000);
+   lapic_timer_arm_once(1, 0x8000);
 
s = intr_disable();
 
@@ -540,10 +564,7 @@ skip_calibration:
lapic_tval = (lapic_per_second * 2) / hz;
lapic_tval = (lapic_tval / 2) + (lapic_tval & 0x1);
 
-   lapic_writereg(LAPIC_LVTT, LAPIC_LVTT_TM | LAPIC_LVTT_M |
-   LAPIC_TIMER_VECTOR);
-   lapic_writereg(LAPIC_DCR_TIMER, LAPIC_DCRT_DIV1);
-   lapic_writereg(LAPIC_ICR_TIMER, lapic_tval);
+   lapic_timer_arm_period(0, lapic_tval);
 
/*
 * Compute fixed-point ratios between cycles and