On Thu, 2022-10-20 at 19:14 +0000, Sean Christopherson wrote:
> On Thu, Oct 20, 2022, Maxim Levitsky wrote:
> > Add a few functions to apic.c to make it easier to enable and disable
> > the local apic timer.
> >
> > Signed-off-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> > ---
> > lib/x86/apic.c | 37 +++++++++++++++++++++++++++++++++++++
> > lib/x86/apic.h | 6 ++++++
> > 2 files changed, 43 insertions(+)
> >
> > diff --git a/lib/x86/apic.c b/lib/x86/apic.c
> > index 5131525a..dc6d3862 100644
> > --- a/lib/x86/apic.c
> > +++ b/lib/x86/apic.c
> > @@ -256,3 +256,40 @@ void init_apic_map(void)
> > id_map[j++] = i;
> > }
> > }
> > +
> > +void apic_setup_timer(int vector, bool periodic)
> > +{
> > + /* APIC runs with 'CPU core clock' divided by value in APIC_TDCR */
> > +
> > + u32 lvtt = vector |
> > + (periodic ? APIC_LVT_TIMER_PERIODIC : APIC_LVT_TIMER_ONESHOT);
>
> Rather than take @periodic, take the mode. That way this funky ternary operator
> goes away and the callers are self-tdocumenting, e.g. this
>
> apic_setup_timer(TIMER_VECTOR, APIC_LVT_TIMER_PERIODIC);
>
> is more obvious than
>
> apic_setup_timer(TIMER_VECTOR, true);
Makes sense. I also wanted to pass the divider, but ended up hardcoding it to 1.
>
> > +
> > + apic_cleanup_timer();
> > + apic_write(APIC_TDCR, APIC_TDR_DIV_1);
> > + apic_write(APIC_LVTT, lvtt);
> > +}
> > +
> > +void apic_start_timer(u32 counter)
> > +{
> > + apic_write(APIC_TMICT, counter);
> > +}
Makes sense.
> > +
> > +void apic_stop_timer(void)
> > +{
> > + apic_write(APIC_TMICT, 0);
> > +}
> > +
> > +void apic_cleanup_timer(void)
> > +{
> > + u32 lvtt = apic_read(APIC_LVTT);
> > +
> > + // stop the counter
> > + apic_stop_timer();
> > +
> > + // mask the timer interrupt
> > + apic_write(APIC_LVTT, lvtt | APIC_LVT_MASKED);
> > +
> > + // ensure that a pending timer is serviced
> > + irq_enable();
>
> Jumping back to the "nop" patch, I'm reinforcing my vote to add sti_nop(). I
> actually starting typing a response to say this is broken before remembering that
> a nop got added to irq_enable().
OK, although, for someone that doesn't know about the interrupt shadow (I guess most of the people that will look at this code),
the above won't confuse them, in fact sti_nop() might confuse someone who doesn't know about why this nop is needed.
Just a note.
Best regards,
Maxim Levitsky
>
> > + irq_disable();
> > +}
> > diff --git a/lib/x86/apic.h b/lib/x86/apic.h
> > index 6d27f047..db691e2a 100644
> > --- a/lib/x86/apic.h
> > +++ b/lib/x86/apic.h
> > @@ -58,6 +58,12 @@ void disable_apic(void);
> > void reset_apic(void);
> > void init_apic_map(void);
> >
> > +void apic_cleanup_timer(void);
> > +void apic_setup_timer(int vector, bool periodic);
> > +
> > +void apic_start_timer(u32 counter);
> > +void apic_stop_timer(void);
> > +
> > /* Converts byte-addressable APIC register offset to 4-byte offset. */
> > static inline u32 apic_reg_index(u32 reg)
> > {
> > --
> > 2.26.3
> >
>
