Re: [PATCH 5/5] Nested VMX patch 5 implements vmlaunch and vmresume

[Gleb Natapov <gleb@redhat.com>]

On Wed, Oct 21, 2009 at 04:43:44PM +0200, Orit Wasserman wrote:
> > > @@ -4641,10 +4955,13 @@ static void vmx_complete_interrupts(struct
> > vcpu_vmx *vmx)
> > >     int type;
> > >     bool idtv_info_valid;
> > >
> > > -   exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
> > > -
> > >     vmx->exit_reason = vmcs_read32(VM_EXIT_REASON);
> > >
> > > +   if (vmx->nested.nested_mode)
> > > +      return;
> > > +
> > Why return here? What the function does that should not be done in
> > nested mode?
> In nested mode L0 injects an interrupt to L2 only in one scenario,
> if there is an IDT_VALID event and L0 decides to run L2 again and not to
> switch back to L1.
> In all other cases the injection is handled by L1.
This is exactly the kind of scenario that is handled by
vmx_complete_interrupts(). (vmx|svm)_complete_interrups() store
pending event in arch agnostic way and re-injection is handled by
x86.c You bypass this logic by inserting return here and introducing
nested_handle_valid_idt() function below.

> >
> > > +   exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
> > > +
> > >     /* Handle machine checks before interrupts are enabled */
> > >     if ((vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
> > >         || (vmx->exit_reason == EXIT_REASON_EXCEPTION_NMI
> > > @@ -4747,6 +5064,60 @@ static void fixup_rmode_irq(struct vcpu_vmx
> *vmx)
> > >        | vmx->rmode.irq.vector;
> > >  }
> > >
> > > +static int nested_handle_valid_idt(struct kvm_vcpu *vcpu)
> > > +{
> > It seems by this function you are trying to bypass general event
> > reinjection logic. Why?
> See above.
The logic implemented by this function is handled in x86.c in arch
agnostic way. Is there something wrong with this?

> > > +   vmx->launched = vmx->nested.l2_state->launched;
> > > +
> > Can you explain why ->launched logic is needed?
> It is possible L1 called vmlaunch but we didn't actually run L2 (for
> example there was an interrupt and
> enable_irq_window switched back to L1 before running L2). L1 thinks the
> vmlaunch was successful and call vmresume in the next time
> but KVM needs to call vmlaunch for L2.
handle_vmlauch() and handle_vmresume() are exactly the same. Why KVM needs
to run one and not the other?
 
> > > +static int nested_vmx_vmexit(struct kvm_vcpu *vcpu,
> > > +              bool is_interrupt)
> > > +{
> > > +   struct vcpu_vmx *vmx = to_vmx(vcpu);
> > > +   int initial_pfu_active = vcpu->fpu_active;
> > > +
> > > +   if (!vmx->nested.nested_mode) {
> > > +      printk(KERN_INFO "WARNING: %s called but not in nested mode\n",
> > > +             __func__);
> > > +      return 0;
> > > +   }
> > > +
> > > +   save_msrs(vmx->guest_msrs, vmx->save_nmsrs);
> > > +
> > > +   sync_cached_regs_to_vmcs(vcpu);
> > > +
> > > +   if (!nested_map_shadow_vmcs(vcpu)) {
> > > +      printk(KERN_INFO "Error mapping shadow vmcs\n");
> > > +      set_rflags_to_vmx_fail_valid(vcpu);
> > Error during vmexit should set abort flag, not change flags.
> I think this is more a vmlaunch/vmresume error (in the code that switch
> back to L1).
How is this vmlaunch/vmresume error? This function is called to exit
from L2 guest while on L2 vcms. It is called asynchronously in respect
to L2 guest and you modify L2 guest rflags register at unpredictable
place here.

--
			Gleb.