On 02/18/19 10:32, Ard Biesheuvel wrote:
> On Mon, 18 Feb 2019 at 10:08, Jordan Justen <jordan.l.jus...@intel.com> wrote:
>>
>> On 2019-02-17 23:53:01, Ard Biesheuvel wrote:
>>> On Mon, 18 Feb 2019 at 05:12, Jordan Justen <jordan.l.jus...@intel.com>
>>> 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 <jordan.l.jus...@intel.com>
>>>> Cc: Laszlo Ersek <ler...@redhat.com>
>>>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
>>>> Cc: Anthony Perard <anthony.per...@citrix.com>
>>>> Cc: Julien Grall <julien.gr...@linaro.org>
>>>> ---
>>>> 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
