On Tue, 2021-08-17 at 18:50 +0200, Paolo Bonzini wrote: > On 17/08/21 18:40, Maxim Levitsky wrote: > > I proposed that on nested entry we leave the processor values in vmcb01, > > as is, and backup the guest visible values in say 'svm->nested.hsave.cr*' > > or something like that. > > Later on nested VM exit we restore vcpu.arch.cr* values from 'svm->nested.hsave.cr*' > > and leave the vmcb01 values alone. > > > > That isn't strictly related to nested migration state but it seemed > > to me that it would be also nice to have both guest visible > > and cpu visible values of L1 save state in migration state > > as well while we are at redefining it. > > But the CPU visible values for L1 are useless, aren't they? They are > computed on another system. So you have to compute them again on the > destination. I understand what you mean, and I agree with you. > > So this idea, which is strictly speaking an optimization of vmexit, is > unrelated from migration and I would leave it aside. > > > > So your proposal would basically be to: > > > > > > * do the equivalent of sync_vmcs02_to_vmcs12+sync_vmcs02_to_vmcs12_rare > > > on KVM_GET_NESTED_STATE > > > > > > * discard the current state on KVM_SET_NESTED_STATE. > > > > I did indeed overlook the fact that vmcb12 save area is not up to date, > > in fact I probably won't even want to read it from the guest memory > > at the KVM_GET_NESTED_STATE time. But it can be constructed from the > > KVM's guest visible CR* values, and values in the VMCB02, roughly like > > how sync_vmcs02_to_vmcs12, or how nested_svm_vmexit does it. > > Right. > > > The core of my proposal is that while it indeed makes the retrieval of the > > nested state a bit more complicated, but it makes restore of the nested state > > much simpler, since it can be treated as if we are just doing a nested entry, > > eliminating the various special cases we have to have in nested state load on SVM. > > This is true. > > > Security wise, a bug during retrieval, isn't as bad as a bug during loading of the > > state, so it makes sense to make the load of the state share as much code > > with normal nested entry. > > > > That means that the whole VMCB12 image can be checked as a whole and loaded > > replacing most of the existing cpu state, in the same manner to regular > > nested entry. > > > > This also makes nested state load less dependant on its ordering vs setting of > > the other cpu state. > > > > So what do you think? Is it worth it for me to write a RFC patch series for this? > > I have a slightly different idea. Do you think it would make sense to > use the current processor state to build the VMCB12? Such a prototype > would show what KVM_SET_NESTED_STATE would look like with your new blob > format. We could use that to see if it worth proceeding further, with > three possible answer: > > 1) the new KVM_SET_NESTED_STATE remains complex, so we scrap the idea > > 2) the new KVM_SET_NESTED_STATE is nice, and we decide it's already good > enough > > 3) the new KVM_SET_NESTED_STATE is nice, but there is enough ugliness > left that the new format seems worthwhile I like this idea! I will prepare a prototype for this soon. Thanks a lot for the help! Best regards, Maxim Levitsky > > Paolo >
