Re: [PATCH] kvm-unit-tests: VMX: Fix confused definition of rflags

[Gleb Natapov <gleb@redhat.com>]

On Mon, Aug 05, 2013 at 04:48:47PM +0800, Arthur Chunqi Li wrote:
> On Mon, Aug 5, 2013 at 4:29 PM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Tue, Jul 30, 2013 at 11:41:00PM +0800, Arthur Chunqi Li wrote:
> >> Change "rflags" in "struct regs" to "host_rflags". Remove settings
> >> to GUEST_RFLAGS since GUEST_RFLAGS can be set by vmwrite. Treat
> >> host_rflags as host rflags before and after vmenter.
> >>
> > I am not sure the change is for the better. Before the change one could
> > set up rflags for guest environment by setting regs.rflags, no special
> > init function had to be written. I do not see any problem with correct
> > code, except that rflags is not correct on a guest entry, but this
> > should be easy to fix.
> regs.rflags are designed to set guest rflags, but the current
> implementation just use it as host_rflags.
Current implementation uses it to set guest flags at the beginning of
a test.

>                                            For every VM entry, it will
> load value set by vmcs_write(GUEST_RFLAGS).
Not for every vmentry, only for the first one. Doing it for every
vmentry without saving it first with vmcs_read would been incorrect.

>                                              Set regs.flags as host
> rflags and then enter VM cannot affect VM's rflags, which is the
> current implementation.
> 
It can, on the first launch. It is easy to add code to exit_handler() to
set current->guest_regs.rflags correctly before calling test's exit handler
and write GUEST_RFLAGS according to current->guest_regs.rflags after
exit handler returns, but as you say below test can do it by itself in
exit handler if it wishes so.

> Besides, if host want to set/get guest's rflags, it just use
> vmcs_write/read(GUEST_RFLAGS).
> 
True, but to do it at the beginning of the test it will require to write
special init function even if non are needed otherwise. So you are
removing this functionality without clear benefit.

> Arthur
> >
> >> Besides, add checks to flags after vmenter.
> >>
> >> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
> >> ---
> >>  x86/vmx.c |   11 ++++++-----
> >>  x86/vmx.h |    2 +-
> >>  2 files changed, 7 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/x86/vmx.c b/x86/vmx.c
> >> index 7467927..082c3bb 100644
> >> --- a/x86/vmx.c
> >> +++ b/x86/vmx.c
> >> @@ -481,6 +481,8 @@ static int vmx_run()
> >>                       "vmresume\n\t"
> >>                       "2: "
> >>                       "setbe %0\n\t"
> >> +                     "jbe vmx_return\n\t"
> >> +                     "ud2\n\t"
> >>                       "vmx_return:\n\t"
> >>                       SAVE_GPR_C
> >>                       SAVE_RFLAGS
> >> @@ -505,15 +507,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 ((!(regs.host_rflags & X86_EFLAGS_CF) && !(regs.host_rflags & X86_EFLAGS_ZF))
> >> +                     || ((regs.host_rflags & X86_EFLAGS_CF) && (regs.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 ((!(regs.host_rflags & X86_EFLAGS_CF) && !(regs.host_rflags & X86_EFLAGS_ZF))
> >> +                     || ((regs.host_rflags & X86_EFLAGS_CF) && (regs.host_rflags & X86_EFLAGS_ZF)))
> >>                       printf("\tvmresume set wrong flags\n");
> >>               report("test vmresume", 0);
> >>               break;
> >> @@ -540,7 +542,6 @@ static int test_run(struct vmx_test *test)
> >>       test->exits = 0;
> >>       current = test;
> >>       regs = test->guest_regs;
> >> -     vmcs_write(GUEST_RFLAGS, regs.rflags | 0x2);
> >>       launched = 0;
> >>       printf("\nTest suite : %s\n", test->name);
> >>       vmx_run();
> >> diff --git a/x86/vmx.h b/x86/vmx.h
> >> index 1fb9738..d80e000 100644
> >> --- a/x86/vmx.h
> >> +++ b/x86/vmx.h
> >> @@ -27,7 +27,7 @@ struct regs {
> >>       u64 r13;
> >>       u64 r14;
> >>       u64 r15;
> >> -     u64 rflags;
> >> +     u64 host_rflags;
> >>  };
> >>
> >>  struct vmx_test {
> >> --
> >> 1.7.9.5
> >
> > --
> >                         Gleb.

--
			Gleb.