On Mon, Apr 27, 2020 at 9:25 PM Sean Christopherson
<sean.j.christopherson@xxxxxxxxx> wrote:
>
> On Sun, Apr 26, 2020 at 01:52:55PM +0200, Uros Bizjak wrote:
> > Improve handle_external_interrupt_irqoff inline assembly in several ways:
> > - use "n" operand constraint instead of "i" and remove
>
> What's the motivation for using 'n'? The 'i' variant is much more common,
> i.e. less likely to trip up readers.
>
> $ git grep -E "\"i\"\s*\(" | wc -l
> 768
> $ git grep -E "\"n\"\s*\(" | wc -l
> 11
When only numerical constants are allowed, "n" should be used, as
demonstrated by the following artificial example:
--cut here--
#define IMM 123
int z;
int
test (void)
{
__label__ lab;
__asm__ __volatile__ ("push %0" :: "n"(IMM));
__asm__ __volatile__ ("push %0" :: "i"(&z));
__asm__ __volatile__ ("push %0" :: "i"(&&lab));
return 1;
lab:
return 0;
}
--cut here--
changing "i" to "n" will trigger a compiler error in the second and
the third case.
The compiler documentation is a bit unclear here:
'i'
An immediate integer operand (one with constant value) is allowed.
This includes symbolic constants whose values will be known only at
assembly time or later.
'n'
An immediate integer operand with a known numeric value is allowed.
Many systems cannot support assembly-time constants for operands
less than a word wide. Constraints for these operands should use
'n' rather than 'i'.
PUSH is able to use "i" here, since the operand is word wide. But, do
we really want to allow symbol references and labels here?
> > unneeded %c operand modifiers and "$" prefixes
> > - use %rsp instead of _ASM_SP, since we are in CONFIG_X86_64 part
> > - use $-16 immediate to align %rsp
>
> Heh, this one depends on the reader, I find 0xfffffffffffffff0 to be much
> more intuitive, though admittedly also far easier to screw up.
I was beaten by this in the past ... but don't want to bikeshed here.
BR,
Uros.
> > - remove unneeded use of __ASM_SIZE macro
> > - define "ss" named operand only for X86_64
> >
> > The patch introduces no functional changes.
> >
> > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > Cc: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > Signed-off-by: Uros Bizjak <ubizjak@xxxxxxxxx>
> > ---
> > arch/x86/kvm/vmx/vmx.c | 14 ++++++++------
> > 1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index c2c6335a998c..7471f1b948b3 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -6283,13 +6283,13 @@ static void handle_external_interrupt_irqoff(struct kvm_vcpu *vcpu)
> >
> > asm volatile(
> > #ifdef CONFIG_X86_64
> > - "mov %%" _ASM_SP ", %[sp]\n\t"
> > - "and $0xfffffffffffffff0, %%" _ASM_SP "\n\t"
> > - "push $%c[ss]\n\t"
> > + "mov %%rsp, %[sp]\n\t"
> > + "and $-16, %%rsp\n\t"
> > + "push %[ss]\n\t"
> > "push %[sp]\n\t"
> > #endif
> > "pushf\n\t"
> > - __ASM_SIZE(push) " $%c[cs]\n\t"
> > + "push %[cs]\n\t"
> > CALL_NOSPEC
> > :
> > #ifdef CONFIG_X86_64
> > @@ -6298,8 +6298,10 @@ static void handle_external_interrupt_irqoff(struct kvm_vcpu *vcpu)
> > ASM_CALL_CONSTRAINT
> > :
> > [thunk_target]"r"(entry),
> > - [ss]"i"(__KERNEL_DS),
> > - [cs]"i"(__KERNEL_CS)
> > +#ifdef CONFIG_X86_64
> > + [ss]"n"(__KERNEL_DS),
> > +#endif
> > + [cs]"n"(__KERNEL_CS)
> > );
> >
> > kvm_after_interrupt(vcpu);
> > --
> > 2.25.3
> >
