[CRIU] [PATCH 4/5] [RFC] cr: implemented the support for the AArch64 architecture

[Alexander Kartashov]

Dear Cristopher,

Thank you for the valuable feedback!

Im sorry for being unresponsive for so long. Finally I've managed
to address most issues you've pointed out, a new version of
the patchset is to be submitted soon.

This reply contains the issues I haven't fixed in the patchset:

On 02/05/2014 07:14 PM, Christopher Covington wrote:
>> +			"mov  x1, %1			\n"	\
>> +			"mov  x0, %2			\n"	\
>> +			"br   x1			\n"	\
>> +			:					\
>> +			: "r"(new_sp),				\
>> +			  "r"(restore_task_exec_start),		\
>> +			  "r"(task_args)			\
>> +			: "sp", "x0", "x1", "memory")
> I didn't notice any memory accesses--should it be in the clobber list?
I left the memory specifier in the clobber list as a compiler barrier.
> +
> +
> +#define ARCH_RT_SIGRETURN(new_sp)						\
> +	asm volatile(								\
> +			"mov sp, %0					\n" 	\
> +			"mov x8, #"__stringify(__NR_rt_sigreturn)"	\n"	\
> +			"svc #0						\n"	\
> +			:							\
> +			: "r"(new_sp)						\
> +			: "sp", "x8", "memory")
> Is the memory clobber necessary?
I believe it is since it's a compiler barrier. Moreover we invoke a 
system call here
that might access memory in an unpredictable way.
>
>> +			"mov %0, x0					\n"	\
>> +			"b   clone_end					\n"	\
>> +										\
>> +			"thread_run:					\n"	\
>> +			"ldp x1, x0, [sp]		        	\n"	\
>> +			"br  x1						\n"	\
>> +										\
>> +			"clone_end:					\n"	\
>> +			: "=r"(ret)						\
>> +			: "r"(clone_flags),					\
>> +			"r"(new_sp),						\
>> +			"r"(&parent_tid),					\
>> +			"r"(&thread_args[i].pid),				\
>> +			"r"(clone_restore_fn),					\
>> +			"r"(&thread_args[i])					\
>> +			: "x0", "x1", "x2", "x3", "x7", "memory")
> It doesn't look like you modify memory, but you do modify x8 instead of x7.
>
>> +#define ARCH_FAIL_CORE_RESTORE					\
>> +	asm volatile(						\
>> +			"mov sp, %0			    \n"	\
>> +			"mov x0, #0			    \n"	\
>> +			"b   x0			            \n"	\
>> +			:					\
>> +			: "r"(ret)				\
>> +			: "memory")
> Should x0 and sp go into the clobber list and memory be taken out?
The clobbered register lists are fixed while I still think that the 
memory specifier
is necessary for the same reason as previous.
> +
> +static inline void ksigfillset(k_rtsigset_t *set)
> +{
> +	int i;
> +	for (i = 0; i < _KNSIG_WORDS; i++)
> +		set->sig[i] = (unsigned long)-1;
> +}
> +
> +#define SA_RESTORER	0x04000000
> This is not defined in the AArch64 kernel. Since the only use is OR'd with
> other bits, can you just define it to 0 and maybe include the "New
> architectures should not define the obsolete SA_RESTORER" comment?
It's still used in the kernel: 
http://lxr.free-electrons.com/source/arch/arm64/kernel/signal.c#L234
However I didn't try to define SA_RESTORER as 0.
>> +typedef struct {
>> +	rt_sighandler_t	rt_sa_handler;
>> +	unsigned long	rt_sa_flags;
>> +	rt_sigrestore_t	rt_sa_restorer;
>> +	k_rtsigset_t	rt_sa_mask;
>> +} rt_sigaction_t;
>> +
>> +/*
>> + * Copied from the Linux kernel header arch/arm64/include/uapi/asm/ptrace.h
>> + *
>> + * A thread ARM CPU context
>> + */
> Why can't you just include <asm/ptrace.h>?
That is what I attempted to do first. Unfortunately it resulted in
conflicts with other headers I failed to resolve.
>> +typedef struct {
>> +	u64	regs[31];
>> +	u64	sp;
>> +	u64	pc;
>> +	u64	pstate;
>> +} user_regs_struct_t;
>> +
>> +
>> +#define ASSIGN_TYPED(a, b) do { a = (typeof(a))b; } while (0)
>> +#define ASSIGN_MEMBER(a,b,m) do { ASSIGN_TYPED((a)->m, (b)->m); } while (0)
>> +
>> +#ifndef PAGE_SIZE
>> +# define PAGE_SIZE	4096
>> +#endif
> Having this as a build-time constant means there can't be a single binary that
> works on all page sizes, but the kernel would probably have to provide a new
> run-time API for checking the page size (unless there's one already?) to do
> things any differently.
Unfortunately this is a global issue: the size of some structures shared
with the PIE blobs depends on the compile time value of the macro PAGE_SIZE.
I'm not sure how the issue should be properly dealt with.

-- 
Sincerely yours,
Alexander Kartashov

Intern
Core team

www.parallels.com

Skype: aleksandr.kartashov
Email: alekskartashov at parallels.com