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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



在 2013-8-5,17:10,Gleb Natapov <gleb@xxxxxxxxxx> 写道:

> 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@xxxxxxxxxx> 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.
True, so it seems better to keep it. However, now it is confused with host_rflags and not correctly set/get when vmentry/vmexit. I will commit another patch to fix both bugs.

Arthur
> 
>> Arthur
>>> 
>>>> Besides, add checks to flags after vmenter.
>>>> 
>>>> Signed-off-by: Arthur Chunqi Li <yzt356@xxxxxxxxx>
>>>> ---
>>>> 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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux