> +#define CR_390_PACK_TRACE_FLAGS(hdr, thread) \
>> + do { \
>> + hdr->em_instr = 0; \
>> + if (thread->per_info.single_step) \
>> + hdr->em_instr |= 1; \
>> + if (thread->per_info.instruction_fetch) \
>> + hdr->em_instr |= 2; \
>> +} while (0)
>> +
>> +#define CR_390_UNPACK_TRACE_FLAGS(hdr, thread) \
>> + do { \
>> + if (hdr->em_instr & 1) \
>> + thread->per_info.single_step = 1; \
>> + if (hdr->em_instr & 2) \
>> + thread->per_info.instruction_fetch = 1; \
>> + } while (0)
DH> Why are these macros?
Well, almost the exact code above was spread between the checkpoint
and restart paths. I think that this makes it a little clearer what 1
and 2 are since you can see them together.
DH> Why do we need to pack them, anyway?
I assume Serge was trying not to waste a word per flag, and expecting
more flags to be needed to justify the savings.
>> +/*
>> + * Notes
>> + * NUM_GPRS defined in <asm/ptrace.h> to be 16
>> + * NUM_FPRS defined in <asm/ptrace.h> to be 16
>> + * NUM_APRS defined in <asm/ptrace.h> to be 16
>> + */
>> +struct cr_hdr_cpu {
>> + __u64 args[1];
>> + __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;
>> + struct {
>> + float f;
>> + double d;
>> + __u64 ui;
>> + __u32 fp_hi;
>> + __u32 fp_lo;
>> + } fprs[NUM_FPRS];
DH> I don't see a lot of floating point code go by, so I'm a bit
DH> stupid on this. But, this doesn't make a lot of sense to me. Are
DH> there really parallel sets of float and double registers? Or, do
DH> we want some kind of union here?
Yeah, this looks like an improper copy of the freg_t union in
s390/include/asm/ptrace.h to me. Serge, was there a specific reason
for doing this? If not I'll make it mirror the actual definition.
>> diff --git a/arch/s390/kernel/compat_wrapper.S
>> b/arch/s390/kernel/compat_wrapper.S
>> index fc2c971..9546a81 100644
>> --- a/arch/s390/kernel/compat_wrapper.S
>> +++ b/arch/s390/kernel/compat_wrapper.S
>> @@ -1767,3 +1767,15 @@ sys_dup3_wrapper:
>> sys_epoll_create1_wrapper:
>> lgfr %r2,%r2 # int
>> jg sys_epoll_create1 # branch to system call
>> +
>> + .globl sys_checkpoint_wrapper
>> +sys_checkpoint_wrapper:
>> + lgfr %r2,%r2 # pid_t
>> + lgfr %r3,%r3 # int
>> + llgfr %r4,%r4 # unsigned long
>> +
>> + .globl sys_restart_wrapper
>> +sys_restart_wrapper:
>> + lgfr %r2,%r2 # int
>> + lgfr %r3,%r3 # int
>> + llgfr %r4,%r4 # unsigned long
DH> So, all s390 syscalls need these wrappers? They don't do anything
DH> in particular to help c/r, right?
Looking at the existing file, I'd say everything is wrapped that way.
You'd have to ask someone who knows about s390 for the reason why, I
guess :)
DH> One minor issue with all of these memcpy()s is that they're not
DH> stupid-proof. To figure out whether the 'sizeof(unsigned int)' is
DH> correct, I need to go back and look to ensure that hh->acrs is, in
DH> fact, an 'unsigned int'. But if you do:
DH> memcpy(hh->acrs, &thread->acrs[0], sizeof(hh->acrs));
This doesn't copy all of the access registers though, just the first
one. Perhaps you were thrown off by the use of &thread->acrs[0]? I'd
prefer to just use thread->acrs there for clarity, so I'll make that
change as well.
>> + hh->psw_t_mask = regs->psw.mask;
>> + hh->psw_t_addr = regs->psw.addr;
>> +
>> + hh->args[0] = regs->args[0];
DH> Why is this an array?
Because it's an array of 1 in regs->args. I'll add a comment :)
>> + hh->svcnr = regs->svcnr;
>> + hh->ilc = regs->ilc;
>> + memcpy(hh->gprs, regs->gprs, NUM_GPRS*sizeof(unsigned long));
>> + hh->orig_gpr2 = regs->orig_gpr2;
>> +
>> + hh->ieee_instruction_pointer = thread->ieee_instruction_pointer;
>> +
>> + /* per_info */
>> + memcpy(&hh->per_control_regs, &thread->per_info.control_regs.words,
>> + 3 * sizeof(unsigned long));
DH> This '3' is a magic number and is used in a few places. Does it
DH> make sense to define it as a variable.
You know, I said the same thing when I reviewed this set initially.
However the actual s390 code where its defined uses the magic 3, so if
we define a constant we could still be out of sync with the actual
definition.
>> + cr_save_cpu_regs(hh, t);
>> +
>> + ret = cr_write_obj(ctx, &h, hh);
>> + cr_hbuf_put(ctx, sizeof(*hh));
>> + WARN_ON_ONCE(ret < 0);
>> +
>> + return ret;
>> +}
DH> The WARN_ON() probably shouldn't be there when this is submitted.
Okay.
>> + /* ilc is syscall restart addr. it looks like we must
>> + restore it since we restore psw.addr */
DH> Don't forget CodingStyle on these, please.
Okay.
DH> I'm struggling to figure out how we're going to keep up with
DH> keeping checkpoint and restart synchronized. This is all pretty
DH> mindless copying in both directions. Is it possible and
DH> worthwhile for us to just define it once, but use it for both
DH> checkpoint and restart with some macro trickery?
DH> #define CKPT 1
DH> #define RST 2
DH> #define CR_COPY(op, a, b)
DH> do {
DH> WARN_ON(sizeof(a) != sizeof(b));
DH> if (op == CKPT)
DH> memcpy(&a, &b, sizeof(b));
DH> else
DH> memcpy(&b, &a, sizeof(b));
DH> } while (0);
DH> void s390_cr_regs(int op, struct task_struct *thread, *hh)
DH> {
DH> CR_COPY(op, thread->per_info.lowcore.words.perc_atmid, hh->perc_atmid);
DH> CR_COPY(op, thread->per_info.lowcore.words.address, hh->address);
DH> CR_COPY(op, thread->per_info.lowcore.words.access_id, hh->access_id);
DH> ...
DH> }
DH> int cr_read_cpu(struct cr_ctx *ctx)
DH> {
DH> s390_cr_regs(RST, thread, hh);
DH> }
DH> int cr_save_cpu(struct cr_ctx *ctx)
DH> {
DH> s390_cr_regs(CKPT, thread, hh);
DH> }
DH> That works for both regular variables and for arrays. Is the
DH> decreased line count worth the weirdo non-standard abstraction?
It looks cleaner to me, so I'm happy to change it if people think that
would be better.
--
Dan Smith
IBM Linux Technology Center
email: [email protected]
_______________________________________________
Containers mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/containers
_______________________________________________
Devel mailing list
[email protected]
https://openvz.org/mailman/listinfo/devel
