diff mbox

kvm-unit-tests: VMX: Separate host and guest rflags

Message ID 1375706605-14980-1-git-send-email-yzt356@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Arthur Chunqi Li Aug. 5, 2013, 12:43 p.m. UTC 
Separate host_rflags and guest_rflags (regs.rflags used for guest).
Fix bug of set/get guest rflags when vmenter/vmexit.

Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
---
 x86/vmx.c |   11 +++++++----
 x86/vmx.h |    4 ++--
 2 files changed, 9 insertions(+), 6 deletions(-)

Comments

Gleb Natapov Aug. 5, 2013, 1:18 p.m. UTC | #1 
On Mon, Aug 05, 2013 at 08:43:25PM +0800, Arthur Chunqi Li wrote:
> Separate host_rflags and guest_rflags (regs.rflags used for guest).
> Fix bug of set/get guest rflags when vmenter/vmexit.
> 
> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
> ---
>  x86/vmx.c |   11 +++++++----
>  x86/vmx.h |    4 ++--
>  2 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 7467927..7b28aca 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -19,6 +19,7 @@ struct regs regs;
>  struct vmx_test *current;
>  u64 hypercall_field = 0;
>  bool launched;
> +u64 host_rflags;
>  
Can't you define in on stack?

>  extern u64 gdt64_desc[];
>  extern u64 idt_descr[];
> @@ -440,12 +441,14 @@ static int exit_handler()
>  	int ret;
>  
>  	current->exits++;
> +	regs.rflags = vmcs_read(GUEST_RFLAGS);
>  	current->guest_regs = regs;
>  	if (is_hypercall())
>  		ret = handle_hypercall();
>  	else
>  		ret = current->exit_handler();
>  	regs = current->guest_regs;
> +	vmcs_write(GUEST_RFLAGS, regs.rflags);
>  	switch (ret) {
>  	case VMX_TEST_VMEXIT:
>  	case VMX_TEST_RESUME:
> @@ -505,15 +508,15 @@ static int vmx_run()
>  		return 0;
>  	case VMX_TEST_LAUNCH_ERR:
>  		printf("%s : vmlaunch failed.\n", __func__);
> -		if ((!(regs.rflags & X86_EFLAGS_CF) && !(regs.rflags & X86_EFLAGS_ZF))
> -			|| ((regs.rflags & X86_EFLAGS_CF) && (regs.rflags & X86_EFLAGS_ZF)))
> +		if ((!(host_rflags & X86_EFLAGS_CF) && !(host_rflags & X86_EFLAGS_ZF))
> +			|| ((host_rflags & X86_EFLAGS_CF) && (host_rflags & X86_EFLAGS_ZF)))
>  			printf("\tvmlaunch set wrong flags\n");
>  		report("test vmlaunch", 0);
>  		break;
>  	case VMX_TEST_RESUME_ERR:
>  		printf("%s : vmresume failed.\n", __func__);
> -		if ((!(regs.rflags & X86_EFLAGS_CF) && !(regs.rflags & X86_EFLAGS_ZF))
> -			|| ((regs.rflags & X86_EFLAGS_CF) && (regs.rflags & X86_EFLAGS_ZF)))
> +		if ((!(host_rflags & X86_EFLAGS_CF) && !(host_rflags & X86_EFLAGS_ZF))
> +			|| ((host_rflags & X86_EFLAGS_CF) && (host_rflags & X86_EFLAGS_ZF)))
>  			printf("\tvmresume set wrong flags\n");
>  		report("test vmresume", 0);
>  		break;
> diff --git a/x86/vmx.h b/x86/vmx.h
> index 1fb9738..d4f979c 100644
> --- a/x86/vmx.h
> +++ b/x86/vmx.h
> @@ -403,10 +403,10 @@ enum Ctrl1 {
>  
>  #define SAVE_RFLAGS		\
>  	"pushf\n\t"			\
> -	"pop regs+0x80\n\t"
> +	"pop host_rflags\n\t"
>  
>  #define LOAD_RFLAGS		\
> -	"push regs+0x80\n\t"	\
> +	"push host_rflags\n\t"	\
>  	"popf\n\t"
>  
>  #define VMX_IO_SIZE_MASK		0x7
> -- 
> 1.7.9.5

--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arthur Chunqi Li Aug. 5, 2013, 1:45 p.m. UTC | #2 
On Mon, Aug 5, 2013 at 9:18 PM, Gleb Natapov <gleb@redhat.com> wrote:
> On Mon, Aug 05, 2013 at 08:43:25PM +0800, Arthur Chunqi Li wrote:
>> Separate host_rflags and guest_rflags (regs.rflags used for guest).
>> Fix bug of set/get guest rflags when vmenter/vmexit.
>>
>> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>> ---
>>  x86/vmx.c |   11 +++++++----
>>  x86/vmx.h |    4 ++--
>>  2 files changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/x86/vmx.c b/x86/vmx.c
>> index 7467927..7b28aca 100644
>> --- a/x86/vmx.c
>> +++ b/x86/vmx.c
>> @@ -19,6 +19,7 @@ struct regs regs;
>>  struct vmx_test *current;
>>  u64 hypercall_field = 0;
>>  bool launched;
>> +u64 host_rflags;
>>
> Can't you define in on stack?
Currently I don't use it outside vmx_run(), but it may be used in user
defined exit_handler in the future, so I put it globally.

Arthur
>
>>  extern u64 gdt64_desc[];
>>  extern u64 idt_descr[];
>> @@ -440,12 +441,14 @@ static int exit_handler()
>>       int ret;
>>
>>       current->exits++;
>> +     regs.rflags = vmcs_read(GUEST_RFLAGS);
>>       current->guest_regs = regs;
>>       if (is_hypercall())
>>               ret = handle_hypercall();
>>       else
>>               ret = current->exit_handler();
>>       regs = current->guest_regs;
>> +     vmcs_write(GUEST_RFLAGS, regs.rflags);
>>       switch (ret) {
>>       case VMX_TEST_VMEXIT:
>>       case VMX_TEST_RESUME:
>> @@ -505,15 +508,15 @@ static int vmx_run()
>>               return 0;
>>       case VMX_TEST_LAUNCH_ERR:
>>               printf("%s : vmlaunch failed.\n", __func__);
>> -             if ((!(regs.rflags & X86_EFLAGS_CF) && !(regs.rflags & X86_EFLAGS_ZF))
>> -                     || ((regs.rflags & X86_EFLAGS_CF) && (regs.rflags & X86_EFLAGS_ZF)))
>> +             if ((!(host_rflags & X86_EFLAGS_CF) && !(host_rflags & X86_EFLAGS_ZF))
>> +                     || ((host_rflags & X86_EFLAGS_CF) && (host_rflags & X86_EFLAGS_ZF)))
>>                       printf("\tvmlaunch set wrong flags\n");
>>               report("test vmlaunch", 0);
>>               break;
>>       case VMX_TEST_RESUME_ERR:
>>               printf("%s : vmresume failed.\n", __func__);
>> -             if ((!(regs.rflags & X86_EFLAGS_CF) && !(regs.rflags & X86_EFLAGS_ZF))
>> -                     || ((regs.rflags & X86_EFLAGS_CF) && (regs.rflags & X86_EFLAGS_ZF)))
>> +             if ((!(host_rflags & X86_EFLAGS_CF) && !(host_rflags & X86_EFLAGS_ZF))
>> +                     || ((host_rflags & X86_EFLAGS_CF) && (host_rflags & X86_EFLAGS_ZF)))
>>                       printf("\tvmresume set wrong flags\n");
>>               report("test vmresume", 0);
>>               break;
>> diff --git a/x86/vmx.h b/x86/vmx.h
>> index 1fb9738..d4f979c 100644
>> --- a/x86/vmx.h
>> +++ b/x86/vmx.h
>> @@ -403,10 +403,10 @@ enum Ctrl1 {
>>
>>  #define SAVE_RFLAGS          \
>>       "pushf\n\t"                     \
>> -     "pop regs+0x80\n\t"
>> +     "pop host_rflags\n\t"
>>
>>  #define LOAD_RFLAGS          \
>> -     "push regs+0x80\n\t"    \
>> +     "push host_rflags\n\t"  \
>>       "popf\n\t"
>>
>>  #define VMX_IO_SIZE_MASK             0x7
>> --
>> 1.7.9.5
>
> --
>                         Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paolo Bonzini Aug. 7, 2013, 3:37 p.m. UTC | #3 
On 08/05/2013 02:43 PM, Arthur Chunqi Li wrote:
> Separate host_rflags and guest_rflags (regs.rflags used for guest).
> Fix bug of set/get guest rflags when vmenter/vmexit.
>
> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>

Applied, thanks.

Not putting it on the stack is fine, but please do a followup patch to 
define vmx.c-private variables as "static" (as soon as I push the 
patches to kernel.org).

Paolo

> ---
>   x86/vmx.c |   11 +++++++----
>   x86/vmx.h |    4 ++--
>   2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 7467927..7b28aca 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -19,6 +19,7 @@ struct regs regs;
>   struct vmx_test *current;
>   u64 hypercall_field = 0;
>   bool launched;
> +u64 host_rflags;
>
>   extern u64 gdt64_desc[];
>   extern u64 idt_descr[];
> @@ -440,12 +441,14 @@ static int exit_handler()
>   	int ret;
>
>   	current->exits++;
> +	regs.rflags = vmcs_read(GUEST_RFLAGS);
>   	current->guest_regs = regs;
>   	if (is_hypercall())
>   		ret = handle_hypercall();
>   	else
>   		ret = current->exit_handler();
>   	regs = current->guest_regs;
> +	vmcs_write(GUEST_RFLAGS, regs.rflags);
>   	switch (ret) {
>   	case VMX_TEST_VMEXIT:
>   	case VMX_TEST_RESUME:
> @@ -505,15 +508,15 @@ static int vmx_run()
>   		return 0;
>   	case VMX_TEST_LAUNCH_ERR:
>   		printf("%s : vmlaunch failed.\n", __func__);
> -		if ((!(regs.rflags & X86_EFLAGS_CF) && !(regs.rflags & X86_EFLAGS_ZF))
> -			|| ((regs.rflags & X86_EFLAGS_CF) && (regs.rflags & X86_EFLAGS_ZF)))
> +		if ((!(host_rflags & X86_EFLAGS_CF) && !(host_rflags & X86_EFLAGS_ZF))
> +			|| ((host_rflags & X86_EFLAGS_CF) && (host_rflags & X86_EFLAGS_ZF)))
>   			printf("\tvmlaunch set wrong flags\n");
>   		report("test vmlaunch", 0);
>   		break;
>   	case VMX_TEST_RESUME_ERR:
>   		printf("%s : vmresume failed.\n", __func__);
> -		if ((!(regs.rflags & X86_EFLAGS_CF) && !(regs.rflags & X86_EFLAGS_ZF))
> -			|| ((regs.rflags & X86_EFLAGS_CF) && (regs.rflags & X86_EFLAGS_ZF)))
> +		if ((!(host_rflags & X86_EFLAGS_CF) && !(host_rflags & X86_EFLAGS_ZF))
> +			|| ((host_rflags & X86_EFLAGS_CF) && (host_rflags & X86_EFLAGS_ZF)))
>   			printf("\tvmresume set wrong flags\n");
>   		report("test vmresume", 0);
>   		break;
> diff --git a/x86/vmx.h b/x86/vmx.h
> index 1fb9738..d4f979c 100644
> --- a/x86/vmx.h
> +++ b/x86/vmx.h
> @@ -403,10 +403,10 @@ enum Ctrl1 {
>
>   #define SAVE_RFLAGS		\
>   	"pushf\n\t"			\
> -	"pop regs+0x80\n\t"
> +	"pop host_rflags\n\t"
>
>   #define LOAD_RFLAGS		\
> -	"push regs+0x80\n\t"	\
> +	"push host_rflags\n\t"	\
>   	"popf\n\t"
>
>   #define VMX_IO_SIZE_MASK		0x7
>

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

Patch

diff --git a/x86/vmx.c b/x86/vmx.c
index 7467927..7b28aca 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -19,6 +19,7 @@  struct regs regs;
 struct vmx_test *current;
 u64 hypercall_field = 0;
 bool launched;
+u64 host_rflags;
 
 extern u64 gdt64_desc[];
 extern u64 idt_descr[];
@@ -440,12 +441,14 @@  static int exit_handler()
 	int ret;
 
 	current->exits++;
+	regs.rflags = vmcs_read(GUEST_RFLAGS);
 	current->guest_regs = regs;
 	if (is_hypercall())
 		ret = handle_hypercall();
 	else
 		ret = current->exit_handler();
 	regs = current->guest_regs;
+	vmcs_write(GUEST_RFLAGS, regs.rflags);
 	switch (ret) {
 	case VMX_TEST_VMEXIT:
 	case VMX_TEST_RESUME:
@@ -505,15 +508,15 @@  static int vmx_run()
 		return 0;
 	case VMX_TEST_LAUNCH_ERR:
 		printf("%s : vmlaunch failed.\n", __func__);
-		if ((!(regs.rflags & X86_EFLAGS_CF) && !(regs.rflags & X86_EFLAGS_ZF))
-			|| ((regs.rflags & X86_EFLAGS_CF) && (regs.rflags & X86_EFLAGS_ZF)))
+		if ((!(host_rflags & X86_EFLAGS_CF) && !(host_rflags & X86_EFLAGS_ZF))
+			|| ((host_rflags & X86_EFLAGS_CF) && (host_rflags & X86_EFLAGS_ZF)))
 			printf("\tvmlaunch set wrong flags\n");
 		report("test vmlaunch", 0);
 		break;
 	case VMX_TEST_RESUME_ERR:
 		printf("%s : vmresume failed.\n", __func__);
-		if ((!(regs.rflags & X86_EFLAGS_CF) && !(regs.rflags & X86_EFLAGS_ZF))
-			|| ((regs.rflags & X86_EFLAGS_CF) && (regs.rflags & X86_EFLAGS_ZF)))
+		if ((!(host_rflags & X86_EFLAGS_CF) && !(host_rflags & X86_EFLAGS_ZF))
+			|| ((host_rflags & X86_EFLAGS_CF) && (host_rflags & X86_EFLAGS_ZF)))
 			printf("\tvmresume set wrong flags\n");
 		report("test vmresume", 0);
 		break;
diff --git a/x86/vmx.h b/x86/vmx.h
index 1fb9738..d4f979c 100644
--- a/x86/vmx.h
+++ b/x86/vmx.h
@@ -403,10 +403,10 @@  enum Ctrl1 {
 
 #define SAVE_RFLAGS		\
 	"pushf\n\t"			\
-	"pop regs+0x80\n\t"
+	"pop host_rflags\n\t"
 
 #define LOAD_RFLAGS		\
-	"push regs+0x80\n\t"	\
+	"push host_rflags\n\t"	\
 	"popf\n\t"
 
 #define VMX_IO_SIZE_MASK		0x7