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

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Juergen Gross" <jgross@suse.com>,
	"Petre Pircalabu" <ppircalabu@bitdefender.com>,
	"Tamas K Lengyel" <tamas@tklengyel.com>, "Wei Liu" <wl@xen.org>,
	"Razvan Cojocaru" <rcojocaru@bitdefender.com>,
	"Alexandru Isaila" <aisaila@bitdefender.com>,
	Xen-devel <xen-devel@lists.xenproject.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/svm: Always intercept ICEBP
Date: Tue, 26 Nov 2019 16:32:08 +0100	[thread overview]
Message-ID: <93e3dd69-698e-17ee-5eef-a932b1989786@suse.com> (raw)
In-Reply-To: <20191126120357.13398-3-andrew.cooper3@citrix.com>

On 26.11.2019 13:03, 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.

Both because of you mentioning the moving forwards of %rip and with the
irc discussion in mind that we had no irc, don't you mean "fault
semantics" here? If so
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Otherwise I guess I'm still missing something.

> ---
>  xen/arch/x86/hvm/svm/svm.c    | 19 -------------------
>  xen/arch/x86/hvm/svm/vmcb.c   |  2 +-
>  xen/arch/x86/monitor.c        |  3 ---
>  xen/include/asm-x86/hvm/hvm.h | 11 -----------
>  4 files changed, 1 insertion(+), 34 deletions(-)

This, of course, is pretty nice in any event.

Jan

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