[PATCH] KVM: Avoid using vmx instruction directly

[PATCH] KVM: Avoid using vmx instruction directly

[Avi Kivity]

Some users have an older assembler installed which doesn't grok the
vmx instructions.

Fix by encoding the instruction opcodes directly.

Signed-off-by: Avi Kivity <avi@qumranet.com>

Index: linux-2.6/drivers/kvm/kvm.h
===================================================================
--- linux-2.6.orig/drivers/kvm/kvm.h
+++ linux-2.6/drivers/kvm/kvm.h
@@ -377,6 +377,16 @@ static inline struct kvm_mmu_page *page_
 	return (struct kvm_mmu_page *)page->private;
 }
 
+#define ASM_VMX_VMCLEAR_RAX       ".byte 0x66, 0x0f, 0xc7, 0x30"
+#define ASM_VMX_VMLAUNCH          ".byte 0x0f, 0x01, 0xc2"
+#define ASM_VMX_VMRESUME          ".byte 0x0f, 0x01, 0xc3"
+#define ASM_VMX_VMPTRLD_RAX       ".byte 0x0f, 0xc7, 0x30"
+#define ASM_VMX_VMREAD_RDX_RAX    ".byte 0x0f, 0x78, 0xd0"
+#define ASM_VMX_VMWRITE_RAX_RDX   ".byte 0x0f, 0x79, 0xd0"
+#define ASM_VMX_VMWRITE_RSP_RDX   ".byte 0x0f, 0x79, 0xd4"
+#define ASM_VMX_VMXOFF            ".byte 0x0f, 0x01, 0xc4"
+#define ASM_VMX_VMXON_RAX         ".byte 0xf3, 0x0f, 0xc7, 0x30"
+
 #ifdef __x86_64__
 
 /*
Index: linux-2.6/drivers/kvm/kvm_main.c
===================================================================
--- linux-2.6.orig/drivers/kvm/kvm_main.c
+++ linux-2.6/drivers/kvm/kvm_main.c
@@ -369,8 +369,8 @@ static void vmcs_clear(struct vmcs *vmcs
 	u64 phys_addr = __pa(vmcs);
 	u8 error;
 
-	asm volatile ("vmclear %1; setna %0"
-		       : "=m"(error) : "m"(phys_addr) : "cc", "memory" );
+	asm volatile (ASM_VMX_VMCLEAR_RAX "; setna %0"
+		       : "=g"(error) : "a"(&phys_addr) : "cc", "memory" );
 	if (error)
 		printk(KERN_ERR "kvm: vmclear fail: %p/%llx\n",
 		       vmcs, phys_addr);
@@ -412,8 +412,8 @@ static struct kvm_vcpu *__vcpu_load(stru
 		u8 error;
 
 		per_cpu(current_vmcs, cpu) = vcpu->vmcs;
-		asm volatile ("vmptrld %1; setna %0"
-			       : "=m"(error) : "m"(phys_addr) : "cc" );
+		asm volatile (ASM_VMX_VMPTRLD_RAX "; setna %0"
+			       : "=g"(error) : "a"(&phys_addr) : "cc" );
 		if (error)
 			printk(KERN_ERR "kvm: vmptrld %p/%llx fail\n",
 			       vcpu->vmcs, phys_addr);
@@ -536,12 +536,12 @@ static __init void kvm_enable(void *garb
 		/* enable and lock */
 		wrmsrl(MSR_IA32_FEATURE_CONTROL, old | 5);
 	write_cr4(read_cr4() | CR4_VMXE); /* FIXME: not cpu hotplug safe */
-	asm volatile ("vmxon %0" : : "m"(phys_addr) : "memory", "cc");
+	asm volatile (ASM_VMX_VMXON_RAX : : "a"(&phys_addr) : "memory", "cc");
 }
 
 static void kvm_disable(void *garbage)
 {
-	asm volatile ("vmxoff" : : : "cc");
+	asm volatile (ASM_VMX_VMXOFF : : : "cc");
 }
 
 static int kvm_dev_open(struct inode *inode, struct file *filp)
@@ -633,7 +633,8 @@ unsigned long vmcs_readl(unsigned long f
 {
 	unsigned long value;
 
-	asm volatile ("vmread %1, %0" : "=g"(value) : "r"(field) : "cc");
+	asm volatile (ASM_VMX_VMREAD_RDX_RAX
+		      : "=a"(value) : "d"(field) : "cc");
 	return value;
 }
 
@@ -641,8 +642,8 @@ void vmcs_writel(unsigned long field, un
 {
 	u8 error;
 
-	asm volatile ("vmwrite %1, %2; setna %0"
-		       : "=g"(error) : "r"(value), "r"(field) : "cc" );
+	asm volatile (ASM_VMX_VMWRITE_RAX_RDX "; setna %0"
+		       : "=q"(error) : "a"(value), "d"(field) : "cc" );
 	if (error)
 		printk(KERN_ERR "vmwrite error: reg %lx value %lx (err %d)\n",
 		       field, value, vmcs_read32(VM_INSTRUCTION_ERROR));
@@ -2634,10 +2635,10 @@ again:
 		"push %%r8;  push %%r9;  push %%r10; push %%r11;"
 		"push %%r12; push %%r13; push %%r14; push %%r15;"
 		"push %%rcx \n\t"
-		"vmwrite %%rsp, %2 \n\t"
+		ASM_VMX_VMWRITE_RSP_RDX "\n\t"
 #else
 		"pusha; push %%ecx \n\t"
-		"vmwrite %%esp, %2 \n\t"
+		ASM_VMX_VMWRITE_RSP_RDX "\n\t"
 #endif
 		/* Check if vmlaunch of vmresume is needed */
 		"cmp $0, %1 \n\t"
@@ -2673,9 +2674,9 @@ again:
 #endif
 		/* Enter guest mode */
 		"jne launched \n\t"
-		"vmlaunch \n\t"
+		ASM_VMX_VMLAUNCH "\n\t"
 		"jmp kvm_vmx_return \n\t"
-		"launched: vmresume \n\t"
+		"launched: " ASM_VMX_VMRESUME "\n\t"
 		".globl kvm_vmx_return \n\t"
 		"kvm_vmx_return: "
 		/* Save guest registers, load host registers, keep flags */
@@ -2722,7 +2723,7 @@ again:
 		"setbe %0 \n\t"
 		"popf \n\t"
 	      : "=g" (fail)
-	      : "r"(vcpu->launched), "r"((unsigned long)HOST_RSP),
+	      : "r"(vcpu->launched), "d"((unsigned long)HOST_RSP),
 		"c"(vcpu),
 		[rax]"i"(offsetof(struct kvm_vcpu, regs[VCPU_REGS_RAX])),
 		[rbx]"i"(offsetof(struct kvm_vcpu, regs[VCPU_REGS_RBX])),

Re: [kvm-devel] [PATCH] KVM: Avoid using vmx instruction directly

[Arnd Bergmann]

On Thursday 09 November 2006 12:08, Avi Kivity wrote:
> Index: linux-2.6/drivers/kvm/kvm_main.c
> ===================================================================
> --- linux-2.6.orig/drivers/kvm/kvm_main.c
> +++ linux-2.6/drivers/kvm/kvm_main.c
> @@ -369,8 +369,8 @@ static void vmcs_clear(struct vmcs *vmcs
>         u64 phys_addr = __pa(vmcs);
>         u8 error;
>  
> -       asm volatile ("vmclear %1; setna %0"
> -                      : "=m"(error) : "m"(phys_addr) : "cc", "memory" );
> +       asm volatile (ASM_VMX_VMCLEAR_RAX "; setna %0"
> +                      : "=g"(error) : "a"(&phys_addr) : "cc", "memory" );
>         if (error)
>                 printk(KERN_ERR "kvm: vmclear fail: %p/%llx\n",
>                        vmcs, phys_addr);

I'm not an expert on inline assembly, but don't you need an extra
'"m" (phys_addr)' to make sure that gcc actually puts the variable
on the stack instead of passing a NULL pointer as '"a"(&phys_addr)'?

	Arnd <><

Re: [kvm-devel] [PATCH] KVM: Avoid using vmx instruction directly

[Avi Kivity]

Arnd Bergmann wrote:
> On Thursday 09 November 2006 12:08, Avi Kivity wrote:
>   
>> Index: linux-2.6/drivers/kvm/kvm_main.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/kvm/kvm_main.c
>> +++ linux-2.6/drivers/kvm/kvm_main.c
>> @@ -369,8 +369,8 @@ static void vmcs_clear(struct vmcs *vmcs
>>         u64 phys_addr = __pa(vmcs);
>>         u8 error;
>>  
>> -       asm volatile ("vmclear %1; setna %0"
>> -                      : "=m"(error) : "m"(phys_addr) : "cc", "memory" );
>> +       asm volatile (ASM_VMX_VMCLEAR_RAX "; setna %0"
>> +                      : "=g"(error) : "a"(&phys_addr) : "cc", "memory" );
>>         if (error)
>>                 printk(KERN_ERR "kvm: vmclear fail: %p/%llx\n",
>>                        vmcs, phys_addr);
>>     
>
> I'm not an expert on inline assembly, but don't you need an extra
> '"m" (phys_addr)' to make sure that gcc actually puts the variable
> on the stack instead of passing a NULL pointer as '"a"(&phys_addr)'?
>
>   

Taking a variable's address should force its contents into memory (like 
calling an uninlined function with &var).


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

Re: [kvm-devel] [PATCH] KVM: Avoid using vmx instruction directly

[Arnd Bergmann]

On Thursday 09 November 2006 14:36, Avi Kivity wrote:
> 
> >
> > I'm not an expert on inline assembly, but don't you need an extra
> > '"m" (phys_addr)' to make sure that gcc actually puts the variable
> > on the stack instead of passing a NULL pointer as '"a"(&phys_addr)'?
> 
> Taking a variable's address should force its contents into memory (like 
> calling an uninlined function with &var).

No it doesn't. You're not telling gcc that the inline assembly cares
about the contents of the variable, so it could be a reference to
a stack slot while the contents are still in a register. Or gcc
might move the assignment of phys_addr to after the inline assembly.

	Arnd <><

Re: [kvm-devel] [PATCH] KVM: Avoid using vmx instruction directly

[Avi Kivity]

Arnd Bergmann wrote:
> On Thursday 09 November 2006 14:36, Avi Kivity wrote:
>   
>>> I'm not an expert on inline assembly, but don't you need an extra
>>> '"m" (phys_addr)' to make sure that gcc actually puts the variable
>>> on the stack instead of passing a NULL pointer as '"a"(&phys_addr)'?
>>>       
>> Taking a variable's address should force its contents into memory (like 
>> calling an uninlined function with &var).
>>     
>
> No it doesn't. You're not telling gcc that the inline assembly cares
> about the contents of the variable, so it could be a reference to
> a stack slot while the contents are still in a register. 

Wouldn't that make inline assembly useless?  Suppose the contents is 
itself a pointer.  What about the pointed-to contents?

e.g.

    int x = 3;
    int *y = &x;
    int z;

    asm ("mov %1, %%rax; movl (%%rax), %0" : "=r"(z) : "g"(y) : "rax");
    assert(z == 3);

> Or gcc
> might move the assignment of phys_addr to after the inline assembly.
>   
"asm volatile" prevents that (and I'm not 100% sure it's necessary).


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

Re: [kvm-devel] [PATCH] KVM: Avoid using vmx instruction directly

[Arnd Bergmann]

On Thursday 09 November 2006 15:52, Avi Kivity wrote:
> Wouldn't that make inline assembly useless?  Suppose the contents is 
> itself a pointer.  What about the pointed-to contents?
> 
> e.g.
> 
>     int x = 3;
>     int *y = &x;
>     int z;
> 
>     asm ("mov %1, %%rax; movl (%%rax), %0" : "=r"(z) : "g"(y) : "rax");
>     assert(z == 3);

Same here, you need to tell gcc what is really accessed, like 

asm ("mov %1, %%rax; movl (%%rax), %0" : "=r"(z) : "g"(y), "m"(*y) : "rax");

I know that the s390 kernel developers have hit that problem
frequently with inline assemblies. It may be that it's harder
to hit on x86, because there are fewer registers available and
data therefore tends to spill to the stack.

> > Or gcc
> > might move the assignment of phys_addr to after the inline assembly.
> >   
> "asm volatile" prevents that (and I'm not 100% sure it's necessary).

Yes, I think that's right. The 'volatile' should not be necessary though,
if you get the inputs right.

	Arnd <><

Re: [kvm-devel] [PATCH] KVM: Avoid using vmx instruction directly

[Avi Kivity]

Arnd Bergmann wrote:
> On Thursday 09 November 2006 15:52, Avi Kivity wrote:
>   
>> Wouldn't that make inline assembly useless?  Suppose the contents is 
>> itself a pointer.  What about the pointed-to contents?
>>
>> e.g.
>>
>>     int x = 3;
>>     int *y = &x;
>>     int z;
>>
>>     asm ("mov %1, %%rax; movl (%%rax), %0" : "=r"(z) : "g"(y) : "rax");
>>     assert(z == 3);
>>     
>
> Same here, you need to tell gcc what is really accessed, like 
>
> asm ("mov %1, %%rax; movl (%%rax), %0" : "=r"(z) : "g"(y), "m"(*y) : "rax");
>
> I know that the s390 kernel developers have hit that problem
> frequently with inline assemblies. It may be that it's harder
> to hit on x86, because there are fewer registers available and
> data therefore tends to spill to the stack.
>   

I'll update my tree to reflect this.  Thanks.

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

Re: [kvm-devel] [PATCH] KVM: Avoid using vmx instruction directly

[Jeremy Fitzhardinge]

Avi Kivity wrote:
>> Or gcc
>> might move the assignment of phys_addr to after the inline assembly.
>>   
> "asm volatile" prevents that (and I'm not 100% sure it's necessary).

No, it won't necessarily.  "asm volatile" simply forces gcc to emit the
assembler, even if it thinks its output doesn't get used.  It makes no
ordering guarantees with respect to other code (or even other "asm
volatiles").   The "memory" clobbers should fix the ordering of the asms
though.


    J

Re: [kvm-devel] [PATCH] KVM: Avoid using vmx instruction directly

[Martin Schwidefsky]

On 11/10/06, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> >> Or gcc
> >> might move the assignment of phys_addr to after the inline assembly.
> >>
> > "asm volatile" prevents that (and I'm not 100% sure it's necessary).
>
> No, it won't necessarily.  "asm volatile" simply forces gcc to emit the
> assembler, even if it thinks its output doesn't get used.  It makes no
> ordering guarantees with respect to other code (or even other "asm
> volatiles").   The "memory" clobbers should fix the ordering of the asms
> though.

The "memory" clobber just tells the compiler that any memory object
might get access by the inline. This forces the compiler to write back
values it cached in registers and to reload the values after the
inline assembly. This does NOT make it generate correct code for local
objects. We had the case where we created a control block on the stack
and passed it to a magic instruction. Since we did not tell the
compiler that the content of the control block is used but only the
address of it, gcc just passed a local stack address to the inline but
optimized the initialization of the control block away. So the
following can break:

struct control_block {
        int a, b;
};

void fn(void)
{
        struct control_block x;

        x.a = 42;
        x.b = 0815;
        asm volatile ("<magic>" : : "a" (&x) : "memory");
}

You won't find the assignments to x.a and x.b in the compiled code.

-- 
blue skies,
  Martin

Re: [kvm-devel] [PATCH] KVM: Avoid using vmx instruction directly

[Jeremy Fitzhardinge]

Martin Schwidefsky wrote:
> On 11/10/06, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>> >> Or gcc
>> >> might move the assignment of phys_addr to after the inline assembly.
>> >>
>> > "asm volatile" prevents that (and I'm not 100% sure it's necessary).
>>
>> No, it won't necessarily.  "asm volatile" simply forces gcc to emit the
>> assembler, even if it thinks its output doesn't get used.  It makes no
>> ordering guarantees with respect to other code (or even other "asm
>> volatiles").   The "memory" clobbers should fix the ordering of the asms
>> though.
>
> The "memory" clobber just tells the compiler that any memory object
> might get access by the inline. 

I just meant that two asms with a "memory" clobber will be generated
with a fixed ordering, which "asm volatile" does not necessarily do.

    J

Re: [kvm-devel] [PATCH] KVM: Avoid using vmx instruction directly

[H. Peter Anvin]

Jeremy Fitzhardinge wrote:
> Avi Kivity wrote:
>>> Or gcc
>>> might move the assignment of phys_addr to after the inline assembly.
>>>   
>> "asm volatile" prevents that (and I'm not 100% sure it's necessary).
> 
> No, it won't necessarily.  "asm volatile" simply forces gcc to emit the
> assembler, even if it thinks its output doesn't get used.  It makes no
> ordering guarantees with respect to other code (or even other "asm
> volatiles").   The "memory" clobbers should fix the ordering of the asms
> though.
> 

I think you're wrong about that; in particular, I'm pretty sure "asm 
volatiles" are ordered among themselves.  What the "volatile" means is 
"this has side effects you (the compiler) don't understand", and gcc 
can't assume that it can reorder such side effects.

	-hpa

Re: [kvm-devel] [PATCH] KVM: Avoid using vmx instruction directly

[Avi Kivity]

H. Peter Anvin wrote:
> Jeremy Fitzhardinge wrote:
>> Avi Kivity wrote:
>>>> Or gcc
>>>> might move the assignment of phys_addr to after the inline assembly.
>>>>   
>>> "asm volatile" prevents that (and I'm not 100% sure it's necessary).
>>
>> No, it won't necessarily.  "asm volatile" simply forces gcc to emit the
>> assembler, even if it thinks its output doesn't get used.  It makes no
>> ordering guarantees with respect to other code (or even other "asm
>> volatiles").   The "memory" clobbers should fix the ordering of the asms
>> though.
>>
>
> I think you're wrong about that; in particular, I'm pretty sure "asm 
> volatiles" are ordered among themselves.  What the "volatile" means is 
> "this has side effects you (the compiler) don't understand", and gcc 
> can't assume that it can reorder such side effects.

The gcc manual has this to say:

   Similarly, you can't expect a sequence of volatile `asm' instructions
  to remain perfectly consecutive.  If you want consecutive output, use a
  single `asm'.  Also, GCC will perform some optimizations across a
  volatile `asm' instruction; GCC does not "forget everything" when it
  encounters a volatile `asm' instruction the way some other compilers do.

I wonder how we are supposed to code the following sequence:


    asm volatile ("blah")  /* sets funky processor mode */

    some_c_code();

    asm volatile ("unblah");

Let's say "blah" disables floating point exceptions, and some_c_code() 
must run without exceptions.  Is is possible to code this in gcc without 
putting functions in another translation unit?  Is a memory clobber 
sufficient?  I'd certainly hate to use it.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [kvm-devel] [PATCH] KVM: Avoid using vmx instruction directly

[Jeremy Fitzhardinge]

H. Peter Anvin wrote:
> I think you're wrong about that; in particular, I'm pretty sure "asm
> volatiles" are ordered among themselves.  What the "volatile" means is
> "this has side effects you (the compiler) don't understand", and gcc
> can't assume that it can reorder such side effects. 
That's not how I read the manual (quoted below).  "asm volatile" is much
weaker than people seem to think it is; the "volatile" puts fewer
constraints on the compiler than it would for a variable.  While the
manual doesn't say that "asm volatiles" could be reordered with respect
to each other, it doesn't say that they won't, and I don't see anything
in this description which could be read to imply it (indeed "can be
moved relative to other code" includes other asm volatiles).

Like "volatile" variables, I think "asm volatile" is probably overused. 
If you want to guarantee specific ordering of asms, it's probably better
to add an explicit dependency between them rather than rely on asm
volatile; this could either be a "memory" clobber, or something more
fine-grained.  For example:

    /* need never be instansiated; never actually referenced */
    extern int spin_sequencer;

    /* %0 never referenced */
    asm("take spinlock" : "+m" (spin_sequencer)...);

    ...

    /* again, %0 never referenced */
    asm("release spinlock" : "+m" (spin_sequencer)...);

This is example is a bit contrived since a real spinlock would also have
to have a memory clobber - or some other barrier - but you get the
idea.  It has the nice property of allowing you to define precise
dependencies between various asm()s, without having to set up
unnecessary dependencies between unrelated asms; and by making use of
in/out/inout asm parameters, you can express different kinds of
dependencies which give gcc a better chance of understanding what's
going on.

The relevent bit of the manual:

    The `volatile' keyword indicates that the instruction has important
    side-effects.  GCC will not delete a volatile `asm' if it is reachable.
    (The instruction can still be deleted if GCC can prove that
    control-flow will never reach the location of the instruction.)  Note
    that even a volatile `asm' instruction can be moved relative to other
    code, including across jump instructions.  For example, on many targets
    there is a system register which can be set to control the rounding
    mode of floating point operations.  You might try setting it with a
    volatile `asm', like this PowerPC example:

                asm volatile("mtfsf 255,%0" : : "f" (fpenv));
                sum = x + y;

    This will not work reliably, as the compiler may move the addition back
    before the volatile `asm'.  To make it work you need to add an
    artificial dependency to the `asm' referencing a variable in the code
    you don't want moved, for example:

             asm volatile ("mtfsf 255,%1" : "=X"(sum): "f"(fpenv));
             sum = x + y;

     Similarly, you can't expect a sequence of volatile `asm' instructions
    to remain perfectly consecutive.  If you want consecutive output, use a
    single `asm'.  Also, GCC will perform some optimizations across a
    volatile `asm' instruction; GCC does not "forget everything" when it
    encounters a volatile `asm' instruction the way some other compilers do.

     An `asm' instruction without any output operands will be treated
    identically to a volatile `asm' instruction.

Re: [kvm-devel] [PATCH] KVM: Avoid using vmx instruction directly

[Avi Kivity]

Jeremy Fitzhardinge wrote:
>
>
> Like "volatile" variables, I think "asm volatile" is probably overused.
> If you want to guarantee specific ordering of asms, it's probably better
> to add an explicit dependency between them rather than rely on asm
> volatile; this could either be a "memory" clobber, or something more
> fine-grained.  For example:
>
>     /* need never be instansiated; never actually referenced */
>     extern int spin_sequencer;
>
>     /* %0 never referenced */
>     asm("take spinlock" : "+m" (spin_sequencer)...);
>
>     ...
>
>     /* again, %0 never referenced */
>     asm("release spinlock" : "+m" (spin_sequencer)...);
>

Very interesting.

Will it work on load/store architectures?  Since all memory access is 
through a register, won't the constraint generate a useless register 
load (and a use of the variable)?


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [kvm-devel] [PATCH] KVM: Avoid using vmx instruction directly

[Jeremy Fitzhardinge]

Avi Kivity wrote:
> Very interesting.
>
> Will it work on load/store architectures?  Since all memory access is
> through a register, won't the constraint generate a useless register
> load (and a use of the variable)?

Don't know; interesting question.  It might be worth lobbying the gcc
folks for an asm() constraint which means "pretend this is being
read/written, but don't generate any code, and raise an error if the asm
actually tries to use it".  Or perhaps there's some way to do that already.

On the other hand, load/store archs tend to have lots of registers
anyway, so maybe it isn't a big deal.

    J