[CRIU] [PATCH 2/2] ppc64: handle transactional memory state

[Dmitry Safonov]

2016-08-31 13:06 GMT+03:00 Laurent Dufour <ldufour at linux.vnet.ibm.com>:
> The Power 8 introduces the transactional memory (TM) operations (see
> Power ISA 3.0 for details).
>
> The support for the transactional memory operation during the
> checkpoint and restart requires extended ptrace API provided by the
> kernel 4.8.
>
> When checkpointing a thread while a transactional memory operation is
> in progress, the TM checkpointed state is checkpointed through the new
> ptrace API. If these new APIs are not available, the checkpoint is
> aborted and an explicit error is reported.
>
> At restart time, the TM state is pushed on the stack frame to be
> reloaded by the kernel when reading the stack frame.
>
> Only suspended TM operation could be checkpointed since active one
> will be aborted once a system call is made. Suspended operation will
> be aborted as well, and the checkpointed thread is expected to handle
> the TM failure as usual (retrying is a good option).
>
> Signed-off-by: Laurent Dufour <ldufour at linux.vnet.ibm.com>

Oh, what a brilliant stuff!
Small nitpicks below.

> +static int get_tm_regs(pid_t pid, CoreEntry *core)
> +{
> +       struct {
> +               uint64_t tfhar, texasr, tfiar;
> +       } tm_spr_regs;
> +       user_regs_struct_t regs;
> +       uint64_t fpregs[NFPREG], vmxregs[34][2], vsxregs[32];

Maybe add definitions like?
#define NVMXREG 34
#define NVSXREG 32

> +       struct iovec iov;
> +       UserPpc64TmRegsEntry *tme = NULL;

Not needed initialization

[...]

> +static int put_tm_regs(struct rt_sigframe *f, UserPpc64TmRegsEntry *tme)
> +{
> +/* WARNING: As stated in kernel's restore_tm_sigcontexts, TEXASR has to be
> + * restored by the process itself :
> + *   TEXASR was set by the signal delivery reclaim, as was TFIAR.
> + *   Users doing anything abhorrent like thread-switching w/ signals for
> + *   TM-Suspended code will have to back TEXASR/TFIAR up themselves.
> + *   For the case of getting a signal and simply returning from it,
> + *   we don't need to re-copy them here.
> + */

Netlayer-alike comment style, don't mind, but AFAIK CRIU uses another style.

> @@ -415,35 +579,60 @@ int get_task_regs(pid_t pid, user_regs_struct_t regs, CoreEntry *core)
>         /* Resetting trap since we are now coming from user space. */
>         regs.trap = 0;
>
> -       copy_gp_regs(core->ti_ppc64->gpregs, &regs);
> +       /* Check for Transactional Memory operation in progress.
> +        * Until we have support of TM register's state through the ptrace API,
> +        * we can't checkpoint process with TM operation in progress (almost
> +        * impossible) or suspended (easy to get).
> +        */

And another

> +       if (MSR_TM_ACTIVE(regs.msr)) {
> +               pr_debug("Task %d has %s TM operation at 0x%lx\n",
> +                        pid,
> +                        (regs.msr & MSR_TMS) ? "a suspended" : "an active",
> +                        regs.nip);
> +               if (get_tm_regs(pid, core))
> +                       return -1;
> +
> +               gpregs = core->ti_ppc64->tmstate->gpregs;
> +               fpstate = &(core->ti_ppc64->tmstate->fpstate);
> +               vrstate = &(core->ti_ppc64->tmstate->vrstate);
> +               vsxstate = &(core->ti_ppc64->tmstate->vsxstate);
> +       }
> +       else {

Reverse egyptian braces

> @@ -515,6 +706,14 @@ int restore_fpu(struct rt_sigframe *sigframe, CoreEntry *core)
>                 ret = -1;
>         }
>
> +       if (!ret && CORE_THREAD_ARCH_INFO(core)->tmstate)
> +               ret = put_tm_regs(sigframe,
> +                                 CORE_THREAD_ARCH_INFO(core)->tmstate);
> +       else if (MSR_TM_ACTIVE(core->ti_ppc64->gpregs->msr)) {
> +               pr_err("Internal error\n");
> +               ret = -1;

AFAIU, that means TM transaction in progress, right?
CRIU does not use them, so should we BUG() here, or
just fail with an error?


> diff --git a/images/core-ppc64.proto b/images/core-ppc64.proto
> index 5bdec9c07809..794a1b029856 100644
> --- a/images/core-ppc64.proto
> +++ b/images/core-ppc64.proto
> @@ -11,6 +11,10 @@ message user_ppc64_regs_entry {
>         required uint64 xer             = 7;
>         required uint64 ccr             = 8;
>         required uint64 trap            = 9;
> +       // For Transactional memory support since P8

C++ alike comment

> +       optional uint64 texasr          = 10;
> +       optional uint64 tfhar           = 11;
> +       optional uint64 tfiar           = 12;
>  }
>
>  message user_ppc64_fpstate_entry {
> @@ -41,10 +45,19 @@ message user_ppc64_vsxstate_entry {
>         repeated uint64 vsxregs         = 1;
>  }
>
> +// Transactional memory operation's state

Another one.

-- 
             Dmitry