From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.31; helo=mga06.intel.com; envelope-from=jordan.l.justen@intel.com; receiver=edk2-devel@lists.01.org Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (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 DB23E201B03EE for ; Wed, 20 Feb 2019 00:52:19 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 Feb 2019 00:52:19 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,390,1544515200"; d="scan'208";a="321828720" Received: from mmdandap-mobl1.amr.corp.intel.com (HELO localhost) ([10.254.8.66]) by fmsmga005.fm.intel.com with ESMTP; 20 Feb 2019 00:52:18 -0800 MIME-Version: 1.0 In-Reply-To: References: <20190218041141.21363-1-jordan.l.justen@intel.com> <20190218041141.21363-7-jordan.l.justen@intel.com> <155048090465.22654.1079629797155553207@jljusten-skl> From: Jordan Justen To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , Laszlo Ersek , Anthony Perard , Julien Grall Message-ID: <155065273839.12518.3314562596426152677@jljusten-skl> User-Agent: alot/0.8 Date: Wed, 20 Feb 2019 00:52:18 -0800 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: Wed, 20 Feb 2019 08:52:20 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2019-02-18 01:32:53, Ard Biesheuvel wrote: > On Mon, 18 Feb 2019 at 10:08, Jordan Justen w= rote: > > > > 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? > > >=20 > 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. >=20 > 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. Based on: https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html It appears that -O0 will not have -fomit-frame-pointer, since that is added in -O1. For both gcc and MSVC, I think we could be more targeted: #ifdef __GNUC__ #pragma GCC push_options #pragma GCC optimize ("no-omit-frame-pointer") #else #pragma optimize ("y", off) #endif Do you prefer this version? -Jordan > For MSVC, I have no idea how to tweak the compiler to force it to emit > frame pointers. >=20 >=20 > > > > > > > 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?