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

[Anthony Liguori <anthony@codemonkey.ws>]

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
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/x86/kvm/svm.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index eb301fe..10ad02b 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1242,6 +1242,62 @@ static int nested_svm_do(struct vcpu_svm *svm,
>  	return retval;
>  }
>  
> +static int nested_svm_vmloadsave(struct vmcb *from_vmcb, struct vmcb *to_vmcb)
> +{
> +	to_vmcb->save.fs = from_vmcb->save.fs;
> +	to_vmcb->save.gs = from_vmcb->save.gs;
> +	to_vmcb->save.tr = from_vmcb->save.tr;
> +	to_vmcb->save.ldtr = from_vmcb->save.ldtr;
> +	to_vmcb->save.kernel_gs_base = from_vmcb->save.kernel_gs_base;
> +	to_vmcb->save.star = from_vmcb->save.star;
> +	to_vmcb->save.lstar = from_vmcb->save.lstar;
> +	to_vmcb->save.cstar = from_vmcb->save.cstar;
> +	to_vmcb->save.sfmask = from_vmcb->save.sfmask;
> +	to_vmcb->save.sysenter_cs = from_vmcb->save.sysenter_cs;
> +	to_vmcb->save.sysenter_esp = from_vmcb->save.sysenter_esp;
> +	to_vmcb->save.sysenter_eip = from_vmcb->save.sysenter_eip;
> +
> +	return 1;
> +}
> +
> +static int nested_svm_vmload(struct vcpu_svm *svm, void *nested_vmcb,
> +			     void *arg2, void *opaque)
> +{
> +	return nested_svm_vmloadsave((struct vmcb *)nested_vmcb, svm->vmcb);
> +}
> +
> +static int nested_svm_vmsave(struct vcpu_svm *svm, void *nested_vmcb,
> +			     void *arg2, void *opaque)
> +{
> +	return nested_svm_vmloadsave(svm->vmcb, (struct vmcb *)nested_vmcb);
> +}
> +
> +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.

Regards,

Anthony Liguori