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

2019-02-20 Thread Ard Biesheuvel
On Wed, 20 Feb 2019 at 09:52, Jordan Justen  wrote:
>
> On 2019-02-18 01:32:53, 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.
>
> 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.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


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

2019-02-20 Thread Jordan Justen
On 2019-02-18 01:32:53, 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.

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


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

2019-02-19 Thread Jordan Justen
On 2019-02-19 14:50:13, Brian J. Johnson wrote:
> 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

Hmm, and based on:

https://docs.microsoft.com/en-us/cpp/preprocessor/optimize?view=vs-2017

#pragma optimize ("", off)

...includes the "y" option.

This 2nd page seems a little confused, as it documents "y" as
"Generate frame pointers on the program stack", while the 1st page
says "Suppresses creation of frame pointers on the call stack".

I think the "suppress" is more accurate as it makes more sense that
suppressing the frame pointer gives better optimization opportunities.

Anyway, I think that means that `#pragma optimize ("", off)` does what
we want on MSVC to force frame pointers to be used.

-Jordan
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


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

2019-02-19 Thread Brian J. Johnson

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

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


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

2019-02-18 Thread Laszlo Ersek
On 02/18/19 10:32, 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.

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


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

2019-02-18 Thread Ard Biesheuvel
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.


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


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

2019-02-18 Thread Jordan Justen
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?

Thanks,

-Jordan

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


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

2019-02-17 Thread Ard Biesheuvel
On Mon, 18 Feb 2019 at 05:12, Jordan Justen  wrote:
>

This needs an explanation why optimization needs to be disabled.

> 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


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

2019-02-17 Thread Jordan Justen
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
-- 
2.20.0.rc1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel