On Thu, 2022-10-20 at 18:01 +0000, Sean Christopherson wrote:
> 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");
"\n\t" is what gcc manual recommends for separating the assembly lines as you know from the gcc manual:
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
"You may place multiple assembler instructions together in a single asm string, separated by
the characters normally used in assembly code for the system. A combination that works in
most places is a newline to break the line, plus a tab character to move to the instruction
field (written as ‘\n\t’). Some assemblers allow semicolons as a line separator.
However, note that some assembler dialects use semicolons to start a comment"
Looks like gnu assembler does use semicolon for new statements and hash for comments
but some assemblers do semicolon for comments.
I usually use just "\n", but the safest is "\n\t".
> }
>
>
> > + "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.
OK.
>
> > + );
> > }
> >
> > 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.
I had doubts about this, IMHO both before and after are equally good, but anyway to be safe,
I'll revert this change.
>
> > 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.
OK.
>
Best regards,
Maxim Levitsky
