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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Laszlo,

Thanks for the details from and ANSI C spec.

For this compiler issue, are there more details on the 
assembly code generated by the GCC 5.4 compiler in the
failing mode?

I also see Liming's observation that the internal
implementation of the SynchronizationLib adds volatile
in some internal worker functions.  Other implementation
artifacts include the use of a read/write memory barrier
to prevent the optimizing compiler from optimizing
across a boundary.  These read/write barriers are used
in the spin lock functions, but not the Interlocked*()
functions.

I want to make sure we have studied the code generation
to see if the issue is related to volatile or a read/write
memory barrier.  It could be adding volatile forced the
compiler to internally add read/write barrier.

Also, given the implementation I see in the SynchronizationLib
I am not sure the cast to (UINT32 *) is required in the 
proposed patch.  Do we get compiler warnings/errors if those
casts are not included?

The second topic is what the SynchronizationLib API interfaces
should have been from the beginning.  In retrospect, I think
they should have been defined with volatile pointers.  The spin
lock APIs do use volatile pointers, but that is embedded in the 
typedef for SPIN_LOCK, so it is not as obvious.

Thanks,

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, November 15, 2016 8:10 AM
> To: Fan, Jeff <jeff.fan@intel.com>; edk2-devel@ml01.01.org
> Cc: Paolo Bonzini <pbonzini@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tian,
> Feng <feng.tian@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [PATCH v2 0/2] Add volatile for mNumberToFinish
> 
> 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