From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 7E8CC387220C for ; Wed, 14 Dec 2022 17:03:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7E8CC387220C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=foss.arm.com Authentication-Results: sourceware.org; spf=none smtp.mailfrom=foss.arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D2C32FEC; Wed, 14 Dec 2022 09:04:02 -0800 (PST) Received: from [10.57.11.95] (unknown [10.57.11.95]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 59BC93F5A1; Wed, 14 Dec 2022 09:03:21 -0800 (PST) Message-ID: Date: Wed, 14 Dec 2022 17:03:19 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [PATCH 12/15 V4] arm: implement bti injection Content-Language: en-GB To: Andrea Corallo Cc: Kyrylo Tkachov , Richard Earnshaw , nd , Andrea Corallo via Gcc-patches References: <30990d91-0c32-a3b0-7954-846bf1eddc8d@foss.arm.com> <7db3982c-cd42-023f-0dd1-0eb7b7dbfb20@foss.arm.com> From: Richard Earnshaw In-Reply-To: <7db3982c-cd42-023f-0dd1-0eb7b7dbfb20@foss.arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3488.1 required=5.0 tests=BAYES_00,BODY_8BITS,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,NICE_REPLY_A,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 14/12/2022 17:00, Richard Earnshaw via Gcc-patches wrote: > > > On 14/12/2022 16:40, Andrea Corallo via Gcc-patches wrote: >> Hi Richard, >> >> thanks for reviewing. >> >> Richard Earnshaw writes: >> >>> On 28/10/2022 17:40, Andrea Corallo via Gcc-patches wrote: >>>> Hi all, >>>> please find attached the third iteration of this patch addresing >>>> review >>>> comments. >>>> Thanks >>>>     Andrea >>>> >>> >>> @@ -23374,12 +23374,6 @@ output_probe_stack_range (rtx reg1, rtx reg2) >>>     return ""; >>>   } >>> >>> -static bool >>> -aarch_bti_enabled () >>> -{ >>> -  return false; >>> -} >>> - >>>   /* Generate the prologue instructions for entry into an ARM or Thumb-2 >>>      function.  */ >>>   void >>> @@ -32992,6 +32986,61 @@ arm_current_function_pac_enabled_p (void) >>>                 && !crtl->is_leaf)); >>>   } >>> >>> +/* Return TRUE if Branch Target Identification Mechanism is >>> enabled.  */ >>> +bool >>> +aarch_bti_enabled (void) >>> +{ >>> +  return aarch_enable_bti == 1; >>> +} >>> >>> See comment in earlier patch about the location of this function >>> moving.   Can aarch_enable_bti take values other than 0 and 1? >> >> Yes default is 2. > > It shouldn't be by this point, because, hopefully you've gone through > the equivalent of this hunk (from aarch64) somewhere in > arm_override_options: > > >    if (aarch_enable_bti == 2) >      { >  #ifdef TARGET_ENABLE_BTI >        aarch_enable_bti = 1; >  #else >        aarch_enable_bti = 0; >  #endif >      } > > And after this point the '2' should never be seen again.  We use this > trick to permit the user to force a default that differs from the > configuration. > > However, I don't see a hunk to do this in patch 3, so perhaps that needs > updating to fix this. I've just remembered that the above is to support a configure-time option of the compiler to enable branch protection. But perhaps we don't want to have that in AArch32, in which case it would be better not to have the default be 2 anyway, just default to off (0). R. > > > >> [...] >> >>> +  return GET_CODE (pat) == UNSPEC_VOLATILE && XINT (pat, 1) == >>> UNSPEC_BTI_NOP; >>> >>> I'm not sure where this crept in, but UNSPEC and UNSPEC_VOLATILE have >>> separate enums in the backend, so UNSPEC_BIT_NOP should really be >>> VUNSPEC_BTI_NOP and defined in the enum "unspecv". >> >> Done >> >>> +aarch_pac_insn_p (rtx x) >>> +{ >>> +  if (!x || !INSN_P (x)) >>> +    return false; >>> + >>> +  rtx pat = PATTERN (x); >>> + >>> +  if (GET_CODE (pat) == SET) >>> +    { >>> +      rtx tmp = XEXP (pat, 1); >>> +      if (tmp >>> +      && GET_CODE (tmp) == UNSPEC >>> +      && (XINT (tmp, 1) == UNSPEC_PAC_NOP >>> +          || XINT (tmp, 1) == UNSPEC_PACBTI_NOP)) >>> +    return true; >>> +    } >>> + >>> >>> This will also need updating (see review on earlier patch) because >>> PACBTI needs to be unspec_volatile, while PAC doesn't. >> >> Done >> >>> +/* The following two functions are for code compatibility with aarch64 >>> +   code, this even if in arm we have only one bti instruction.  */ >>> + >>> >>> I'd just write >>>   /* Target specific mapping for aarch_gen_bti_c and >>>   aarch_gen_bti_j. For Arm, both of these map to a simple BTI >>> instruction.  */ >> >> Done >> >>> >>> @@ -162,6 +162,7 @@ (define_c_enum "unspec" [ >>>     UNSPEC_PAC_NOP    ; Represents PAC signing LR >>>     UNSPEC_PACBTI_NOP    ; Represents PAC signing LR + valid landing pad >>>     UNSPEC_AUT_NOP    ; Represents PAC verifying LR >>> +  UNSPEC_BTI_NOP    ; Represent BTI >>>   ]) >>> >>> BTI is an unspec volatile, so this should be in the "vunspec" enum and >>> renamed accordingly (see above). >> >> Done. >> >> Please find attached the updated version of this patch. >> >> BR >> >>    Andrea >> > > Apart from that, this is OK. > > R.