Re: [PATCH 5/9] Add VMLOAD and VMSAVE handlers v5

[Anthony Liguori <anthony@codemonkey.ws>]

Alexander Graf wrote:
>
> On 30.10.2008, at 19:06, Anthony Liguori wrote:
>
>> Alexander Graf wrote:
>>> This implements the VMLOAD and VMSAVE instructions, that usually 
>>> surround
>>> the VMRUN instructions. Both instructions load / restore the same 
>>> elements,
>>> so we only need to implement them once.
>>>
>>> v2 fixes CPL checking and replaces memcpy by assignments
>>> v3 makes use of the new permission checking
>>>
>
> snip
>
>>>
>>> +static int vmload_interception(struct vcpu_svm *svm, struct kvm_run 
>>> *kvm_run)
>>> +{
>>> +    if (nested_svm_check_permissions(svm))
>>> +        return 1;
>>> +
>>> +    svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
>>> +    skip_emulated_instruction(&svm->vcpu);
>>> +
>>> +    nested_svm_do(svm, svm->vmcb->save.rax, 0, NULL, 
>>> nested_svm_vmload);
>>>
>>
>> The whole trampoline thing seems awkward to me.  I think it would be 
>> more reasonable to just open code this routine and use helper 
>> functions when appropriate.
>
> What exactly do you mean? Do you prefer to duplicate the 
> guest-page-mapping code several times?

You do:

foo(x, y, cb);

where foo is:

// something with x
// something with y

cb();

// something else

And then do:

foo(1, 2, bar);

An alternative module would be:

foo_helper1(x)
// something with x

foo_helper2(y)
// something with y

foo_cleanup()
// something else

And then:

foo_helper1(1);
foo_helper2(2)
bar()
foo_cleanup();

And since you only sometimes pass both x, y parameters, I do think this 
later style would result in more readable code.

Regards,

Anthony Liguori

> Alex
>