Re: [PATCH v2 0/2] Add volatile for mNumberToFinish

[Laszlo Ersek <lersek@redhat.com>]

Jeff,

On 11/15/16 15:08, Jeff Fan wrote:
> v2: 
>   Add patch #1 per Laszlo's comments
>   at https://lists.01.org/pipermail/edk2-devel/2016-November/004697.html
> 
> About the comments updated SynchronizationLib to add volatile for
> input parameter, I will send in another serial patches.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff.fan@intel.com>
> 
> Jeff Fan (2):
>   UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for parameter NumberToFinish
>   UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish
> 
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c             | 4 ++--
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 4 ++--
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h    | 2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c  | 2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 

if you want to keep GCC5.4 from optimizing away the access, the
synchronization object itself, and all pointers  to it must remain
volatile. Wherever you cast away the volatile qualifier, for example in
a function call, GCC can break code on the next level, even if you don't
actually access the object through that pointer (i.e., if you cast the
pointer back to volatile just in time for the access).

So, the safe way to go about this is to change function prototypes from
callee towards callers -- first change the callee (because both volatile
and non-volatile can be accepted as volatile), then change the caller
(make sure what you pass in is volatile, and propagate it outwards).

It is also okay to convert the original volatile pointer to UINTN, and
to pass it to assembly code like that, or to convert it back to a
volatile pointer from UINTN before use.

>From the C99 standard:

6.3 Conversions
6.3.2.3 Pointers

  2 For any qualifier q, a pointer to a non-q-qualified type may be
    converted to a pointer to the q-qualified version of the type; the
    values stored in the original and converted pointers shall compare
    equal.

  5 An integer may be converted to any pointer type. Except as
    previously specified, the result is implementation-defined, might
    not be correctly aligned, might not point to an entity of the
    referenced type, and might be a trap representation.

  6 Any pointer type may be converted to an integer type. Except as
    previously specified, the result is implementation-defined. If the
    result cannot be represented in the integer type, the behavior is
    undefined. The result need not be in the range of values of any
    integer type.

6.7.3 Type qualifiers, paragraph 5:

    If an attempt is made to modify an object defined with a
    const-qualified type through use of an lvalue with
    non-const-qualified type, the behavior is undefined. If an attempt
    is made to refer to an object defined with a volatile-qualified
    type through use of an lvalue with non-volatile-qualified type, the
    behavior is undefined.

In summary:

- casting away "volatile" even just temporarily (without actual
  accesses) may give gcc license to break the code (6.3.2.3 p2)

- accessing without volatile is known to break the code (6.7.3 p5)

- you can always cast from non-volatile to volatile (6.3.2.3 p2),
  but not the other way around!

- you can cast from (volatile VOID *) to UINTN and back as much as
  you want (6.3.2.3 p5 p6), because our execution environment makes
  that safe ("implementation-defined")

We might want to play loose with 6.3.2.3 p2 -- that is, cast away
volatile from the pointer only temporarily, and cast it back just before
accessing the object through the pointer --, but that could be unsafe in
the long term. The *really* safe method is to cast it to UINTN, and then
back the same way.

Yes, this would affect functions like SwitchStack() too -- the Context1
and Context2 parameters would have to change their types to UINTN.

I think what we should discuss at this point is whether we'd like to
care about 6.3.2.3 p2; that is, whether we consider casting away
volatile temporarily.

The direction I've been experiencing with GCC is that its optimization
methods are becoming more aggressive. For some optimizations, there are
flags that disable them; I'm not sure they provide a specific flag for
preventing GCC from exploiting violations of 6.3.2.3 p2.

Thanks
Laszlo