| Message ID | 20200426115255.305060-1-ubizjak@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] KVM: VMX: Improve handle_external_interrupt_irqoff inline assembly | expand |
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 >
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 > >
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.
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.
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
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.
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.
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);
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(-)