| Message ID | 1345345030-22211-38-git-send-email-andi@firstfloor.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/19/2012 05:56 AM, Andi Kleen wrote: > From: Andi Kleen <ak@linux.intel.com> > > The VMX code references a local assembler label between two inline > assembler statements. This assumes they both end up in the same > assembler files. In some experimental builds of gcc this is not > necessarily true, causing linker failures. > > Replace the local label reference with a more traditional asmlinkage > extern. > > This also eliminates one assembler statement and > generates a bit better code on 64bit: the compiler can > use a RIP relative LEA instead of a movabs, saving > a few bytes. I'm happy to see work on lto-enabling the kernel. > > +extern __visible unsigned long kvm_vmx_return; > + > /* > * Set up the vmcs's constant host-state fields, i.e., host-state fields that > * will not change in the lifetime of the guest. > @@ -3753,8 +3755,7 @@ static void vmx_set_constant_host_state(void) > native_store_idt(&dt); > vmcs_writel(HOST_IDTR_BASE, dt.address); /* 22.2.4 */ > > - asm("mov $.Lkvm_vmx_return, %0" : "=r"(tmpl)); > - vmcs_writel(HOST_RIP, tmpl); /* 22.2.5 */ > + vmcs_writel(HOST_RIP, (unsigned long)&kvm_vmx_return); /* 22.2.5 */ > > rdmsr(MSR_IA32_SYSENTER_CS, low32, high32); > vmcs_write32(HOST_IA32_SYSENTER_CS, low32); > @@ -6305,9 +6306,10 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) > /* Enter guest mode */ > "jne .Llaunched \n\t" > __ex(ASM_VMX_VMLAUNCH) "\n\t" > - "jmp .Lkvm_vmx_return \n\t" > + "jmp kvm_vmx_return \n\t" > ".Llaunched: " __ex(ASM_VMX_VMRESUME) "\n\t" > - ".Lkvm_vmx_return: " > + ".globl kvm_vmx_return\n" > + "kvm_vmx_return: " > /* Save guest registers, load host registers, keep flags */ > "mov %0, %c[wordsize](%%"R"sp) \n\t" > "pop %0 \n\t" > The reason we use a local label is so that we the function isn't split into two from the profiler's point of view. See cd2276a795b013d1. One way to fix this is to have a .data variable initialized to point to .Lkvm_vmx_return (this can be done from the same asm statement in vmx_vcpu_run), and reference that variable in vmx_set_constant_host_state(). If no one comes up with a better idea, I'll write a patch doing this.
> The reason we use a local label is so that we the function isn't split > into two from the profiler's point of view. See cd2276a795b013d1. Hmm that commit message is not very enlightening. The goal was to force a compiler error? With LTO there is no way to force two functions be in the same assembler file. The partitioner is always allowed to split. > > One way to fix this is to have a .data variable initialized to point to > .Lkvm_vmx_return (this can be done from the same asm statement in > vmx_vcpu_run), and reference that variable in > vmx_set_constant_host_state(). If no one comes up with a better idea, > I'll write a patch doing this. I'm not clear how that is better than my patch. -andi
On 08/19/2012 06:09 PM, Andi Kleen wrote: >> The reason we use a local label is so that we the function isn't split >> into two from the profiler's point of view. See cd2276a795b013d1. > > Hmm that commit message is not very enlightening. > > The goal was to force a compiler error? No, the goal was to avoid a global label in the middle of a function. The profiler interprets it as a new function. After your patch, profiles will show a function named kvm_vmx_return taking a few percent cpu, although there is no such function. > > With LTO there is no way to force two functions be in the same assembler > file. The partitioner is always allowed to split. I'm not trying to force two functions to be in the same assembler file. > >> >> One way to fix this is to have a .data variable initialized to point to >> .Lkvm_vmx_return (this can be done from the same asm statement in >> vmx_vcpu_run), and reference that variable in >> vmx_set_constant_host_state(). If no one comes up with a better idea, >> I'll write a patch doing this. > > I'm not clear how that is better than my patch. My patch will not generate the artifact with kvm_vmx_return.
On Sun, Aug 19, 2012 at 06:12:57PM +0300, Avi Kivity wrote: > On 08/19/2012 06:09 PM, Andi Kleen wrote: > >> The reason we use a local label is so that we the function isn't split > >> into two from the profiler's point of view. See cd2276a795b013d1. > > > > Hmm that commit message is not very enlightening. > > > > The goal was to force a compiler error? > > No, the goal was to avoid a global label in the middle of a function. > The profiler interprets it as a new function. After your patch, Ah got it now. I always used to have the same problem with sys_call_return.` I wonder if there shouldn't be a way to tell perf to ignore a symbol. > >> > >> One way to fix this is to have a .data variable initialized to point to > >> .Lkvm_vmx_return (this can be done from the same asm statement in > >> vmx_vcpu_run), and reference that variable in > >> vmx_set_constant_host_state(). If no one comes up with a better idea, > >> I'll write a patch doing this. > > > > I'm not clear how that is better than my patch. > > My patch will not generate the artifact with kvm_vmx_return. Ok fine for me. I'll keep this patch for now, until you have something better. -Andi
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index c00f03d..2fe1de3 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -3718,6 +3718,8 @@ static void vmx_disable_intercept_for_msr(u32 msr, bool longmode_only) __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode, msr); } +extern __visible unsigned long kvm_vmx_return; + /* * Set up the vmcs's constant host-state fields, i.e., host-state fields that * will not change in the lifetime of the guest. @@ -3753,8 +3755,7 @@ static void vmx_set_constant_host_state(void) native_store_idt(&dt); vmcs_writel(HOST_IDTR_BASE, dt.address); /* 22.2.4 */ - asm("mov $.Lkvm_vmx_return, %0" : "=r"(tmpl)); - vmcs_writel(HOST_RIP, tmpl); /* 22.2.5 */ + vmcs_writel(HOST_RIP, (unsigned long)&kvm_vmx_return); /* 22.2.5 */ rdmsr(MSR_IA32_SYSENTER_CS, low32, high32); vmcs_write32(HOST_IA32_SYSENTER_CS, low32); @@ -6305,9 +6306,10 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) /* Enter guest mode */ "jne .Llaunched \n\t" __ex(ASM_VMX_VMLAUNCH) "\n\t" - "jmp .Lkvm_vmx_return \n\t" + "jmp kvm_vmx_return \n\t" ".Llaunched: " __ex(ASM_VMX_VMRESUME) "\n\t" - ".Lkvm_vmx_return: " + ".globl kvm_vmx_return\n" + "kvm_vmx_return: " /* Save guest registers, load host registers, keep flags */ "mov %0, %c[wordsize](%%"R"sp) \n\t" "pop %0 \n\t"