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.
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
Paolo
