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

[Laszlo Ersek <lersek@redhat.com>]

On 09/25/18 16:22, Laszlo Ersek wrote:
> 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.

The operand lists for almost all of the functions in

- MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
- MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c

are wrong. The InternalSyncIncrement() and InternalSyncDecrement() issue
has now bitten us in practice. The InternalSyncCompareExchangeXx()
functions also have incorrect operand lists, they just haven't blown up yet.

The core problem is that input-output operands for the inline assembly
templates must be marked as such. The GCC documentation describes two
ways for that:

- List the r/w operand only in the output operand list, and use the "+"
(not "=") constraint.

- Alternatively, list the operand in both input and output lists, and
use the "=" constraint in the output list. However, in the input list,
the constraing must be "N" (a decimal number), where N stands for the
operand number in the output list. This is how GCC knows those operands
are actually the same. Using just the same *expression* to fill in the
two instances (= input and output) of the operand is not guaranteed to
work; the gcc documentation explicitly highlights this fact.

In addition, in the ASM template, operands can be referenced by "[name]"
instead of %N. This has been supported since at least gcc-3.1. This
makes the template a lot more readable.

I'm working on a patch set.

Thanks
Laszlo