From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C171381E08 for ; Wed, 16 Nov 2016 10:18:34 -0800 (PST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP; 16 Nov 2016 10:18:39 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,649,1473145200"; d="scan'208";a="5159537" Received: from orsmsx107.amr.corp.intel.com ([10.22.240.5]) by orsmga002.jf.intel.com with ESMTP; 16 Nov 2016 10:18:39 -0800 Received: from orsmsx163.amr.corp.intel.com (10.22.240.88) by ORSMSX107.amr.corp.intel.com (10.22.240.5) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 16 Nov 2016 10:18:39 -0800 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.63]) by ORSMSX163.amr.corp.intel.com ([169.254.9.53]) with mapi id 14.03.0248.002; Wed, 16 Nov 2016 10:18:39 -0800 From: "Kinney, Michael D" To: Laszlo Ersek , "Fan, Jeff" , "edk2-devel@ml01.01.org" , "Kinney, Michael D" CC: Paolo Bonzini , "Yao, Jiewen" , "Tian, Feng" Thread-Topic: [PATCH v2 0/2] Add volatile for mNumberToFinish Thread-Index: AQHSP1rNjKZBHgmXqkGDdQ/d754BhqDb6ZDA Date: Wed, 16 Nov 2016 18:18:38 +0000 Message-ID: References: <20161115140809.5292-1-jeff.fan@intel.com> <605a5187-5b91-e0f8-5560-8a02ddad5057@redhat.com> In-Reply-To: <605a5187-5b91-e0f8-5560-8a02ddad5057@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMzY1YTdjMjEtODg0ZC00MDgxLWIzYjQtM2U4MjUxMzExNDJiIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IjhWeCt4WVZ5NVV5XC81VWhLUGNHZFhWTkdtSnplc1JUZVlcL1h0RHU0b1hLbz0ifQ== x-originating-ip: [10.22.254.140] MIME-Version: 1.0 Subject: Re: [PATCH v2 0/2] Add volatile for mNumberToFinish X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Nov 2016 18:18:34 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, Thanks for the details from and ANSI C spec. For this compiler issue, are there more details on the=20 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=20 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=20 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 ; edk2-devel@ml01.01.org > Cc: Paolo Bonzini ; Yao, Jiewen ; Tian, > Feng ; Kinney, Michael D > Subject: Re: [PATCH v2 0/2] Add volatile for mNumberToFinish >=20 > Jeff, >=20 > 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.htm= l > > > > About the comments updated SynchronizationLib to add volatile for > > input parameter, I will send in another serial patches. > > > > Cc: Paolo Bonzini > > Cc: Laszlo Ersek > > Cc: Jiewen Yao > > Cc: Feng Tian > > Cc: Michael D Kinney > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Jeff Fan > > > > 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(-) > > >=20 > 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). >=20 > 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). >=20 > 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. >=20 > From the C99 standard: >=20 > 6.3 Conversions > 6.3.2.3 Pointers >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 6.7.3 Type qualifiers, paragraph 5: >=20 > 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. >=20 > In summary: >=20 > - casting away "volatile" even just temporarily (without actual > accesses) may give gcc license to break the code (6.3.2.3 p2) >=20 > - accessing without volatile is known to break the code (6.7.3 p5) >=20 > - you can always cast from non-volatile to volatile (6.3.2.3 p2), > but not the other way around! >=20 > - 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") >=20 > 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. >=20 > Yes, this would affect functions like SwitchStack() too -- the Context1 > and Context2 parameters would have to change their types to UINTN. >=20 > 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. >=20 > 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. >=20 > Thanks > Laszlo