Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Fri, Apr 17, 2020 at 06:29:23PM -0700, Krish Sadhukhan wrote:
> 
> On 4/16/20 2:18 AM, Paolo Bonzini wrote:
> >On 15/04/20 22:18, Jim Mattson wrote:
> >>>Has anyone worked through all the flows to verify this won't break any
> >>>assumptions with respect to enable_unrestricted_guest?  I would be
> >>>(pleasantly) surprised if this was sufficient to run L2 without
> >>>unrestricted guest when it's enabled for L1, e.g. vmx_set_cr0() looks
> >>>suspect.
> >>I think you're right to be concerned.
> >Thirded, but it shouldn't be too hard.  Basically,
> >enable_unrestricted_guest must be moved into loaded_vmcs for this to
> >work.  It may be more work to write the test cases for L2 real mode <->
> >protected mode switch, which do not entirely fit into the vmx_tests.c
> >framework (but with the v2 tests it should not be hard to adapt).
> 
> 
> OK, I will move enable_unrestricted_guest  to loaded_vmcs.

Hmm, enable_unrestricted_guest doesn't _need_ to be moved to loaded_vmcs,
L1 can never diverge from enable_unrestricted_guest.  E.g. the main control
variable can stay global, we just need a flag in nested_vmx to override the
main control.  A simple wrapper can then take care of the check, e.g.

  static inline bool is_unrestricted_guest(struct kvm_vcpu *vcpu)
  {
	return enable_unrestricted_guest && (!is_guest_mode(vcpu) ||
	       to_vmx(vcpu)->nested.unrestricted_guest);
  }

Putting the flag in loaded_vmcs might be more performant?  My guess is it'd
be in the noise, at which point I'd rather have it be clear the override is
only possible/necessary for nested guests.

> I also see that enable_ept controls the setting of
> enable_unrestricted_guest. Perhaps both need to be moved to loaded_vmcs ?

No, letting L1 disable EPT in L0 would be pure insanity, and the overall
paging mode of L2 is already reflected in the MMU.

The dependency on EPT is that VMX requires paging of some form and
unrestricted guest allows entering non-root with CR0.PG=0, i.e. requires EPT
be enabled.

> About testing, I am thinking the test will first vmlaunch L2 in real mode or
> in protected mode, then vmexit on vmcall and then vmresume in the other
> mode. Is that how the test should flow ?
> 
> >
> >Paolo
> >