Re: [Xen-devel] [PATCH v2 4/7] x86: NOP out most XPTI entry/exit code when it's not in use

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.03.18 at 20:42, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/02/18 16:13, Jan Beulich wrote:
>> --- a/xen/arch/x86/x86_64/compat/entry.S
>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>> @@ -199,7 +199,7 @@ ENTRY(compat_post_handle_exception)
>>  
>>  /* See lstar_enter for entry register state. */
>>  ENTRY(cstar_enter)
>> -        /* sti could live here when we don't switch page tables below. */
>> +        ALTERNATIVE nop, sti, X86_FEATURE_NO_XPTI
>>          CR4_PV32_RESTORE
>>          movq  8(%rsp),%rax /* Restore %rax. */
>>          movq  $FLAT_KERNEL_SS,8(%rsp)
>> @@ -214,6 +214,7 @@ ENTRY(cstar_enter)
>>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>>  
>>          GET_STACK_END(bx)
>> +.Lcstar_cr3_start:
>>          mov   STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rcx
>>          neg   %rcx
>>          jz    .Lcstar_cr3_okay
>> @@ -223,6 +224,8 @@ ENTRY(cstar_enter)
>>          movq  $0, STACK_CPUINFO_FIELD(xen_cr3)(%rbx)
>>  .Lcstar_cr3_okay:
>>          sti
>> +.Lcstar_cr3_end:
>> +        ALTERNATIVE_NOP .Lcstar_cr3_start, .Lcstar_cr3_end, 
> X86_FEATURE_NO_XPTI
> 
> This is much clearer with the nop infrastructure abstracted away.
> 
> However, I remain unconvinced that this dynamic handling of interrupt
> re-enabling is worth the hoops you've jumped through to make it happen. 
> It might be interesting to hear others thoughts on the matter.
> 
> In particular, we've got a race window depending on the order in which
> the alternatives list is traversed where we might be unsafe.

But that ordering dependency is not just an issue with the interrupt
enabling: Note how certain pairs of ALTERNATIVE_NOP are carefully
ordered to first NOP out a later piece of code, and only then the
earlier one. There's an argument to be made that the solitary writing
back of %r15 could be left in place, as with the other pieces of code
patched out plus the earlier zeroing of registers, %r15 will then only
ever be zero, which is safe to write back. That zeroing code,
however, wasn't in the tree yet when this patch was first submitted.

Bottom line though - right now processing in order is a strict
requirement. Note that multiple patches to the same patch site
also depend on such ordering.

> On a tangent (which probably wont affect the result of this patch),
> given your thoughts to allow guests to notice and extend their own
> featureset, what about Xen?  If so, we're going to need something more
> clever than simply nopping out the code.

Well, if we want a runtime disable of xpti (like has been requested
by one of our customers, and ISTR you saying something like this
as well), then I don't think we should do this patching. But that
could be achieved by simply not setting the NO_XPTI feature (and
dropping cpu_has_no_xpti and its use), e.g. via an "xpti=dynamic"
command line option extension. As per the feature request we've
got this would need to be no more than the option of a one-time
disable; anything more involved (like flipping between modes)
would certainly be more complicated than is probably worth it.

Of course then there's this other consideration that Jürgen has
had with his series: If we want to make the page table switching
domain dependent (and in particular don't do so for Dom0), then
this patch either needs to go away altogether, or there would
need to be further restriction on when to set NO_XPTI (and
trigger patching).

Jan

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