Re: [Xen-devel] [PATCH v2 1/2] SVM: re-work VMCB sync-ing

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.04.18 at 19:50, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 04/30/2018 01:07 PM, Andrew Cooper wrote:
>> On 30/04/18 12:37, Jan Beulich wrote:
>>> While the main problem to be addressed here is the issue of what so far
>>> was named "vmcb_in_sync" starting out with the wrong value (should have
>>> been true instead of false, to prevent performing a VMSAVE without ever
>>> having VMLOADed the vCPU's state), go a step further and make the
>>> sync-ed state a tristate: CPU and memory may be in sync or an update
>>> may be required in either direction. Rename the field and introduce an
>>> enum. Callers of svm_sync_vmcb() now indicate the intended new state
>>> (with a slight "anomaly" when requesting VMLOAD: we could store
>>> vmcb_needs_vmsave in those cases as the callers request, but the VMCB
>>> really is in sync at that point, and hence there's no need to VMSAVE in
>>> case we don't make it out to guest context), and all syncing goes
>>> through that function.
>>>
>>> With that, there's no need to VMLOAD the state perhaps multiple times;
>>> all that's needed is loading it once before VM entry.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> ---
>>> v2: Also handle VMLOAD in svm_sync_vmcb(). Add comment to enum
>>>     vmcb_sync_state.
>> -1 from me.  This is even more confusing to use than v1.
>>
>> It is not obvious at all that using svm_sync_vmcb(v, vmcb_needs_vmsave);
>> means "vmload", and its actively wrong that the state doesn't remain
>> in-sync.
> 
> It does become in-sync:
> 
> 
> +    if ( new_state == vmcb_needs_vmsave )
> +    {
> +        ASSERT(arch_svm->vmcb_sync_state == vmcb_needs_vmload);
> +        svm_vmload(arch_svm->vmcb);
> +        arch_svm->vmcb_sync_state = vmcb_in_sync;
> +    }
> +    else
> 
> (although Jan is questioning whether to drop that change in the comments to 
> patch 2, if I understood him correctly)

Indeed - in patch 2 this could be made go away. Hence the posting of patch 2
at this point in time in the first place (otherwise I would have waited until 
4.12
has opened).

In any event - I need some sort of indication of a way forward here.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel