From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 3219B2194EB70 for ; Mon, 18 Feb 2019 05:01:12 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A3789C7E8B; Mon, 18 Feb 2019 13:01:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-242.rdu2.redhat.com [10.10.120.242]) by smtp.corp.redhat.com (Postfix) with ESMTP id 37B5B5D706; Mon, 18 Feb 2019 13:01:10 +0000 (UTC) To: Ard Biesheuvel , Jordan Justen Cc: "edk2-devel@lists.01.org" , Anthony Perard , Julien Grall References: <20190218041141.21363-1-jordan.l.justen@intel.com> <20190218041141.21363-7-jordan.l.justen@intel.com> <155048090465.22654.1079629797155553207@jljusten-skl> From: Laszlo Ersek Message-ID: <40de24be-a8e0-7fcb-5c46-8367cc64a0bb@redhat.com> Date: Mon, 18 Feb 2019 14:01:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Mon, 18 Feb 2019 13:01:11 +0000 (UTC) Subject: Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Feb 2019 13:01:12 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/18/19 10:32, Ard Biesheuvel wrote: > On Mon, 18 Feb 2019 at 10:08, Jordan Justen wrote: >> >> On 2019-02-17 23:53:01, Ard Biesheuvel wrote: >>> On Mon, 18 Feb 2019 at 05:12, Jordan Justen wrote: >>>> >>> >>> This needs an explanation why optimization needs to be disabled. >> >> I'm not sure this is required. The reason I added these patches is to >> hopefully prevent the compiler from removing the frame pointer. We >> adjust the frame pointer in the code, and that is a little sketchy if >> the frame pointer isn't being used. >> >> Unfortunately, it can reasonably be argued that the >> TemporaryRamSupport PPI definition ultimately makes it unsafe to write >> the migration code in C. >> >> I tried reverting both the EmulatorPkg and OvmfPkg patches for >> disabling the optimizations, and with my setup there was no impact. I >> think there is a good change that we'd be pretty safe to just drop >> these two patches to wait and see if someone encounters a situation >> that requires it. >> >> Ok, so based on this explanation, do you think I should add info to >> the commit message and keep the patches, or just drop them? >> > > I think 'little sketchy' is an understatement here (as is > setjmp/longjmp in general), but it is the reality we have to deal with > when writing startup code in C. Looking at the code, I agree that the > fact that [re]bp is assigned directly implies that we should not > permit it to be used as a general purpose register, especially when > you throw LTO into the mix, which could produce all kinds of > surprising results when it decides to inline functions being called > from here. > > For GCC/Clang, I don't think it is correct to assume that changing the > optimization level will result in -fno-omit-frame-pointer to be set, > so I'd prefer setting that option directly, either via the pragma, or > for the whole file. > > For MSVC, I have no idea how to tweak the compiler to force it to emit > frame pointers. I think modifying the build options in the INF file would be more readable. Thanks Laszlo >>> >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Jordan Justen >>>> Cc: Laszlo Ersek >>>> Cc: Ard Biesheuvel >>>> Cc: Anthony Perard >>>> Cc: Julien Grall >>>> --- >>>> OvmfPkg/Sec/SecMain.c | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c >>>> index 46ac739862..86c22a2ac9 100644 >>>> --- a/OvmfPkg/Sec/SecMain.c >>>> +++ b/OvmfPkg/Sec/SecMain.c >>>> @@ -873,6 +873,13 @@ SecStartupPhase2( >>>> CpuDeadLoop (); >>>> } >>>> >>>> +#ifdef __GNUC__ >>>> +#pragma GCC push_options >>>> +#pragma GCC optimize ("O0") >>>> +#else >>>> +#pragma optimize ("", off) >>>> +#endif >>>> + >>>> EFI_STATUS >>>> EFIAPI >>>> TemporaryRamMigration ( >>>> @@ -946,3 +953,8 @@ TemporaryRamMigration ( >>>> return EFI_SUCCESS; >>>> } >>>> >>>> +#ifdef __GNUC__ >>>> +#pragma GCC pop_options >>>> +#else >>>> +#pragma optimize ("", on) >>>> +#endif >>> >>> I can't tell from the context if this is the end of the file, but if >>> it is not, aren't you turning on optimization here for non-GCC even if >>> it was not enabled on the command line to begin with?