Re: [kvm-unit-tests PATCH 01/16] x86: make irq_enable avoid the interrupt shadow

[Sean Christopherson <seanjc@google.com>]

On Thu, Oct 20, 2022, Maxim Levitsky wrote:
> Tests that need interrupt shadow can't rely on irq_enable function anyway,
> as its comment states,  and it is useful to know for sure that interrupts
> are enabled after the call to this function.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  lib/x86/processor.h       | 9 ++++-----
>  x86/apic.c                | 1 -
>  x86/ioapic.c              | 1 -
>  x86/svm_tests.c           | 9 ---------
>  x86/tscdeadline_latency.c | 1 -
>  x86/vmx_tests.c           | 7 -------
>  6 files changed, 4 insertions(+), 24 deletions(-)
> 
> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
> index 03242206..9db07346 100644
> --- a/lib/x86/processor.h
> +++ b/lib/x86/processor.h
> @@ -720,13 +720,12 @@ static inline void irq_disable(void)
>  	asm volatile("cli");
>  }
>  
> -/* Note that irq_enable() does not ensure an interrupt shadow due
> - * to the vagaries of compiler optimizations.  If you need the
> - * shadow, use a single asm with "sti" and the instruction after it.
> - */
>  static inline void irq_enable(void)
>  {
> -	asm volatile("sti");
> +	asm volatile(
> +			"sti \n\t"

Formatting is odd.  Doesn't really matter, but I think this can simply be:

static inline void sti_nop(void)
{
	asm volatile("sti; nop");
}


> +			"nop\n\t"

I like the idea of a helper to enable IRQs and consume pending interrupts, but I
think we should add a new helper instead of changing irq_enable().

Hmm, or alternatively, kill off irq_enable() and irq_disable() entirely and instead
add sti_nop().  I like this idea even better.  The helpers are all x86-specific,
so there's no need to add a layer of abstraction, and sti() + sti_nop() has the
benefit of making it very clear what code is being emitted without having to come
up with clever function names.

And I think we should go even further and provide a helper to do the entire sequence
of enable->nop->disable, which is a very common pattern.  No idea what to call
this one, though I suppose sti_nop_cli() would work.

My vote is to replace all irq_enable() and irq_disable() usage with sti() and cli(),
and then introduce sti_nop() and sti_nop_cli() (or whatever it gets called) and
convert users as appropriate.

> +	);
>  }
>  
>  static inline void invlpg(volatile void *va)
> diff --git a/x86/apic.c b/x86/apic.c
> index 23508ad5..a8964d88 100644
> --- a/x86/apic.c
> +++ b/x86/apic.c
> @@ -36,7 +36,6 @@ static void __test_tsc_deadline_timer(void)
>      irq_enable();
>  
>      wrmsr(MSR_IA32_TSCDEADLINE, rdmsr(MSR_IA32_TSC));
> -    asm volatile ("nop");

I'm not entirely sure the existing nop is necessary here, but it's a functional
change since it hoists the nop above the WRMSR.  To be safe, probably best to
leave this as-is for now.

>      report(tdt_count == 1, "tsc deadline timer");
>      report(rdmsr(MSR_IA32_TSCDEADLINE) == 0, "tsc deadline timer clearing");
>  }

...

> diff --git a/x86/tscdeadline_latency.c b/x86/tscdeadline_latency.c
> index a3bc4ea4..c54530dd 100644
> --- a/x86/tscdeadline_latency.c
> +++ b/x86/tscdeadline_latency.c
> @@ -73,7 +73,6 @@ static void start_tsc_deadline_timer(void)
>      irq_enable();
>  
>      wrmsr(MSR_IA32_TSCDEADLINE, rdmsr(MSR_IA32_TSC)+delta);
> -    asm volatile ("nop");

Another functional change that should be skipped, at least for now.