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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.05.18 at 16:45, <boris.ostrovsky@xxxxxxxxxx> wrote:
> 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.)

Hmm, interesting. To me it would seem to improve things.

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

How that? Switching to vmcb_needs_vmload also implies a VMSAVE, after
all (if none has happened before).

Jan



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