* [PATCH] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT @ 2020-05-20 16:51 Laurent Dufour 2020-05-20 17:32 ` Greg Kurz 0 siblings, 1 reply; 8+ messages in thread From: Laurent Dufour @ 2020-05-20 16:51 UTC (permalink / raw) To: kvm-ppc, linuxppc-dev, linux-kernel, paulus; +Cc: mpe, sukadev, linuxram The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls") added checks of secure bit of SRR1 to filter out the Hcall reserved to the Ultravisor. However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the context of the VM calling UV_ESM. This allows the Hypervisor to return to the guest without going through the Ultravisor. Thus the Secure bit of SRR1 is not set in that particular case. In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is not set in that case. Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls") Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> --- arch/powerpc/kvm/book3s_hv.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c index 93493f0cbfe8..eb1f96cb7b72 100644 --- a/arch/powerpc/kvm/book3s_hv.c +++ b/arch/powerpc/kvm/book3s_hv.c @@ -1099,9 +1099,7 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) ret = kvmppc_h_svm_init_done(vcpu->kvm); break; case H_SVM_INIT_ABORT: - ret = H_UNSUPPORTED; - if (kvmppc_get_srr1(vcpu) & MSR_S) - ret = kvmppc_h_svm_init_abort(vcpu->kvm); + ret = kvmppc_h_svm_init_abort(vcpu->kvm); break; default: -- 2.26.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT 2020-05-20 16:51 [PATCH] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT Laurent Dufour @ 2020-05-20 17:32 ` Greg Kurz 2020-05-20 17:35 ` Laurent Dufour 2020-05-20 17:43 ` [PATCH v2] " Laurent Dufour 0 siblings, 2 replies; 8+ messages in thread From: Greg Kurz @ 2020-05-20 17:32 UTC (permalink / raw) To: Laurent Dufour Cc: kvm-ppc, linuxppc-dev, linux-kernel, paulus, mpe, sukadev, linuxram On Wed, 20 May 2020 18:51:10 +0200 Laurent Dufour <ldufour@linux.ibm.com> wrote: > The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* > Hcalls") added checks of secure bit of SRR1 to filter out the Hcall > reserved to the Ultravisor. > > However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the > context of the VM calling UV_ESM. This allows the Hypervisor to return to > the guest without going through the Ultravisor. Thus the Secure bit of SRR1 > is not set in that particular case. > > In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be > filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is > not set in that case. > Why not checking vcpu->kvm->arch.secure_guest then ? > Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls") > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > --- > arch/powerpc/kvm/book3s_hv.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c > index 93493f0cbfe8..eb1f96cb7b72 100644 > --- a/arch/powerpc/kvm/book3s_hv.c > +++ b/arch/powerpc/kvm/book3s_hv.c > @@ -1099,9 +1099,7 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) > ret = kvmppc_h_svm_init_done(vcpu->kvm); > break; > case H_SVM_INIT_ABORT: > - ret = H_UNSUPPORTED; > - if (kvmppc_get_srr1(vcpu) & MSR_S) > - ret = kvmppc_h_svm_init_abort(vcpu->kvm); or at least put a comment to explain why H_SVM_INIT_ABORT doesn't have the same sanity check as the other SVM hcalls. > + ret = kvmppc_h_svm_init_abort(vcpu->kvm); > break; > > default: ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT 2020-05-20 17:32 ` Greg Kurz @ 2020-05-20 17:35 ` Laurent Dufour 2020-05-20 17:43 ` [PATCH v2] " Laurent Dufour 1 sibling, 0 replies; 8+ messages in thread From: Laurent Dufour @ 2020-05-20 17:35 UTC (permalink / raw) To: Greg Kurz Cc: kvm-ppc, linuxppc-dev, linux-kernel, paulus, mpe, sukadev, linuxram Le 20/05/2020 à 19:32, Greg Kurz a écrit : > On Wed, 20 May 2020 18:51:10 +0200 > Laurent Dufour <ldufour@linux.ibm.com> wrote: > >> The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* >> Hcalls") added checks of secure bit of SRR1 to filter out the Hcall >> reserved to the Ultravisor. >> >> However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the >> context of the VM calling UV_ESM. This allows the Hypervisor to return to >> the guest without going through the Ultravisor. Thus the Secure bit of SRR1 >> is not set in that particular case. >> >> In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be >> filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is >> not set in that case. >> > > Why not checking vcpu->kvm->arch.secure_guest then ? I don't think that's the right place. > >> Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls") >> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> >> --- >> arch/powerpc/kvm/book3s_hv.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c >> index 93493f0cbfe8..eb1f96cb7b72 100644 >> --- a/arch/powerpc/kvm/book3s_hv.c >> +++ b/arch/powerpc/kvm/book3s_hv.c >> @@ -1099,9 +1099,7 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) >> ret = kvmppc_h_svm_init_done(vcpu->kvm); >> break; >> case H_SVM_INIT_ABORT: >> - ret = H_UNSUPPORTED; >> - if (kvmppc_get_srr1(vcpu) & MSR_S) >> - ret = kvmppc_h_svm_init_abort(vcpu->kvm); > > or at least put a comment to explain why H_SVM_INIT_ABORT > doesn't have the same sanity check as the other SVM hcalls. I agree that might help. I'll send a v2 with a comment there. > >> + ret = kvmppc_h_svm_init_abort(vcpu->kvm); >> break; >> >> default: > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT 2020-05-20 17:32 ` Greg Kurz 2020-05-20 17:35 ` Laurent Dufour @ 2020-05-20 17:43 ` Laurent Dufour 2020-05-20 18:23 ` Greg Kurz ` (2 more replies) 1 sibling, 3 replies; 8+ messages in thread From: Laurent Dufour @ 2020-05-20 17:43 UTC (permalink / raw) To: kvm-ppc, linuxppc-dev, linux-kernel, paulus; +Cc: groug, mpe, sukadev, linuxram The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls") added checks of secure bit of SRR1 to filter out the Hcall reserved to the Ultravisor. However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the context of the VM calling UV_ESM. This allows the Hypervisor to return to the guest without going through the Ultravisor. Thus the Secure bit of SRR1 is not set in that particular case. In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is not set in that case. Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls") Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> --- arch/powerpc/kvm/book3s_hv.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c index 93493f0cbfe8..6ad1a3b14300 100644 --- a/arch/powerpc/kvm/book3s_hv.c +++ b/arch/powerpc/kvm/book3s_hv.c @@ -1099,9 +1099,12 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) ret = kvmppc_h_svm_init_done(vcpu->kvm); break; case H_SVM_INIT_ABORT: - ret = H_UNSUPPORTED; - if (kvmppc_get_srr1(vcpu) & MSR_S) - ret = kvmppc_h_svm_init_abort(vcpu->kvm); + /* + * Even if that call is made by the Ultravisor, the SSR1 value + * is the guest context one, with the secure bit clear as it has + * not yet been secured. So we can't check it here. + */ + ret = kvmppc_h_svm_init_abort(vcpu->kvm); break; default: -- 2.26.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT 2020-05-20 17:43 ` [PATCH v2] " Laurent Dufour @ 2020-05-20 18:23 ` Greg Kurz 2020-05-21 6:08 ` Ram Pai 2020-05-27 4:16 ` Paul Mackerras 2 siblings, 0 replies; 8+ messages in thread From: Greg Kurz @ 2020-05-20 18:23 UTC (permalink / raw) To: Laurent Dufour Cc: kvm-ppc, linuxppc-dev, linux-kernel, paulus, mpe, sukadev, linuxram On Wed, 20 May 2020 19:43:08 +0200 Laurent Dufour <ldufour@linux.ibm.com> wrote: > The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* > Hcalls") added checks of secure bit of SRR1 to filter out the Hcall > reserved to the Ultravisor. > > However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the > context of the VM calling UV_ESM. This allows the Hypervisor to return to > the guest without going through the Ultravisor. Thus the Secure bit of SRR1 > is not set in that particular case. > > In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be > filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is > not set in that case. > > Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls") > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > arch/powerpc/kvm/book3s_hv.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c > index 93493f0cbfe8..6ad1a3b14300 100644 > --- a/arch/powerpc/kvm/book3s_hv.c > +++ b/arch/powerpc/kvm/book3s_hv.c > @@ -1099,9 +1099,12 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) > ret = kvmppc_h_svm_init_done(vcpu->kvm); > break; > case H_SVM_INIT_ABORT: > - ret = H_UNSUPPORTED; > - if (kvmppc_get_srr1(vcpu) & MSR_S) > - ret = kvmppc_h_svm_init_abort(vcpu->kvm); > + /* > + * Even if that call is made by the Ultravisor, the SSR1 value > + * is the guest context one, with the secure bit clear as it has > + * not yet been secured. So we can't check it here. > + */ > + ret = kvmppc_h_svm_init_abort(vcpu->kvm); > break; > > default: ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT 2020-05-20 17:43 ` [PATCH v2] " Laurent Dufour 2020-05-20 18:23 ` Greg Kurz @ 2020-05-21 6:08 ` Ram Pai 2020-05-27 4:16 ` Paul Mackerras 2 siblings, 0 replies; 8+ messages in thread From: Ram Pai @ 2020-05-21 6:08 UTC (permalink / raw) To: Laurent Dufour Cc: kvm-ppc, linuxppc-dev, linux-kernel, paulus, groug, mpe, sukadev On Wed, May 20, 2020 at 07:43:08PM +0200, Laurent Dufour wrote: > The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* > Hcalls") added checks of secure bit of SRR1 to filter out the Hcall > reserved to the Ultravisor. > > However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the > context of the VM calling UV_ESM. This allows the Hypervisor to return to > the guest without going through the Ultravisor. Thus the Secure bit of SRR1 > is not set in that particular case. > > In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be > filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is > not set in that case. > > Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls") > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> Reviewed-by: Ram Pai <linuxram@us.ibm.com> > --- > arch/powerpc/kvm/book3s_hv.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c > index 93493f0cbfe8..6ad1a3b14300 100644 > --- a/arch/powerpc/kvm/book3s_hv.c > +++ b/arch/powerpc/kvm/book3s_hv.c > @@ -1099,9 +1099,12 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) > ret = kvmppc_h_svm_init_done(vcpu->kvm); > break; > case H_SVM_INIT_ABORT: > - ret = H_UNSUPPORTED; > - if (kvmppc_get_srr1(vcpu) & MSR_S) > - ret = kvmppc_h_svm_init_abort(vcpu->kvm); > + /* > + * Even if that call is made by the Ultravisor, the SSR1 value > + * is the guest context one, with the secure bit clear as it has > + * not yet been secured. So we can't check it here. > + */ Frankly speaking, the comment above when read in isolation; i.e without the delete code above, feels out of place. The reasoning for change is anyway captured in the changelog. So, I think, we should delete this comment. Also the comment above assumes the Ultravisor will call H_SVM_INIT_ABORT with SRR1(S) bit not set; which may or may not be true. Regardless of who and how H_SVM_INIT_ABORT is called, we should just call kvmppc_h_svm_init_abort() and let it deal with the complexities. RP ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT 2020-05-20 17:43 ` [PATCH v2] " Laurent Dufour 2020-05-20 18:23 ` Greg Kurz 2020-05-21 6:08 ` Ram Pai @ 2020-05-27 4:16 ` Paul Mackerras 2020-05-27 9:23 ` Laurent Dufour 2 siblings, 1 reply; 8+ messages in thread From: Paul Mackerras @ 2020-05-27 4:16 UTC (permalink / raw) To: Laurent Dufour Cc: kvm-ppc, linuxppc-dev, linux-kernel, groug, mpe, sukadev, linuxram On Wed, May 20, 2020 at 07:43:08PM +0200, Laurent Dufour wrote: > The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* > Hcalls") added checks of secure bit of SRR1 to filter out the Hcall > reserved to the Ultravisor. > > However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the > context of the VM calling UV_ESM. This allows the Hypervisor to return to > the guest without going through the Ultravisor. Thus the Secure bit of SRR1 > is not set in that particular case. > > In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be > filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is > not set in that case. > > Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls") > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> Thanks, applied to my kvm-ppc-next branch. I expanded the comment in the code a little. Paul. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT 2020-05-27 4:16 ` Paul Mackerras @ 2020-05-27 9:23 ` Laurent Dufour 0 siblings, 0 replies; 8+ messages in thread From: Laurent Dufour @ 2020-05-27 9:23 UTC (permalink / raw) To: Paul Mackerras Cc: kvm-ppc, linuxppc-dev, linux-kernel, groug, mpe, sukadev, linuxram Le 27/05/2020 à 06:16, Paul Mackerras a écrit : > On Wed, May 20, 2020 at 07:43:08PM +0200, Laurent Dufour wrote: >> The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* >> Hcalls") added checks of secure bit of SRR1 to filter out the Hcall >> reserved to the Ultravisor. >> >> However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the >> context of the VM calling UV_ESM. This allows the Hypervisor to return to >> the guest without going through the Ultravisor. Thus the Secure bit of SRR1 >> is not set in that particular case. >> >> In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be >> filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is >> not set in that case. >> >> Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls") >> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > > Thanks, applied to my kvm-ppc-next branch. I expanded the comment in > the code a little. Thanks, the comment is more explicit now. Laurent. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-05-27 9:23 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-20 16:51 [PATCH] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT Laurent Dufour 2020-05-20 17:32 ` Greg Kurz 2020-05-20 17:35 ` Laurent Dufour 2020-05-20 17:43 ` [PATCH v2] " Laurent Dufour 2020-05-20 18:23 ` Greg Kurz 2020-05-21 6:08 ` Ram Pai 2020-05-27 4:16 ` Paul Mackerras 2020-05-27 9:23 ` Laurent Dufour
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).