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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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 doesn't look like a good idea either. But maybe I'm not
seeing your intentions; I'm certainly open to (more specific)
suggestions.

Jan



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