On Mon, 2022-10-24 at 22:49 +0000, Sean Christopherson wrote:
> 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@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".
>
> 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.
IMHO this is corner cutting and you yourself said that this is wrong.
The other instances which use semicolon should be fixed IMHO.
Best regards,
Maxim Levitsky
>
