Re: [C/R ARM][PATCH 3/3] c/r: ARM implementation of checkpoint/restart

["Serge E. Hallyn" <serue@us.ibm.com>]

Quoting Christoffer Dall (christofferdall@christofferdall.dk):
> Implements architecture specific requirements for checkpoint/restart on
> ARM. The changes touch almost only c/r related code. Most of the work is
> done in arch/arm/checkpoint.c, which implements checkpointing of the CPU
> and necessary fields on the thread_info struct.
> 
> The ISA version (given by __LINUX_ARM_ARCH__) is checkpointed and verified
> against the machine architecture on restart. If they differ, an error is
> raised and restart aborted. It should be possible to restart on newer
> architectures, but further investigation is warranted.
> 
> Regarding ThumbEE, the thumbee_state field on the thread_info is stored
> in checkpoints when CONFIG_ARM_THUMBEE and 0 is stored otherwise. If
> a value different than 0 is checkpointed and CONFIG_ARM_THUMBEE is not
> set on the restore system, the restore is aborted. Feedback on this
> implementation is very welcome.
> 
> We checkpoint whether the system is running with CONFIG_MMU or not and
> require the same configuration for the system on which we restore the
> process. It might be possible to allow something more fine-grained,
> if it's worth the energy. Input on this item is also very welcome,
> specifically from someone who knows the exact meaning of the end_brk
> field.
> 
> Added support for syscall sys_checkpoint and sys_restart for ARM:
> __NR_checkpoint         367
> __NR_restart            368
> 
> 
> Cc: rmk@arm.linux.org.uk
> Signed-off-by: Christoffer Dall <christofferdall@christofferdall.dk>
> Acked-by: Oren Laadan <orenl@cs.columbia.edu>

In terms of the cr api I don't see any problems.  Two nits below,
but in any case

Acked-by: Serge Hallyn <serue@us.ibm.com>

thanks, this is really cool, especially how minimal it is :)
-serge

...

> +static int load_cpu_regs(struct ckpt_hdr_cpu *h, struct task_struct *t)
> +{
> +	int i;
> +	struct pt_regs *regs = task_pt_regs(t);
> +
> +	memcpy(regs, &h->uregs, sizeof(struct pt_regs));
> +
> +	for (i = 0; i < 16; i++)
> +		regs->uregs[i] = h->uregs[i];
> +
> +	/*
> +	 * Restore only user-writable bits on the CPSR
> +	 */
> +	regs->ARM_cpsr = regs->ARM_cpsr |
> +			 (h->ARM_cpsr & (PSR_N_BIT | PSR_Z_BIT |
> +					 PSR_C_BIT | PSR_V_BIT |
> +					 PSR_V_BIT | PSR_Q_BIT |
> +					 PSR_E_BIT | PSR_GE_BITS));
> +	regs->ARM_ORIG_r0 = h->ARM_ORIG_r0;
> +
> +	return 0;
> +}
> +
> +/* read the cpu state and registers for the current task */
> +int restore_cpu(struct ckpt_ctx *ctx)
> +{
> +	struct ckpt_hdr_cpu *h;
> +	struct task_struct *t = current;
> +	int ret;
> +
> +	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_CPU);
> +	if (IS_ERR(h))
> +		return PTR_ERR(h);
> +
> +	ret = load_cpu_regs(h, t);

will load_cpu_regs() ever be changed to return anything but 0?  If
not both fns can be simplified.

...

> +int restore_mm_context(struct ckpt_ctx *ctx, struct mm_struct *mm)
> +{
> +	struct ckpt_hdr_mm_context *h;
> +	int ret = 0;
> +
> +	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_MM_CONTEXT);
> +	if (IS_ERR(h))
> +		return PTR_ERR(h);
> +
> +#if !CONFIG_MMU
> +	mm->context.end_brk = h->end_brk;
> +#endif
> +
> +	ckpt_hdr_put(ctx, h);
> +	return ret;

Again ret doesn't seem needed here.

-serge