[PATCH v2] KVM: VMX: Improve handle_external_interrupt_irqoff inline assembly

[PATCH v2] KVM: VMX: Improve handle_external_interrupt_irqoff inline assembly

[Uros Bizjak]

Improve handle_external_interrupt_irqoff inline assembly in several ways:
- use "n" operand constraint instead of "i" and remove
  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
- 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@redhat.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
---
 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

Re: [PATCH v2] KVM: VMX: Improve handle_external_interrupt_irqoff inline assembly

[Sean Christopherson]

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

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

> - 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@redhat.com>
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> ---
>  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
> 

Re: [PATCH v2] KVM: VMX: Improve handle_external_interrupt_irqoff inline assembly

[Uros Bizjak]

On Mon, Apr 27, 2020 at 9:25 PM Sean Christopherson
<sean.j.christopherson@intel.com> 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@redhat.com>
> > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> > ---
> >  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
> >

Re: [PATCH v2] KVM: VMX: Improve handle_external_interrupt_irqoff inline assembly

[Sean Christopherson]

On Mon, Apr 27, 2020 at 10:08:01PM +0200, Uros Bizjak wrote:
> On Mon, Apr 27, 2020 at 9:25 PM Sean Christopherson
> <sean.j.christopherson@intel.com> 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

...

> 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?

No, but on the other hand, I doubt this particular code is going to change
much.  I don't have a strong preference.

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

I'm good with either approach.  Same as above, the argument for keeping the
existing code is that it's there, it works, and from some people it's more
readable.

Re: [PATCH v2] KVM: VMX: Improve handle_external_interrupt_irqoff inline assembly

[Uros Bizjak]

On Tue, Apr 28, 2020 at 12:30 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Mon, Apr 27, 2020 at 10:08:01PM +0200, Uros Bizjak wrote:
> > On Mon, Apr 27, 2020 at 9:25 PM Sean Christopherson
> > <sean.j.christopherson@intel.com> 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
>
> ...
>
> > 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?
>
> No, but on the other hand, I doubt this particular code is going to change
> much.  I don't have a strong preference.
>
> > > >   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.
>
> I'm good with either approach.  Same as above, the argument for keeping the
> existing code is that it's there, it works, and from some people it's more
> readable.

Thanks, I'll leave these two discussed points as they were and prepare a v3.

Uros.

Re: [PATCH v2] KVM: VMX: Improve handle_external_interrupt_irqoff inline assembly

[Paolo Bonzini]

On 26/04/20 13:52, Uros Bizjak wrote:
> Improve handle_external_interrupt_irqoff inline assembly in several ways:
> - use "n" operand constraint instead of "i" and remove
>   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
> - remove unneeded use of __ASM_SIZE macro
> - define "ss" named operand only for X86_64
> 
> The patch introduces no functional changes.

I think I agree with all of these, so the patch is okay!  Thanks,

Paolo

Re: [PATCH v2] KVM: VMX: Improve handle_external_interrupt_irqoff inline assembly

[Uros Bizjak]

On Tue, Apr 28, 2020 at 3:48 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 26/04/20 13:52, Uros Bizjak wrote:
> > Improve handle_external_interrupt_irqoff inline assembly in several ways:
> > - use "n" operand constraint instead of "i" and remove
> >   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
> > - remove unneeded use of __ASM_SIZE macro
> > - define "ss" named operand only for X86_64
> >
> > The patch introduces no functional changes.
>
> I think I agree with all of these, so the patch is okay!  Thanks,

Actually, after some more thinking, neither "i", and neither "n" is
correct for x86_64 as far as push is concerned. The correct constraint
is "e", but in case the value doesn't fit this constraint, we have to
allow "r" and eventually "m". Let's use "rme", which allows everything
the insn is able to handle, and leave to the compiler to use the
optimal one. GCC uses this constraint internally, and it also fits
32bit targets.

V3 patch is in the works.

Uros.

Re: [PATCH v2] KVM: VMX: Improve handle_external_interrupt_irqoff inline assembly

[Uros Bizjak]

On Mon, May 4, 2020 at 12:23 AM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> On Tue, Apr 28, 2020 at 3:48 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On 26/04/20 13:52, Uros Bizjak wrote:
> > > Improve handle_external_interrupt_irqoff inline assembly in several ways:
> > > - use "n" operand constraint instead of "i" and remove
> > >   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
> > > - remove unneeded use of __ASM_SIZE macro
> > > - define "ss" named operand only for X86_64
> > >
> > > The patch introduces no functional changes.
> >
> > I think I agree with all of these, so the patch is okay!  Thanks,
>
> Actually, after some more thinking, neither "i", and neither "n" is
> correct for x86_64 as far as push is concerned. The correct constraint
> is "e", but in case the value doesn't fit this constraint, we have to
> allow "r" and eventually "m". Let's use "rme", which allows everything
> the insn is able to handle, and leave to the compiler to use the
> optimal one. GCC uses this constraint internally, and it also fits
> 32bit targets.

And yes... I forgot that "m" allows stack slots, which won't fly due
to clobbered stack pointer.

> V3 patch is in the works.

V4, actually. Sorry for the mess.

Uros.