Quoting Dan Smith (danms@xxxxxxxxxx):
Thanks, Dan, this looks very nice with the CR_COPY, plus you
fixed the bug I couldn't find :) A few comments:
> +struct cr_hdr_cpu {
> + __u64 args[1];
Dave wanted this to not be an array, right?
> + __u64 gprs[NUM_GPRS];
> + __u64 orig_gpr2;
> + __u16 svcnr;
> + __u16 ilc;
> + __u32 acrs[NUM_ACRS];
> + __u64 ieee_instruction_pointer;
> +
> + /* psw_t */
> + __u64 psw_t_mask;
> + __u64 psw_t_addr;
> +
> + /* s390_fp_regs_t */
> + __u32 fpc;
> + union {
> + float f;
> + double d;
> + __u64 ui;
Since this is a union, and you don't deal with its
members but just memcpy it, why not just change it
to
__u64 fprs[NUM_FPRS];
and not have a union at all?
> + struct {
> + __u32 fp_hi;
> + __u32 fp_lo;
> + } fp;
> + } fprs[NUM_FPRS];
> +
> + /* per_struct */
> + __u64 per_control_regs[3];
I assume Dave still wants you to add a
#define PER_NUM_REGS 3
into the arch/s390/include/asm/processor.h or something.
> +void cr_s390_mm(int op, struct cr_hdr_mm_context *hh, struct mm_struct *mm)
> +{
> +#if 0
The comment about why this is ifdefed out for now should stay
here.
> + CR_COPY(op, hh->vdso_base, mm->context.vdso_base);
> +#endif
> + CR_COPY(op, hh->noexec, mm->context.noexec);
> + CR_COPY(op, hh->has_pgste, mm->context.has_pgste);
> + CR_COPY(op, hh->alloc_pgste, mm->context.alloc_pgste);
> + CR_COPY(op, hh->asce_bits, mm->context.asce_bits);
> + CR_COPY(op, hh->asce_limit, mm->context.asce_limit);
> +}
> +
> +int cr_write_thread(struct cr_ctx *ctx, struct task_struct *t)
> +{
> + return 0;
> +}
> +
> +/* dump the cpu state and registers of a given task */
> +int cr_write_cpu(struct cr_ctx *ctx, struct task_struct *t)
> +{
> + struct cr_hdr h;
> + struct cr_hdr_cpu *hh = cr_hbuf_get(ctx, sizeof(*hh));
> + int ret;
> +
> + h.type = CR_HDR_CPU;
> + h.len = sizeof(*hh);
> + h.parent = task_pid_vnr(t);
BTW - I know Dave mentioened using a generic helper for this
often-used stanza above, but I continue to be against that
bc the helper ends up having to take a bunch of eye-numbing
arguments and I think the code ends up hard to read. But
maybe you can think of a way to make that clearer...
> +
> + cr_s390_regs(CR_CPT, hh, t);
> +
> + ret = cr_write_obj(ctx, &h, hh);
> + cr_hbuf_put(ctx, sizeof(*hh));
> +
> + return ret;
> +}
> +
> +int cr_write_head_arch(struct cr_ctx *ctx)
> +{
> + return 0;
> +}
> +
> +/* Nothing to do for mm context state */
The above comment is clearly wrong :)
> +int cr_write_mm_context(struct cr_ctx *ctx, struct mm_struct *mm, int parent)
> +{
> + struct cr_hdr h;
> + struct cr_hdr_mm_context *hh = cr_hbuf_get(ctx, sizeof(*hh));
> + int ret;
> +
> + h.type = CR_HDR_MM_CONTEXT;
> + h.len = sizeof(*hh);
> + h.parent = parent;
> +
> + cr_s390_mm(CR_CPT, hh, mm);
> +
> + ret = cr_write_obj(ctx, &h, hh);
> + cr_hbuf_put(ctx, sizeof(*hh));
> +
> + return ret;
> +}
...
> +int cr_read_cpu(struct cr_ctx *ctx)
> +{
> + struct cr_hdr_cpu *hh = cr_hbuf_get(ctx, sizeof(*hh));
> + struct pt_regs *regs = task_pt_regs(current);
> + int parent, ret;
> +
> + parent = cr_read_obj_type(ctx, hh, sizeof(*hh), CR_HDR_CPU);
> + if (parent < 0) {
> + ret = parent;
> + goto out;
> + }
> + ret = 0;
> +
> + regs->psw.addr &= ~PSW_ADDR_INSN;
> + cr_s390_regs(CR_RST, hh, current);
> + restore_access_regs(hh->acrs);
Just a comment explaining why?
> +out:
> + cr_hbuf_put(ctx, sizeof(*hh));
> + return ret;
> +}
thanks
-serge
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
