[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] x86/time: switch platform timer hooks to altcall


  • To: Andrew Cooper <amc96@xxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 12 Jan 2022 10:46:49 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=8wRJE8r0ttHr+IZQL1eZS+bKHIuMaBvoceWfiwkU7IQ=; b=DZM5vBm5SrLttJZLsgV27lp+Fc5MUXvA2XABSNzKlEONHkzdcYWmMfcDW0idpf6CqMS1kTFmH0jYptazk+k8WDveOLsetGgwM53jRZa3mhcaERppnw+3nJyK4+FUIYQtxQhiT+PvVNYJsmMMtpS3AC5GAYmlGOahsNyGi9J2/wqEJzGJudMiRxPbFIkbooszRXhXJDLmd5EQPalPq+P5b21kb+dTnyFASevgWKjbLKHFZpfO4ghvmBCv93LbMSAvhsBt8d7zQXRRDa+FcUVjbjBEqlrksGLfKlqcNqtZLV1txASpAp5CuoXoqjdI+4OYsOot5Bl6613u+kzX8PnjnA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=VC+UsErqswww8UFI/xsebg/n6492voGxq+fXa0HpLmClFFchzJEZN7kMxIQugdBtP9zc6ufvoiLVPtky4ooFAU8rPaFn/6X10FSX+mBht195uljlN55TdHOxhZ/Yc7UV2l4uD+15oH5drKEs7FFKrnUPdXF/QU0BHSwqAG8ffdQcaU3c4QaDzx0RIyDIowxQ9j6m2VjTD5D98RMTtxKiB+AUc5/w4n8yqyHLokZviWRcHiMTSfpeTOML//ERTFItvnk1if0MEF0JE32rGftNr26zcI6b9hKvyK2fOJsiljhiA5z+/6GPLli0/00GJeSvGa3kkoaFv3gCikAbHQ+v6w==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 12 Jan 2022 09:47:07 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 12.01.2022 10:17, Andrew Cooper wrote:
> On 12/01/2022 08:58, Jan Beulich wrote:
>> Except in the "clocksource=tsc" case we can replace the indirect calls
>> involved in accessing the platform timers by direct ones, as they get
>> established once and never changed.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> Sort of RFC, for both the whether and the how aspects.
>>
>> TBD: Overriding X86_FEATURE_ALWAYS is somewhat dangerous; there's only
>>      no issue with e.g. hvm_set_tsc_offset() used later in time.c
>>      because that's an inline function which did already "latch" the
>>      usual value of the macro. But the alternative of introducing an
>>      alternative_call() variant allowing to specify the controlling
>>      feature also doesn't look overly nice to me either. Then again the
>>      .resume hook invocation could be patched unconditionally, as the
>>      TSC clocksource leaves this hook set to NULL.
>>
>> --- a/xen/arch/x86/alternative.c
>> +++ b/xen/arch/x86/alternative.c
>> @@ -268,8 +268,7 @@ static void init_or_livepatch _apply_alt
>>               * point the branch destination is still NULL, insert "UD2; UD0"
>>               * (for ease of recognition) instead of CALL/JMP.
>>               */
>> -            if ( a->cpuid == X86_FEATURE_ALWAYS &&
>> -                 *(int32_t *)(buf + 1) == -5 &&
>> +            if ( *(int32_t *)(buf + 1) == -5 &&
> 
> I'm afraid that this must not become conditional.
> 
> One of the reasons I was hesitant towards the mechanics of altcall in
> the first place was that it intentionally broke Spectre v2 protections
> by manually writing out a non-retpoline'd indirect call.
> 
> This is made safe in practice because all altcall sites either get
> converted to a direct call, or rewritten to be a UD2.

Oh, sorry, I really should have realized this.

> If you want to make altcalls conversions conditional, then the code gen
> must be rearranged to use INDIRECT_CALL first, but that comes with
> overheads too because then call callsites would load the function
> pointer into a register, just to not use it in the patched case.

I don't view this as desirable.

> I suspect it will be easier to figure out how to make the TSC case also
> invariant after boot.

Since switching to "tsc" happens only after bringing up all CPUs, I don't
see how this could become possible; in particular I don't fancy an
alternatives patching pass with all CPUs online (albeit technically this
could be an option).

The solution (workaround) I can see for now is using

    if ( plt_src.read_counter != read_tsc )
        count = alternative_call(plt_src.read_counter);
    else
        count = rdtsc_ordered();

everywhere. Potentially we'd then want to hide this in a static (inline?)
function.

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.