On 04/26/2018 11:55 AM, Jan Beulich wrote:
>>>> On 26.04.18 at 17:20, <boris.ostrov...@oracle.com> 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.
-boris
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
