Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-24 Thread Arthur Chunqi Li
Hi Gleb and Paolo,
What about organizing vmx_run() as follows:

static int vmx_run()
{
u32 eax;
bool ret;

vmcs_write(HOST_RSP, get_rsp());
ret = vmlaunch();
while (!ret) {
asm volatile(
vmx_return:\n\t
SAVE_GPR
);
eax = exit_handler();
switch (eax) {
case VMX_TEST_VMEXIT:
return 0;
case VMX_TEST_RESUME:
break;
default:
printf(%s : unhandled ret from exit_handler.\n, __func__);
return 1;
}
ret = vmresume();
}
printf(%s : vmenter failed.\n, __func__);
return 1;
}

Arthur

On Fri, Jul 19, 2013 at 8:06 PM, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 19/07/2013 11:40, Gleb Natapov ha scritto:
 Because this is written in C, and I know trying to fool the compiler is
 a losing game.  So my reaction is okay, HOST_RIP must be set so that
 code will not jump around.  If I see

asm(vmlaunch)
exit(-1)

 the reaction is the opposite: hmm, anything that jumps around would
 have a hard time with the compiler, there must be some assembly
 trampoline somewhere; let's check what HOST_RIP is.

 We do try to fool compiler often even without vmx: code patching. This is
 why asm goto was invented, no? So, like you said in previous emails,
 asm goto is appropriate way here to tell compiler what is going on.

 Code patching is only reimplementing an existing C structure (if/else)
 in a different manner.  Here the actual code flow (location of HOST_RIP
 and value of HOST_RSP) cannot be expressed correctly to the compiler
 because we do not use the C label (we use an asm label).

 I don't think asm goto can be made to work for vmx_return, though if we
 go for a big blob it could be useful to jump to the error handling code.

 I don't see anything bad in jumping completely to a different context.
 The guest and host are sort of like two coroutines, they hardly have any
 connection with the code that called vmlaunch.
 You can see it as two coroutines, or you can see it as linear logic:
   do host stuff
   enter guest
   do guest stuff
   exit guest
   continue doing host stuff

 Both can be implemented, I prefer linear one. I would prefer linear one
 to coroutine in any code design, no connection to vmx. Sometimes
 coroutine are better than alternative (and in those cases alternative is
 never a linear logic), but this is not the case.

 Fair enough.

 As things stand, we have only one version that works reliably with
 past/present/future compilers, and that is the one with setjmp/longjmp.
  A v5 would be needed anyway.  If Arthur (and Jan) want to write the
 vmentry as a big asm blob, that's fine by me.  Still, some variety adds
 value in a testsuite: think of a hypothetic nested VMX implementation
 that ignores HOST_RIP and just falls through to the next host %rip, we
 want that to fail the tests! (*)

 (*) In fact, I think this must be a requirement even if Arthur
 goes for a big asm blob.

 If they prefer to keep setjmp/longjmp and fix the few remaining nits, I
 think that should be acceptable anyway.  It would even make sense to
 have multiple vmentries if you can show they stress the hypervisor
 differently.

 In any case, I think we all agree (Arthur too) that this RFC doing mixed
 asm and C is the worst of both worlds.

 The actually differences in asm instruction between both
 version will not be bigger then a couple of lines (if we will not take
 setjmp/longjmp implementation into account :)),

 I was waiting for this parenthetical remark to appear. ;)

 I've put a smile there :) I realize this argument is not completely fair,
 but for the sake of argument everything goes!

 Yes, I caught the irony. ;)

 Paolo



-- 
Arthur Chunqi Li
Department of Computer Science
School of EECS
Peking University
Beijing, China
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-24 Thread Paolo Bonzini
Il 24/07/2013 08:11, Arthur Chunqi Li ha scritto:
 
 static int vmx_run()
 {
 u32 eax;
 bool ret;
 
 vmcs_write(HOST_RSP, get_rsp());
 ret = vmlaunch();

The compiler can still change rsp between here...

 while (!ret) {
 asm volatile(
 vmx_return:\n\t

... and here.

If you want to write it in C, the only thing that can be after
vmlaunch/vmresume is exit().  Else it has to be asm.

Paolo

 SAVE_GPR
 );
 eax = exit_handler();
 switch (eax) {
 case VMX_TEST_VMEXIT:
 return 0;
 case VMX_TEST_RESUME:
 break;
 default:
 printf(%s : unhandled ret from exit_handler.\n, __func__);
 return 1;
 }
 ret = vmresume();
 }
 printf(%s : vmenter failed.\n, __func__);
 return 1;
 }

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-24 Thread Arthur Chunqi Li
On Wed, Jul 24, 2013 at 2:40 PM, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 24/07/2013 08:11, Arthur Chunqi Li ha scritto:

 static int vmx_run()
 {
 u32 eax;
 bool ret;

 vmcs_write(HOST_RSP, get_rsp());
 ret = vmlaunch();

 The compiler can still change rsp between here...

 while (!ret) {
 asm volatile(
 vmx_return:\n\t

 ... and here.

 If you want to write it in C, the only thing that can be after
 vmlaunch/vmresume is exit().  Else it has to be asm.
Actually, you mean we need to write all the codes in asm to avoid
changing to rsp, right?

Arthur

 Paolo

 SAVE_GPR
 );
 eax = exit_handler();
 switch (eax) {
 case VMX_TEST_VMEXIT:
 return 0;
 case VMX_TEST_RESUME:
 break;
 default:
 printf(%s : unhandled ret from exit_handler.\n, __func__);
 return 1;
 }
 ret = vmresume();
 }
 printf(%s : vmenter failed.\n, __func__);
 return 1;
 }

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-24 Thread Paolo Bonzini
Il 24/07/2013 08:46, Arthur Chunqi Li ha scritto:
 On Wed, Jul 24, 2013 at 2:40 PM, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 24/07/2013 08:11, Arthur Chunqi Li ha scritto:

 static int vmx_run()
 {
 u32 eax;
 bool ret;

 vmcs_write(HOST_RSP, get_rsp());
 ret = vmlaunch();

 The compiler can still change rsp between here...

 while (!ret) {
 asm volatile(
 vmx_return:\n\t

 ... and here.

 If you want to write it in C, the only thing that can be after
 vmlaunch/vmresume is exit().  Else it has to be asm.
 Actually, you mean we need to write all the codes in asm to avoid
 changing to rsp, right?

Not necessarily all the code.  It is also ok to use setjmp/longjmp with
a small asm trampoline, because this method won't care about the exact
rsp values that are used.  But if you want to do as Gleb said, and put
vmx_return just after vmlaunch, it has to be all asm as in KVM's
arch/x86/kvm/vmx.c.

Paolo
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-24 Thread Arthur Chunqi Li
So as what Gleb said, what about the following codes:

static int vmx_run2()
{
u32 eax;
bool ret;

asm volatile(
mov %%rsp, %%rsi\n\t
mov %2, %%edi\n\t
call vmcs_write\n\t
vmlaunch\n\t
setbe %0\n\t
jne 4f\n\t

vmx_return:\n\t
SAVE_GPR_C
call exit_handler\n\t
cmp %3, %%eax\n\t
je 2f\n\t
cmp %4, %%eax\n\t
je 1f\n\t
jmp 3f\n\t

/* VMX_TEST_RESUME */
1:\n\t
LOAD_GPR_C
vmresume\n\t
setbe %0\n\t
jne 4f\n\t
/* VMX_TEST_VMEXIT */
2:\n\t
mov $0, %1\n\t
jmp 5f\n\t
/* undefined ret from exit_handler */
3:\n\t
mov $2, %1\n\t
jmp 5f\n\t
/* vmlaunch/vmresume failed, exit */
4:\n\t
mov $1, %1\n\t
5:\n\t
: =r(ret), =r(eax)
: i(HOST_RSP), i(VMX_TEST_VMEXIT),
  i(VMX_TEST_RESUME)
: rax, rbx, rdi, rsi,
   r8, r9, r10, r11, r12, r13, r14, r15,
  memory, cc
);
switch (eax) {
case 0:
return 0;
case 1:
printf(%s : vmenter failed.\n, __func__);
break;
default:
printf(%s : unhandled ret from exit_handler.\n, __func__);
break;
}
return 1;
}

On Wed, Jul 24, 2013 at 2:48 PM, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 24/07/2013 08:46, Arthur Chunqi Li ha scritto:
 On Wed, Jul 24, 2013 at 2:40 PM, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 24/07/2013 08:11, Arthur Chunqi Li ha scritto:

 static int vmx_run()
 {
 u32 eax;
 bool ret;

 vmcs_write(HOST_RSP, get_rsp());
 ret = vmlaunch();

 The compiler can still change rsp between here...

 while (!ret) {
 asm volatile(
 vmx_return:\n\t

 ... and here.

 If you want to write it in C, the only thing that can be after
 vmlaunch/vmresume is exit().  Else it has to be asm.
 Actually, you mean we need to write all the codes in asm to avoid
 changing to rsp, right?

 Not necessarily all the code.  It is also ok to use setjmp/longjmp with
 a small asm trampoline, because this method won't care about the exact
 rsp values that are used.  But if you want to do as Gleb said, and put
 vmx_return just after vmlaunch, it has to be all asm as in KVM's
 arch/x86/kvm/vmx.c.

 Paolo



-- 
Arthur Chunqi Li
Department of Computer Science
School of EECS
Peking University
Beijing, China
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-24 Thread Jan Kiszka
On 2013-07-24 10:48, Arthur Chunqi Li wrote:
 So as what Gleb said, what about the following codes:
 
 static int vmx_run2()
 {
 u32 eax;
 bool ret;
 
 asm volatile(
 mov %%rsp, %%rsi\n\t
 mov %2, %%edi\n\t
 call vmcs_write\n\t
 vmlaunch\n\t

Just like in KVM, provide a flag to the asm block that selects vmlaunch
or vmresume, then grab all the required information on return and leave
the asm block quickly again.

Jan

 setbe %0\n\t
 jne 4f\n\t
 
 vmx_return:\n\t
 SAVE_GPR_C
 call exit_handler\n\t
 cmp %3, %%eax\n\t
 je 2f\n\t
 cmp %4, %%eax\n\t
 je 1f\n\t
 jmp 3f\n\t
 
 /* VMX_TEST_RESUME */
 1:\n\t
 LOAD_GPR_C
 vmresume\n\t
 setbe %0\n\t
 jne 4f\n\t
 /* VMX_TEST_VMEXIT */
 2:\n\t
 mov $0, %1\n\t
 jmp 5f\n\t
 /* undefined ret from exit_handler */
 3:\n\t
 mov $2, %1\n\t
 jmp 5f\n\t
 /* vmlaunch/vmresume failed, exit */
 4:\n\t
 mov $1, %1\n\t
 5:\n\t
 : =r(ret), =r(eax)
 : i(HOST_RSP), i(VMX_TEST_VMEXIT),
   i(VMX_TEST_RESUME)
 : rax, rbx, rdi, rsi,
r8, r9, r10, r11, r12, r13, r14, r15,
   memory, cc
 );
 switch (eax) {
 case 0:
 return 0;
 case 1:
 printf(%s : vmenter failed.\n, __func__);
 break;
 default:
 printf(%s : unhandled ret from exit_handler.\n, __func__);
 break;
 }
 return 1;
 }
 
 On Wed, Jul 24, 2013 at 2:48 PM, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 24/07/2013 08:46, Arthur Chunqi Li ha scritto:
 On Wed, Jul 24, 2013 at 2:40 PM, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 24/07/2013 08:11, Arthur Chunqi Li ha scritto:

 static int vmx_run()
 {
 u32 eax;
 bool ret;

 vmcs_write(HOST_RSP, get_rsp());
 ret = vmlaunch();

 The compiler can still change rsp between here...

 while (!ret) {
 asm volatile(
 vmx_return:\n\t

 ... and here.

 If you want to write it in C, the only thing that can be after
 vmlaunch/vmresume is exit().  Else it has to be asm.
 Actually, you mean we need to write all the codes in asm to avoid
 changing to rsp, right?

 Not necessarily all the code.  It is also ok to use setjmp/longjmp with
 a small asm trampoline, because this method won't care about the exact
 rsp values that are used.  But if you want to do as Gleb said, and put
 vmx_return just after vmlaunch, it has to be all asm as in KVM's
 arch/x86/kvm/vmx.c.

 Paolo
 
 
 




signature.asc
Description: OpenPGP digital signature


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-24 Thread Paolo Bonzini
Il 24/07/2013 10:48, Arthur Chunqi Li ha scritto:
 So as what Gleb said, what about the following codes:
 
 static int vmx_run2()
 {
 u32 eax;
 bool ret;
 
 asm volatile(
 mov %%rsp, %%rsi\n\t
 mov %2, %%edi\n\t
 call vmcs_write\n\t
 vmlaunch\n\t
 setbe %0\n\t
 jne 4f\n\t

setbe doesn't set the flags, so you need jbe here (and then you can have
a mov $1, %0 at label 4 instead of using setbe).

 
 vmx_return:\n\t
 SAVE_GPR_C
 call exit_handler\n\t
 cmp %3, %%eax\n\t
 je 2f\n\t
 cmp %4, %%eax\n\t
 je 1f\n\t
 jmp 3f\n\t
 
 /* VMX_TEST_RESUME */
 1:\n\t
 LOAD_GPR_C
 vmresume\n\t

You need a SAVE_GPR_C here.  Then it is better to put the jbe at the
vmx_return label:

... do vmlaunch or vmreturn as Jan said ...
 vmx_return:
SAVE_GPR_C
jbe 4f
call exit_handler
etc.

Here is the relevant code from KVM that Jan was referring to:

jne 1f \n\t
__ex(ASM_VMX_VMLAUNCH) \n\t
jmp 2f \n\t
1:  __ex(ASM_VMX_VMRESUME) \n\t
2: 

I'd prefer to have a jbe vmx_return; ud2 after the vmlaunch/vmresume,
in order to test that the hypervisor respects HOST_RIP.

Paolo

 setbe %0\n\t
 jne 4f\n\t
 /* VMX_TEST_VMEXIT */
 2:\n\t
 mov $0, %1\n\t
 jmp 5f\n\t
 /* undefined ret from exit_handler */
 3:\n\t
 mov $2, %1\n\t
 jmp 5f\n\t
 /* vmlaunch/vmresume failed, exit */
 4:\n\t
 mov $1, %1\n\t
 5:\n\t
 : =r(ret), =r(eax)
 : i(HOST_RSP), i(VMX_TEST_VMEXIT),
   i(VMX_TEST_RESUME)
 : rax, rbx, rdi, rsi,
r8, r9, r10, r11, r12, r13, r14, r15,
   memory, cc
 );
 switch (eax) {
 case 0:
 return 0;
 case 1:
 printf(%s : vmenter failed.\n, __func__);
 break;
 default:
 printf(%s : unhandled ret from exit_handler.\n, __func__);
 break;
 }
 return 1;
 }
 
 On Wed, Jul 24, 2013 at 2:48 PM, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 24/07/2013 08:46, Arthur Chunqi Li ha scritto:
 On Wed, Jul 24, 2013 at 2:40 PM, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 24/07/2013 08:11, Arthur Chunqi Li ha scritto:

 static int vmx_run()
 {
 u32 eax;
 bool ret;

 vmcs_write(HOST_RSP, get_rsp());
 ret = vmlaunch();

 The compiler can still change rsp between here...

 while (!ret) {
 asm volatile(
 vmx_return:\n\t

 ... and here.

 If you want to write it in C, the only thing that can be after
 vmlaunch/vmresume is exit().  Else it has to be asm.
 Actually, you mean we need to write all the codes in asm to avoid
 changing to rsp, right?

 Not necessarily all the code.  It is also ok to use setjmp/longjmp with
 a small asm trampoline, because this method won't care about the exact
 rsp values that are used.  But if you want to do as Gleb said, and put
 vmx_return just after vmlaunch, it has to be all asm as in KVM's
 arch/x86/kvm/vmx.c.

 Paolo
 
 
 

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-24 Thread Arthur Chunqi Li
So what about this one. I merged all the exit reason to ret and
remove the flag detection after vmlaunch/vmresume (because I think
this detection is useless). Currently we support only one guest, so
variant launched is located in vmx_run(). If we want to support
multiple guest, we could move it to some structures (e.g.
environment_ctxt). Now I just put it here.

static int vmx_run()
{
u32 ret = 0;
bool launched = 0;

asm volatile(
mov %%rsp, %%rsi\n\t
mov %2, %%edi\n\t
call vmcs_write\n\t

0: 
LOAD_GPR_C
cmp $0, %1\n\t
jne 1f\n\t
vmlaunch\n\t
SAVE_GPR_C
/* vmlaunch error, return VMX_TEST_LAUNCH_ERR */
mov %3, %0\n\t
jmp 2f\n\t
1: 
vmresume\n\t
SAVE_GPR_C
/* vmresume error, return VMX_TEST_RESUME_ERR */
mov %4, %0\n\t
jmp 2f\n\t
vmx_return:\n\t
SAVE_GPR_C
call exit_handler\n\t
/* set launched = 1 */
mov $0x1, %1\n\t
cmp %5, %%eax\n\t
je 0b\n\t
mov %%eax, %0\n\t
2: 
: =r(ret), =r(launched)
: i(HOST_RSP), i(VMX_TEST_LAUNCH_ERR),
  i(VMX_TEST_RESUME_ERR), i(VMX_TEST_RESUME)
: rax, rbx, rdi, rsi,
  r8, r9, r10, r11, r12, r13, r14, r15,
  memory, cc
);
switch (ret) {
case VMX_TEST_VMEXIT:
return 0;
case VMX_TEST_LAUNCH_ERR:
printf(%s : vmlaunch failed.\n, __func__);
break;
case VMX_TEST_RESUME_ERR:
printf(%s : vmresume failed.\n, __func__);
break;
default:
printf(%s : unhandled ret from exit_handler, ret=%d.\n,
__func__, ret);
break;
}
return 1;
}

On Wed, Jul 24, 2013 at 5:16 PM, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 24/07/2013 10:48, Arthur Chunqi Li ha scritto:
 So as what Gleb said, what about the following codes:

 static int vmx_run2()
 {
 u32 eax;
 bool ret;

 asm volatile(
 mov %%rsp, %%rsi\n\t
 mov %2, %%edi\n\t
 call vmcs_write\n\t
 vmlaunch\n\t
 setbe %0\n\t
 jne 4f\n\t

 setbe doesn't set the flags, so you need jbe here (and then you can have
 a mov $1, %0 at label 4 instead of using setbe).


 vmx_return:\n\t
 SAVE_GPR_C
 call exit_handler\n\t
 cmp %3, %%eax\n\t
 je 2f\n\t
 cmp %4, %%eax\n\t
 je 1f\n\t
 jmp 3f\n\t

 /* VMX_TEST_RESUME */
 1:\n\t
 LOAD_GPR_C
 vmresume\n\t

 You need a SAVE_GPR_C here.  Then it is better to put the jbe at the
 vmx_return label:

 ... do vmlaunch or vmreturn as Jan said ...
  vmx_return:
 SAVE_GPR_C
 jbe 4f
 call exit_handler
 etc.

 Here is the relevant code from KVM that Jan was referring to:

 jne 1f \n\t
 __ex(ASM_VMX_VMLAUNCH) \n\t
 jmp 2f \n\t
 1:  __ex(ASM_VMX_VMRESUME) \n\t
 2: 

 I'd prefer to have a jbe vmx_return; ud2 after the vmlaunch/vmresume,
 in order to test that the hypervisor respects HOST_RIP.

 Paolo

 setbe %0\n\t
 jne 4f\n\t
 /* VMX_TEST_VMEXIT */
 2:\n\t
 mov $0, %1\n\t
 jmp 5f\n\t
 /* undefined ret from exit_handler */
 3:\n\t
 mov $2, %1\n\t
 jmp 5f\n\t
 /* vmlaunch/vmresume failed, exit */
 4:\n\t
 mov $1, %1\n\t
 5:\n\t
 : =r(ret), =r(eax)
 : i(HOST_RSP), i(VMX_TEST_VMEXIT),
   i(VMX_TEST_RESUME)
 : rax, rbx, rdi, rsi,
r8, r9, r10, r11, r12, r13, r14, r15,
   memory, cc
 );
 switch (eax) {
 case 0:
 return 0;
 case 1:
 printf(%s : vmenter failed.\n, __func__);
 break;
 default:
 printf(%s : unhandled ret from exit_handler.\n, __func__);
 break;
 }
 return 1;
 }

 On Wed, Jul 24, 2013 at 2:48 PM, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 24/07/2013 08:46, Arthur Chunqi Li ha scritto:
 On Wed, Jul 24, 2013 at 2:40 PM, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 24/07/2013 08:11, Arthur Chunqi Li ha scritto:

 static int vmx_run()
 {
 u32 eax;
 bool ret;

 vmcs_write(HOST_RSP, get_rsp());
 ret = vmlaunch();

 The compiler can still change rsp between here...

 while (!ret) {
 asm volatile(
 vmx_return:\n\t

 ... and here.

 If you want to write it in C, the only thing that can be after
 vmlaunch/vmresume is exit().  Else it has to be asm.
 Actually, you mean we need to write all the codes in asm to avoid
 changing to rsp, right?

 Not necessarily all the code.  It is also ok to use setjmp/longjmp with
 a small asm trampoline, because this method won't care about the exact
 rsp values that are used.  But if you want to do as Gleb said, and put
 vmx_return just after vmlaunch, it has to be all asm as in KVM's
 arch/x86/kvm/vmx.c.

 Paolo







-- 
Arthur 

Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-24 Thread Jan Kiszka
On 2013-07-24 11:56, Arthur Chunqi Li wrote:
 So what about this one. I merged all the exit reason to ret and
 remove the flag detection after vmlaunch/vmresume (because I think
 this detection is useless). Currently we support only one guest, so
 variant launched is located in vmx_run(). If we want to support
 multiple guest, we could move it to some structures (e.g.
 environment_ctxt). Now I just put it here.
 
 static int vmx_run()
 {
 u32 ret = 0;
 bool launched = 0;
 
 asm volatile(
 mov %%rsp, %%rsi\n\t
 mov %2, %%edi\n\t
 call vmcs_write\n\t
 
 0: 
 LOAD_GPR_C
 cmp $0, %1\n\t
 jne 1f\n\t
 vmlaunch\n\t
 SAVE_GPR_C
 /* vmlaunch error, return VMX_TEST_LAUNCH_ERR */
 mov %3, %0\n\t
 jmp 2f\n\t
 1: 
 vmresume\n\t
 SAVE_GPR_C
 /* vmresume error, return VMX_TEST_RESUME_ERR */
 mov %4, %0\n\t
 jmp 2f\n\t

Where do you store the flags now? You may want to differentiate / test
if ZF of CF is set.

Jan

 vmx_return:\n\t
 SAVE_GPR_C
 call exit_handler\n\t
 /* set launched = 1 */
 mov $0x1, %1\n\t
 cmp %5, %%eax\n\t
 je 0b\n\t
 mov %%eax, %0\n\t
 2: 
 : =r(ret), =r(launched)
 : i(HOST_RSP), i(VMX_TEST_LAUNCH_ERR),
   i(VMX_TEST_RESUME_ERR), i(VMX_TEST_RESUME)
 : rax, rbx, rdi, rsi,
   r8, r9, r10, r11, r12, r13, r14, r15,
   memory, cc
 );
 switch (ret) {
 case VMX_TEST_VMEXIT:
 return 0;
 case VMX_TEST_LAUNCH_ERR:
 printf(%s : vmlaunch failed.\n, __func__);
 break;
 case VMX_TEST_RESUME_ERR:
 printf(%s : vmresume failed.\n, __func__);
 break;
 default:
 printf(%s : unhandled ret from exit_handler, ret=%d.\n,
 __func__, ret);
 break;
 }
 return 1;
 }
 




signature.asc
Description: OpenPGP digital signature


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-24 Thread Arthur Chunqi Li
On Wed, Jul 24, 2013 at 6:03 PM, Jan Kiszka jan.kis...@web.de wrote:
 On 2013-07-24 11:56, Arthur Chunqi Li wrote:
 So what about this one. I merged all the exit reason to ret and
 remove the flag detection after vmlaunch/vmresume (because I think
 this detection is useless). Currently we support only one guest, so
 variant launched is located in vmx_run(). If we want to support
 multiple guest, we could move it to some structures (e.g.
 environment_ctxt). Now I just put it here.

 static int vmx_run()
 {
 u32 ret = 0;
 bool launched = 0;

 asm volatile(
 mov %%rsp, %%rsi\n\t
 mov %2, %%edi\n\t
 call vmcs_write\n\t

 0: 
 LOAD_GPR_C
 cmp $0, %1\n\t
 jne 1f\n\t
 vmlaunch\n\t
 SAVE_GPR_C
 /* vmlaunch error, return VMX_TEST_LAUNCH_ERR */
 mov %3, %0\n\t
 jmp 2f\n\t
 1: 
 vmresume\n\t
 SAVE_GPR_C
 /* vmresume error, return VMX_TEST_RESUME_ERR */
 mov %4, %0\n\t
 jmp 2f\n\t

 Where do you store the flags now? You may want to differentiate / test
 if ZF of CF is set.
I store the flags as a global variant. You mean I need to detect ZF/CF
after vmlaunch/vmresume?

Arthur

 Jan

 vmx_return:\n\t
 SAVE_GPR_C
 call exit_handler\n\t
 /* set launched = 1 */
 mov $0x1, %1\n\t
 cmp %5, %%eax\n\t
 je 0b\n\t
 mov %%eax, %0\n\t
 2: 
 : =r(ret), =r(launched)
 : i(HOST_RSP), i(VMX_TEST_LAUNCH_ERR),
   i(VMX_TEST_RESUME_ERR), i(VMX_TEST_RESUME)
 : rax, rbx, rdi, rsi,
   r8, r9, r10, r11, r12, r13, r14, r15,
   memory, cc
 );
 switch (ret) {
 case VMX_TEST_VMEXIT:
 return 0;
 case VMX_TEST_LAUNCH_ERR:
 printf(%s : vmlaunch failed.\n, __func__);
 break;
 case VMX_TEST_RESUME_ERR:
 printf(%s : vmresume failed.\n, __func__);
 break;
 default:
 printf(%s : unhandled ret from exit_handler, ret=%d.\n,
 __func__, ret);
 break;
 }
 return 1;
 }






-- 
Arthur Chunqi Li
Department of Computer Science
School of EECS
Peking University
Beijing, China
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-24 Thread Jan Kiszka
On 2013-07-24 12:16, Arthur Chunqi Li wrote:
 On Wed, Jul 24, 2013 at 6:03 PM, Jan Kiszka jan.kis...@web.de wrote:
 On 2013-07-24 11:56, Arthur Chunqi Li wrote:
 So what about this one. I merged all the exit reason to ret and
 remove the flag detection after vmlaunch/vmresume (because I think
 this detection is useless). Currently we support only one guest, so
 variant launched is located in vmx_run(). If we want to support
 multiple guest, we could move it to some structures (e.g.
 environment_ctxt). Now I just put it here.

 static int vmx_run()
 {
 u32 ret = 0;
 bool launched = 0;

 asm volatile(
 mov %%rsp, %%rsi\n\t
 mov %2, %%edi\n\t
 call vmcs_write\n\t

 0: 
 LOAD_GPR_C
 cmp $0, %1\n\t
 jne 1f\n\t
 vmlaunch\n\t
 SAVE_GPR_C
 /* vmlaunch error, return VMX_TEST_LAUNCH_ERR */
 mov %3, %0\n\t
 jmp 2f\n\t
 1: 
 vmresume\n\t
 SAVE_GPR_C
 /* vmresume error, return VMX_TEST_RESUME_ERR */
 mov %4, %0\n\t
 jmp 2f\n\t

 Where do you store the flags now? You may want to differentiate / test
 if ZF of CF is set.
 I store the flags as a global variant. You mean I need to detect ZF/CF
 after vmlaunch/vmresume?

Yes - if you want to check correct emulation of those instructions
completely.

Jan




signature.asc
Description: OpenPGP digital signature


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-24 Thread Arthur Chunqi Li
And what about this version:

static int vmx_run()
{
u32 ret = 0;

asm volatile(
mov %%rsp, %%rsi\n\t
mov %2, %%edi\n\t
call vmcs_write\n\t

0: 
LOAD_GPR_C
cmpl $0, %1\n\t
jne 1f\n\t
vmlaunch;seta %1\n\t
/* vmlaunch error, return VMX_TEST_LAUNCH_ERR */
movl %3, %0\n\t
SAVE_GPR_C
jmp 2f\n\t
1: 
vmresume;seta %1\n\t
/* vmresume error, return VMX_TEST_RESUME_ERR */
movl %4, %0\n\t
SAVE_GPR_C
jmp 2f\n\t

vmx_return:\n\t
SAVE_GPR_C
call exit_handler\n\t
/* set launched = 1 */
movl $0x1, %1\n\t
/* jump to resume when VMX_TEST_RESUME */
cmp %5, %%eax\n\t
je 0b\n\t
mov %%eax, %0\n\t
2: 
: =m(ret), =m(launched)
: i(HOST_RSP), i(VMX_TEST_LAUNCH_ERR),
  i(VMX_TEST_RESUME_ERR), i(VMX_TEST_RESUME)
: rax, rbx, rdi, rsi,
 r8, r9, r10, r11, r12, r13, r14, r15,
  memory, cc
);
switch (ret) {
case VMX_TEST_VMEXIT:
launched = 0;
return 0;
case VMX_TEST_LAUNCH_ERR:
printf(%s : vmlaunch failed.\n, __func__);
if (launched != 0)
printf(\tvmlaunch set flags error\n);
report(test vmlaunch, 0);
break;
case VMX_TEST_RESUME_ERR:
printf(%s : vmresume failed.\n, __func__);
if (launched != 0)
printf(\tvmlaunch set flags error\n);
report(test vmresume, 0);
break;
default:
launched = 0;
printf(%s : unhandled ret from exit_handler, ret=%d.\n,
__func__, ret);
break;
}
return 1;
}

On Wed, Jul 24, 2013 at 6:24 PM, Jan Kiszka jan.kis...@web.de wrote:
 On 2013-07-24 12:16, Arthur Chunqi Li wrote:
 On Wed, Jul 24, 2013 at 6:03 PM, Jan Kiszka jan.kis...@web.de wrote:
 On 2013-07-24 11:56, Arthur Chunqi Li wrote:
 So what about this one. I merged all the exit reason to ret and
 remove the flag detection after vmlaunch/vmresume (because I think
 this detection is useless). Currently we support only one guest, so
 variant launched is located in vmx_run(). If we want to support
 multiple guest, we could move it to some structures (e.g.
 environment_ctxt). Now I just put it here.

 static int vmx_run()
 {
 u32 ret = 0;
 bool launched = 0;

 asm volatile(
 mov %%rsp, %%rsi\n\t
 mov %2, %%edi\n\t
 call vmcs_write\n\t

 0: 
 LOAD_GPR_C
 cmp $0, %1\n\t
 jne 1f\n\t
 vmlaunch\n\t
 SAVE_GPR_C
 /* vmlaunch error, return VMX_TEST_LAUNCH_ERR */
 mov %3, %0\n\t
 jmp 2f\n\t
 1: 
 vmresume\n\t
 SAVE_GPR_C
 /* vmresume error, return VMX_TEST_RESUME_ERR */
 mov %4, %0\n\t
 jmp 2f\n\t

 Where do you store the flags now? You may want to differentiate / test
 if ZF of CF is set.
 I store the flags as a global variant. You mean I need to detect ZF/CF
 after vmlaunch/vmresume?

 Yes - if you want to check correct emulation of those instructions
 completely.

 Jan





-- 
Arthur Chunqi Li
Department of Computer Science
School of EECS
Peking University
Beijing, China
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-24 Thread Jan Kiszka
On 2013-07-24 13:20, Arthur Chunqi Li wrote:
 And what about this version:
 
 static int vmx_run()
 {
 u32 ret = 0;
 
 asm volatile(
 mov %%rsp, %%rsi\n\t
 mov %2, %%edi\n\t
 call vmcs_write\n\t
 
 0: 
 LOAD_GPR_C
 cmpl $0, %1\n\t
 jne 1f\n\t
 vmlaunch;seta %1\n\t

That doesn't differentiate between CF and ZF (so that you can check if
vmlaunch set the right flag).

Also, only one instruction per line, please.

Jan




signature.asc
Description: OpenPGP digital signature


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-19 Thread Paolo Bonzini
Il 18/07/2013 21:57, Gleb Natapov ha scritto:
 On Thu, Jul 18, 2013 at 02:08:51PM +0200, Paolo Bonzini wrote:
 Il 18/07/2013 13:06, Gleb Natapov ha scritto:
 On Thu, Jul 18, 2013 at 12:47:46PM +0200, Paolo Bonzini wrote:
 and for a testsuite I'd prefer the latter---which means I'd still favor
 setjmp/longjmp.

 Now, here is the long explanation.

 I must admit that the code looks nice.  There are some nits I'd like to
 see done differently (such as putting vmx_return at the beginning of the
 while (1), and the vmresume asm at the end), but it is indeed nice.

 Why do you prefer setjmp/longjmp then?

 Because it is still deceiving, and I dislike the deceit more than I like
 the linear code flow.

 What is deceiving about it? Of course for someone who has no idea how
 vmx works the code will not be obvious, but why should we care. For
 someone who knows what is deceiving about returning into the same
 function guest was launched from by using VMX mechanism

 The way the code is written is deceiving.  If I see

   asm(vmlaunch; seta %0)
   while (ret)

 I expect HOST_RIP to point at the seta or somewhere near, not at a
 completely different label somewhere else.

 Why would you expect that assuming you know what vmlaunch is?

Because this is written in C, and I know trying to fool the compiler is
a losing game.  So my reaction is okay, HOST_RIP must be set so that
code will not jump around.  If I see

   asm(vmlaunch)
   exit(-1)

the reaction is the opposite: hmm, anything that jumps around would
have a hard time with the compiler, there must be some assembly
trampoline somewhere; let's check what HOST_RIP is.

 instead of longjmp()?

 Look again at the setjmp/longjmp version.  longjmp is not used to handle
 vmexit.  It is used to jump back from the vmexit handler to main, which
 is exactly what setjmp/longjmp is meant for.

 That's because simple return will not work in that version, this is
 artifact of how vmexit was done.

I think it can be made to work without setjmp/longjmp, but the code
would be ugly.

 the compiler, and you rely on the compiler not changing %rsp between the
 vmlaunch and the vmx_return label.  Minor nit, you cannot anymore print
 HOST_RSP should be loaded on each guest entry.

 Right, but my point is: without a big asm blob, you don't know the right
 value to load.  It depends on the generated code.  And the big asm blob
 limits a lot the code looks nice value of this approach.

 I said it number of times already, this is not about code looking nice,
 code looks like KVM or use less assembler as possible, this is about
 linear data flow. It is not fun chasing code execution path. Yes, you
 can argue that vmlaunch/vmresume inherently non linear, but there is a
 difference between skipping one instruction and remain in the same
 function and on the same stack, or jump completely to a different
 context.

I don't see anything bad in jumping completely to a different context.
The guest and host are sort of like two coroutines, they hardly have any
connection with the code that called vmlaunch.

 The actually differences in asm instruction between both
 version will not be bigger then a couple of lines (if we will not take
 setjmp/longjmp implementation into account :)),

I was waiting for this parenthetical remark to appear. ;)

 but I do not even see
 why we discuss this argument since minimizing asm instructions here is
 not an point. We should not use more then needed to achieve the goal of
 course, but design should not be around number of assembly lines.

I agree, I only mentioned it because you talked about

   asm
   C
   asm
   C

and this is not what the setjmp/longjmp code looks like---using inlines
for asm as if they were builtin functions is good, interspersing asm and
C in the same function is bad.

Paolo
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-19 Thread Gleb Natapov
On Fri, Jul 19, 2013 at 08:42:20AM +0200, Paolo Bonzini wrote:
 Il 18/07/2013 21:57, Gleb Natapov ha scritto:
  On Thu, Jul 18, 2013 at 02:08:51PM +0200, Paolo Bonzini wrote:
  Il 18/07/2013 13:06, Gleb Natapov ha scritto:
  On Thu, Jul 18, 2013 at 12:47:46PM +0200, Paolo Bonzini wrote:
  and for a testsuite I'd prefer the latter---which means I'd still favor
  setjmp/longjmp.
 
  Now, here is the long explanation.
 
  I must admit that the code looks nice.  There are some nits I'd like to
  see done differently (such as putting vmx_return at the beginning of 
  the
  while (1), and the vmresume asm at the end), but it is indeed nice.
 
  Why do you prefer setjmp/longjmp then?
 
  Because it is still deceiving, and I dislike the deceit more than I like
  the linear code flow.
 
  What is deceiving about it? Of course for someone who has no idea how
  vmx works the code will not be obvious, but why should we care. For
  someone who knows what is deceiving about returning into the same
  function guest was launched from by using VMX mechanism
 
  The way the code is written is deceiving.  If I see
 
asm(vmlaunch; seta %0)
while (ret)
 
  I expect HOST_RIP to point at the seta or somewhere near, not at a
  completely different label somewhere else.
 
  Why would you expect that assuming you know what vmlaunch is?
 
 Because this is written in C, and I know trying to fool the compiler is
 a losing game.  So my reaction is okay, HOST_RIP must be set so that
 code will not jump around.  If I see
 
asm(vmlaunch)
exit(-1)
 
 the reaction is the opposite: hmm, anything that jumps around would
 have a hard time with the compiler, there must be some assembly
 trampoline somewhere; let's check what HOST_RIP is.
 
We do try to fool compiler often even without vmx: code patching. This is
why asm goto was invented, no? So, like you said in previous emails,
asm goto is appropriate way here to tell compiler what is going on.

  instead of longjmp()?
 
  Look again at the setjmp/longjmp version.  longjmp is not used to handle
  vmexit.  It is used to jump back from the vmexit handler to main, which
  is exactly what setjmp/longjmp is meant for.
 
  That's because simple return will not work in that version, this is
  artifact of how vmexit was done.
 
 I think it can be made to work without setjmp/longjmp, but the code
 would be ugly.
 
  the compiler, and you rely on the compiler not changing %rsp between the
  vmlaunch and the vmx_return label.  Minor nit, you cannot anymore print
  HOST_RSP should be loaded on each guest entry.
 
  Right, but my point is: without a big asm blob, you don't know the right
  value to load.  It depends on the generated code.  And the big asm blob
  limits a lot the code looks nice value of this approach.
 
  I said it number of times already, this is not about code looking nice,
  code looks like KVM or use less assembler as possible, this is about
  linear data flow. It is not fun chasing code execution path. Yes, you
  can argue that vmlaunch/vmresume inherently non linear, but there is a
  difference between skipping one instruction and remain in the same
  function and on the same stack, or jump completely to a different
  context.
 
 I don't see anything bad in jumping completely to a different context.
 The guest and host are sort of like two coroutines, they hardly have any
 connection with the code that called vmlaunch.
You can see it as two coroutines, or you can see it as linear logic:
  do host stuff
  enter guest
  do guest stuff
  exit guest
  continue doing host stuff

Both can be implemented, I prefer linear one. I would prefer linear one
to coroutine in any code design, no connection to vmx. Sometimes
coroutine are better than alternative (and in those cases alternative is
never a linear logic), but this is not the case.


 
  The actually differences in asm instruction between both
  version will not be bigger then a couple of lines (if we will not take
  setjmp/longjmp implementation into account :)),
 
 I was waiting for this parenthetical remark to appear. ;)
 
I've put a smile there :) I realize this argument is not completely fair,
but for the sake of argument everything goes!

--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-19 Thread Paolo Bonzini
Il 19/07/2013 11:40, Gleb Natapov ha scritto:
 Because this is written in C, and I know trying to fool the compiler is
 a losing game.  So my reaction is okay, HOST_RIP must be set so that
 code will not jump around.  If I see

asm(vmlaunch)
exit(-1)

 the reaction is the opposite: hmm, anything that jumps around would
 have a hard time with the compiler, there must be some assembly
 trampoline somewhere; let's check what HOST_RIP is.

 We do try to fool compiler often even without vmx: code patching. This is
 why asm goto was invented, no? So, like you said in previous emails,
 asm goto is appropriate way here to tell compiler what is going on.

Code patching is only reimplementing an existing C structure (if/else)
in a different manner.  Here the actual code flow (location of HOST_RIP
and value of HOST_RSP) cannot be expressed correctly to the compiler
because we do not use the C label (we use an asm label).

I don't think asm goto can be made to work for vmx_return, though if we
go for a big blob it could be useful to jump to the error handling code.

 I don't see anything bad in jumping completely to a different context.
 The guest and host are sort of like two coroutines, they hardly have any
 connection with the code that called vmlaunch.
 You can see it as two coroutines, or you can see it as linear logic:
   do host stuff
   enter guest
   do guest stuff
   exit guest
   continue doing host stuff
 
 Both can be implemented, I prefer linear one. I would prefer linear one
 to coroutine in any code design, no connection to vmx. Sometimes
 coroutine are better than alternative (and in those cases alternative is
 never a linear logic), but this is not the case.

Fair enough.

As things stand, we have only one version that works reliably with
past/present/future compilers, and that is the one with setjmp/longjmp.
 A v5 would be needed anyway.  If Arthur (and Jan) want to write the
vmentry as a big asm blob, that's fine by me.  Still, some variety adds
value in a testsuite: think of a hypothetic nested VMX implementation
that ignores HOST_RIP and just falls through to the next host %rip, we
want that to fail the tests! (*)

(*) In fact, I think this must be a requirement even if Arthur
goes for a big asm blob.

If they prefer to keep setjmp/longjmp and fix the few remaining nits, I
think that should be acceptable anyway.  It would even make sense to
have multiple vmentries if you can show they stress the hypervisor
differently.

In any case, I think we all agree (Arthur too) that this RFC doing mixed
asm and C is the worst of both worlds.

 The actually differences in asm instruction between both
 version will not be bigger then a couple of lines (if we will not take
 setjmp/longjmp implementation into account :)),

 I was waiting for this parenthetical remark to appear. ;)

 I've put a smile there :) I realize this argument is not completely fair,
 but for the sake of argument everything goes!

Yes, I caught the irony. ;)

Paolo
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-18 Thread Gleb Natapov
On Thu, Jul 18, 2013 at 07:52:21AM +0200, Paolo Bonzini wrote:
 Il 17/07/2013 20:54, Arthur Chunqi Li ha scritto:
  +   .globl entry_sysenter\n\t
  +   entry_sysenter:\n\t
  +   SAVE_GPR
  +  and $0xf, %rax\n\t
  +  push%rax\n\t
 
 push should be wrong here, the first argument is in %rdi.
 
  +  callsyscall_handler\n\t
  +   LOAD_GPR
  +  vmresume\n\t
  +);
  +
  +int exit_handler()
 
 This (and syscall_handler too) needs __attribute__((__used__)) because
 it's only used from assembly.
 
That was my question actually, why is it used from assembly? Calling it
from see should not be a problem.

 Please add static to all functions except main, it triggers better
 compiler optimization and warnings and it will avoid nasty surprises in
 the future if the compiler becomes smarter.
 
  +{
  +   int ret;
  +
  +   current-exits++;
  +   current-guest_regs = regs;
  +   if (is_hypercall())
  +   ret = handle_hypercall();
  +   else
  +   ret = current-exit_handler();
  +   regs = current-guest_regs;
  +   switch (ret) {
  +   case VMX_TEST_VMEXIT:
  +   case VMX_TEST_RESUME:
  +   return ret;
  +   case VMX_TEST_EXIT:
  +   break;
  +   default:
  +   printf(ERROR : Invalid exit_handler return val %d.\n
  +   , ret);
  +   }
 
 Here have to propagate the exit codes through multiple levels, because
 we're not using setjmp/longjmp.  Not a big deal.  My worries with this
 version are below.
 
  +   print_vmexit_info();
  +   exit(-1);
  +   return 0;
  +}
  +
  +int vmx_run()
  +{
  +   bool ret;
  +   u32 eax;
  +   u64 rsp;
  +
  +   asm volatile(mov %%rsp, %0\n\t : =r(rsp));
  +   vmcs_write(HOST_RSP, rsp);
  +   asm volatile(vmlaunch;seta %0\n\t : =m(ret));
 
 setbe (this path is probably untested because it never happens in practice).
 
At least one of the tests should set up wrong vmcs state to verify that
nested vmx handles it correctly. In fact one of the patches that Arthur
have sent to nested vmx fixes exactly that code path.

 Also, missing memory clobber.
 
  +   if (ret) {
  +   printf(%s : vmlaunch failed.\n, __func__);
  +   return 1;
  +   }
  +   while (1) {
  +   asm volatile(
  +   LOAD_GPR_C
  +   vmresume;seta %0\n\t
  +   : =m(ret));
 
 setbe and missing memory clobber.
 
  +   if (ret) {
  +   printf(%s : vmresume failed.\n, __func__);
  +   return 1;
  +   }
  +   asm volatile(
  +   .global vmx_return\n\t
 
 .global should not be needed.
 
  +   vmx_return:\n\t
  +   SAVE_GPR_C
  +   call exit_handler\n\t
  +   mov %%eax, %0\n\t
  +   : =r(eax)
  +   );
 
 I had written a long explanation here about why I don't trust the
 compiler to do the right thing, and ideas about how to fix that.  But in
 the end the only workable solution is a single assembly blob like vmx.c
 in KVM to do vmlaunch/vmresume, so I'll get right to the point:
 
*  the similarity with KVM code and as little assembly as  *
*  possible goals are mutually exclusive *
 
I never said that code should be similar to KVM code or have as little
assembly as possible :) Reread the thread. The only thing I asked for
is to make code flow linear, if it makes code looks more like KVM or
reduce amount of assembly this is just a bonus.

 and for a testsuite I'd prefer the latter---which means I'd still favor
 setjmp/longjmp.
 
 Now, here is the long explanation.
 
 I must admit that the code looks nice.  There are some nits I'd like to
 see done differently (such as putting vmx_return at the beginning of the
 while (1), and the vmresume asm at the end), but it is indeed nice.
 
Why do you prefer setjmp/longjmp then?

 However, it is also very deceiving, because the processor is not doing
 what the code says.  If I see:
 
vmlaunch();
exit(-1);
 
 it is clear that magic happens in vmlaunch().  If I see
 
asm volatile(vmlaunch;setbe %0\n\t : =m(ret));
if (ret) {
...
}
asm(vmx_return:)
 
 it is absolutely not clear that the setbe and if are skipped on a
 successful vmlaunch.  So, at the very least you need a comment before
 those if (ret) to document the control flow, or you can use a jmp like
 this:
 
asm volatile goto (vmlaunch;jmp %0\n\t
   : : : memory : vmlaunch_error);
if (0) {
vmlaunch_error: ...
}
 
 The unconditional jump and asm goto make it clear that magic happens.
Agree, I dislike this magic too, but this is fixed by you suggestion
above about putting vmx_return at the beginning of while(). So the logic
will looks like that:

asm volatile(vmlaunch;setbe %0\n\t : =m(ret));
while(ret) {
   vmx_return:
   SAVE_GPR_C
   eax = exit_handler();
   switch(eax) {
   }
  

Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-18 Thread Paolo Bonzini
Il 18/07/2013 09:26, Gleb Natapov ha scritto:
  I had written a long explanation here about why I don't trust the
  compiler to do the right thing, and ideas about how to fix that.  But in
  the end the only workable solution is a single assembly blob like vmx.c
  in KVM to do vmlaunch/vmresume, so I'll get right to the point:
  
 *  the similarity with KVM code and as little assembly as  *
 *  possible goals are mutually exclusive *
 
 I never said that code should be similar to KVM code or have as little
 assembly as possible :) Reread the thread. The only thing I asked for
 is to make code flow linear, if it makes code looks more like KVM or
 reduce amount of assembly this is just a bonus.

Point taken.

  and for a testsuite I'd prefer the latter---which means I'd still favor
  setjmp/longjmp.
  
  Now, here is the long explanation.
  
  I must admit that the code looks nice.  There are some nits I'd like to
  see done differently (such as putting vmx_return at the beginning of the
  while (1), and the vmresume asm at the end), but it is indeed nice.

 Why do you prefer setjmp/longjmp then?

Because it is still deceiving, and I dislike the deceit more than I like
the linear code flow.

 Agree, I dislike this magic too, but this is fixed by you suggestion
 above about putting vmx_return at the beginning of while(). So the logic
 will looks like that:
 
 asm volatile(vmlaunch;setbe %0\n\t : =m(ret));
 while(ret) {

while(!ret) if you use setbe, of course.

vmx_return:
SAVE_GPR_C
eax = exit_handler();
switch(eax) {
}
LOAD_GPR_C
asm volatile(vmresume;seta %0\n\t : =m(ret));
 }

It is still somewhat magic: the while (ret) is only there to please
the compiler, and you rely on the compiler not changing %rsp between the
vmlaunch and the vmx_return label.  Minor nit, you cannot anymore print
different error messages for vmlaunch vs. vmresume failure.

In the end the choice is between all in asm and small asm trampoline
(which also happens to use setjmp/longjmp, but perhaps Arthur can
propagate return codes without using setjmp/longjmp, too).

 Rewriting the whole guest entry exit path in asm like you suggest bellow
 will eliminate the magic too.

 I much prefer one big asm statement than many small asm statement
 scattered among two or three C lines.

It's not many asm statements, it's just a dozen lines:


align   4, 0x90
entry_vmx:
SAVE_GPR
call default_exit_handler
/* Should not reach here*/
mov $1, %eax
call exit
.align  4, 0x90
entry_sysenter:
SAVE_GPR
and $0xf, %eax
mov %eax, %edi
calldefault_syscall_handler
/* Arthur, is something missing here? :)) */

Paolo
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-18 Thread Gleb Natapov
On Thu, Jul 18, 2013 at 12:47:46PM +0200, Paolo Bonzini wrote:
   and for a testsuite I'd prefer the latter---which means I'd still favor
   setjmp/longjmp.
   
   Now, here is the long explanation.
   
   I must admit that the code looks nice.  There are some nits I'd like to
   see done differently (such as putting vmx_return at the beginning of the
   while (1), and the vmresume asm at the end), but it is indeed nice.
 
  Why do you prefer setjmp/longjmp then?
 
 Because it is still deceiving, and I dislike the deceit more than I like
 the linear code flow.
 
What is deceiving about it? Of course for someone who has no idea how
vmx works the code will not be obvious, but why should we care. For
someone who knows what is deceiving about returning into the same
function guest was launched from by using VMX mechanism instead of
longjmp()?

  Agree, I dislike this magic too, but this is fixed by you suggestion
  above about putting vmx_return at the beginning of while(). So the logic
  will looks like that:
  
  asm volatile(vmlaunch;setbe %0\n\t : =m(ret));
  while(ret) {
 
 while(!ret) if you use setbe, of course.
 
Yeah, this not meant to be compilable code :)

 vmx_return:
 SAVE_GPR_C
 eax = exit_handler();
 switch(eax) {
 }
 LOAD_GPR_C
 asm volatile(vmresume;seta %0\n\t : =m(ret));
  }
 
 It is still somewhat magic: the while (ret) is only there to please
No, it it there to catch vmlaunch/vmresume errors.

 the compiler, and you rely on the compiler not changing %rsp between the
 vmlaunch and the vmx_return label.  Minor nit, you cannot anymore print
HOST_RSP should be loaded on each guest entry.

 different error messages for vmlaunch vs. vmresume failure.
Just because the same variable is used to store error values :)
Make vmlaunch use err1 and vmresume err2 and do while (!err1  !err2)

 
 In the end the choice is between all in asm and small asm trampoline
 (which also happens to use setjmp/longjmp, but perhaps Arthur can
 propagate return codes without using setjmp/longjmp, too).
 
  Rewriting the whole guest entry exit path in asm like you suggest bellow
  will eliminate the magic too.
 
  I much prefer one big asm statement than many small asm statement
  scattered among two or three C lines.
 
 It's not many asm statements, it's just a dozen lines:
 
I am not talking about overall file, but the how vmx_run() is written:
asm()
C code
asm()
C code
...

I much prefer:
C code

big asm() blob

C code.


--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-18 Thread Paolo Bonzini
Il 18/07/2013 13:06, Gleb Natapov ha scritto:
 On Thu, Jul 18, 2013 at 12:47:46PM +0200, Paolo Bonzini wrote:
 and for a testsuite I'd prefer the latter---which means I'd still favor
 setjmp/longjmp.

 Now, here is the long explanation.

 I must admit that the code looks nice.  There are some nits I'd like to
 see done differently (such as putting vmx_return at the beginning of the
 while (1), and the vmresume asm at the end), but it is indeed nice.

 Why do you prefer setjmp/longjmp then?

 Because it is still deceiving, and I dislike the deceit more than I like
 the linear code flow.

 What is deceiving about it? Of course for someone who has no idea how
 vmx works the code will not be obvious, but why should we care. For
 someone who knows what is deceiving about returning into the same
 function guest was launched from by using VMX mechanism

The way the code is written is deceiving.  If I see

  asm(vmlaunch; seta %0)
  while (ret)

I expect HOST_RIP to point at the seta or somewhere near, not at a
completely different label somewhere else.

 instead of longjmp()?

Look again at the setjmp/longjmp version.  longjmp is not used to handle
vmexit.  It is used to jump back from the vmexit handler to main, which
is exactly what setjmp/longjmp is meant for.

 It is still somewhat magic: the while (ret) is only there to please
 the compiler

 No, it it there to catch vmlaunch/vmresume errors.

You could change it to while (0) and it would still work.  That's what
I mean by only there to please the compiler.

 the compiler, and you rely on the compiler not changing %rsp between the
 vmlaunch and the vmx_return label.  Minor nit, you cannot anymore print
 HOST_RSP should be loaded on each guest entry.

Right, but my point is: without a big asm blob, you don't know the right
value to load.  It depends on the generated code.  And the big asm blob
limits a lot the code looks nice value of this approach.

 different error messages for vmlaunch vs. vmresume failure.
 Just because the same variable is used to store error values :)
 Make vmlaunch use err1 and vmresume err2 and do while (!err1  !err2)

Yeah. :)

 In the end the choice is between all in asm and small asm trampoline
 (which also happens to use setjmp/longjmp, but perhaps Arthur can
 propagate return codes without using setjmp/longjmp, too).

 Rewriting the whole guest entry exit path in asm like you suggest bellow
 will eliminate the magic too.

 I much prefer one big asm statement than many small asm statement
 scattered among two or three C lines.

 It's not many asm statements, it's just a dozen lines:

 I am not talking about overall file, but the how vmx_run() is written:
 asm()
 C code
 asm()
 C code
 ...
 
 I much prefer:
 C code
 
 big asm() blob
 
 C code.

Me too.  But the trampoline version is neither, it is

static inline void vmlaunch() { asm(vmlaunch) }
static inline void vmresume() { asm(vmresume) }
small asm() blob
C code

Paolo

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-18 Thread Arthur Chunqi Li
On Thu, Jul 18, 2013 at 8:08 PM, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 18/07/2013 13:06, Gleb Natapov ha scritto:
 On Thu, Jul 18, 2013 at 12:47:46PM +0200, Paolo Bonzini wrote:
 and for a testsuite I'd prefer the latter---which means I'd still favor
 setjmp/longjmp.

 Now, here is the long explanation.

 I must admit that the code looks nice.  There are some nits I'd like to
 see done differently (such as putting vmx_return at the beginning of the
 while (1), and the vmresume asm at the end), but it is indeed nice.

 Why do you prefer setjmp/longjmp then?

 Because it is still deceiving, and I dislike the deceit more than I like
 the linear code flow.

 What is deceiving about it? Of course for someone who has no idea how
 vmx works the code will not be obvious, but why should we care. For
 someone who knows what is deceiving about returning into the same
 function guest was launched from by using VMX mechanism

 The way the code is written is deceiving.  If I see

   asm(vmlaunch; seta %0)
   while (ret)

 I expect HOST_RIP to point at the seta or somewhere near, not at a
 completely different label somewhere else.
Here for me, I prefer a separate asm blob of entry_vmx instead of one
hidden in a C function, which may make it much harder to trace for
someone not familiar with vmx in KVM.

 instead of longjmp()?

 Look again at the setjmp/longjmp version.  longjmp is not used to handle
 vmexit.  It is used to jump back from the vmexit handler to main, which
 is exactly what setjmp/longjmp is meant for.

 It is still somewhat magic: the while (ret) is only there to please
 the compiler

 No, it it there to catch vmlaunch/vmresume errors.

 You could change it to while (0) and it would still work.  That's what
 I mean by only there to please the compiler.

 the compiler, and you rely on the compiler not changing %rsp between the
 vmlaunch and the vmx_return label.  Minor nit, you cannot anymore print
 HOST_RSP should be loaded on each guest entry.

 Right, but my point is: without a big asm blob, you don't know the right
 value to load.  It depends on the generated code.  And the big asm blob
 limits a lot the code looks nice value of this approach.

 different error messages for vmlaunch vs. vmresume failure.
 Just because the same variable is used to store error values :)
 Make vmlaunch use err1 and vmresume err2 and do while (!err1  !err2)

 Yeah. :)

 In the end the choice is between all in asm and small asm trampoline
 (which also happens to use setjmp/longjmp, but perhaps Arthur can
 propagate return codes without using setjmp/longjmp, too).

 Rewriting the whole guest entry exit path in asm like you suggest bellow
 will eliminate the magic too.

 I much prefer one big asm statement than many small asm statement
 scattered among two or three C lines.

 It's not many asm statements, it's just a dozen lines:

 I am not talking about overall file, but the how vmx_run() is written:
 asm()
 C code
 asm()
 C code
 ...

 I much prefer:
 C code

 big asm() blob

 C code.

 Me too.  But the trampoline version is neither, it is

 static inline void vmlaunch() { asm(vmlaunch) }
 static inline void vmresume() { asm(vmresume) }
 small asm() blob
 C code
So this is a style problem on the basis of right code generation
indeed. When I write codes of this version, it occurs that the
compiler optimized some of my codes and something goes wrong. If we
use C style, we need setjmp/longjmp, otherwise we need big asm blob to
forbid compiler optimization.

I prefer to Paolo indeed as to my writing the two versions. Writing
asm in C is sometimes uncomfortable (e.g. %rax and %%rax, and
considering the C codes before and after the asm blob). Actually, we
can hide setjmp in vmx_on and longjmp in the asm codes after executing
exit_handler, thus we just need to call vmx_on to enter VM and return
a designed value (e.g. -1) in exit_handler to exit VM. Then any
following codes don't need to care about setjmp/longjmp.

Arthur

 Paolo

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-18 Thread Gleb Natapov
On Thu, Jul 18, 2013 at 02:08:51PM +0200, Paolo Bonzini wrote:
 Il 18/07/2013 13:06, Gleb Natapov ha scritto:
  On Thu, Jul 18, 2013 at 12:47:46PM +0200, Paolo Bonzini wrote:
  and for a testsuite I'd prefer the latter---which means I'd still favor
  setjmp/longjmp.
 
  Now, here is the long explanation.
 
  I must admit that the code looks nice.  There are some nits I'd like to
  see done differently (such as putting vmx_return at the beginning of the
  while (1), and the vmresume asm at the end), but it is indeed nice.
 
  Why do you prefer setjmp/longjmp then?
 
  Because it is still deceiving, and I dislike the deceit more than I like
  the linear code flow.
 
  What is deceiving about it? Of course for someone who has no idea how
  vmx works the code will not be obvious, but why should we care. For
  someone who knows what is deceiving about returning into the same
  function guest was launched from by using VMX mechanism
 
 The way the code is written is deceiving.  If I see
 
   asm(vmlaunch; seta %0)
   while (ret)
 
 I expect HOST_RIP to point at the seta or somewhere near, not at a
 completely different label somewhere else.
 
Why would you expect that assuming you know what vmlaunch is? So it is OK
when HOST_RIP point to somewhere outside a function, but deceiving if
it point 3 lines down in the same function? I have a hard time following
this logic.

  instead of longjmp()?
 
 Look again at the setjmp/longjmp version.  longjmp is not used to handle
 vmexit.  It is used to jump back from the vmexit handler to main, which
 is exactly what setjmp/longjmp is meant for.
 
That's because simple return will not work in that version, this is
artifact of how vmexit was done.

  It is still somewhat magic: the while (ret) is only there to please
  the compiler
 
  No, it it there to catch vmlaunch/vmresume errors.
 
 You could change it to while (0) and it would still work.  That's what
 I mean by only there to please the compiler.
But while (1) will not, so the code is executed, it is not just for
compiler there, but it is executed only if vmlaunch/vmresume fails.

 
  the compiler, and you rely on the compiler not changing %rsp between the
  vmlaunch and the vmx_return label.  Minor nit, you cannot anymore print
  HOST_RSP should be loaded on each guest entry.
 
 Right, but my point is: without a big asm blob, you don't know the right
 value to load.  It depends on the generated code.  And the big asm blob
 limits a lot the code looks nice value of this approach.
 
I said it number of times already, this is not about code looking nice,
code looks like KVM or use less assembler as possible, this is about
linear data flow. It is not fun chasing code execution path. Yes, you
can argue that vmlaunch/vmresume inherently non linear, but there is a
difference between skipping one instruction and remain in the same
function and on the same stack, or jump completely to a different
context.

Speaking about KVM. Guest enter/exit assembly blob is around ~50 lines
if assembly code and more then half of that is saving restoring context.
And the code plays some tricks there for optimization that we do not
need to do here, so I expect the code to be even smaller, not much more
then 10 lines of assembly excluding state save/restore.

  different error messages for vmlaunch vs. vmresume failure.
  Just because the same variable is used to store error values :)
  Make vmlaunch use err1 and vmresume err2 and do while (!err1  !err2)
 
 Yeah. :)
 
  In the end the choice is between all in asm and small asm trampoline
  (which also happens to use setjmp/longjmp, but perhaps Arthur can
  propagate return codes without using setjmp/longjmp, too).
 
  Rewriting the whole guest entry exit path in asm like you suggest bellow
  will eliminate the magic too.
 
  I much prefer one big asm statement than many small asm statement
  scattered among two or three C lines.
 
  It's not many asm statements, it's just a dozen lines:
 
  I am not talking about overall file, but the how vmx_run() is written:
  asm()
  C code
  asm()
  C code
  ...
  
  I much prefer:
  C code
  
  big asm() blob
  
  C code.
 
 Me too.  But the trampoline version is neither, it is
 
 static inline void vmlaunch() { asm(vmlaunch) }
 static inline void vmresume() { asm(vmresume) }
 small asm() blob
I is small only because context save restore is hidden behind
macro and 4 asm instructions (vmlaunch/seta/vmresume/seta) a hidden
somewhere else. The actually differences in asm instruction between both
version will not be bigger then a couple of lines (if we will not take
setjmp/longjmp implementation into account :)), but I do not even see
why we discuss this argument since minimizing asm instructions here is
not an point. We should not use more then needed to achieve the goal of
course, but design should not be around number of assembly lines.

 C code
 
 Paolo

--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in

[RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-17 Thread Arthur Chunqi Li
This is the first version of VMX nested environment. It contains the
basic VMX instructions test cases, including VMXON/VMXOFF/VMXPTRLD/
VMXPTRST/VMCLEAR/VMLAUNCH/VMRESUME/VMCALL. This patchalso tests the
basic execution routine in VMX nested environment andlet the VM print
Hello World to inform its successfully run.

The first release also includes a test suite for vmenter (vmlaunch and
vmresume). Besides, hypercall mechanism is included and currently it is
used to invoke VM normal exit.

New files added:
x86/vmx.h : contains all VMX related macro declerations
x86/vmx.c : main file for VMX nested test case

Signed-off-by: Arthur Chunqi Li yzt...@gmail.com
---
 config-x86-common.mak |2 +
 config-x86_64.mak |1 +
 lib/x86/msr.h |5 +
 x86/cstart64.S|4 +
 x86/unittests.cfg |6 +
 x86/vmx.c |  681 +
 x86/vmx.h |  455 +
 7 files changed, 1154 insertions(+)
 create mode 100644 x86/vmx.c
 create mode 100644 x86/vmx.h

diff --git a/config-x86-common.mak b/config-x86-common.mak
index 455032b..34a41e1 100644
--- a/config-x86-common.mak
+++ b/config-x86-common.mak
@@ -101,6 +101,8 @@ $(TEST_DIR)/asyncpf.elf: $(cstart.o) $(TEST_DIR)/asyncpf.o
 
 $(TEST_DIR)/pcid.elf: $(cstart.o) $(TEST_DIR)/pcid.o
 
+$(TEST_DIR)/vmx.elf: $(cstart.o) $(TEST_DIR)/vmx.o
+
 arch_clean:
$(RM) $(TEST_DIR)/*.o $(TEST_DIR)/*.flat $(TEST_DIR)/*.elf \
$(TEST_DIR)/.*.d $(TEST_DIR)/lib/.*.d $(TEST_DIR)/lib/*.o
diff --git a/config-x86_64.mak b/config-x86_64.mak
index 4e525f5..bb8ee89 100644
--- a/config-x86_64.mak
+++ b/config-x86_64.mak
@@ -9,5 +9,6 @@ tests = $(TEST_DIR)/access.flat $(TEST_DIR)/apic.flat \
  $(TEST_DIR)/xsave.flat $(TEST_DIR)/rmap_chain.flat \
  $(TEST_DIR)/pcid.flat
 tests += $(TEST_DIR)/svm.flat
+tests += $(TEST_DIR)/vmx.flat
 
 include config-x86-common.mak
diff --git a/lib/x86/msr.h b/lib/x86/msr.h
index 509a421..281255a 100644
--- a/lib/x86/msr.h
+++ b/lib/x86/msr.h
@@ -396,6 +396,11 @@
 #define MSR_IA32_VMX_VMCS_ENUM  0x048a
 #define MSR_IA32_VMX_PROCBASED_CTLS20x048b
 #define MSR_IA32_VMX_EPT_VPID_CAP   0x048c
+#define MSR_IA32_VMX_TRUE_PIN  0x048d
+#define MSR_IA32_VMX_TRUE_PROC 0x048e
+#define MSR_IA32_VMX_TRUE_EXIT 0x048f
+#define MSR_IA32_VMX_TRUE_ENTRY0x0490
+
 
 /* AMD-V MSRs */
 
diff --git a/x86/cstart64.S b/x86/cstart64.S
index 24df5f8..0fe76da 100644
--- a/x86/cstart64.S
+++ b/x86/cstart64.S
@@ -4,6 +4,10 @@
 .globl boot_idt
 boot_idt = 0
 
+.globl idt_descr
+.globl tss_descr
+.globl gdt64_desc
+
 ipi_vector = 0x20
 
 max_cpus = 64
diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index bc9643e..85c36aa 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -149,3 +149,9 @@ extra_params = --append 1000 `date +%s`
 file = pcid.flat
 extra_params = -cpu qemu64,+pcid
 arch = x86_64
+
+[vmx]
+file = vmx.flat
+extra_params = -cpu host,+vmx
+arch = x86_64
+
diff --git a/x86/vmx.c b/x86/vmx.c
new file mode 100644
index 000..a1023b1
--- /dev/null
+++ b/x86/vmx.c
@@ -0,0 +1,681 @@
+#include libcflat.h
+#include processor.h
+#include vm.h
+#include desc.h
+#include vmx.h
+#include msr.h
+#include smp.h
+#include io.h
+
+int fails = 0, tests = 0;
+u32 *vmxon_region;
+struct vmcs *vmcs_root;
+u32 vpid_cnt;
+void *guest_stack, *guest_syscall_stack;
+u32 ctrl_pin, ctrl_enter, ctrl_exit, ctrl_cpu[2];
+ulong fix_cr0_set, fix_cr0_clr;
+ulong fix_cr4_set, fix_cr4_clr;
+struct regs regs;
+struct vmx_test *current;
+u64 hypercall_field = 0;
+
+extern u64 gdt64_desc[];
+extern u64 idt_descr[];
+extern u64 tss_descr[];
+extern void *vmx_return;
+extern void *entry_sysenter;
+extern void *guest_entry;
+
+void report(const char *name, int result)
+{
+   ++tests;
+   if (result)
+   printf(PASS: %s\n, name);
+   else {
+   printf(FAIL: %s\n, name);
+   ++fails;
+   }
+}
+
+int vmcs_clear(struct vmcs *vmcs)
+{
+   bool ret;
+   asm volatile (vmclear %1; setbe %0 : =q (ret) : m (vmcs) : cc);
+   return ret;
+}
+
+u64 vmcs_read(enum Encoding enc)
+{
+   u64 val;
+   asm volatile (vmread %1, %0 : =rm (val) : r ((u64)enc) : cc);
+   return val;
+}
+
+int vmcs_write(enum Encoding enc, u64 val)
+{
+   bool ret;
+   asm volatile (vmwrite %1, %2; setbe %0
+   : =q(ret) : rm (val), r ((u64)enc) : cc);
+   return ret;
+}
+
+int make_vmcs_current(struct vmcs *vmcs)
+{
+   bool ret;
+
+   asm volatile (vmptrld %1; setbe %0 : =q (ret) : m (vmcs) : cc);
+   return ret;
+}
+
+int save_vmcs(struct vmcs **vmcs)
+{
+   bool ret;
+
+   asm volatile (vmptrst %1; setbe %0 : =q (ret) : m (*vmcs) : cc);
+   return ret;
+}
+
+/* entry_sysenter */
+asm(
+   .align 4, 0x90\n\t
+   .globl entry_sysenter\n\t
+   entry_sysenter:\n\t
+   SAVE_GPR
+ 

Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

2013-07-17 Thread Paolo Bonzini
Il 17/07/2013 20:54, Arthur Chunqi Li ha scritto:
 +/* entry_sysenter */
 +asm(
 + .align 4, 0x90\n\t
 + .globl entry_sysenter\n\t
 + entry_sysenter:\n\t
 + SAVE_GPR
 +and $0xf, %rax\n\t
 +push%rax\n\t

push should be wrong here, the first argument is in %rdi.

 +callsyscall_handler\n\t
 + LOAD_GPR
 +vmresume\n\t
 +);
 +
 +int exit_handler()

This (and syscall_handler too) needs __attribute__((__used__)) because
it's only used from assembly.

Please add static to all functions except main, it triggers better
compiler optimization and warnings and it will avoid nasty surprises in
the future if the compiler becomes smarter.

 +{
 + int ret;
 +
 + current-exits++;
 + current-guest_regs = regs;
 + if (is_hypercall())
 + ret = handle_hypercall();
 + else
 + ret = current-exit_handler();
 + regs = current-guest_regs;
 + switch (ret) {
 + case VMX_TEST_VMEXIT:
 + case VMX_TEST_RESUME:
 + return ret;
 + case VMX_TEST_EXIT:
 + break;
 + default:
 + printf(ERROR : Invalid exit_handler return val %d.\n
 + , ret);
 + }

Here have to propagate the exit codes through multiple levels, because
we're not using setjmp/longjmp.  Not a big deal.  My worries with this
version are below.

 + print_vmexit_info();
 + exit(-1);
 + return 0;
 +}
 +
 +int vmx_run()
 +{
 + bool ret;
 + u32 eax;
 + u64 rsp;
 +
 + asm volatile(mov %%rsp, %0\n\t : =r(rsp));
 + vmcs_write(HOST_RSP, rsp);
 + asm volatile(vmlaunch;seta %0\n\t : =m(ret));

setbe (this path is probably untested because it never happens in practice).

Also, missing memory clobber.

 + if (ret) {
 + printf(%s : vmlaunch failed.\n, __func__);
 + return 1;
 + }
 + while (1) {
 + asm volatile(
 + LOAD_GPR_C
 + vmresume;seta %0\n\t
 + : =m(ret));

setbe and missing memory clobber.

 + if (ret) {
 + printf(%s : vmresume failed.\n, __func__);
 + return 1;
 + }
 + asm volatile(
 + .global vmx_return\n\t

.global should not be needed.

 + vmx_return:\n\t
 + SAVE_GPR_C
 + call exit_handler\n\t
 + mov %%eax, %0\n\t
 + : =r(eax)
 + );

I had written a long explanation here about why I don't trust the
compiler to do the right thing, and ideas about how to fix that.  But in
the end the only workable solution is a single assembly blob like vmx.c
in KVM to do vmlaunch/vmresume, so I'll get right to the point:

   *  the similarity with KVM code and as little assembly as  *
   *  possible goals are mutually exclusive *

and for a testsuite I'd prefer the latter---which means I'd still favor
setjmp/longjmp.

Now, here is the long explanation.

I must admit that the code looks nice.  There are some nits I'd like to
see done differently (such as putting vmx_return at the beginning of the
while (1), and the vmresume asm at the end), but it is indeed nice.

However, it is also very deceiving, because the processor is not doing
what the code says.  If I see:

   vmlaunch();
   exit(-1);

it is clear that magic happens in vmlaunch().  If I see

   asm volatile(vmlaunch;setbe %0\n\t : =m(ret));
   if (ret) {
   ...
   }
   asm(vmx_return:)

it is absolutely not clear that the setbe and if are skipped on a
successful vmlaunch.  So, at the very least you need a comment before
those if (ret) to document the control flow, or you can use a jmp like
this:

   asm volatile goto (vmlaunch;jmp %0\n\t
  : : : memory : vmlaunch_error);
   if (0) {
   vmlaunch_error: ...
   }

The unconditional jump and asm goto make it clear that magic happens.

By the way, asm goto is also needed to tell the compiler that the
vmlaunch and vmresume asms reenter at vmx_resume(*).  Without it, there
is no guarantee that rsp is the same when you save it and at the
vmx_resume entry point (in fact, I'd feel safer if you did the vmwrite
to HOST_RSP and vmlaunch in the same asm goto.

Using asm goto to tell the compiler about vmx_resume poses another
problem.  asm goto takes a C label, vmx_resume is an assembly label.
The compiler might put extra instruction between the C label and
vmx_resume, for example to read back values it has spilled to the stack.

Overall, I don't trust the compiler to compile this code right
(especially if we later include a 32-bit version) and the only solution
is to write the whole thing in assembly.  If you want to write the logic
in C, you need the setjmp/longjmp version.  That version needs no such
guarantee because all the trampolines are away from the hands