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

Re: [PATCH v2 06/70] x86: Introduce support for CET-IBT


  • To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 17 Feb 2022 12:32:50 +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=kvTUJhiMRJWr+rrdO40Y/ElFkK53sUNEBhoi51pO0uU=; b=ZZ20rR85gUgiwCNmnNy45/Wuo//sJGMjK5P2N4MVzcDM7hkGJvUQ3QqGLq0FDUo9rO8AZ1KgpaprnCBoqzi5IYU45FmfVhi5Y7D2+Zbhp1pbincC6Ud11HAHiqcpK8tdRAq42O18m4HrETZQAKfty2SDl4HcxmY6VVC7ip2Kb6+AUvz21MPreh1LR7o/64DIpD9Lt8rK8FYuvaGQdnChhpHztEEy4/HezwrhajoKo72LvahP+HrMWU/08QBwwY2K8yBf/4UNWM7/SKQgtm0J4xrJMSCztlgXYr5awNZAF+ANENfxNIkPZDDnuzx2HkumblU5ErsrROes+sTa446m6g==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=fdgJfoxp9BWHCY0n9qJf6q8SrKsv5iDf4GOFSFmDxoOp9agqKrKHuxH9rIVESp/df+Vs/oiKFMz0Mgu6JoTCPlL6YbfwGmwbuHK8yDj8Ax1jSyp6wcVQ0phwxgprU1pJ1g5QIY6ijAnQ07dq7JRcFdxYthlIGmAvmB3f2LLHZqjPHUcx2CcT1iXbt+aDEkyDevToaEkCBHtdhb0SWtsRwnWcmmMUDx8nD2tb4C/oGebr2Z/NMAL2B1M5tBDm2hllYV5sexixPxWpsRLUfujAQHi7HB1rrdoFgZWu5KXr3iiNp3jC1G6EQimDz5nr1OSUt8Jax0w/Lbt7t8R8afdZxQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 17 Feb 2022 11:32:59 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 16.02.2022 22:54, Andrew Cooper wrote:
> On 15/02/2022 14:01, Jan Beulich wrote:
>> On 14.02.2022 13:50, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/Kconfig
>>> +++ b/xen/arch/x86/Kconfig
>>> @@ -39,6 +39,11 @@ config HAS_AS_CET_SS
>>>     # binutils >= 2.29 or LLVM >= 6
>>>     def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy)
>>>  
>>> +config HAS_CC_CET_IBT
>>> +   # GCC >= 9 and binutils >= 2.29
>>> +   # Retpoline check to work around 
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93654
>>> +   def_bool $(cc-option,-fcf-protection=branch -mmanual-endbr 
>>> -mindirect-branch=thunk-extern) && $(as-instr,endbr64)
>> At the top of asm-defns.h we have a number of similarly operand-less
>> instructions expressed via .macro expanding to .byte. I don't see why
>> we couldn't do so here as well, eliminating the need for the
>> $(as-instr ...). In fact ...
>>
>>> --- a/xen/arch/x86/include/asm/asm-defns.h
>>> +++ b/xen/arch/x86/include/asm/asm-defns.h
>>> @@ -57,6 +57,12 @@
>>>      INDIRECT_BRANCH jmp \arg
>>>  .endm
>>>  
>>> +#ifdef CONFIG_XEN_IBT
>>> +# define ENDBR64 endbr64
>>> +#else
>>> +# define ENDBR64
>>> +#endif
>> ... it could also be this macro which ends up conditionally empty,
>> but would then want expressing as an assembler macro. Albeit no, the
>> lower case form would probably still be needed to deal with compiler
>> emitted insns, as the compiler doesn't appear to make recognition of
>> the command line option dependent on the underlying assembler's
>> capabilities.
> 
> $(as-instr) isn't only for endbr64.  It also for the notrack prefix,
> which GCC does emit for any function pointer call laundered through void
> * even when everything was otherwise cf_check.
> 
> It's another area where treating the cf_check-ness as type-checking
> falls down, and created some very weird build failures until I figured
> out that Juergen's "Don't use the hypercall table for calling compat
> hypercalls" really did need to be a prerequisite.

Oh, I see. I can certainly accept this as a reason, but half a sentence
mentioning this would be nice in the description.

>>> --- a/xen/arch/x86/include/asm/cpufeatures.h
>>> +++ b/xen/arch/x86/include/asm/cpufeatures.h
>>> @@ -39,6 +39,7 @@ XEN_CPUFEATURE(SC_VERW_PV,        X86_SYNTH(23)) /* VERW 
>>> used by Xen for PV */
>>>  XEN_CPUFEATURE(SC_VERW_HVM,       X86_SYNTH(24)) /* VERW used by Xen for 
>>> HVM */
>>>  XEN_CPUFEATURE(SC_VERW_IDLE,      X86_SYNTH(25)) /* VERW used by Xen for 
>>> idle */
>>>  XEN_CPUFEATURE(XEN_SHSTK,         X86_SYNTH(26)) /* Xen uses CET Shadow 
>>> Stacks */
>>> +XEN_CPUFEATURE(XEN_IBT,           X86_SYNTH(27)) /* Xen uses CET Indirect 
>>> Branch Tracking */
>> Is a feature flag actually warranted here, rather than a single
>> global boolean? You don't key any alternatives patching to this
>> bit, unlike was the case for XEN_SHSTK. And the only consumer is
>> cpu_has_xen_ibt, expanding to the boot CPU's instance of the bit.
> 
> These are just bits.  They long predate alternatives finding a
> convenient use for the form, and are 8 times more compact than a global
> boolean, with better locality of reference too.

Well, I disagree (and we were here before, so I think you could have
predicted such a comment coming back). We should never have cloned this
directly from Linux. It's only bits, but with enough CPUs it sums up.
We shouldn't duplicate data when we need only a single instance (and
when no other infrastructure, like alternative patching, depends on it).

Last time you put me in a situation like this one, I told myself to not
ack such changes anymore, but here I am again - in the interest of not
being blamed for blocking this series:
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan




 


Rackspace

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