Re: [PATCH v5 06/26] KVM: nSVM: Triple fault if mapping VMCB12 fails on nested #VMEXIT

[Yosry Ahmed]

> > @@ -1146,8 +1136,16 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
> >       /* in case we halted in L2 */
> >       kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
> >
> > +     svm->nested.vmcb12_gpa = 0;
> > +
> > +     if (kvm_vcpu_map(vcpu, gpa_to_gfn(vmcb12_gpa), &map)) {
> > +             kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
> > +             return 1;
>
> Returning early isn't entirely correct.  In fact, I think it's worse than the
> current behavior in many aspects.
>
> By doing leave_guest_mode() and not switching back to vmcb01 and not putting
> vcpu->arch.mmu back to root_mmu, the vCPU will be in L1 but with vmcb02 and L2's
> MMU active.

Hmm yeah, the same problem also exists in
nested_svm_vmrun_error_vmexit() after "KVM: nSVM: Restrict mapping
VMCB12 on nested VMRUN". In that path, we only need to map vmcb12 to
zero event_inj in __nested_svm_vmexit(). We can probably move them to
the callers (nested_svm_vmrun_error_vmexit() and nested_svm_vmexit())
to make it easier to skip if mapping fails.

>
> The idea I can come up with is to isolate the vmcb12 writes (which is suprisingly
> straightforward), and then simply skip the vmcb12 updates.  E.g.
>
> ---
[..]
> @@ -1184,14 +1168,53 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
>         if (guest_cpu_cap_has(vcpu, X86_FEATURE_NRIPS))
>                 vmcb12->control.next_rip  = vmcb02->control.next_rip;
>
> +       if (nested_vmcb12_has_lbrv(vcpu))
> +               svm_copy_lbrs(&vmcb12->save, &vmcb02->save);
> +
>         vmcb12->control.int_ctl           = svm->nested.ctl.int_ctl;
>         vmcb12->control.event_inj         = svm->nested.ctl.event_inj;
>         vmcb12->control.event_inj_err     = svm->nested.ctl.event_inj_err;
>
> +       trace_kvm_nested_vmexit_inject(vmcb12->control.exit_code,
> +                                      vmcb12->control.exit_info_1,
> +                                      vmcb12->control.exit_info_2,
> +                                      vmcb12->control.exit_int_info,
> +                                      vmcb12->control.exit_int_info_err,
> +                                      KVM_ISA_SVM);
> +}
> +
> +int nested_svm_vmexit(struct vcpu_svm *svm)
> +{
> +       struct kvm_vcpu *vcpu = &svm->vcpu;
> +       struct vmcb *vmcb01 = svm->vmcb01.ptr;
> +       struct vmcb *vmcb02 = svm->nested.vmcb02.ptr;
> +       struct vmcb *vmcb12;
> +       struct kvm_host_map map;
> +       int rc;
> +
> +       if (!kvm_vcpu_map(vcpu, gpa_to_gfn(svm->nested.vmcb12_gpa), &map)) {
> +               vmcb12 = map.hva;

Maybe also kvm_vcpu_map() mapping call to
nested_svm_vmexit_update_vmcb12() and inject a tripe fault if it
fails? Probably plays nicer with "KVM: nSVM: Restrict mapping VMCB12
on nested VMRUN".

Otherwise it looks good to me.

Should I send a new version to add all the changes?