Re: [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value

[Laszlo Ersek <lersek@redhat.com>]

On 09/13/18 11:50, Ruiyu Ni wrote:

> diff --git a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
> index 5224dd063f..4c4d6e3fc7 100644
> --- a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
> +++ b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
> @@ -15,14 +15,12 @@
>
>
>
> -
>  /**
>    Performs an atomic increment of an 32-bit unsigned integer.
>
>    Performs an atomic increment of the 32-bit unsigned integer specified by
>    Value and returns the incremented value. The increment operation must be
> -  performed using MP safe mechanisms. The state of the return value is not
> -  guaranteed to be MP safe.
> +  performed using MP safe mechanisms.
>
>    @param  Value A pointer to the 32-bit value to increment.
>
> @@ -38,9 +36,10 @@ InternalSyncIncrement (
>    UINT32  Result;
>
>    __asm__ __volatile__ (
> +    "movl    $1, %%eax  \n\t"
>      "lock               \n\t"
> -    "incl    %2         \n\t"
> -    "mov     %2, %%eax      "
> +    "xadd    %%eax, %2  \n\t"
> +    "inc     %%eax          "
>      : "=a" (Result),          // %0
>        "=m" (*Value)           // %1
>      : "m"  (*Value)           // %2

The operand list for the inline assembly is incorrect, I suspect. When I
build this code with GCC48 as part of OVMF, for the NOOPT target, then
disassemble "CpuMpPei.debug", I get:

> 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 generated code is trying to use EAX for both (a) the
exchange-and-add, i.e.  as the source operand of the XADD instruction,
and (b) as (part of) the address of the (*Value) argument.

In effect, before we reach the XADD instruction, the least significant
dword of Value's address is overwritten with 0x0000_0001. The exchange
then happens against that memory location.

It is interesting that GCC first spills the parameter from RCX to the
stack, and then it loads the parameter from the stack to both RDX and
RAX.

I think the operand list used with the inline assembly should be updated
to prevent GCC from associating %2 with EAX.

I'll try to experiment some more.

Thanks
Laszlo