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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Petre Pircalabu" <ppircalabu@bitdefender.com>,
	"Juergen Gross" <jgross@suse.com>,
	"Tamas KLengyel" <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:16:22 +0000	[thread overview]
Message-ID: <5e3be59f-e68d-cc29-f39e-e49466522e5c@citrix.com> (raw)
In-Reply-To: <e29e2966-7d04-7e12-a15a-46a14765cae4@suse.com>

On 26/11/2019 16:14, Jan Beulich wrote:
> On 26.11.2019 17:11, Andrew Cooper wrote:
>> On 26/11/2019 16:05, Jan Beulich wrote:
>>> On 26.11.2019 16:59, Andrew Cooper wrote:
>>>> On 26/11/2019 15:32, Jan Beulich wrote:
>>>>> 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?
>>>> ICEBP really is too broken under SVM to handle architecturally.
>>>>
>>>> The ICEBP intercept has nRIP decode support, because it is an
>>>> instruction intercept.  We emulate the injection (because it is ICEBP),
>>>> which means we re-enter the guest with %rip moved forward, and #DB
>>>> (HW_EXCEPTION) pending for injection.  This means that...
>>>>
>>>>>  If so
>>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>> ... the ICEBP-#DB-vectored TASK_SWITCH will now find %rip pointing after
>>>> the ICEBP instruction, rather than at it, making it consistent with
>>>> every other #DB-vectored TASK_SWITCH.
>>>>
>>>> This does means that an early task-switch fault for ICEBP will reliably
>>>> be delivered with the wrong (i.e. trap) semantics, but this is less bad
>>>> than mixed fault/trap semantics depending on whether the source of the
>>>> ICEBP was introspection/emulation or native execution.
>>>>
>>>> We could restore proper fault behaviour by extending
>>>> svm_emul_swint_injection() to figure out that a task switch is needed,
>>>> and invoke hvm_task_switch() directly, but I don't have enough TUITS
>>>> right now.
>>>>
>>>>> Otherwise I guess I'm still missing something.
>>>> I hope this clears it up.
>>> Well, it helps, but you don't really answer the question: Is "trap"
>>> in that sentence of the description really correct? I.e. don't you
>>> instead mean "fault" there?
>> I've reworded that bit to:
>>
>> Unconditionally intercept ICEBP.  This does have NRIPs support as it is an
>> instruction intercept, which allows us allows us to move %rip forwards
>> appropriately before the TASK_SWITCH intercept is hit.  This allows...
>>
>> Any better?
> Ah yes, thanks. (But please drop one of the two "allows us".)

Oops yes.  Irritatingly, that causes #DB-vectored to move onto a new
line, and trigger Git's comment syntax.  I'll tweak a little bit more.

~Andrew

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