RFR(S) 8162369: PPC64: Wrong ucontext used after SIGTRAP while in HTM critical section

Mon Aug 8 13:16:05 UTC 2016

Hi Volker,

Thanks a lot for reviewing.

Regards,
Gustavo

On 08-08-2016 09:30, Volker Simonis wrote:
> Hi Gustavo,
> 
> thanks a lot for the nice example and the detailed explanation.
> The new webrev looks good - Martin will push it.
> 
> Regards,
> Volker
> 
> 
> On Fri, Aug 5, 2016 at 4:58 AM, Gustavo Romero
> <gromero at linux.vnet.ibm.com> wrote:
>> Hi Volker,
>>
>> Thanks a lot for reviewing.
>>
>> On 04-08-2016 13:17, Volker Simonis wrote:
>>> thanks for the new webrev. It looks good in general. However I think we
>>> must check for uc being NULL. Something like:
>>>
>>> +  if (uc && uc->uc_link) {+    ucontext_t* second_uc = uc->uc_link;+
>>
>> Fixed.
>>
>>
>>> I also didn't fully understand the comment about LSB/MSB. Doesn't it mean
>>> that we need to compare the msr register against different masks depending
>>> on the endianess?
>>
>> It's just matter of notation, a different way to point out which bit is in
>> position 0 in a register. It's not related to the endianness because we are not
>> dealing how a word (word could mean 16 bits [Intel], 32 bits [POWER], or
>> anything else) or a register value is stored in memory, i.e the order bytes
>> composing the value in a register will be stored in memory. The MSR (a 64-bit
>> register) value can be stored in memory as BE or LE but when it is loaded back
>> into another 64-bit GPR register it (the value) is the same, regardless
>> the endianness.
>>
>> That's why in the Linux kernel, for instance, MSR Transactional State bits are
>> defined just once and not specific to any endianness (note that they differ here
>> from Power ISA, since C/C++ expect LSB 0 notation by convention in bitwise
>> operations, regardless of endianness):
>> https://github.com/torvalds/linux/blob/master/arch/powerpc/include/asm/reg.h#L32-L33
>>
>> MSB 0 is also known as IBM bit ordering/numbering. For example, as we can find
>> in this subtle comment in the code regarding the implementation of HTM
>> intrinsics on gcc:
>> https://github.com/gcc-mirror/gcc/blob/master/gcc/config/rs6000/htmintrin.h#L44-L45
>>
>> I wrote a simple example to show that the value of MSR won't change from BE to
>> LE: https://github.com/gromero/htm/blob/master/threads10.c
>>
>> This code when compiled and run in BE will produce the same result as in LE (you
>> can see the MSR TS bits that match the mask 0x600000000 flipping):
>>
>> [LE]
>>
>> gromero at gromero2:~/git/htm$ uname -a
>> Linux gromero2 4.4.0-28-generic #47-Ubuntu SMP Fri Jun 24 10:09:20 UTC 2016 ppc64le ppc64le ppc64le GNU/Linux
>> gromero at gromero2:~/git/htm$ make threads10
>> gcc -c -g -O0 threads10.c -o threads10.o
>> gcc -pthread -mhtm -mcpu=power8 -g -O0 threads10.o -o threads10
>> gromero at gromero2:~/git/htm$ ./threads10
>> => I'm thread 3fffa078f1a0. I'll perform JUST A TRAP every 2 seconds
>> => I'm thread 3fff9ff8f1a0. I'll perform A TRAP IN TRANSACTION every 1 second
>> * Caught a SIGTRAP in transaction:         uc->uc_mcontext.regs->msr = 0x800000050282d033
>> => I'm thread 3fff9ff8f1a0. I'll perform A TRAP IN TRANSACTION every 1 second
>> * Caught a SIGTRAP but NOT in transaction: uc->uc_mcontext.regs->msr = 0x800000010282d033
>> => I'm thread 3fffa078f1a0. I'll perform JUST A TRAP every 2 seconds
>> * Caught a SIGTRAP in transaction:         uc->uc_mcontext.regs->msr = 0x800000050282d033
>> => I'm thread 3fff9ff8f1a0. I'll perform A TRAP IN TRANSACTION every 1 second
>> * Caught a SIGTRAP in transaction:         uc->uc_mcontext.regs->msr = 0x800000050282d033
>> => I'm thread 3fff9ff8f1a0. I'll perform A TRAP IN TRANSACTION every 1 second
>> * Caught a SIGTRAP but NOT in transaction: uc->uc_mcontext.regs->msr = 0x800000010282d033
>> => I'm thread 3fffa078f1a0. I'll perform JUST A TRAP every 2 seconds
>> * Caught a SIGTRAP in transaction:         uc->uc_mcontext.regs->msr = 0x800000050282d033
>> => I'm thread 3fff9ff8f1a0. I'll perform A TRAP IN TRANSACTION every 1 second
>> * Caught a SIGTRAP in transaction:         uc->uc_mcontext.regs->msr = 0x800000050282d033
>> => I'm thread 3fff9ff8f1a0. I'll perform A TRAP IN TRANSACTION every 1 second
>> ...
>>
>> [BE]
>>
>> [root at localhost htm]# uname -a
>> Linux localhost 3.10.0-327.18.2.el7.ppc64 #1 SMP Fri Apr 8 05:12:29 EDT 2016 ppc64 ppc64 ppc64 GNU/Linux
>> [root at localhost htm]# make threads10
>> gcc -c -g -O0 threads10.c -o threads10.o
>> gcc -pthread -mhtm -mcpu=power8 -g -O0 threads10.o -o threads10
>> [root at localhost htm]# ./threads10
>> => I'm thread 3fffa902f1d0. I'll perform JUST A TRAP every 2 seconds
>> => I'm thread 3fffa882f1d0. I'll perform A TRAP IN TRANSACTION every 1 second
>> * Caught a SIGTRAP in transaction:         uc->uc_mcontext.regs->msr = 0x800000050282d032
>> => I'm thread 3fffa882f1d0. I'll perform A TRAP IN TRANSACTION every 1 second
>> * Caught a SIGTRAP but NOT in transaction: uc->uc_mcontext.regs->msr = 0x800000010282d032
>> => I'm thread 3fffa902f1d0. I'll perform JUST A TRAP every 2 seconds
>> * Caught a SIGTRAP in transaction:         uc->uc_mcontext.regs->msr = 0x800000050282d032
>> => I'm thread 3fffa882f1d0. I'll perform A TRAP IN TRANSACTION every 1 second
>> * Caught a SIGTRAP in transaction:         uc->uc_mcontext.regs->msr = 0x800000050282d032
>> => I'm thread 3fffa882f1d0. I'll perform A TRAP IN TRANSACTION every 1 second
>> * Caught a SIGTRAP but NOT in transaction: uc->uc_mcontext.regs->msr = 0x800000010282d032
>> => I'm thread 3fffa902f1d0. I'll perform JUST A TRAP every 2 seconds
>> * Caught a SIGTRAP in transaction:         uc->uc_mcontext.regs->msr = 0x800000050282d032
>> => I'm thread 3fffa882f1d0. I'll perform A TRAP IN TRANSACTION every 1 second
>> * Caught a SIGTRAP in transaction:         uc->uc_mcontext.regs->msr = 0x800000050282d032
>> => I'm thread 3fffa882f1d0. I'll perform A TRAP IN TRANSACTION every 1 second
>> ...
>>
>> new webrev:
>> http://81.de.7a9f.ip4.static.sl-reverse.com/8162369/v3
>>
>> Thank you!
>>
>> Regards,
>> Gustavo
>>
> 