diff mbox series

[v10,7/8] KVM: PPC: Implement H_SVM_INIT_ABORT hcall

Message ID 20191104041800.24527-8-bharata@linux.ibm.com (mailing list archive)
State Not Applicable
Headers show
Series KVM: PPC: Driver to manage pages of secure guest | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch powerpc/merge (b9ba205b97bda75388e4014914ae0bdc0022464c)
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 166 lines checked

Commit Message

Bharata B Rao Nov. 4, 2019, 4:17 a.m. UTC
From: Sukadev Bhattiprolu <sukadev@linux.ibm.com>

Implement the H_SVM_INIT_ABORT hcall which the Ultravisor can use to
abort an SVM after it has issued the H_SVM_INIT_START and before the
H_SVM_INIT_DONE hcalls. This hcall could be used when Ultravisor
encounters security violations or other errors when starting an SVM.

Note that this hcall is different from UV_SVM_TERMINATE ucall which
is used by HV to terminate/cleanup an SVM.

In case of H_SVM_INIT_ABORT, we should page-out all the pages back to
HV (i.e., we should not skip the page-out). Otherwise the VM's pages,
possibly including its text/data would be stuck in secure memory.
Since the SVM did not go secure, its MSR_S bit will be clear and the
VM wont be able to access its pages even to do a clean exit.

Based on patches and discussion with Ram Pai and Bharata Rao.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
Signed-off-by: Ram Pai <linuxram@linux.ibm.com>
Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
---
 Documentation/powerpc/ultravisor.rst        | 39 +++++++++++++++++++++
 arch/powerpc/include/asm/hvcall.h           |  1 +
 arch/powerpc/include/asm/kvm_book3s_uvmem.h |  6 ++++
 arch/powerpc/include/asm/kvm_host.h         |  1 +
 arch/powerpc/kvm/book3s_hv.c                |  3 ++
 arch/powerpc/kvm/book3s_hv_rmhandlers.S     | 23 ++++++++++--
 arch/powerpc/kvm/book3s_hv_uvmem.c          | 29 +++++++++++++++
 7 files changed, 100 insertions(+), 2 deletions(-)

Comments

Paul Mackerras Nov. 11, 2019, 4:19 a.m. UTC | #1
On Mon, Nov 04, 2019 at 09:47:59AM +0530, Bharata B Rao wrote:
> From: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
> 
> Implement the H_SVM_INIT_ABORT hcall which the Ultravisor can use to
> abort an SVM after it has issued the H_SVM_INIT_START and before the
> H_SVM_INIT_DONE hcalls. This hcall could be used when Ultravisor
> encounters security violations or other errors when starting an SVM.
> 
> Note that this hcall is different from UV_SVM_TERMINATE ucall which
> is used by HV to terminate/cleanup an SVM.
> 
> In case of H_SVM_INIT_ABORT, we should page-out all the pages back to
> HV (i.e., we should not skip the page-out). Otherwise the VM's pages,
> possibly including its text/data would be stuck in secure memory.
> Since the SVM did not go secure, its MSR_S bit will be clear and the
> VM wont be able to access its pages even to do a clean exit.

It seems fragile to me to have one more transfer back into the
ultravisor after this call.  Why does the UV need to do this call and
then get control back again one more time?  Why can't the UV defer
doing this call until it can do it without expecting to see a return
from the hcall?  And if it does need to see a return from the hcall,
what would happen if a malicious hypervisor doesn't do the return?

Paul.
Ram Pai Nov. 12, 2019, 1:01 a.m. UTC | #2
On Mon, Nov 11, 2019 at 03:19:24PM +1100, Paul Mackerras wrote:
> On Mon, Nov 04, 2019 at 09:47:59AM +0530, Bharata B Rao wrote:
> > From: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
> > 
> > Implement the H_SVM_INIT_ABORT hcall which the Ultravisor can use to
> > abort an SVM after it has issued the H_SVM_INIT_START and before the
> > H_SVM_INIT_DONE hcalls. This hcall could be used when Ultravisor
> > encounters security violations or other errors when starting an SVM.
> > 
> > Note that this hcall is different from UV_SVM_TERMINATE ucall which
> > is used by HV to terminate/cleanup an SVM.
> > 
> > In case of H_SVM_INIT_ABORT, we should page-out all the pages back to
> > HV (i.e., we should not skip the page-out). Otherwise the VM's pages,
> > possibly including its text/data would be stuck in secure memory.
> > Since the SVM did not go secure, its MSR_S bit will be clear and the
> > VM wont be able to access its pages even to do a clean exit.
> 
> It seems fragile to me to have one more transfer back into the
> ultravisor after this call.  Why does the UV need to do this call and
> then get control back again one more time?  
> Why can't the UV defer
> doing this call until it can do it without expecting to see a return
> from the hcall?  

Sure. But, what if the hypervisor calls back into the UV through a
ucall, asking for some page to be paged-out?  If the ultravisor has
cleaned up the state associated with the SVM, it wont be able to service
that request.

H_SVM_INIT_ABORT is invoked to tell the hypervisor that the
secure-state-transition for the VM cannot be continued any further.
Hypervisor can than choose to do whatever with that information. It can
cleanup its state, and/or make ucalls to get some information from the
ultravisor.  It can also choose not to return control back to the ultravisor.


> And if it does need to see a return from the hcall,
> what would happen if a malicious hypervisor doesn't do the return?

That is fine.  At most it will be a denail-of-service attack.

RP

> 
> Paul.





If the ultravisor cleans up the SVM's state on its side and then informs
the Hypervisor to abort the SVM, the hypervisor will not be able to
cleanly terminate the VM.  Because to terminate the SVM, the hypervisor
still needs the services of the Ultravisor. For example: to get the
pages back into the hypervisor if needed. Another example is, the
hypervisor can call UV_SVM_TERMINATE.  Regardless of which ucall
gets called, the ultravisor has to hold on to enough state of the
SVM to service that request.

The current design assumes that the hypervisor explicitly informs the
ultravisor, that it is done with the SVM, through the UV_SVM_TERMINATE
ucall. Till that point the Ultravisor must to be ready to service any
ucalls made by the hypervisor on the SVM's behalf.


And if the ultravisor has cleaned-up the state of the SVM on it side,
any such ucall requests by the hypervisor will return with error. 

In summary -- for the hypervisor to cleanly terminate an SVM, it needs the
services of the ultravisor.  Only the hypervisor knows, when it would
NOT anymore need the services of the ultravisor for a SVM. Only after
the hypervisor communicates that through the UV_SVM_TERMINATE ucall,
the ultravisor will be able to confidently clean the state of the SVM
on its side.


The H_SVM_INIT_ABORT is a mechanism for the UV to inform the HV
to do whatever it needs to do to cleanup its state of the SVM; which
includes making ucalls to the ultravisor.
Paul Mackerras Nov. 12, 2019, 5:38 a.m. UTC | #3
On Mon, Nov 11, 2019 at 05:01:58PM -0800, Ram Pai wrote:
> On Mon, Nov 11, 2019 at 03:19:24PM +1100, Paul Mackerras wrote:
> > On Mon, Nov 04, 2019 at 09:47:59AM +0530, Bharata B Rao wrote:
> > > From: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
> > > 
> > > Implement the H_SVM_INIT_ABORT hcall which the Ultravisor can use to
> > > abort an SVM after it has issued the H_SVM_INIT_START and before the
> > > H_SVM_INIT_DONE hcalls. This hcall could be used when Ultravisor
> > > encounters security violations or other errors when starting an SVM.
> > > 
> > > Note that this hcall is different from UV_SVM_TERMINATE ucall which
> > > is used by HV to terminate/cleanup an SVM.
> > > 
> > > In case of H_SVM_INIT_ABORT, we should page-out all the pages back to
> > > HV (i.e., we should not skip the page-out). Otherwise the VM's pages,
> > > possibly including its text/data would be stuck in secure memory.
> > > Since the SVM did not go secure, its MSR_S bit will be clear and the
> > > VM wont be able to access its pages even to do a clean exit.
> > 
> > It seems fragile to me to have one more transfer back into the
> > ultravisor after this call.  Why does the UV need to do this call and
> > then get control back again one more time?  
> > Why can't the UV defer
> > doing this call until it can do it without expecting to see a return
> > from the hcall?  
> 
> Sure. But, what if the hypervisor calls back into the UV through a
> ucall, asking for some page to be paged-out?  If the ultravisor has
> cleaned up the state associated with the SVM, it wont be able to service
> that request.
> 
> H_SVM_INIT_ABORT is invoked to tell the hypervisor that the
> secure-state-transition for the VM cannot be continued any further.
> Hypervisor can than choose to do whatever with that information. It can
> cleanup its state, and/or make ucalls to get some information from the
> ultravisor.  It can also choose not to return control back to the ultravisor.
> 
> 
> > And if it does need to see a return from the hcall,
> > what would happen if a malicious hypervisor doesn't do the return?
> 
> That is fine.  At most it will be a denail-of-service attack.
> 
> RP
> 
> > 
> > Paul.
> 
> 
> 
> 
> 
> If the ultravisor cleans up the SVM's state on its side and then informs
> the Hypervisor to abort the SVM, the hypervisor will not be able to
> cleanly terminate the VM.  Because to terminate the SVM, the hypervisor
> still needs the services of the Ultravisor. For example: to get the
> pages back into the hypervisor if needed. Another example is, the
> hypervisor can call UV_SVM_TERMINATE.  Regardless of which ucall
> gets called, the ultravisor has to hold on to enough state of the
> SVM to service that request.

OK, that's a good reason.  That should be explained in the commit
message.

> The current design assumes that the hypervisor explicitly informs the
> ultravisor, that it is done with the SVM, through the UV_SVM_TERMINATE
> ucall. Till that point the Ultravisor must to be ready to service any
> ucalls made by the hypervisor on the SVM's behalf.

I see that UV_SVM_TERMINATE is done when the VM is being destroyed (at
which point kvm->arch.secure_guest doesn't matter any more), and in
kvmhv_svm_off(), where kvm->arch.secure_guest gets cleared
explicitly.  Hence I don't see any need for clearing it in the
assembly code on the next secure guest entry.  I think the change to
book3s_hv_rmhandlers.S can just be dropped.

Paul.
Ram Pai Nov. 12, 2019, 7:52 a.m. UTC | #4
On Tue, Nov 12, 2019 at 04:38:36PM +1100, Paul Mackerras wrote:
> On Mon, Nov 11, 2019 at 05:01:58PM -0800, Ram Pai wrote:
> > On Mon, Nov 11, 2019 at 03:19:24PM +1100, Paul Mackerras wrote:
> > > On Mon, Nov 04, 2019 at 09:47:59AM +0530, Bharata B Rao wrote:
> > > > From: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
> > > > 
> > > > Implement the H_SVM_INIT_ABORT hcall which the Ultravisor can use to
> > > > abort an SVM after it has issued the H_SVM_INIT_START and before the
> > > > H_SVM_INIT_DONE hcalls. This hcall could be used when Ultravisor
> > > > encounters security violations or other errors when starting an SVM.
> > > > 
> > > > Note that this hcall is different from UV_SVM_TERMINATE ucall which
> > > > is used by HV to terminate/cleanup an SVM.
> > > > 
> > > > In case of H_SVM_INIT_ABORT, we should page-out all the pages back to
> > > > HV (i.e., we should not skip the page-out). Otherwise the VM's pages,
> > > > possibly including its text/data would be stuck in secure memory.
> > > > Since the SVM did not go secure, its MSR_S bit will be clear and the
> > > > VM wont be able to access its pages even to do a clean exit.
> > > 
...skip...
> > 
> > If the ultravisor cleans up the SVM's state on its side and then informs
> > the Hypervisor to abort the SVM, the hypervisor will not be able to
> > cleanly terminate the VM.  Because to terminate the SVM, the hypervisor
> > still needs the services of the Ultravisor. For example: to get the
> > pages back into the hypervisor if needed. Another example is, the
> > hypervisor can call UV_SVM_TERMINATE.  Regardless of which ucall
> > gets called, the ultravisor has to hold on to enough state of the
> > SVM to service that request.
> 
> OK, that's a good reason.  That should be explained in the commit
> message.
> 
> > The current design assumes that the hypervisor explicitly informs the
> > ultravisor, that it is done with the SVM, through the UV_SVM_TERMINATE
> > ucall. Till that point the Ultravisor must to be ready to service any
> > ucalls made by the hypervisor on the SVM's behalf.
> 
> I see that UV_SVM_TERMINATE is done when the VM is being destroyed (at
> which point kvm->arch.secure_guest doesn't matter any more), and in
> kvmhv_svm_off(), where kvm->arch.secure_guest gets cleared
> explicitly.  Hence I don't see any need for clearing it in the
> assembly code on the next secure guest entry.  I think the change to
> book3s_hv_rmhandlers.S can just be dropped.

There is subtle problem removing that code from the assembly.

If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing
kvm->arch.secure_guest, the hypervisor will continue to think that the
VM is a secure VM.   However the primary reason the H_SVM_INIT_ABORT
hcall was invoked, was to inform the Hypervisor that it should no longer
consider the VM as a Secure VM. So there is a inconsistency there.

This is fine, as long as the VM does not invoke any hcall or does not
receive any hypervisor-exceptions.  The moment either of those happen,
the control goes into the hypervisor, the hypervisor services
the exception/hcall and while returning, it will see that the
kvm->arch.secure_guest flag is set and **incorrectly** return
to the ultravisor through a UV_RETURN ucall.  Ultravisor will
not know what to do with it, because it does not consider that
VM as a Secure VM.  Bad things happen.

( Sidenote: when H_SVM_INIT_ABORT hcalls returns from the hypervisor,
  the ultravisor cleans up its internal state corresponding of that
  aborted-SVM and returns back to the caller with MSR[S]=0 )


RP
Paul Mackerras Nov. 12, 2019, 11:32 a.m. UTC | #5
On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote:
> On Tue, Nov 12, 2019 at 04:38:36PM +1100, Paul Mackerras wrote:
> > On Mon, Nov 11, 2019 at 05:01:58PM -0800, Ram Pai wrote:
> > > On Mon, Nov 11, 2019 at 03:19:24PM +1100, Paul Mackerras wrote:
> > > > On Mon, Nov 04, 2019 at 09:47:59AM +0530, Bharata B Rao wrote:
> > > > > From: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
> > > > > 
> > > > > Implement the H_SVM_INIT_ABORT hcall which the Ultravisor can use to
> > > > > abort an SVM after it has issued the H_SVM_INIT_START and before the
> > > > > H_SVM_INIT_DONE hcalls. This hcall could be used when Ultravisor
> > > > > encounters security violations or other errors when starting an SVM.
> > > > > 
> > > > > Note that this hcall is different from UV_SVM_TERMINATE ucall which
> > > > > is used by HV to terminate/cleanup an SVM.
> > > > > 
> > > > > In case of H_SVM_INIT_ABORT, we should page-out all the pages back to
> > > > > HV (i.e., we should not skip the page-out). Otherwise the VM's pages,
> > > > > possibly including its text/data would be stuck in secure memory.
> > > > > Since the SVM did not go secure, its MSR_S bit will be clear and the
> > > > > VM wont be able to access its pages even to do a clean exit.
> > > > 
> ...skip...
> > > 
> > > If the ultravisor cleans up the SVM's state on its side and then informs
> > > the Hypervisor to abort the SVM, the hypervisor will not be able to
> > > cleanly terminate the VM.  Because to terminate the SVM, the hypervisor
> > > still needs the services of the Ultravisor. For example: to get the
> > > pages back into the hypervisor if needed. Another example is, the
> > > hypervisor can call UV_SVM_TERMINATE.  Regardless of which ucall
> > > gets called, the ultravisor has to hold on to enough state of the
> > > SVM to service that request.
> > 
> > OK, that's a good reason.  That should be explained in the commit
> > message.
> > 
> > > The current design assumes that the hypervisor explicitly informs the
> > > ultravisor, that it is done with the SVM, through the UV_SVM_TERMINATE
> > > ucall. Till that point the Ultravisor must to be ready to service any
> > > ucalls made by the hypervisor on the SVM's behalf.
> > 
> > I see that UV_SVM_TERMINATE is done when the VM is being destroyed (at
> > which point kvm->arch.secure_guest doesn't matter any more), and in
> > kvmhv_svm_off(), where kvm->arch.secure_guest gets cleared
> > explicitly.  Hence I don't see any need for clearing it in the
> > assembly code on the next secure guest entry.  I think the change to
> > book3s_hv_rmhandlers.S can just be dropped.
> 
> There is subtle problem removing that code from the assembly.
> 
> If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing
> kvm->arch.secure_guest, the hypervisor will continue to think that the
> VM is a secure VM.   However the primary reason the H_SVM_INIT_ABORT
> hcall was invoked, was to inform the Hypervisor that it should no longer
> consider the VM as a Secure VM. So there is a inconsistency there.

Most of the checks that look at whether a VM is a secure VM use code
like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)".  Now
since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will
take the false branch once we have set kvm->arch.secure_guest to
KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort.  So in fact in
most places we will treat the VM as a normal VM from then on.  If
there are any places where we still need to treat the VM as a secure
VM while we are processing the abort we can easily do that too.

> This is fine, as long as the VM does not invoke any hcall or does not
> receive any hypervisor-exceptions.  The moment either of those happen,
> the control goes into the hypervisor, the hypervisor services
> the exception/hcall and while returning, it will see that the
> kvm->arch.secure_guest flag is set and **incorrectly** return
> to the ultravisor through a UV_RETURN ucall.  Ultravisor will
> not know what to do with it, because it does not consider that
> VM as a Secure VM.  Bad things happen.

If bad things happen in the ultravisor because the hypervisor did
something it shouldn't, then it's game over, you just lost, thanks for
playing.  The ultravisor MUST be able to cope with bogus UV_RETURN
calls for a VM that it doesn't consider to be a secure VM.  You need
to work out how to handle such calls safely and implement that in the
ultravisor.

Paul.
Ram Pai Nov. 12, 2019, 2:45 p.m. UTC | #6
On Tue, Nov 12, 2019 at 10:32:04PM +1100, Paul Mackerras wrote:
> On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote:
> > On Tue, Nov 12, 2019 at 04:38:36PM +1100, Paul Mackerras wrote:
> > > On Mon, Nov 11, 2019 at 05:01:58PM -0800, Ram Pai wrote:
> > > > On Mon, Nov 11, 2019 at 03:19:24PM +1100, Paul Mackerras wrote:
> > > > > On Mon, Nov 04, 2019 at 09:47:59AM +0530, Bharata B Rao wrote:
> > > > > > From: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
> > > > > > 
> > > > > > Implement the H_SVM_INIT_ABORT hcall which the Ultravisor can use to
> > > > > > abort an SVM after it has issued the H_SVM_INIT_START and before the
> > > > > > H_SVM_INIT_DONE hcalls. This hcall could be used when Ultravisor
> > > > > > encounters security violations or other errors when starting an SVM.
> > > > > > 
> > > > > > Note that this hcall is different from UV_SVM_TERMINATE ucall which
> > > > > > is used by HV to terminate/cleanup an SVM.
> > > > > > 
> > > > > > In case of H_SVM_INIT_ABORT, we should page-out all the pages back to
> > > > > > HV (i.e., we should not skip the page-out). Otherwise the VM's pages,
> > > > > > possibly including its text/data would be stuck in secure memory.
> > > > > > Since the SVM did not go secure, its MSR_S bit will be clear and the
> > > > > > VM wont be able to access its pages even to do a clean exit.
> > > > > 
> > ...skip...
> > > > 
> > > > If the ultravisor cleans up the SVM's state on its side and then informs
> > > > the Hypervisor to abort the SVM, the hypervisor will not be able to
> > > > cleanly terminate the VM.  Because to terminate the SVM, the hypervisor
> > > > still needs the services of the Ultravisor. For example: to get the
> > > > pages back into the hypervisor if needed. Another example is, the
> > > > hypervisor can call UV_SVM_TERMINATE.  Regardless of which ucall
> > > > gets called, the ultravisor has to hold on to enough state of the
> > > > SVM to service that request.
> > > 
> > > OK, that's a good reason.  That should be explained in the commit
> > > message.
> > > 
> > > > The current design assumes that the hypervisor explicitly informs the
> > > > ultravisor, that it is done with the SVM, through the UV_SVM_TERMINATE
> > > > ucall. Till that point the Ultravisor must to be ready to service any
> > > > ucalls made by the hypervisor on the SVM's behalf.
> > > 
> > > I see that UV_SVM_TERMINATE is done when the VM is being destroyed (at
> > > which point kvm->arch.secure_guest doesn't matter any more), and in
> > > kvmhv_svm_off(), where kvm->arch.secure_guest gets cleared
> > > explicitly.  Hence I don't see any need for clearing it in the
> > > assembly code on the next secure guest entry.  I think the change to
> > > book3s_hv_rmhandlers.S can just be dropped.
> > 
> > There is subtle problem removing that code from the assembly.
> > 
> > If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing
> > kvm->arch.secure_guest, the hypervisor will continue to think that the
> > VM is a secure VM.   However the primary reason the H_SVM_INIT_ABORT
> > hcall was invoked, was to inform the Hypervisor that it should no longer
> > consider the VM as a Secure VM. So there is a inconsistency there.
> 
> Most of the checks that look at whether a VM is a secure VM use code
> like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)".  Now
> since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will
> take the false branch once we have set kvm->arch.secure_guest to
> KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort.  So in fact in
> most places we will treat the VM as a normal VM from then on.  If
> there are any places where we still need to treat the VM as a secure
> VM while we are processing the abort we can easily do that too.

Is the suggestion --  KVMPPC_SECURE_INIT_ABORT should never return back
to the Ultravisor?   Because removing that assembly code will NOT lead the
Hypervisor back into the Ultravisor.  This is fine with the
ultravisor. But then the hypervisor will not know where to return to.
If it wants to return directly to the VM, it wont know to
which address. It will be in a limbo.

> 
> > This is fine, as long as the VM does not invoke any hcall or does not
> > receive any hypervisor-exceptions.  The moment either of those happen,
> > the control goes into the hypervisor, the hypervisor services
> > the exception/hcall and while returning, it will see that the
> > kvm->arch.secure_guest flag is set and **incorrectly** return
> > to the ultravisor through a UV_RETURN ucall.  Ultravisor will
> > not know what to do with it, because it does not consider that
> > VM as a Secure VM.  Bad things happen.
> 
> If bad things happen in the ultravisor because the hypervisor did
> something it shouldn't, then it's game over, you just lost, thanks for
> playing.  The ultravisor MUST be able to cope with bogus UV_RETURN
> calls for a VM that it doesn't consider to be a secure VM.  You need
> to work out how to handle such calls safely and implement that in the
> ultravisor.

Actually we do handle this gracefully in the ultravisor :). 
We just retun back to the hypervisor saying "sorry dont know what
to do with it, please handle it yourself".

However hypervisor would not know what to do with that return, and bad
things happen in the hypervisor.

RP
Paul Mackerras Nov. 13, 2019, 12:14 a.m. UTC | #7
On Tue, Nov 12, 2019 at 06:45:55AM -0800, Ram Pai wrote:
> On Tue, Nov 12, 2019 at 10:32:04PM +1100, Paul Mackerras wrote:
> > On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote:
> > > There is subtle problem removing that code from the assembly.
> > > 
> > > If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing
> > > kvm->arch.secure_guest, the hypervisor will continue to think that the
> > > VM is a secure VM.   However the primary reason the H_SVM_INIT_ABORT
> > > hcall was invoked, was to inform the Hypervisor that it should no longer
> > > consider the VM as a Secure VM. So there is a inconsistency there.
> > 
> > Most of the checks that look at whether a VM is a secure VM use code
> > like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)".  Now
> > since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will
> > take the false branch once we have set kvm->arch.secure_guest to
> > KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort.  So in fact in
> > most places we will treat the VM as a normal VM from then on.  If
> > there are any places where we still need to treat the VM as a secure
> > VM while we are processing the abort we can easily do that too.
> 
> Is the suggestion --  KVMPPC_SECURE_INIT_ABORT should never return back
> to the Ultravisor?   Because removing that assembly code will NOT lead the

No.  The suggestion is that vcpu->arch.secure_guest stays set to
KVMPPC_SECURE_INIT_ABORT until userspace calls KVM_PPC_SVM_OFF.

> Hypervisor back into the Ultravisor.  This is fine with the
> ultravisor. But then the hypervisor will not know where to return to.
> If it wants to return directly to the VM, it wont know to
> which address. It will be in a limbo.
> 
> > 
> > > This is fine, as long as the VM does not invoke any hcall or does not
> > > receive any hypervisor-exceptions.  The moment either of those happen,
> > > the control goes into the hypervisor, the hypervisor services
> > > the exception/hcall and while returning, it will see that the
> > > kvm->arch.secure_guest flag is set and **incorrectly** return
> > > to the ultravisor through a UV_RETURN ucall.  Ultravisor will
> > > not know what to do with it, because it does not consider that
> > > VM as a Secure VM.  Bad things happen.
> > 
> > If bad things happen in the ultravisor because the hypervisor did
> > something it shouldn't, then it's game over, you just lost, thanks for
> > playing.  The ultravisor MUST be able to cope with bogus UV_RETURN
> > calls for a VM that it doesn't consider to be a secure VM.  You need
> > to work out how to handle such calls safely and implement that in the
> > ultravisor.
> 
> Actually we do handle this gracefully in the ultravisor :). 
> We just retun back to the hypervisor saying "sorry dont know what
> to do with it, please handle it yourself".
> 
> However hypervisor would not know what to do with that return, and bad
> things happen in the hypervisor.

Right.  We need something after the "sc 2" to handle the case where
the ultravisor returns with an error from the UV_RETURN.

Paul.
Ram Pai Nov. 13, 2019, 6:32 a.m. UTC | #8
On Wed, Nov 13, 2019 at 11:14:27AM +1100, Paul Mackerras wrote:
> On Tue, Nov 12, 2019 at 06:45:55AM -0800, Ram Pai wrote:
> > On Tue, Nov 12, 2019 at 10:32:04PM +1100, Paul Mackerras wrote:
> > > On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote:
> > > > There is subtle problem removing that code from the assembly.
> > > > 
> > > > If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing
> > > > kvm->arch.secure_guest, the hypervisor will continue to think that the
> > > > VM is a secure VM.   However the primary reason the H_SVM_INIT_ABORT
> > > > hcall was invoked, was to inform the Hypervisor that it should no longer
> > > > consider the VM as a Secure VM. So there is a inconsistency there.
> > > 
> > > Most of the checks that look at whether a VM is a secure VM use code
> > > like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)".  Now
> > > since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will
> > > take the false branch once we have set kvm->arch.secure_guest to
> > > KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort.  So in fact in
> > > most places we will treat the VM as a normal VM from then on.  If
> > > there are any places where we still need to treat the VM as a secure
> > > VM while we are processing the abort we can easily do that too.
> > 
> > Is the suggestion --  KVMPPC_SECURE_INIT_ABORT should never return back
> > to the Ultravisor?   Because removing that assembly code will NOT lead the
> 
> No.  The suggestion is that vcpu->arch.secure_guest stays set to
> KVMPPC_SECURE_INIT_ABORT until userspace calls KVM_PPC_SVM_OFF.

In the fast_guest_return path, if it finds 
(kvm->arch.secure_guest & KVMPPC_SECURE_INIT_ABORT) is true, should it return to
UV or not?

Ideally it should return back to the ultravisor the first time
KVMPPC_SECURE_INIT_ABORT is set, and not than onwards.

RP
Paul Mackerras Nov. 13, 2019, 9:18 p.m. UTC | #9
On Tue, Nov 12, 2019 at 10:32:33PM -0800, Ram Pai wrote:
> On Wed, Nov 13, 2019 at 11:14:27AM +1100, Paul Mackerras wrote:
> > On Tue, Nov 12, 2019 at 06:45:55AM -0800, Ram Pai wrote:
> > > On Tue, Nov 12, 2019 at 10:32:04PM +1100, Paul Mackerras wrote:
> > > > On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote:
> > > > > There is subtle problem removing that code from the assembly.
> > > > > 
> > > > > If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing
> > > > > kvm->arch.secure_guest, the hypervisor will continue to think that the
> > > > > VM is a secure VM.   However the primary reason the H_SVM_INIT_ABORT
> > > > > hcall was invoked, was to inform the Hypervisor that it should no longer
> > > > > consider the VM as a Secure VM. So there is a inconsistency there.
> > > > 
> > > > Most of the checks that look at whether a VM is a secure VM use code
> > > > like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)".  Now
> > > > since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will
> > > > take the false branch once we have set kvm->arch.secure_guest to
> > > > KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort.  So in fact in
> > > > most places we will treat the VM as a normal VM from then on.  If
> > > > there are any places where we still need to treat the VM as a secure
> > > > VM while we are processing the abort we can easily do that too.
> > > 
> > > Is the suggestion --  KVMPPC_SECURE_INIT_ABORT should never return back
> > > to the Ultravisor?   Because removing that assembly code will NOT lead the
> > 
> > No.  The suggestion is that vcpu->arch.secure_guest stays set to
> > KVMPPC_SECURE_INIT_ABORT until userspace calls KVM_PPC_SVM_OFF.
> 
> In the fast_guest_return path, if it finds 
> (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_ABORT) is true, should it return to
> UV or not?
> 
> Ideally it should return back to the ultravisor the first time
> KVMPPC_SECURE_INIT_ABORT is set, and not than onwards.

What is ideal about that behavior?  Why would that be a particularly
good thing to do?

Paul.
Ram Pai Nov. 13, 2019, 9:50 p.m. UTC | #10
On Thu, Nov 14, 2019 at 08:18:24AM +1100, Paul Mackerras wrote:
> On Tue, Nov 12, 2019 at 10:32:33PM -0800, Ram Pai wrote:
> > On Wed, Nov 13, 2019 at 11:14:27AM +1100, Paul Mackerras wrote:
> > > On Tue, Nov 12, 2019 at 06:45:55AM -0800, Ram Pai wrote:
> > > > On Tue, Nov 12, 2019 at 10:32:04PM +1100, Paul Mackerras wrote:
> > > > > On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote:
> > > > > > There is subtle problem removing that code from the assembly.
> > > > > > 
> > > > > > If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing
> > > > > > kvm->arch.secure_guest, the hypervisor will continue to think that the
> > > > > > VM is a secure VM.   However the primary reason the H_SVM_INIT_ABORT
> > > > > > hcall was invoked, was to inform the Hypervisor that it should no longer
> > > > > > consider the VM as a Secure VM. So there is a inconsistency there.
> > > > > 
> > > > > Most of the checks that look at whether a VM is a secure VM use code
> > > > > like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)".  Now
> > > > > since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will
> > > > > take the false branch once we have set kvm->arch.secure_guest to
> > > > > KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort.  So in fact in
> > > > > most places we will treat the VM as a normal VM from then on.  If
> > > > > there are any places where we still need to treat the VM as a secure
> > > > > VM while we are processing the abort we can easily do that too.
> > > > 
> > > > Is the suggestion --  KVMPPC_SECURE_INIT_ABORT should never return back
> > > > to the Ultravisor?   Because removing that assembly code will NOT lead the
> > > 
> > > No.  The suggestion is that vcpu->arch.secure_guest stays set to
> > > KVMPPC_SECURE_INIT_ABORT until userspace calls KVM_PPC_SVM_OFF.
> > 
> > In the fast_guest_return path, if it finds 
> > (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_ABORT) is true, should it return to
> > UV or not?
> > 
> > Ideally it should return back to the ultravisor the first time
> > KVMPPC_SECURE_INIT_ABORT is set, and not than onwards.
> 
> What is ideal about that behavior?  Why would that be a particularly
> good thing to do?

It is following the rule -- "return back to the caller".

RP
Paul Mackerras Nov. 14, 2019, 5:08 a.m. UTC | #11
On Wed, Nov 13, 2019 at 01:50:42PM -0800, Ram Pai wrote:
> On Thu, Nov 14, 2019 at 08:18:24AM +1100, Paul Mackerras wrote:
> > On Tue, Nov 12, 2019 at 10:32:33PM -0800, Ram Pai wrote:
> > > On Wed, Nov 13, 2019 at 11:14:27AM +1100, Paul Mackerras wrote:
> > > > On Tue, Nov 12, 2019 at 06:45:55AM -0800, Ram Pai wrote:
> > > > > On Tue, Nov 12, 2019 at 10:32:04PM +1100, Paul Mackerras wrote:
> > > > > > On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote:
> > > > > > > There is subtle problem removing that code from the assembly.
> > > > > > > 
> > > > > > > If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing
> > > > > > > kvm->arch.secure_guest, the hypervisor will continue to think that the
> > > > > > > VM is a secure VM.   However the primary reason the H_SVM_INIT_ABORT
> > > > > > > hcall was invoked, was to inform the Hypervisor that it should no longer
> > > > > > > consider the VM as a Secure VM. So there is a inconsistency there.
> > > > > > 
> > > > > > Most of the checks that look at whether a VM is a secure VM use code
> > > > > > like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)".  Now
> > > > > > since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will
> > > > > > take the false branch once we have set kvm->arch.secure_guest to
> > > > > > KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort.  So in fact in
> > > > > > most places we will treat the VM as a normal VM from then on.  If
> > > > > > there are any places where we still need to treat the VM as a secure
> > > > > > VM while we are processing the abort we can easily do that too.
> > > > > 
> > > > > Is the suggestion --  KVMPPC_SECURE_INIT_ABORT should never return back
> > > > > to the Ultravisor?   Because removing that assembly code will NOT lead the
> > > > 
> > > > No.  The suggestion is that vcpu->arch.secure_guest stays set to
> > > > KVMPPC_SECURE_INIT_ABORT until userspace calls KVM_PPC_SVM_OFF.
> > > 
> > > In the fast_guest_return path, if it finds 
> > > (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_ABORT) is true, should it return to
> > > UV or not?
> > > 
> > > Ideally it should return back to the ultravisor the first time
> > > KVMPPC_SECURE_INIT_ABORT is set, and not than onwards.
> > 
> > What is ideal about that behavior?  Why would that be a particularly
> > good thing to do?
> 
> It is following the rule -- "return back to the caller".

That doesn't address the question of why vcpu->arch.secure_guest
should be cleared at the point where we do that.

Paul.
Ram Pai Nov. 14, 2019, 7:02 a.m. UTC | #12
On Thu, Nov 14, 2019 at 04:08:25PM +1100, Paul Mackerras wrote:
> On Wed, Nov 13, 2019 at 01:50:42PM -0800, Ram Pai wrote:
> > On Thu, Nov 14, 2019 at 08:18:24AM +1100, Paul Mackerras wrote:
> > > On Tue, Nov 12, 2019 at 10:32:33PM -0800, Ram Pai wrote:
> > > > On Wed, Nov 13, 2019 at 11:14:27AM +1100, Paul Mackerras wrote:
> > > > > On Tue, Nov 12, 2019 at 06:45:55AM -0800, Ram Pai wrote:
> > > > > > On Tue, Nov 12, 2019 at 10:32:04PM +1100, Paul Mackerras wrote:
> > > > > > > On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote:
> > > > > > > > There is subtle problem removing that code from the assembly.
> > > > > > > > 
> > > > > > > > If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing
> > > > > > > > kvm->arch.secure_guest, the hypervisor will continue to think that the
> > > > > > > > VM is a secure VM.   However the primary reason the H_SVM_INIT_ABORT
> > > > > > > > hcall was invoked, was to inform the Hypervisor that it should no longer
> > > > > > > > consider the VM as a Secure VM. So there is a inconsistency there.
> > > > > > > 
> > > > > > > Most of the checks that look at whether a VM is a secure VM use code
> > > > > > > like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)".  Now
> > > > > > > since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will
> > > > > > > take the false branch once we have set kvm->arch.secure_guest to
> > > > > > > KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort.  So in fact in
> > > > > > > most places we will treat the VM as a normal VM from then on.  If
> > > > > > > there are any places where we still need to treat the VM as a secure
> > > > > > > VM while we are processing the abort we can easily do that too.
> > > > > > 
> > > > > > Is the suggestion --  KVMPPC_SECURE_INIT_ABORT should never return back
> > > > > > to the Ultravisor?   Because removing that assembly code will NOT lead the
> > > > > 
> > > > > No.  The suggestion is that vcpu->arch.secure_guest stays set to
> > > > > KVMPPC_SECURE_INIT_ABORT until userspace calls KVM_PPC_SVM_OFF.
> > > > 
> > > > In the fast_guest_return path, if it finds 
> > > > (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_ABORT) is true, should it return to
> > > > UV or not?
> > > > 
> > > > Ideally it should return back to the ultravisor the first time
> > > > KVMPPC_SECURE_INIT_ABORT is set, and not than onwards.
> > > 
> > > What is ideal about that behavior?  Why would that be a particularly
> > > good thing to do?
> > 
> > It is following the rule -- "return back to the caller".
> 
> That doesn't address the question of why vcpu->arch.secure_guest
> should be cleared at the point where we do that.

Ok. here is the sequence that I expect to happen.

-------------------------------------------------------------------
1) VM makes a ESM(Enter Secure Mode) ucall.

  1A) UV does the H_SVM_INIT_START hcall... the chit-chat between UV and HV happens
	and somewhere in that chit-chat, UV decides that the ESM call
	cannot be successfully completed.  Hence it calls
	H_SVM_INIT_ABORT to inform the Hypervisor.

    1A_a) Hypervisor aborts the VM's transition and returns back to the ultravisor.

  1B) UV cleans up the state of the VM on its side and returns
    	back to the VM, with failure.  VM is still in a normal VM state.
	Its MSR(S) is still 0.

2) VM gets a HDEC exception.

   2A) Hypervisor receives that HDEC exception. It handles the exception
   	and returns back to the VM.
-------------------------------------------------------------------

If you agree with the above sequence than, the patch needs all the proposed changes.


At step (1A_a) and step (2A),  the hypervisor is faced with the question
	--  Where should the control be returned to?

  for step 1A_a it has to return to the UV, and for step 2A it has to
  return to the VM. In other words the control has to **return back to
  the caller**.


It certainly has to rely on the vcpu->arch.secure_guest flag to do so.
If any flag in vcpu->arch.secure_guest is set, it has to return to 
UV.  Otherwise it has to return to VM.

This is the reason, the proposed patch in the fast_guest_return path
looks at the vcpu->arch.secure_guest. If it finds any flag set, it
returns to UV. However before returning to UV, it also clears all the
flags if  H_SVM_INIT_ABORT is set. This is to ensure that HV does not
return to the wrong place; i.e to UV, but returns to the VM at step 2A.

RP
diff mbox series

Patch

diff --git a/Documentation/powerpc/ultravisor.rst b/Documentation/powerpc/ultravisor.rst
index 730854f73830..286cabadc566 100644
--- a/Documentation/powerpc/ultravisor.rst
+++ b/Documentation/powerpc/ultravisor.rst
@@ -948,6 +948,45 @@  Use cases
     up its internal state for this virtual machine.
 
 
+H_SVM_INIT_ABORT
+----------------
+
+    Abort the process of securing an SVM.
+
+Syntax
+~~~~~~
+
+.. code-block:: c
+
+	uint64_t hypercall(const uint64_t H_SVM_INIT_ABORT)
+
+Return values
+~~~~~~~~~~~~~
+
+    One of the following values:
+
+	* H_SUCCESS 		on success.
+	* H_UNSUPPORTED		if called from the wrong context (e.g.
+				from an SVM or before an H_SVM_INIT_START
+				hypercall).
+
+Description
+~~~~~~~~~~~
+
+    Abort the process of securing a virtual machine. This call must
+    be made after a prior call to ``H_SVM_INIT_START`` hypercall.
+
+Use cases
+~~~~~~~~~
+
+
+    On successfully securing a virtual machine, the Ultravisor informs
+    If the Ultravisor is unable to secure a virtual machine either due
+    to lack of resources or because the VM's security information could
+    not be validated, Ultravisor informs the Hypervisor about it.
+    Hypervisor can use this call to clean up any internal state for this
+    virtual machine.
+
 H_SVM_PAGE_IN
 -------------
 
diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
index 13bd870609c3..e90c073e437e 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -350,6 +350,7 @@ 
 #define H_SVM_PAGE_OUT		0xEF04
 #define H_SVM_INIT_START	0xEF08
 #define H_SVM_INIT_DONE		0xEF0C
+#define H_SVM_INIT_ABORT	0xEF14
 
 /* Values for 2nd argument to H_SET_MODE */
 #define H_SET_MODE_RESOURCE_SET_CIABR		1
diff --git a/arch/powerpc/include/asm/kvm_book3s_uvmem.h b/arch/powerpc/include/asm/kvm_book3s_uvmem.h
index 3cf8425b9838..eaea400ea715 100644
--- a/arch/powerpc/include/asm/kvm_book3s_uvmem.h
+++ b/arch/powerpc/include/asm/kvm_book3s_uvmem.h
@@ -18,6 +18,7 @@  unsigned long kvmppc_h_svm_page_out(struct kvm *kvm,
 				    unsigned long page_shift);
 unsigned long kvmppc_h_svm_init_start(struct kvm *kvm);
 unsigned long kvmppc_h_svm_init_done(struct kvm *kvm);
+unsigned long kvmppc_h_svm_init_abort(struct kvm *kvm);
 int kvmppc_send_page_to_uv(struct kvm *kvm, unsigned long gfn);
 void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free,
 			     struct kvm *kvm, bool skip_page_out);
@@ -62,6 +63,11 @@  static inline unsigned long kvmppc_h_svm_init_done(struct kvm *kvm)
 	return H_UNSUPPORTED;
 }
 
+static inline unsigned long kvmppc_h_svm_init_abort(struct kvm *kvm)
+{
+	return H_UNSUPPORTED;
+}
+
 static inline int kvmppc_send_page_to_uv(struct kvm *kvm, unsigned long gfn)
 {
 	return -EFAULT;
diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index 577ca95fac7c..8310c0407383 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -278,6 +278,7 @@  struct kvm_resize_hpt;
 /* Flag values for kvm_arch.secure_guest */
 #define KVMPPC_SECURE_INIT_START 0x1 /* H_SVM_INIT_START has been called */
 #define KVMPPC_SECURE_INIT_DONE  0x2 /* H_SVM_INIT_DONE completed */
+#define KVMPPC_SECURE_INIT_ABORT 0x4 /* H_SVM_INIT_ABORT issued */
 
 struct kvm_arch {
 	unsigned int lpid;
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index d2bc4e9bbe7e..ad4e38ce7b55 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1099,6 +1099,9 @@  int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
 	case H_SVM_INIT_DONE:
 		ret = kvmppc_h_svm_init_done(vcpu->kvm);
 		break;
+	case H_SVM_INIT_ABORT:
+		ret = kvmppc_h_svm_init_abort(vcpu->kvm);
+		break;
 
 	default:
 		return RESUME_HOST;
diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index faebcbb8c4db..8d192c9947cd 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -1112,10 +1112,10 @@  END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
 	ld	r6, VCPU_KVM(r4)
 	lbz	r7, KVM_SECURE_GUEST(r6)
 	cmpdi	r7, 0
+	bne	check_svm_abort
+
 	ld	r6, VCPU_GPR(R6)(r4)
 	ld	r7, VCPU_GPR(R7)(r4)
-	bne	ret_to_ultra
-
 	lwz	r0, VCPU_CR(r4)
 	mtcr	r0
 
@@ -1125,6 +1125,21 @@  END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
 	ld	r4, VCPU_GPR(R4)(r4)
 	HRFI_TO_GUEST
 	b	.
+
+/*
+ * If SVM is about to abort, return to UV one last time but clear the
+ * secure_guest state so future fast_guest_returns return to the normal
+ * VM. We expect following state and we will restore the state.
+ *   R6 = kvm
+ *   R7 = kvm->secure_guest
+ */
+check_svm_abort:
+
+	cmpdi	r7, 4	/* KVMPPC_SECURE_INIT_ABORT */
+	bne	ret_to_ultra
+	li	r7, 0
+	stb	r7, KVM_SECURE_GUEST(r6)
+
 /*
  * Use UV_RETURN ultracall to return control back to the Ultravisor after
  * processing an hypercall or interrupt that was forwarded (a.k.a. reflected)
@@ -1134,8 +1149,12 @@  END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
  *   R0 = hcall result
  *   R2 = SRR1, so UV can detect a synthesized interrupt (if any)
  *   R3 = UV_RETURN
+ *   R6 = kvm (to be restored)
+ *   R7 = kvm->secure_guest (to be restored)
  */
 ret_to_ultra:
+	ld	r6, VCPU_GPR(R6)(r4)
+	ld	r7, VCPU_GPR(R7)(r4)
 	lwz	r0, VCPU_CR(r4)
 	mtcr	r0
 
diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
index 2df0d3f80c60..627dfe4abf08 100644
--- a/arch/powerpc/kvm/book3s_hv_uvmem.c
+++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
@@ -284,6 +284,35 @@  void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free,
 	}
 }
 
+unsigned long kvmppc_h_svm_init_abort(struct kvm *kvm)
+{
+	int i;
+	int srcu_idx;
+
+	if (!(kvm->arch.secure_guest & KVMPPC_SECURE_INIT_START))
+		return H_UNSUPPORTED;
+
+	srcu_idx = srcu_read_lock(&kvm->srcu);
+	for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
+		struct kvm_memory_slot *memslot;
+		struct kvm_memslots *slots = __kvm_memslots(kvm, i);
+
+		if (!slots)
+			continue;
+
+		kvm_for_each_memslot(memslot, slots) {
+			kvmppc_uvmem_drop_pages(memslot, kvm, false);
+			uv_unregister_mem_slot(kvm->arch.lpid, memslot->id);
+			kvmppc_uvmem_slot_free(kvm, memslot);
+		}
+	}
+	srcu_read_unlock(&kvm->srcu, srcu_idx);
+
+	kvm->arch.secure_guest = KVMPPC_SECURE_INIT_ABORT;
+	pr_info("LPID %d: Switching to secure aborted\n", kvm->arch.lpid);
+	return H_SUCCESS;
+}
+
 /*
  * Get a free device PFN from the pool
  *