From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.100; helo=mga07.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (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 8147621B02822 for ; Wed, 26 Sep 2018 02:33:50 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Sep 2018 02:33:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,305,1534834800"; d="scan'208";a="76301318" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.8]) ([10.239.9.8]) by orsmga008.jf.intel.com with ESMTP; 26 Sep 2018 02:33:48 -0700 To: Laszlo Ersek , edk2-devel-01 Cc: Michael Kinney , Jiewen Yao , Liming Gao References: <20180925194857.10514-1-lersek@redhat.com> <7935451f-6648-a383-5be2-007ab77c3a36@redhat.com> From: "Ni, Ruiyu" Message-ID: Date: Wed, 26 Sep 2018 17:34:45 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <7935451f-6648-a383-5be2-007ab77c3a36@redhat.com> Subject: Re: [PATCH] MdePkg/BaseSynchronizationLib: fix XADD operands in GCC IA32/X64 assembly X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 26 Sep 2018 09:33:50 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 9/26/2018 5:05 PM, Laszlo Ersek wrote: > Hi, > > On 09/25/18 21:48, Laszlo Ersek wrote: >> Currently, "gcc-4.8.5-28.el7_5.1.x86_64" generates the following code for >> me, from the XADD inline assembly added to "X64/GccInline.c" in commit >> 17634d026f96: >> >>> 0000000000004383 : >>> UINT32 >>> EFIAPI >>> InternalSyncIncrement ( >>> IN volatile UINT32 *Value >>> ) >>> { >>> 4383: 55 push %rbp >>> 4384: 48 89 e5 mov %rsp,%rbp >>> 4387: 48 83 ec 10 sub $0x10,%rsp >>> 438b: 48 89 4d 10 mov %rcx,0x10(%rbp) >>> UINT32 Result; >>> >>> __asm__ __volatile__ ( >>> 438f: 48 8b 55 10 mov 0x10(%rbp),%rdx >>> 4393: 48 8b 45 10 mov 0x10(%rbp),%rax >>> 4397: b8 01 00 00 00 mov $0x1,%eax >>> 439c: f0 0f c1 00 lock xadd %eax,(%rax) >>> 43a0: ff c0 inc %eax >>> 43a2: 89 45 fc mov %eax,-0x4(%rbp) >>> : "m" (*Value) // %2 >>> : "memory", >>> "cc" >>> ); >>> >>> return Result; >>> 43a5: 8b 45 fc mov -0x4(%rbp),%eax >>> } >>> 43a8: c9 leaveq >>> 43a9: c3 retq >>> >> >> The MOV $0X1,%EAX instruction corrupts the address of Value in %RAX before >> we reach the XADD instruction. In fact, it makes no sense for XADD to use >> %EAX as source operand and (%RAX) as destination operand at the same time. > > may I get a fast review for this patch, please? The regression from > commit 17634d026f96 prevents OVMF from booting. Sure. Reviewed-by: Ruiyu Ni > > Thanks > Laszlo > -- Thanks, Ray