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 34878383FD74 for ; Tue, 6 Dec 2022 16:24:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 34878383FD74 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 ADBBE23A; Tue, 6 Dec 2022 08:24:30 -0800 (PST) Received: from [10.2.78.76] (unknown [10.2.78.76]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 4E87B3F73B; Tue, 6 Dec 2022 08:24:23 -0800 (PST) Message-ID: <4917eabd-7b0b-a402-3680-f5591a7bb039@foss.arm.com> Date: Tue, 6 Dec 2022 16:24:21 +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 10/15 V4] arm: Implement cortex-M return signing address codegen Content-Language: en-GB To: Andrea Corallo Cc: Andrea Corallo via Gcc-patches , Richard Earnshaw , nd References: From: Richard Earnshaw In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3489.9 required=5.0 tests=BAYES_00,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 06/12/2022 15:46, Andrea Corallo wrote: > Hi Richard, > > thanks for reviewing. > > Just one clarification before I complete the respin of this patch. > > Richard Earnshaw writes: > > [...] > >> Also, I think (out of an abundance of caution) we really need a >> scheduling barrier placed before calls to gen_aut_nop() pattern is >> emitted, to ensure that the scheduler never tries to move this >> instruction away from the position we place it. Use gen_blockage() >> for that (see TARGET_SCHED_PROLOG). Alternatively, we could make the >> UNSPEC_PAC_NOP an unspec_volatile, which has the same effect (IIRC) >> without needing an additional insn - if you use this approach, then >> please make sure this is explained in a comment. >> >> +(define_insn "pacbti_nop" >> + [(set (reg:SI IP_REGNUM) >> + (unspec:SI [(reg:SI SP_REGNUM) (reg:SI LR_REGNUM)] >> + UNSPEC_PACBTI_NOP))] >> + "arm_arch8m_main" >> + "pacbti\t%|ip, %|lr, %|sp" >> + [(set_attr "conds" "unconditional")]) >> >> The additional side-effect of this being a BTI landing pad means that >> we mustn't move any other instruction before it. So I think this >> needs to be an unspec_volatile as well. > > IIUC from this we want to make all the three (UNSPEC_PAC_NOP, > UNSPEC_PACBTI_NOP, UNSPEC_AUT_NOP) unspec volatile, correct? UNSPEC_PAC_NOP doesn't need to be volatile. The register constraints will be enough to ensure it is run before any instruction that consumes the result it produces. UNSPEC_PAC_BTI_NOP needs to be volatile, as it's essential that when we have an instruction (for example ldr r3, [r3]) early in the program that doesn't interact with the prologue then it cannot be migrated before the BTI as the BTI is a landing pad and must be the first instruction in the function. This is why UNSPEC_BTI_NOP is volatile. UNSPEC_AUT_NOP must be volatile because we want to ensure that no instruction is moved after this one and before the return as that might expose a ROP gadget to hackers. R. > > IIUC correctly the scheduler should not reorder them as we have > expressed which register they consume and produce but is for double > caution correct? > >> On the tests, they are OK as they stand, but we lack anything that >> will be tested when suitable hardware is unavailable (all tests are >> "dg-do run"). Can we please have some compile-only tests as well? > > Ack. > > BR > > Andrea