Re: [Xen-devel] [PATCH] SVM: re-work VMCB sync-ing

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.04.18 at 20:29, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 04/27/2018 11:52 AM, Jan Beulich wrote:
>>>>> On 26.04.18 at 19:27, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> On 04/26/2018 11:55 AM, Jan Beulich wrote:
>>>>>>> On 26.04.18 at 17:20, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>> On 04/26/2018 09:33 AM, Jan Beulich wrote:
>>>>>>>> -static void svm_sync_vmcb(struct vcpu *v)
>>>>>>>> +static void svm_sync_vmcb(struct vcpu *v, enum vmcb_sync_state 
>>>>>>>> new_state)
>>>>>>>>  {
>>>>>>>>      struct arch_svm_struct *arch_svm = &v->arch.hvm_svm;
>>>>>>>>  
>>>>>>>> -    if ( arch_svm->vmcb_in_sync )
>>>>>>>> -        return;
>>>>>>>> -
>>>>>>>> -    arch_svm->vmcb_in_sync = 1;
>>>>>>>> +    if ( arch_svm->vmcb_sync_state == vmcb_needs_vmsave )
>>>>>>>> +        svm_vmsave(arch_svm->vmcb);
>>>>>>>>  
>>>>>>>> -    svm_vmsave(arch_svm->vmcb);
>>>>>>>> +    if ( arch_svm->vmcb_sync_state != vmcb_needs_vmload )
>>>>>>>> +        arch_svm->vmcb_sync_state = new_state;
>>>>>>> This is slightly awkward for a couple of reasons.  First, passing
>>>>>>> vmcb_in_sync in forget the fact that a vmload is needed.
>>>>>> Certainly not - that's the purpose of the if() around it.
>>>>>>
>>>>>>> In my patch, I introduced svm_sync_vmcb_for_update(), rather than
>>>>>>> requiring a parameter to be passed in.  I think this is a better API,
>>>>>>> and it shrinks the size of the patch.
>>>>>> I'm not convinced of the "better", and even less so of the "shrinks". But
>>>>>> I'll wait to see what the SVM maintainers say.
>>>>> I think a single function is better. In fact, I was wondering whether
>>>>> svm_vmload() could also be folded into svm_sync_vmcb() since it is also
>>>>> a syncing operation.
>>>> That doesn't look like it would produce a usable interface: How would
>>>> you envision the state transition to be specified by the caller? Right
>>>> now the intended new state gets passed in, but in your model
>>>> vmcb_in_sync could mean either vmload or vmsave is needed. The
>>>> two svm_vmload() uses right now would pass that value in addition
>>>> to the svm_sync_vmcb() calls already doing so. And the function
>>>> couldn't tell what to do from the current state (if it's
>>>> vmcb_needs_vmload, a load is only needed in the cases where
>>>> svm_vmload() is called right now). Adding a 3rd parameter or a
>>>> second enum 
>>> I was thinking about another enum value, e.g. sync_to_cpu (and
>>> sync_to_vmcb replacing vmcb_needs_vmsave).
>>>
>>> This will allow us to hide (v->arch.hvm_svm.vmcb_sync_state ==
>>> vmcb_needs_vmload) test.
>> I'm still not entirely clear how you want that to look like. At the example
>> of svm_get_segment_register() and svm_set_segment_register(), how
>> would the svm_sync_vmcb() calls look like? I.e. how do you distinguish
>> the sync_to_vmcb/transition-to-clean case from the
>> sync_to_vmcb/transition-to-dirty one?
> 
> 
> Something like
> 
> static void svm_sync_vmcb(struct vcpu *v, enum vmcb_sync_state new_state)
> {
>      struct arch_svm_struct *arch_svm = &v->arch.hvm_svm;
> 
>      
>     if ( new_state == vmcb_sync_to_cpu )

But that's not a state, but a transition. An enum should consist of only
states or only transitions. I'll produce a variant of this as v2, but I'm
afraid you won't be overly happy with it.

Jan

>     {
>       if (v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload )
>         {     
>             svm_vmload(vmcb);
>             v->arch.hvm_svm.vmcb_sync_state = vmcb_in_sync;
>         }
>       return;
>     }
> 
>     if ( arch_svm->vmcb_sync_state == vmcb_needs_vmsave )
>         svm_vmsave(arch_svm->vmcb);
>  
>     if ( arch_svm->vmcb_sync_state != vmcb_needs_vmload )
>         arch_svm->vmcb_sync_state = new_state;
>  }
> 
> 
> -boris





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