On 05/12/2011 06:42 PM, Nadav Har'El wrote:
Our second conclusion (and I hope that I'm not offending anyone here) is that the changes for L2 interrupt injection in both SVM and VMX are both ugly - they are just ugly in different ways. Both modified the non-nested code in strange places in strange and unexpected ways, and tried to circumvent the usual code path in x86.c without touching x86.c. They just did this in two slightly different ways, neither (I think) is inherently uglier than the other: For accurate emulation (as I explain in the patch below), both codes need to cause x86.c to change its normal behavior: It checks for interrupt_allowed() and then (discovering that it isn't) enable_irq_window(). We want it to instead exit to L1, and then enable the irq window on that. In the SVM code, interrupt_allowed() is modified to always return false if nested, and enable_irq_window() is modified to flag for an exit to L1 (which is performed later) and turn on the interrupt window. In VMX, we modify the same places but differently: In interrupt_allowed() we exit to L1 immediately (it's a short operation, we didn't mind to do it in atomic context), and enable_irq_window() doesn't need to be changed (it already runs in L1).
I think that interrupt_allowed() should return true in L2 (if L1 has configured external interrupts to be trapped), and interrupt injection modified to cause an exit instead of queueing an interrupt. Note that on vmx, intercepted interrupt injection can take two different paths depending on whether the L1 wants interrupts acked or not.
Continuing to survey the difference between nested VMX and and SVM, there were other different choices made besides the ones mentioned above. nested SVM uses an additional trick, of skipping one round of running the guest, when it discovered the need for an exit in the "wrong" place, so it can get to the "right" place again. Nested VMX solved the same problems with other mechanisms, like a separate piece of code for handling IDT_VECTORING_INFO, and nested_run_pending. Some differences can also be explained by the different design of (non-nested) vmx.c vs svm.c - e.g., svm_complete_interrupts() is called during the handle_exit(), while vmx_complete_interrupts() is called after handle_exit() has completed (in atomic context) - this is one of the reasons the nested IDT_VECTORING_INFO path is different. I think that both solutions are far from being beautiful or easy to understand. Nested SVM is perhaps slightly less ugly but also has a small performance cost (with the extra vcpu_run iteration doing nothing) - and I think neither is inherently better than the other. So I guess my question is, and Avi and Gleb I'd love your comments about this question: Is it really beneficial that I rewrite the "ugly" nested-VMX injection code to be somewhat-ugly in exactly the same way that nested-SVM injection code? Won't it be more beneficial to rewrite *both* codes to be cleaner? This would probably mean changes to the common x86.c, that both will use. For example, x86.c's injection code could check the nested case itself, perhaps calling a special x86_op to handle the nested injection (exit, set interrupt window, etc.) instead of calling the regular interrupt_allowed/enable_irq_window and forcing those to be modified in mysterious ways. Now that there's a is_guest_mode(vcpu) function, more nested-related code can be moved to x86.c, to make both nested VMX and nested SVM code cleaner.
I am fine with committing as is. Later we can modify both vmx and svm to do the right thing (whatever that is), and later merge them into x86.c.
-- error compiling committee.c: too many arguments to function -- 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
