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@xxxxxxxxxx>
> ---
> 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.
