Re: [PATCH 37/74] lto, KVM: Don't assume asm statements end up in the same assembler file

Re: [PATCH 37/74] lto, KVM: Don't assume asm statements end up in the same assembler file

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 37/74] lto, KVM: Don't assume asm statements end up in the same assembler file

[Andi Kleen]

> 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

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 37/74] lto, KVM: Don't assume asm statements end up in the same assembler file

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 37/74] lto, KVM: Don't assume asm statements end up in the same assembler file

[Andi Kleen]

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


-- 
ak@linux.intel.com -- Speaking for myself only.