Re: [V6,1/9] elf: Add new powerpc specifc core note sections

[Ulrich Weigand]

Anshuman Khandual <khandual@xxxxxxxxxxxxxxxxxx> wrote on 13.04.2015
10:48:57:
> On 04/10/2015 04:03 PM, Ulrich Weigand wrote:
> > - You provide checkpointed FPR and VMX registers, but there doesn't
seem
> >   to be any way to get at the checkpointed *VSX* registers (i.e. the
part
> >   that is neither covered by FPR or VMX, corresponding to NT_PPC_VSX).
>
> Will change vsr_get, vsr_set functions as we have done for fpr_get and
fpr_set
> functions. Also will add one more ELF core note NT_PPC_TM_CVSX to fetch
the
> check pointed state of VSX register while inside the transaction.

OK.

> >   I would much prefer three separate regsets (e.g. NT_PPC_DSCR,
NT_PPC_PPR,
> >   and NT_PPC_TAR), each of which is available and valid if and only if
the
> >   current processor actually has the register in question.
>
> Thats like adding one ELF core note for every single register
> because we cannot
> put them in any category. Then as Michael Ellerman had pointed out to
include
> a lot more registers in this MISC category (which we are not doing right
now
> in the interest of having minimum support available before we look at the
full
> possible list of MISC registers), we should add one ELF core note section
for
> each of those individual registers ? I am not sure.

This confuses me a bit.  My understanding was that ptrace regsets, once
defined, should never change in the future.  (GDB will only check whether
or not a regset is supported; if it is, it will expect the contents to be
as it expects them to be.)  "Including a lot more registers" would
therefore
seem to require adding new regsets anyway, which is one of the reasons why
I disagree a "MISC" regset is particularly useful.

> > - Similarly, the NT_PPC_TM_SPR regset as currently defined mixes and
> > matches
> >   registers with different "lifetimes".  The transactional memory
registers
> >   (TFHAR, TEXASR, TFIAR) are available *always* on machines that
support
> >   transactions.  But the other registers in that regset are
checkpointed
> >   versions that are only available/valid within a transaction.  I think
a
> >   better way to faithfully represent this would be to have the
> > NT_PPC_TM_SPR
> >   regset only contain the transcational memory registers, and use
separate
> >   regsets for the checkpointed registers -- those should parallel the
non-
> >   checkpointed register regset.
>
> Right now, we support NT_PPC_TM_SPR only inside the transaction, so we
dont
> have the problem with different "lifetimes" registers accessed together.
But
> yes, I get your point.

Since the transactional SPRs are accessible from user space outside of a
transaction, it would make sense for them to accessible from ptrace as
well.
If the current patch set doesn't do that, I guess it would be better to
change that.

> > - Particularly confusing to me is the "checkpointed original MSR" which
> >   currently also resides in NT_PPC_TM_SPR.  What exactly is this?  How
> >   does that differ from the MSR slot in the NT_PPC_TM_CGPR regset?
>
> I believed it stores the check pointed MSR value which was in the
register
> before the transaction started. But then how it is different from the
> ckpt_regs.msr, I am not sure. Mikey or Michael should be able to clarify
> more on this. I can see "orig_msr" getting used in many places to hold
the
> check pointed value of MSR.

Your other mail states that the orig_mst may be irrelevant for ptrace
anyway ... that would be OK with me as well.

> >   In any case, it seems the ptrace set-register case currently allows
user
> >   space to restore *any* arbitrary value into the checkpointed MSR,
which
> >   would presumably get restored into the real MSR at some point, unless
I'm
> >   missing something here.  Do we not need a check that only safe bits
are
> >   modified, just like with ptrace access to the real MSR?
>
> Where and which safe bits do we check before writing any value into the
MSR
> register from ptrace interface ? May be I am missing something here.

All ptrace accesses to *set* the regular msr go via this routine:

static int set_user_msr(struct task_struct *task, unsigned long msr)
{
        task->thread.regs->msr &= ~MSR_DEBUGCHANGE;
        task->thread.regs->msr |= msr & MSR_DEBUGCHANGE;
        return 0;
}

I think we'd need to do the equivalent whenever changing the checkpointed
MSR.

Bye,
Ulrich

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/