[PATCH] KVM: SVM: Fix __svm_vcpu_run declaration.

[PATCH] KVM: SVM: Fix __svm_vcpu_run declaration.

[Uros Bizjak]

The function returns no value.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Fixes: 199cd1d7b534 ("KVM: SVM: Split svm_vcpu_run inline assembly to separate file")
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
---
 arch/x86/kvm/svm/svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 2be5bbae3a40..061d19e69c73 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3276,7 +3276,7 @@ static void svm_cancel_injection(struct kvm_vcpu *vcpu)
 	svm_complete_interrupts(svm);
 }
 
-bool __svm_vcpu_run(unsigned long vmcb_pa, unsigned long *regs);
+void __svm_vcpu_run(unsigned long vmcb_pa, unsigned long *regs);
 
 static void svm_vcpu_run(struct kvm_vcpu *vcpu)
 {
-- 
2.25.2

Re: [PATCH] KVM: SVM: Fix __svm_vcpu_run declaration.

[Xu, Like]

Hi Bizjak,

On 2020/4/9 19:49, Uros Bizjak wrote:
> The function returns no value.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Fixes: 199cd1d7b534 ("KVM: SVM: Split svm_vcpu_run inline assembly to separate file")
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> ---
>   arch/x86/kvm/svm/svm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 2be5bbae3a40..061d19e69c73 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3276,7 +3276,7 @@ static void svm_cancel_injection(struct kvm_vcpu *vcpu)
>   	svm_complete_interrupts(svm);
>   }
>   
> -bool __svm_vcpu_run(unsigned long vmcb_pa, unsigned long *regs);
Just curious if __svm_vcpu_run() will fail to enter SVM guest mode,
and a return value could indicate that nothing went wrong rather than 
blindly keeping silent.

Thanks,
Like Xu
> +void __svm_vcpu_run(unsigned long vmcb_pa, unsigned long *regs);
>   
>   static void svm_vcpu_run(struct kvm_vcpu *vcpu)
>   {

Re: [PATCH] KVM: SVM: Fix __svm_vcpu_run declaration.

[Paolo Bonzini]

On 09/04/20 17:11, Xu, Like wrote:
>>
>>   -bool __svm_vcpu_run(unsigned long vmcb_pa, unsigned long *regs);
> Just curious if __svm_vcpu_run() will fail to enter SVM guest mode,
> and a return value could indicate that nothing went wrong rather than
> blindly keeping silent.

That's already available in the exit code (which is 0xffffffff when
vmentry fails).

Paolo

Re: [PATCH] KVM: SVM: Fix __svm_vcpu_run declaration.

[Uros Bizjak]

On Thu, Apr 9, 2020 at 5:11 PM Xu, Like <like.xu@intel.com> wrote:
>
> Hi Bizjak,
>
> On 2020/4/9 19:49, Uros Bizjak wrote:
> > The function returns no value.
> >
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Fixes: 199cd1d7b534 ("KVM: SVM: Split svm_vcpu_run inline assembly to separate file")
> > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> > ---
> >   arch/x86/kvm/svm/svm.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 2be5bbae3a40..061d19e69c73 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -3276,7 +3276,7 @@ static void svm_cancel_injection(struct kvm_vcpu *vcpu)
> >       svm_complete_interrupts(svm);
> >   }
> >
> > -bool __svm_vcpu_run(unsigned long vmcb_pa, unsigned long *regs);
> Just curious if __svm_vcpu_run() will fail to enter SVM guest mode,
> and a return value could indicate that nothing went wrong rather than
> blindly keeping silent.

vmload, vmrun and vmsave do not return anything in flags or registers,
so we can't detect anything at this point, modulo exception that is
handled below the respective instruction.

BTW: the change by itself does not change the generated code, the fake
return value from __svm_vcpu_run is already ignored. So, the change is
mostly cosmetic.

Uros.

Re: [PATCH] KVM: SVM: Fix __svm_vcpu_run declaration.

[Xu, Like]

Hi Paolo,

On 2020/4/9 23:18, Paolo Bonzini wrote:
> On 09/04/20 17:11, Xu, Like wrote:
>>>    -bool __svm_vcpu_run(unsigned long vmcb_pa, unsigned long *regs);
>> Just curious if __svm_vcpu_run() will fail to enter SVM guest mode,
>> and a return value could indicate that nothing went wrong rather than
>> blindly keeping silent.
> That's already available in the exit code (which is 0xffffffff when
> vmentry fails).
Yes, I assume the svm->vmcb->control.exit_code is referred.

What makes me confused is
why we need "vmx->exit_reason" and "vmx->fail"
for the same general purpose, but svm does not.

Thanks,
Like Xu
>
> Paolo
>

Re: [PATCH] KVM: SVM: Fix __svm_vcpu_run declaration.

[Paolo Bonzini]

On 09/04/20 17:42, Xu, Like wrote:
>>
> Yes, I assume the svm->vmcb->control.exit_code is referred.
> 
> What makes me confused is
> why we need "vmx->exit_reason" and "vmx->fail"
> for the same general purpose, but svm does not.

Because VMLAUNCH/VMRESUME can also report vmFailValid and vmFailInvalid
via the carry and zero flags, there is no equivalent of that for AMD
virtualization extensions.

Paolo

Re: [PATCH] KVM: SVM: Fix __svm_vcpu_run declaration.

[Xu, Like]

On 2020/4/9 23:27, Uros Bizjak wrote:
> On Thu, Apr 9, 2020 at 5:11 PM Xu, Like <like.xu@intel.com> wrote:
>> Hi Bizjak,
>>
>> On 2020/4/9 19:49, Uros Bizjak wrote:
>>> The function returns no value.
>>>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Fixes: 199cd1d7b534 ("KVM: SVM: Split svm_vcpu_run inline assembly to separate file")
>>> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
>>> ---
>>>    arch/x86/kvm/svm/svm.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>> index 2be5bbae3a40..061d19e69c73 100644
>>> --- a/arch/x86/kvm/svm/svm.c
>>> +++ b/arch/x86/kvm/svm/svm.c
>>> @@ -3276,7 +3276,7 @@ static void svm_cancel_injection(struct kvm_vcpu *vcpu)
>>>        svm_complete_interrupts(svm);
>>>    }
>>>
>>> -bool __svm_vcpu_run(unsigned long vmcb_pa, unsigned long *regs);
>> Just curious if __svm_vcpu_run() will fail to enter SVM guest mode,
>> and a return value could indicate that nothing went wrong rather than
>> blindly keeping silent.
> vmload, vmrun and vmsave do not return anything in flags or registers,
> so we can't detect anything at this point, modulo exception that is
> handled below the respective instruction.
If we check the __vmx_vcpu_run(), we have something like:

     /* VM-Fail.  Out-of-line to avoid a taken Jcc after VM-Exit. */
2:    mov $1, %eax
     jmp 1b

so do we need it in the __svm_vcpu_run() or why not ?
>
> BTW: the change by itself does not change the generated code, the fake
> return value from __svm_vcpu_run is already ignored. So, the change is
> mostly cosmetic.
Yes, the generated code is not affected and the change is reasonable.
>
> Uros.

Re: [PATCH] KVM: SVM: Fix __svm_vcpu_run declaration.

[Xu, Like]

On 2020/4/9 23:55, Paolo Bonzini wrote:
> On 09/04/20 17:42, Xu, Like wrote:
>> Yes, I assume the svm->vmcb->control.exit_code is referred.
>>
>> What makes me confused is
>> why we need "vmx->exit_reason" and "vmx->fail"
>> for the same general purpose, but svm does not.
> Because VMLAUNCH/VMRESUME can also report vmFailValid and vmFailInvalid
> via the carry and zero flags, there is no equivalent of that for AMD
> virtualization extensions.
Oh, this makes sense to me. Thanks!
Let me confirm it from both specifications.
>
> Paolo
>

Re: [PATCH] KVM: SVM: Fix __svm_vcpu_run declaration.

[Paolo Bonzini]

On 09/04/20 13:49, Uros Bizjak wrote:
> The function returns no value.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Fixes: 199cd1d7b534 ("KVM: SVM: Split svm_vcpu_run inline assembly to separate file")
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> ---
>  arch/x86/kvm/svm/svm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 2be5bbae3a40..061d19e69c73 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3276,7 +3276,7 @@ static void svm_cancel_injection(struct kvm_vcpu *vcpu)
>  	svm_complete_interrupts(svm);
>  }
>  
> -bool __svm_vcpu_run(unsigned long vmcb_pa, unsigned long *regs);
> +void __svm_vcpu_run(unsigned long vmcb_pa, unsigned long *regs);
>  
>  static void svm_vcpu_run(struct kvm_vcpu *vcpu)
>  {
> 

Queued, thanks.

Paolo