Mailing List Archive: Linux: Kernel

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

Index | Next | Previous | View Threaded

andi at firstfloor

Aug 18, 2012, 7:56 PM

Post #1 of 5 (71 views)
Permalink

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

From: Andi Kleen <ak [at] linux>

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.

Cc: avi [at] redhat
Signed-off-by: Andi Kleen <ak [at] linux>
---
arch/x86/kvm/vmx.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)

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"
--
1.7.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

avi at redhat

Aug 19, 2012, 1:59 AM

Post #2 of 5 (69 views)
Permalink

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

On 08/19/2012 05:56 AM, Andi Kleen wrote:
> From: Andi Kleen <ak [at] linux>
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

andi at firstfloor

Aug 19, 2012, 8:09 AM

Post #3 of 5 (63 views)
Permalink

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

> 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 [at] linux -- Speaking for myself only.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

avi at redhat

Aug 19, 2012, 8:12 AM

Post #4 of 5 (61 views)
Permalink

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

andi at firstfloor

Aug 19, 2012, 8:20 AM

Post #5 of 5 (63 views)
Permalink

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

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 [at] linux -- Speaking for myself only.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads