Re: [PATCH] MdePkg/BaseSynchronizationLib: fix XADD operands in GCC IA32/X64 assembly

[Laszlo Ersek <lersek@redhat.com>]

On 09/26/18 11:34, Ni, Ruiyu wrote:
> 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 <InternalSyncIncrement>:
>>>> 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 <ruiyu.ni@intel.com>

Thanks, Ray! Commit 8a94eb9283fa.
Laszlo