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

[Jordan Justen <jordan.l.justen@intel.com>]

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 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.
> 
> 
> > >
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > Cc: Anthony Perard <anthony.perard@citrix.com>
> > > > Cc: Julien Grall <julien.grall@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?