| Message ID | 20200503230545.442042-1-ubizjak@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] KVM: VMX: Improve handle_external_interrupt_irqoff inline assembly | expand |
On Mon, May 04, 2020 at 01:05:45AM +0200, Uros Bizjak wrote: > Improve handle_external_interrupt_irqoff inline assembly in several ways: > - use "re" 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. Hmm, for handcoded assembly I would argue that the switch from "i" to "re" is a functional change of sorts. The switch also needs explicit justification to explain why it's correct/desirable. Maybe make it a separate patch? > 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..56c742effb30 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]"re"(__KERNEL_DS), > +#endif > + [cs]"re"(__KERNEL_CS) > ); > > kvm_after_interrupt(vcpu); > -- > 2.25.4 >
On Mon, May 4, 2020 at 5:25 PM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Mon, May 04, 2020 at 01:05:45AM +0200, Uros Bizjak wrote: > > Improve handle_external_interrupt_irqoff inline assembly in several ways: > > - use "re" 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. > > Hmm, for handcoded assembly I would argue that the switch from "i" to "re" > is a functional change of sorts. The switch also needs explicit > justification to explain why it's correct/desirable. Maybe make it a > separate patch? I think this would be a good idea. So, in this patch the first point should read "- remove unneeded %c operand modifiers and "$" prefixes" The add-on patch will then explain that PUSH can only handle signed 32bit immediates and change "i" to "re". Is this what you had in mind? Thanks, Uros.
On Mon, May 04, 2020 at 05:32:19PM +0200, Uros Bizjak wrote: > On Mon, May 4, 2020 at 5:25 PM Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > > > On Mon, May 04, 2020 at 01:05:45AM +0200, Uros Bizjak wrote: > > > Improve handle_external_interrupt_irqoff inline assembly in several ways: > > > - use "re" 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. > > > > Hmm, for handcoded assembly I would argue that the switch from "i" to "re" > > is a functional change of sorts. The switch also needs explicit > > justification to explain why it's correct/desirable. Maybe make it a > > separate patch? > > I think this would be a good idea. So, in this patch the first point should read > > "- remove unneeded %c operand modifiers and "$" prefixes" > > The add-on patch will then explain that PUSH can only handle signed > 32bit immediates and change "i" to "re". > > Is this what you had in mind? Yep, exactly.
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index c2c6335a998c..56c742effb30 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]"re"(__KERNEL_DS), +#endif + [cs]"re"(__KERNEL_CS) ); kvm_after_interrupt(vcpu);
Improve handle_external_interrupt_irqoff inline assembly in several ways: - use "re" 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(-)