Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Wed, 20 Feb 2019 at 09:52, Jordan Justen <jordan.l.justen@intel.com> wrote:
>
> On 2019-02-18 01:32:53, Ard Biesheuvel wrote:
> > On Mon, 18 Feb 2019 at 10:08, Jordan Justen <jordan.l.justen@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.justen@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.
>
> 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 current versions of GCC, perhaps. But what about older versions?
What about future versions? What about Clang?

> 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?
>

Assuming that "y" affects frame pointer generation, yes.