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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 05/02/2018 03:11 AM, Jan Beulich wrote:
>>>> 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.

I think the extra optimization that you suggested in patch 2 would make
things a bit less obvious so I'd be inclined not to do that (but maybe a
comment in svm_sync_vmcb() that we are doing it only for clarity might
be useful.)

I also see a point in Andrew's observation that vmcb_needs_vmsave
implying a vmload may not be not immediately obvious so if he feels
strongly about that I will be OK with going back to v1.

-boris



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