Re: [PATCH 0/30] nVMX: Nested VMX, v9

[Avi Kivity <avi@redhat.com>]

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