Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor

[Radim KrÄmÃÅ]

2017-07-11 16:34-0400, Bandan Das:
> Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx> writes:
> 
> > 2017-07-11 15:50-0400, Bandan Das:
> >> Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx> writes:
> >> > 2017-07-11 14:24-0400, Bandan Das:
> >> >> Bandan Das <bsd@xxxxxxxxxx> writes:
> >> >> > If there's a triple fault, I think it's a good idea to inject it
> >> >> > back. Basically, there's no need to take care of damage control
> >> >> > that L1 is intentionally doing.
> >> >> >
> >> >> >>> +			goto fail;
> >> >> >>> +		kvm_mmu_unload(vcpu);
> >> >> >>> +		vmcs12->ept_pointer = address;
> >> >> >>> +		kvm_mmu_reload(vcpu);
> >> >> >>
> >> >> >> I was thinking about something like this:
> >> >> >>
> >> >> >> kvm_mmu_unload(vcpu);
> >> >> >> old = vmcs12->ept_pointer;
> >> >> >> vmcs12->ept_pointer = address;
> >> >> >> if (kvm_mmu_reload(vcpu)) {
> >> >> >> 	/* pointer invalid, restore previous state */
> >> >> >> 	kvm_clear_request(KVM_REQ_TRIPLE_FAULT, vcpu);
> >> >> >> 	vmcs12->ept_pointer = old;
> >> >> >> 	kvm_mmu_reload(vcpu);
> >> >> >> 	goto fail;
> >> >> >> }
> >> >> >>
> >> >> >> The you can inherit the checks from mmu_check_root().
> >> >> 
> >> >> Actually, thinking about this a bit more, I agree with you. Any fault
> >> >> with a vmfunc operation should end with a vmfunc vmexit, so this
> >> >> is a good thing to have. Thank you for this idea! :)
> >> >
> >> > SDM says
> >> >
> >> >   IF tent_EPTP is not a valid EPTP value (would cause VM entry to fail
> >> >   if in EPTP) THEN VMexit;
> >> 
> >> This section here:
> >> As noted in Section 25.5.5.2, an execution of the
> >> EPTP-switching VM function that causes a VM exit (as specified
> >> above), uses the basic exit reason 59, indicating âVMFUNCâ.
> >> The length of the VMFUNC instruction is saved into the
> >> VM-exit instruction-length field. No additional VM-exit
> >> information is provided.
> >> 
> >> Although, it adds (as specified above), from testing, any vmexit that
> >> happens as a result of the execution of the vmfunc instruction always
> >> has exit reason 59.
> >> 
> >> IMO, the case David pointed out comes under "as a result of the
> >> execution of the vmfunc instruction", so I would prefer exiting
> >> with reason 59.
> >
> > Right, the exit reason is 59 for reasons that trigger a VM exit
> > (i.e. invalid EPTP value, the four below), but kvm_mmu_reload() checks
> > unrelated stuff.
> >
> > If the EPTP value is correct, then the switch should succeed.
> > If the EPTP is correct, but bogus, then the guest should get
> > EPT_MISCONFIG VM exit on its first access (when reading the
> > instruction).  Source: I added
> 
> My point is that we are using kvm_mmu_reload() to emulate eptp
> switching. If that emulation of vmfunc fails, it should exit with reason
> 59.

Yeah, we just disagree on what is a vmfunc failure.

> >   vmcs_write64(EPT_POINTER, vmcs_read64(EPT_POINTER) | (1ULL << 40));
> >
> > shortly before a VMLAUNCH on L0. :)
> 
> What happens if this ept pointer is actually in the eptp list and the guest
> switches to it using vmfunc ? I think it will exit with reason 59.

I think otherwise, because it doesn't cause a VM entry failure on
bare-metal (and SDM says that we get a VM exit only if there would be a
VM entry failure).
I expect the vmfunc to succeed and to get a EPT_MISCONFIG right after.
(Experiment pending :])

> > I think that we might be emulating this case incorrectly and throwing
> > triple faults when it should be VM exits in vcpu_run().
> 
> No, I agree with not throwing a triple fault. We should clear it out.
> But we should emulate a vmfunc vmexit back to L1 when kvm_mmu_load fails.

Here we disagree.  I think that it's a bug do the VM exit, so we can
just keep the original bug -- we want to eventually fix it and it's no
worse till then.