Re: [kvm-unit-tests PATCH 02/16] x86: add few helper functions for apic local timer

[Sean Christopherson <seanjc@google.com>]

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@redhat.com>
> ---
>  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);
	
> +
> +	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);
> +}
> +
> +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().

> +	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
>