From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=148.163.143.35; helo=mx0b-002e3701.pphosted.com; envelope-from=brian.johnson@hpe.com; receiver=edk2-devel@lists.01.org Received: from mx0b-002e3701.pphosted.com (mx0b-002e3701.pphosted.com [148.163.143.35]) (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 74D4C21959CB2 for ; Tue, 19 Feb 2019 14:50:18 -0800 (PST) Received: from pps.filterd (m0148664.ppops.net [127.0.0.1]) by mx0b-002e3701.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id x1JMew3j002186; Tue, 19 Feb 2019 22:50:15 GMT Received: from g9t5008.houston.hpe.com (g9t5008.houston.hpe.com [15.241.48.72]) by mx0b-002e3701.pphosted.com with ESMTP id 2qrnq3js9b-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 19 Feb 2019 22:50:15 +0000 Received: from g4t3433.houston.hpecorp.net (g4t3433.houston.hpecorp.net [16.208.49.245]) by g9t5008.houston.hpe.com (Postfix) with ESMTP id 9EAEC53; Tue, 19 Feb 2019 22:50:14 +0000 (UTC) Received: from [10.33.152.19] (bjj-laptop2.americas.hpqcorp.net [10.33.152.19]) by g4t3433.houston.hpecorp.net (Postfix) with ESMTP id 05C1048; Tue, 19 Feb 2019 22:50:13 +0000 (UTC) To: Ard Biesheuvel , Jordan Justen Cc: Anthony Perard , "edk2-devel@lists.01.org" , Laszlo Ersek References: <20190218041141.21363-1-jordan.l.justen@intel.com> <20190218041141.21363-7-jordan.l.justen@intel.com> <155048090465.22654.1079629797155553207@jljusten-skl> From: "Brian J. Johnson" Message-ID: <39b584b5-7ab8-83b4-3e44-a861a7ef6491@hpe.com> Date: Tue, 19 Feb 2019 16:50:13 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: X-HPE-SCL: -1 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:, , definitions=2019-02-19_15:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1902190155 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: Tue, 19 Feb 2019 22:50:19 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2/18/19 3:32 AM, 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. > https://docs.microsoft.com/en-us/cpp/build/reference/oy-frame-pointer-omission?view=vs-2017 Brian > >>> >>>> 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? > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > -- Brian J. Johnson Enterprise X86 Lab Hewlett Packard Enterprise