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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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


Hmm.. Right. I don't know why it appeared less than obvious to me then
when I looked at it last time.

-boris

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