diff mbox series

[v4,63/75] x86/sev-es: Handle #DB Events

Message ID 20200714120917.11253-64-joro@8bytes.org (mailing list archive)
State New, archived
Headers show
Series x86: SEV-ES Guest Support | expand

Commit Message

Joerg Roedel July 14, 2020, 12:09 p.m. UTC 
From: Joerg Roedel <jroedel@suse.de>

Handle #VC exceptions caused by #DB exceptions in the guest. Those
must be handled outside of instrumentation_begin()/end() so that the
handler will not be raised recursivly.

Handle them by calling the kernels debug exception handler.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/kernel/sev-es.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Peter Zijlstra July 15, 2020, 8:47 a.m. UTC | #1 
On Tue, Jul 14, 2020 at 02:09:05PM +0200, Joerg Roedel wrote:

> @@ -1028,6 +1036,16 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)
>  	struct ghcb *ghcb;
>  
>  	lockdep_assert_irqs_disabled();
> +
> +	/*
> +	 * #DB is special and needs to be handled outside of the intrumentation_begin()/end().
> +	 * Otherwise the #VC handler could be raised recursivly.
> +	 */
> +	if (error_code == SVM_EXIT_EXCP_BASE + X86_TRAP_DB) {
> +		vc_handle_trap_db(regs);
> +		return;
> +	}
> +
>  	instrumentation_begin();

Wait what?! That makes no sense what so ever.
Joerg Roedel July 15, 2020, 9:13 a.m. UTC | #2 
On Wed, Jul 15, 2020 at 10:47:52AM +0200, Peter Zijlstra wrote:
> On Tue, Jul 14, 2020 at 02:09:05PM +0200, Joerg Roedel wrote:
> 
> > @@ -1028,6 +1036,16 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)
> >  	struct ghcb *ghcb;
> >  
> >  	lockdep_assert_irqs_disabled();
> > +
> > +	/*
> > +	 * #DB is special and needs to be handled outside of the intrumentation_begin()/end().
> > +	 * Otherwise the #VC handler could be raised recursivly.
> > +	 */
> > +	if (error_code == SVM_EXIT_EXCP_BASE + X86_TRAP_DB) {
> > +		vc_handle_trap_db(regs);
> > +		return;
> > +	}
> > +
> >  	instrumentation_begin();
> 
> Wait what?! That makes no sense what so ever.

Then my understanding of intrumentation_begin/end() is wrong, I thought
that the kernel will forbid setting breakpoints before
instrumentation_begin(), which is necessary here because a break-point
in the #VC handler might cause recursive #VC-exceptions when #DB is
intercepted.
Maybe you can elaborate on why this makes no sense?

Regards,

	Joerg
Peter Zijlstra July 15, 2020, 9:51 a.m. UTC | #3 
On Wed, Jul 15, 2020 at 11:13:37AM +0200, Joerg Roedel wrote:
> On Wed, Jul 15, 2020 at 10:47:52AM +0200, Peter Zijlstra wrote:
> > On Tue, Jul 14, 2020 at 02:09:05PM +0200, Joerg Roedel wrote:
> > 
> > > @@ -1028,6 +1036,16 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)
> > >  	struct ghcb *ghcb;
> > >  
> > >  	lockdep_assert_irqs_disabled();
> > > +
> > > +	/*
> > > +	 * #DB is special and needs to be handled outside of the intrumentation_begin()/end().
> > > +	 * Otherwise the #VC handler could be raised recursivly.
> > > +	 */
> > > +	if (error_code == SVM_EXIT_EXCP_BASE + X86_TRAP_DB) {
> > > +		vc_handle_trap_db(regs);
> > > +		return;
> > > +	}
> > > +
> > >  	instrumentation_begin();
> > 
> > Wait what?! That makes no sense what so ever.
> 
> Then my understanding of intrumentation_begin/end() is wrong, I thought
> that the kernel will forbid setting breakpoints before
> instrumentation_begin(), which is necessary here because a break-point
> in the #VC handler might cause recursive #VC-exceptions when #DB is
> intercepted.
> Maybe you can elaborate on why this makes no sense?

Kernel avoids breakpoints in any noinstr text, irrespective of
instrumentation_begin().

instrumentation_begin() merely allows one to call !noinstr functions.
Joerg Roedel July 15, 2020, 10:08 a.m. UTC | #4 
On Wed, Jul 15, 2020 at 11:51:36AM +0200, Peter Zijlstra wrote:
> On Wed, Jul 15, 2020 at 11:13:37AM +0200, Joerg Roedel wrote:
> > Then my understanding of intrumentation_begin/end() is wrong, I thought
> > that the kernel will forbid setting breakpoints before
> > instrumentation_begin(), which is necessary here because a break-point
> > in the #VC handler might cause recursive #VC-exceptions when #DB is
> > intercepted.
> > Maybe you can elaborate on why this makes no sense?
> 
> Kernel avoids breakpoints in any noinstr text, irrespective of
> instrumentation_begin().
> 
> instrumentation_begin() merely allows one to call !noinstr functions.

Right, but the handler calls into various other functions. I actually
started to annotate them all with noinstr, but that was a can of worms
when calling into generic kernel functions. And the only problem with
intrumentation in the #VC handler is the #VC-for-#DB exit-code, so I
decided to only handle this one with instrumentation forbidden and allow
it for the rest of the handler.

Regards,

	Joerg
Peter Zijlstra July 15, 2020, 10:13 a.m. UTC | #5 
On Wed, Jul 15, 2020 at 12:08:08PM +0200, Joerg Roedel wrote:
> On Wed, Jul 15, 2020 at 11:51:36AM +0200, Peter Zijlstra wrote:
> > On Wed, Jul 15, 2020 at 11:13:37AM +0200, Joerg Roedel wrote:
> > > Then my understanding of intrumentation_begin/end() is wrong, I thought
> > > that the kernel will forbid setting breakpoints before
> > > instrumentation_begin(), which is necessary here because a break-point
> > > in the #VC handler might cause recursive #VC-exceptions when #DB is
> > > intercepted.
> > > Maybe you can elaborate on why this makes no sense?
> > 
> > Kernel avoids breakpoints in any noinstr text, irrespective of
> > instrumentation_begin().
> > 
> > instrumentation_begin() merely allows one to call !noinstr functions.
> 
> Right, but the handler calls into various other functions. I actually
> started to annotate them all with noinstr, but that was a can of worms
> when calling into generic kernel functions. And the only problem with
> intrumentation in the #VC handler is the #VC-for-#DB exit-code, so I
> decided to only handle this one with instrumentation forbidden and allow
> it for the rest of the handler.

OK, then maybe change the comment to something like:

 /*
  * Handle #DB before calling any !noinstr code to avoid
  * recursive #DB.
  */

?
Joerg Roedel July 15, 2020, 10:31 a.m. UTC | #6 
On Wed, Jul 15, 2020 at 12:13:10PM +0200, Peter Zijlstra wrote:
> On Wed, Jul 15, 2020 at 12:08:08PM +0200, Joerg Roedel wrote:
> > Right, but the handler calls into various other functions. I actually
> > started to annotate them all with noinstr, but that was a can of worms
> > when calling into generic kernel functions. And the only problem with
> > intrumentation in the #VC handler is the #VC-for-#DB exit-code, so I
> > decided to only handle this one with instrumentation forbidden and allow
> > it for the rest of the handler.
> 
> OK, then maybe change the comment to something like:
> 
>  /*
>   * Handle #DB before calling any !noinstr code to avoid
>   * recursive #DB.
>   */

Sounds good, will do.

Thanks,

	Joerg
diff mbox series

Patch

diff --git a/arch/x86/kernel/sev-es.c b/arch/x86/kernel/sev-es.c
index 8f275e5d1ce7..b0f08d9669f1 100644
--- a/arch/x86/kernel/sev-es.c
+++ b/arch/x86/kernel/sev-es.c
@@ -928,6 +928,14 @@  static enum es_result vc_handle_trap_ac(struct ghcb *ghcb,
 	return ES_EXCEPTION;
 }
 
+static __always_inline void vc_handle_trap_db(struct pt_regs *regs)
+{
+	if (user_mode(regs))
+		noist_exc_debug(regs);
+	else
+		exc_debug(regs);
+}
+
 static enum es_result vc_handle_exitcode(struct es_em_ctxt *ctxt,
 					 struct ghcb *ghcb,
 					 unsigned long exit_code)
@@ -1028,6 +1036,16 @@  DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)
 	struct ghcb *ghcb;
 
 	lockdep_assert_irqs_disabled();
+
+	/*
+	 * #DB is special and needs to be handled outside of the intrumentation_begin()/end().
+	 * Otherwise the #VC handler could be raised recursivly.
+	 */
+	if (error_code == SVM_EXIT_EXCP_BASE + X86_TRAP_DB) {
+		vc_handle_trap_db(regs);
+		return;
+	}
+
 	instrumentation_begin();
 
 	/*