Powerpc64: Enable Hardware Transactional Memory

[jhibbits]

Summary

This patch enable Hardware Transactional Memory (HTM) facility.

The facility is only enabled on a process that executes a HTM instruction. It
will raise and facility trap, which will enable flag the process with PCB_HTM,
from there after, the context switch will save and restore the 3 SPRs that are
associated with HTM.

[jhibbits]

sys/powerpc/include/htm.h

45–51

Why not make these return bool? It looks like they're not used anywhere other than conditional checks.

sys/powerpc/powerpc/trap.c

303

I think SIGTRAP is the wrong signal here. Maybe SIGILL with ucode of ILL_ILLTRP (illegal trap)?

387

Maybe we want to abort the transaction and kill the process in here? SIGILL, or SIGTRAP, or such like that? I don't think it's right to just ignore a trap instruction.

Actually, reading above, the transaction is aborted already anyway at this point so why not follow through? I'm not sure what the right thing to do is. Maybe, like above, it should be SIGILL/ILL_ILLTRP.

Nathan, do you have an opinion?

[breno.leitao_gmail.com]

Changes according to the Justin's review:

• Changed return type for htm_enabled, htm_transactional and htm_suspended
• Returning SIGILL if calling systemcall (instead of SIGTRAP) inside a transaction.
• Returning SIGILL instead of ignoring a trap instruction inside a transaction, thus, avoiding calling kdb_trap() also.

[breno.leitao_gmail.com]

sys/powerpc/powerpc/trap.c

387

Hi Justin,

Right, the tabort just dommed the transaction, but it is still suspended, and it will be aborted when we return the userspace (rfid). In this case, the transaction will try to continue and it will fail, returning to the the next line after tbegin. (usually a bne XX).

I just created another revision with SIGILL/ILL_ILLTRP, but I needed to avoid adding a breakpoint inside the transaction, as showed in my new patch at db_trap_glue.

[jhibbits]

The code looks fine to me. Can you include a test for it? You showed me the torture test, so that'd be good to include in tools/tools. With a test, I'll approve it.

[breno.leitao_gmail.com]

Add aditional tests. Mainly two things:

• Instead of testing for transactional states, look for active (Transactional + Suspended)
• Avoid signal handling inside a transaction, since the code will be rolled back.

[breno.leitao_gmail.com]

Adding additional testsuite for the code.

• Powerpc64: Enable Hardware Transactional Memory
• Powerpc64: Add HTM example

[breno.leitao_gmail.com]

This patchset has one caveat at this time that will be fixed soon. If we go into kernel space during the transaction, and the process is rescheduled to another cpu, the transaction state will be on the original CPU, and the new CPU will not have the same CPU state, caused an undefined behaviour.

The right way to solve it is calling 'treclaim' (reclaim the checkpointed values from registers and save them in memory) and, later, calling trechekpoint (put the register back to the checkpointed area, i.e, loading the checkpointed registers, calling trecheckpoint, and restoring the GPRs, once the CPU is rescheduled.

In order to do so, I will need to call 'treclaim' instead of 'tabort' at the beginning of the execption (in file sys/powerpc/powerpc/trap.c). and implement treclaim, which is not very trivial, since it will put all the registers back to userspace while we are in kernel space. Later, call trecheckpoint when a new process, which was aborted in the middle of a transaction, is rescheduled back to the CPU.

[breno.leitao_gmail.com]

Minor fixes

[jhibbits]

@breno.leitao_gmail.com Can you pull out the pcb.h and pcpu.h changes into a separate diff? That way we can get that in before the KBI freeze, and wait until 12.1 to get the actual code in. Once KBI is frozen it's frozen for the full 12.0 life, and any KBI breakage we want to perform will have to wait for 13.0.

sys/powerpc/include/pcpu.h

48

One thing to note is you need to adjust the padding down below, else it should CTASSERT. I'm surprised it doesn't already. This may have changed recently, though. Can you check to confirm?

[breno.leitao_gmail.com]

sys/powerpc/include/pcpu.h

48

Hi Justin,

Sure. I also included the struct pcb change, since I am not sure if that will also be considered kABI.

The change is at https://reviews.freebsd.org/D16889

[jhibbits]

Will take this to completion.

[jhibbits]

So, HTM is removed in PowerISA 3.1 and it's broken on POWER9. Is it even worth doing at this point? I'm inclined to abandon this.

[nwhitehorn]

In D15243#545983, @jhibbits wrote:

So, HTM is removed in PowerISA 3.1 and it's broken on POWER9. Is it even worth doing at this point? I'm inclined to abandon this.

I think the answer is clearly "no", since those facts make the chance that any 3rd-party userland code wants the feature pretty close to zero.

[jhibbits]

Since PowerISA v3.1, and therefore POWER10, dropped this feature, maintaining only token support, and it's unusably broken on POWER9, dropping this work. Sorry @breno.leitao_gmail.com .

[kbowling]

@jhibbits just curious where are you reading ISA 3.1 info?

[jhibbits]

@kbowling it's available from https://www-355.ibm.com/systems/power/openpower/.