| Message ID | 20200417113524.1280130-1-ubizjak@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: VMX: Improve handle_external_interrupt_irqoff inline assembly | expand |
On 17/04/20 13:35, Uros Bizjak wrote: > Improve handle_external_interrupt_irqoff inline assembly in several ways: > - use __stringify to use __KERNEL_DS and __KERNEL_CS directly in assembler What's the advantage of this to pushing cs and ss? We are faking an interrupt gate, and interrupts push cs and ss not __KERNEL_DS and __KERNEL_CS. > - use %rsp instead of _ASM_SP, since we are in CONFIG_X86_64 part > - use $-16 immediate to align %rsp > - avoid earlyclobber by using "l" GCC input operand constraint What is this and where is it documented?!? :) Paolo > - avoid temporary by using current_stack_pointer > > The patch introduces no functional change. > > 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 | 19 ++++++------------- > 1 file changed, 6 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 01330096ff3e..4b0d5f0044ff 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -6233,9 +6233,6 @@ static void handle_external_interrupt_irqoff(struct kvm_vcpu *vcpu) > { > unsigned int vector; > unsigned long entry; > -#ifdef CONFIG_X86_64 > - unsigned long tmp; > -#endif > gate_desc *desc; > u32 intr_info; > > @@ -6252,23 +6249,19 @@ 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" > + "and $-16, %%rsp\n\t" > + "push $" __stringify(__KERNEL_DS) "\n\t" > "push %[sp]\n\t" > #endif > "pushf\n\t" > - __ASM_SIZE(push) " $%c[cs]\n\t" > + __ASM_SIZE(push) " $" __stringify(__KERNEL_CS) "\n\t" > CALL_NOSPEC > + : ASM_CALL_CONSTRAINT > :
On Fri, Apr 17, 2020 at 2:07 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 17/04/20 13:35, Uros Bizjak wrote: > > Improve handle_external_interrupt_irqoff inline assembly in several ways: > > - use __stringify to use __KERNEL_DS and __KERNEL_CS directly in assembler > > What's the advantage of this to pushing cs and ss? We are faking an > interrupt gate, and interrupts push cs and ss not __KERNEL_DS and > __KERNEL_CS. I see. If the current form is documenting the above, then there is indeed no compelling reason for the change. > > > - use %rsp instead of _ASM_SP, since we are in CONFIG_X86_64 part > > - use $-16 immediate to align %rsp > > - avoid earlyclobber by using "l" GCC input operand constraint > > What is this and where is it documented?!? :) I was going to say that it is documented in [1] under x86 family section, but indeed, there is no description of "l" constraint. It is internal to the compiler. :( (define_register_constraint "l" "INDEX_REGS" "@internal Any register that can be used as the index in a base+index memory access: that is, any general register except the stack pointer.") Other changes are minor and really not worth the maintainer's time to handle the patch. Please consider the patch retracted. [1] https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html Uros.
On 17/04/20 14:25, Uros Bizjak wrote: > I was going to say that it is documented in [1] under x86 family > section, but indeed, there is no description of "l" constraint. It is > internal to the compiler. :( Yes, I looked there. It's worth documenting it in GCC, I think. Paolo > (define_register_constraint "l" "INDEX_REGS" > "@internal Any register that can be used as the index in a base+index > memory access: that is, any general register except the stack pointer.")
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 01330096ff3e..4b0d5f0044ff 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -6233,9 +6233,6 @@ static void handle_external_interrupt_irqoff(struct kvm_vcpu *vcpu) { unsigned int vector; unsigned long entry; -#ifdef CONFIG_X86_64 - unsigned long tmp; -#endif gate_desc *desc; u32 intr_info; @@ -6252,23 +6249,19 @@ 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" + "and $-16, %%rsp\n\t" + "push $" __stringify(__KERNEL_DS) "\n\t" "push %[sp]\n\t" #endif "pushf\n\t" - __ASM_SIZE(push) " $%c[cs]\n\t" + __ASM_SIZE(push) " $" __stringify(__KERNEL_CS) "\n\t" CALL_NOSPEC + : ASM_CALL_CONSTRAINT : #ifdef CONFIG_X86_64 - [sp]"=&r"(tmp), + [sp]"l"(current_stack_pointer), #endif - ASM_CALL_CONSTRAINT - : - [thunk_target]"r"(entry), - [ss]"i"(__KERNEL_DS), - [cs]"i"(__KERNEL_CS) + [thunk_target]"r"(entry) ); kvm_after_interrupt(vcpu);
Improve handle_external_interrupt_irqoff inline assembly in several ways: - use __stringify to use __KERNEL_DS and __KERNEL_CS directly in assembler - use %rsp instead of _ASM_SP, since we are in CONFIG_X86_64 part - use $-16 immediate to align %rsp - avoid earlyclobber by using "l" GCC input operand constraint - avoid temporary by using current_stack_pointer The patch introduces no functional change. 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 | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)