Messages in this thread | | | From | Paolo Bonzini <> | Subject | Re: [PATCH v2 2/2] KVM: SVM: Use a separate vmcb for the nested L2 guest | Date | Fri, 13 Nov 2020 18:58:42 +0100 |
| |
On 11/10/20 20:48, Cathy Avery wrote: > @@ -432,6 +432,16 @@ int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa, > int ret; > > svm->nested.vmcb = vmcb_gpa; > + > + WARN_ON(svm->vmcb == svm->nested.vmcb02); > + > + svm->nested.vmcb02->control = svm->vmcb01->control;
This assignment of the control area should be in nested_prepare_vmcb_control, which is already filling in most of vmcb02->control.
Right now, we save a copy_vmcb_control-area in nested_svm_vmexit so it evens out.
Later, it should be possible to remove most of the assignments from nested_prepare_vmcb_control.
> + svm->nested.vmcb02->save.cr4 = svm->vmcb01->save.cr4;
I cannot understand this statement.
> + nested_svm_vmloadsave(svm->vmcb01, svm->nested.vmcb02);
This is because the vmsave just after the vmexit has moved the vmloadsave registers from vmcb12 to vmcb01, but the next vmload will use vmcb02.
> + svm->vmcb = svm->nested.vmcb02; > + svm->vmcb_pa = svm->nested.vmcb02_pa; > load_nested_vmcb_control(svm, &nested_vmcb->control); > nested_prepare_vmcb_save(svm, nested_vmcb); > nested_prepare_vmcb_control(svm);
> @@ -628,8 +620,10 @@ int nested_svm_vmexit(struct vcpu_svm *svm) > nested_vmcb->control.pause_filter_thresh = > svm->vmcb->control.pause_filter_thresh; > > - /* Restore the original control entries */ > - copy_vmcb_control_area(&vmcb->control, &hsave->control); > + nested_svm_vmloadsave(svm->nested.vmcb02, svm->vmcb01);
Same here: the next vmentry's vmload will move the vmloadsave registers from vmcb01 to vmcb12, but for now they are in vmcb02.
It's 16+16 memory-to-memory u64 copies. They mostly even out with the 14+14 copies that we don't have to do anymore on registers handled by VMRUN (es/cs/ss/ds/gdt/idt/rsp/rax---they don't have to be stashed away in hsave anymore). Also, we are able to reuse nested_svm_vmloadsave, which makes it overall a fair tradeoff... but it would have been worth a comment or two. :)
> + svm->nested.vmcb02->control = svm->vmcb01->control; > + svm->nested.vmcb02->save = svm->vmcb01->save; > + svm->vmcb01->save = save;
I would have moved these after the comment, matching the existing copy_vmcb_control_area and save-area assignment.
Also, the first save-area assignment should be (because the WARN_ON below must be removed)
svm->nested.vmcb02->save = svm->vmcb->save;
or
if (svm->vmcb == svm->vmcb01) svm->nested.vmcb02->save = svm->vmcb01->save;
I have applied the patch and fixed the conflicts, so when I have some time I will play a bit more with it and/or pass the updated version back to you.
In the meanwhile, perhaps you could write a new selftests testcase that tries to do KVM_SET_NESTED_STATE while in L2. It can be a simplified version of state-test that invokes KVM_GET_NESTED_STATE + KVM_SET_NESTED_STATE when it gets back to L0.
Paolo
| |