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

[Sean Christopherson <seanjc@google.com>]

On Mon, Oct 24, 2022, Maxim Levitsky wrote:
> 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@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");
> 
> "\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".

I'm pretty sure we can ignore GCC's warning here and maximize readability.  There
are already plenty of asm blobs that use a semicolon.