| Message ID | 20200409114926.1407442-1-ubizjak@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SVM: Fix __svm_vcpu_run declaration. | expand |
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) > {
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
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.
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 >
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
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.
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 >
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
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) {
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(-)