On Thu, Aug 08, 2019 at 07:30:51PM +0200, Vitaly Kuznetsov wrote:
> Just like we do with other intercepts, in vmrun_interception() we should be
> doing kvm_skip_emulated_instruction() and not just RIP += 3. Also, it is
> wrong to increment RIP before nested_svm_vmrun() as it can result in
> kvm_inject_gp().
>
> We can't call kvm_skip_emulated_instruction() after nested_svm_vmrun() so
> move it inside. To preserve the return value from it nested_svm_vmrun()
> needs to start returning an int.
>
> Suggested-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> ---
> arch/x86/kvm/svm.c | 27 ++++++++++++---------------
> 1 file changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 43bc4a5e4948..6c4046eb26b3 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -3586,9 +3586,9 @@ static void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
> mark_all_dirty(svm->vmcb);
> }
>
> -static bool nested_svm_vmrun(struct vcpu_svm *svm)
> +static int nested_svm_vmrun(struct vcpu_svm *svm)
> {
> - int rc;
> + int rc, ret;
> struct vmcb *nested_vmcb;
> struct vmcb *hsave = svm->nested.hsave;
> struct vmcb *vmcb = svm->vmcb;
> @@ -3598,12 +3598,15 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
> vmcb_gpa = svm->vmcb->save.rax;
>
> rc = kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb_gpa), &map);
> - if (rc) {
> - if (rc == -EINVAL)
> - kvm_inject_gp(&svm->vcpu, 0);
> - return false;
> + if (rc == -EINVAL) {
> + kvm_inject_gp(&svm->vcpu, 0);
> + return 1;
> }
>
> + ret = kvm_skip_emulated_instruction(&svm->vcpu);
> + if (rc)
> + return ret;
This should probably have a comment, the 'if (rc)' looks so wrong at first
glance. Maybe not the best suggestion on my part...
Alternatively, this sequence is more obvious and at worst adds a few bytes
to the code footprint.
if (ret == EINVAL) {
kvm_inject_gp(&svm->vcpu, 0);
return 1;
} else if (ret) {
return kvm_skip_emulated_instruction(&svm->vcpu);
}
ret = kvm_skip_emulated_instruction(&svm->vcpu);
> +
> nested_vmcb = map.hva;
>
> if (!nested_vmcb_checks(nested_vmcb)) {
> @@ -3614,7 +3617,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
>
> kvm_vcpu_unmap(&svm->vcpu, &map, true);
>
> - return false;
> + return ret;
> }
>
> trace_kvm_nested_vmrun(svm->vmcb->save.rip, vmcb_gpa,
> @@ -3667,7 +3670,7 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
> nested_svm_vmexit(svm);
> }
>
> - return true;
> + return ret;
> }
>
> static void nested_svm_vmloadsave(struct vmcb *from_vmcb, struct vmcb *to_vmcb)
> @@ -3743,13 +3746,7 @@ static int vmrun_interception(struct vcpu_svm *svm)
> if (nested_svm_check_permissions(svm))
> return 1;
>
> - /* Save rip after vmrun instruction */
> - kvm_rip_write(&svm->vcpu, kvm_rip_read(&svm->vcpu) + 3);
> -
> - if (!nested_svm_vmrun(svm))
> - return 1;
> -
> - return 1;
> + return nested_svm_vmrun(svm);
> }
>
> static int stgi_interception(struct vcpu_svm *svm)
> --
> 2.20.1
>
