On Thu, 09 Jun 2016 15:12:51 +1000
Daniel Axtens <d...@axtens.net> wrote:
As stated in the cover-letter, this patch needs the most work and quite a lot
too.
Turns out the comment you took is wrong (the code actually does the opposite),
once again, just sent the series to get eyes on the actual patch.
> I'm trying not to be too nit picky or difficult on tests, so here's a
> pre-written commit message for you:
>
> "The kernel sets up two sets of ucontexts if the signal was to be
> delivered while the thread was in a transaction. Expected behaviour is
> that the currently executing code is in the first and the checkpointed
> state (the state that will be rolled back to) is in the uc_link
> ucontext.
>
> The reason for this is that:
>
> - code which is not TM aware and installs a signal handler will expect
> to see/modify its currently running state in the uc.
>
> - but, that TM-unaware code may have dynamicially linked against code
> which is TM aware and is doing HTM under the hood, so the
> checkpointed state needs to be made available somewhere
>
> Test if the live and checkpointed state is made stored correctly.
> Test:
> - GPRs
> - FP registers
> - VMX
> - VSX
> "
>
> > +#define TBEGIN .long 0x7C00051D
> > +#define TSUSPEND .long 0x7C0005DD
> > +#define TRESUME .long 0x7C2005DD
> You define these 3 opcodes in a number of files. I assume you're going
> to consolidate them in v2?
>
> > + * The kernel sets up two sets of ucontexts if the signal was to be
> > delivered
> > + * while the thread was in a transaction. Expected behaviour is that the
> > + * currently executing code is in the first and the checkpointed state (the
> > + * state that will be rolled back to) is in the uc_link ucontext.
> > + *
> > + * The reason for this is that code which is not TM aware and installs a
> > signal
> > + * handler will expect to see/modify its currently running state in the uc,
> > + * this code may have dynamicially linked against code which is TM aware
> > and is
> > + * doing HTM under the hood.
>
> I had real trouble parsing this sentence the first few times. I think
> it's missing a while:
>
> The reason for this is that _while_ code which is not TM aware...
>
> (Although it would be better in several sentences :P)
>
> > +++ b/tools/testing/selftests/powerpc/tm/tm-signal-context-chk-gpr.c
> > @@ -0,0 +1,96 @@
> > +/*
> > + * Copyright 2016, Cyril Bur, IBM Corp.
> > + * Licensed under GPLv2.
> Ironically, it seems this now needs to be GPLv2+, probably with the
> regular license grant paragraph.
>
> > + /* Always be 64bit, don't really care about 32bit */
> Forgive my ignorance of the test suite: are we guaranteed this by the
> build system, or should we add a SKIP_IF() for it?
> > + for (i = 0; i < NV_GPR_REGS && !fail; i++) {
> > + fail = (ucp->uc_mcontext.gp_regs[i + 14] != gps[i]);
> > + fail |= (tm_ucp->uc_mcontext.gp_regs[i + 14] != gps[i +
> > NV_GPR_REGS]);
> > + }
> > + if (fail)
> > + printf("Failed on %d GPR %lu or %lu\n", i - 1,
> > + ucp->uc_mcontext.gp_regs[i + 13],
> > tm_ucp->uc_mcontext.gp_regs[i + 13]);
> > +}
> > +
>
> Looking good otherwise!
>
> Regards,
> Daniel
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
