Re: [Xen-devel] [PATCH v2 2/3] x86/svm: Always intercept ICEBP

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Petre Pircalabu <ppircalabu@bitdefender.com>,
	Juergen Gross <jgross@suse.com>,
	"hange-folder>?" <toggle-mailboxes@Air-de-Roger>,
	Tamas K Lengyel <tamas@tklengyel.com>,
	Razvan Cojocaru <rcojocaru@bitdefender.com>, Wei Liu <wl@xen.org>,
	Jan Beulich <JBeulich@suse.com>,
	y@Air-de-Roger, Alexandru Isaila <aisaila@bitdefender.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/svm: Always intercept ICEBP
Date: Wed, 27 Nov 2019 09:55:26 +0100	[thread overview]
Message-ID: <20191127085526.GJ980@Air-de-Roger> (raw)
In-Reply-To: <12ed4e11-357d-6fb4-ce5a-ec022b23c7c1@citrix.com>

On Tue, Nov 26, 2019 at 04:09:08PM +0000, Andrew Cooper wrote:
> On 26/11/2019 15:34, Roger Pau Monné wrote:
> > On Tue, Nov 26, 2019 at 12:03:56PM +0000, Andrew Cooper wrote:
> >> ICEBP isn't handled well by SVM.
> >>
> >> The VMexit state for a #DB-vectored TASK_SWITCH has %rip pointing to the
> >> appropriate instruction boundary (fault or trap, as appropriate), except for
> >> an ICEBP-induced #DB TASK_SWITCH, where %rip points at the ICEBP instruction
> >> rather than after it.  As ICEBP isn't distinguished in the vectoring event
> >> type, the state is ambiguous.
> >>
> >> To add to the confusion, an ICEBP which occurs due to Introspection
> >> intercepting the instruction, or from x86_emulate() will have %rip updated as
> >> a consequence of partial emulation required to inject an ICEBP event in the
> >> first place.
> >>
> >> We could in principle spot the non-injected case in the TASK_SWITCH handler,
> >> but this still results in complexity if the ICEBP instruction also has an
> >> Instruction Breakpoint active on it (which genuinely has fault semantics).
> >>
> >> Unconditionally intercept ICEBP.  This does have a trap semantics for the
> >> intercept, and allows us to move %rip forwards appropriately before the
> >> TASK_SWITCH intercept is hit.  This makes the behaviour of #DB-vectored
> >> switches consistent however the ICEBP #DB came about, and avoids special cases
> >> in the TASK_SWITCH intercept.
> >>
> >> This in turn allows for the removal of the conditional
> >> hvm_set_icebp_interception() logic used by the monitor subsystem, as ICEBP's
> >> will now always be submitted for monitoring checks.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> >
> > AFAICT this brings AMD implementation inline with Intel that also will
> > unconditionally vmexit on icebp?
> 
> VT-x and SVM handle things quite differently.
> 
> VT-x has no instruction intercept for ICEBP, but the #DB intercept will
> triggered by an ICEBP instruction.  ICEBP has its own event type
> (Privileged Software Exception, which is an amusing name considering it
> is an unprivleged instruction, bypasses privilege checks, and sets the
> External bit in an error code).
> 
> SVM does have an instruction intercept for ICEBP, but the #DB from
> ICEBP's don't trigger the normal #DB intercept.  However, secondary
> #DB's generated by ICEBP's unintercepted #DB do trigger the #DB intercept.
> 
> For safety reasons we must intercept #DB to prevent CPU deadlocks.  This
> means that ICEBP are in practice always intercepted on Intel due to
> their #DB side effect, but they weren't intercepted on AMD, which is why
> the monitor subsystem had a way of turning interception on.
> 
> So yes, the overall effect is that ICEBPs will now unconditionally
> vmexit on both Intel and AMD, but underlying mechanism for why they
> vmexit is still vendor-specific.

Thanks for the detailed explanation, I realized vmx didn't have a
ICEBP specific VMEXIT reason, but I assumed the #DB would be trapped
as that's how the monitor system intercepts those.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel