On Wed, Jul 17, 2013 at 12:45 AM, Gleb Natapov <gleb@xxxxxxxxxx> wrote:
> On Tue, Jul 16, 2013 at 11:29:20PM +0800, Arthur Chunqi Li wrote:
>> On Tue, Jul 16, 2013 at 11:20 PM, Gleb Natapov <gleb@xxxxxxxxxx> wrote:
>> > On Tue, Jul 16, 2013 at 12:28:05PM +0200, Paolo Bonzini wrote:
>> >> > +void vmx_exit(void)
>> >> > +{
>> >> > + test_vmxoff();
>> >> > + printf("\nSUMMARY: %d tests, %d failures\n", tests, fails);
>> >> > + exit(fails ? -1 : 0);
>> >> > +}
>> >>
>> >> Can you try to jump back to main, and do test_vmxoff there? This will
>> >> avoid having to write our tests in callback style, which is a pain.
>> >> Basically something similar to setjmp/longjmp. In main:
>> >>
>> >> if (setjmp(jmpbuf) == 0) {
>> >> vmx_run();
>> >> /* Should not reach here */
>> >> report("test vmlaunch", 0);
>> >> }
>> >> test_vmxoff();
>> >>
>> >> exit:
>> >> printf("\nSUMMARY: %d tests, %d failures\n", tests, fails);
>> >> return fails ? 1 : 0;
>> >>
>> >> In vmx_handler:
>> >>
>> >> case VMX_HLT:
>> >> printf("\nVM exit.\n");
>> >> longjmp(jmpbuf, 1);
>> >>
>> > Why not just make vmexit occur after vmlaunch/vmresume like KVM does. It
>> > will make code much more straightforward and easer to follow.
>> The concept "easier to follow" may have different meanings in
>> different view. This achievement puts all the test cases in main
>> function instead of scattering everywhere, which is another view to
>> "easy to follow". As this is just a test case, I prefer this one.
>>
> I do not see why what I propose will prevent you to put all tests into main.
>
> vmx_run() will looks like that:
>
> vmlaunch
> while(1) {
> vmresume
> <---- vmexit jumps here
> switch(exit reason) {
> case reason1:
> break;
> case reason2:
> break;
> case HLT
> return;
> }
> }
Yes, this recalls me some KVM codes I have read before. This mixes
vmlaunch/resume and vmx_handler into one piece of code. It is a good
way to explicitly show the execution sequence though, it increases LOC
in one function.
>
>> Besides, this way we can start another VM following the previous one
>> simply in main function. This is flexible if we want to test re-enter
>> to VMX mode or so.
>>
> That's what I am missing. How do one writes more tests now?
>
> I was thinking about interface like that:
>
> guest_func_test1()
> {
> }
>
> tes1t_exit_handlers[] = {test1_handle_hlt, test1_handle_exception, ....}
>
> main()
> {
>
> init_vmcs(); /* generic stuff */
> init_vmcs_test1(); /* test1 related stuff */
> r = run_in_guest(guest_func_test1, test1_exit_handlers);
> report("test1", r);
> }
>
I have thought about this question and I'm not quite sure how to solve
it now. I have two ways. The first is that we just leave vmx.c as the
VMX instructions and execution routine test suite, and develop other
test cases in other files. Since all other tests of nested vmx is
independent to the basic routine and it is hard for us to put all test
cases for nested VMX in one file, so we just let this file do simple
things and reuse some of its functions in other test suites of nested
vmx. Your proposal of adding new test cases can be implemented in
other test suites.
The other way is not splitting nested vmx tests cases in contrast.
This method may cause a HUGE vmx.c file, and tests for different parts
are not distinctive.
Actually, I prefer the former solution.
> --
> Gleb.
Besides, there is also another "pseudo" bug in PATCH 2/2, here:
+int test_vmx_capability(void)
+{
+ struct cpuid r;
+ u64 ret1, ret2;
+ r = cpuid(1);
+ ret1 = ((r.c) >> 5) & 1;
+ ret2 = ((rdmsr(MSR_IA32_FEATURE_CONTROL) & 0x5) == 0x5);
+ report("test vmx capability", ret1 & ret2);
+ return !(ret1 & ret2);
+}
The IA32_FEATURE_CONTROL MSR should be set by seabios. SInce there's
no patch for seabios and in fact software can also set this MSR if its
lock bit is unset. So I prefer to change it like this:
+int test_vmx_capability(void)
+{
+ struct cpuid r;
+ u64 ret1, ret2;
+ u64 ia32_feature_control;
+ r = cpuid(1);
+ ret1 = ((r.c) >> 5) & 1;
+ ia32_feature_control = rdmsr(MSR_IA32_FEATURE_CONTROL);
+ ret2 = ((ia32_feature_control & 0x5) == 0x5);
+ if ((!ret2) && ((ia32_feature_control & 0x1) == 0)){
+ wrmsr(MSR_IA32_FEATURE_CONTROL, 0x5);
+ ia32_feature_control = rdmsr(MSR_IA32_FEATURE_CONTROL);
+ ret2 = ((ia32_feature_control & 0x5) == 0x5);
+ }
+ report("test vmx capability", ret1 & ret2);
+ return !(ret1 & ret2);
+}
Arthur
--
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
