Message-ID: <5535D83C.1060302@linux.vnet.ibm.com>
Date:	Tue, 21 Apr 2015 10:25:24 +0530
From:	Anshuman Khandual <khandual@...ux.vnet.ibm.com>
To:	Ulrich Weigand <Ulrich.Weigand@...ibm.com>
CC:	shuahkh@....samsung.com, Michael Neuling <mikey@...ling.org>,
	james.hogan@...tec.com, avagin@...nvz.org,
	Paul.Clothier@...tec.com, peterz@...radead.org, palves@...hat.com,
	linux-kernel@...r.kernel.org, davem@...emloft.net,
	dhowells@...hat.com, linuxppc-dev@...abs.org, kirjanov@...il.com,
	tglx@...utronix.de, oleg@...hat.com, davej@...hat.com,
	akpm@...ux-foundation.org, sukadev@...ux.vnet.ibm.com,
	Edjunior Barbosa Machado <emachado@...ux.vnet.ibm.com>,
	sam.bobroff@....ibm.com
Subject: Re: [V6,1/9] elf: Add new powerpc specifc core note sections

On 04/20/2015 05:57 PM, Ulrich Weigand wrote:
> Anshuman Khandual <khandual@...ux.vnet.ibm.com> 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.

Yeah right. Started thinking that (NT_PPC_TAR, NT_PPC_CTAR),
(NT_PPC_PPR, NT_PPC_CPPR), (NT_PPC_DSCR, NT_PPC_CDSCR) kind of combinations
make more sense !

> 
>>> - 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.

Yeah I agree. Will change it.

> 
>>> - 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.

Yeah. The variable tm_orig_msr is used to compute MSR state inside
the kernel or what would be passed to the user space while returning
at various stages of the transaction, where as ckpt_regs.msr contains
the latest check pointed MSR value to be fetched by ptrace. Thats my
understanding as of now.

> 
>>>   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.

Agree, will incorporate this change.

In summary, after putting together all the issues that we have
discussed till now regarding the number and scope of all new ELF
core note sections being added, the probable elements there in
can be listed as below.

Changed ELF core note sections
------------------------------
These core note sections need to be changed to accommodate the in
transaction ptrace requests when the running/current value of these
registers will reside some where else instead of the original places
of thread_struct.

/* Running register state */
(1) NT_PRFPREG				(Accessible always)
(2) NT_PPC_VMX				(Accessible always)
(3) NT_PPC_VSX				(Accessible always)

New ELF core note sections
--------------------------
/* TM check pointed register set */
(1) NT_PPC_TM_CGPR	--> NT_PRSTATUS	(Accessible inside TM)	
(2) NT_PPC_TM_CFPR	--> NT_PRFPREG	(Accessible inside TM)
(3) NT_PPC_TM_CVMX	--> NT_PPC_VMX	(Accessible inside TM)
(4) NT_PPC_TM_CVSX	--> NT_PPC_VSX	(Accessible inside TM)

NOTE: The register set data structure for these ELF core not
sections would exactly match with that of the corresponding
running value register sets indicated above.

/* TM SPR set */			(Accessible always)
(5) NT_PPC_TM_SPR	thread->tm_tfhar
			thread->tm_tfiar
			thread->ttm_exasr

/* TM check pointed misc register set */
(6) NT_PPC_TM_TAR	thread->tm_tar	(Accessible inside TM)
(7) NT_PPC_TM_PPR	thread->tm_ppr	(Accessible inside TM)
(8) NT_PPC_TM_DSCR	thread->tm_dscr (Accessible inside TM)

NOTE: Application can have a different set of TAR, PPR and DSCR
registers inside the transaction compared that of the outside.
Also seems like they are *not* the check pointed ones, will
double check on this. Changed the core note section name from
NT_PPC_TM_CTAR to just NT_PPC_TM_TAR and for all the others.

/* Running misc register set */
(9) 	NT_PPC_TAR	thread->tar	(Accessible always)
(10)	NT_PPC_PPR	thread->ppr	(Accessible always)
(11)	NT_PPC_DSCR	thread->dscr	(Accessible always)

NOTE: They are like any other special purpose register which can
be changed from the user space. So the elf core note section name
can be generic. Here are some optional ELF core note sections
which we can also add like the above ones.

(12)	NT_PPC_EBBRR	thread->ebbrr	(Accessible inside EBB)
(13)	NT_PPC_EBBHR	thread->ebbhr	(Accessible inside EBB)
(14)	NT_PPC_BESCR	thread->bescr	(Accessible inside EBB)
(15)	NT_PPC_SIAR	thread->siar	(Accessible inside EBB)
(16) 	NT_PPC_SDAR	thread->sdar	(Accessible inside EBB)
(17)	NT_PPC_SIER	thread->sier	(Accessible inside EBB)
(18)	NT_PPC_MMCR2	thread->mmcr2	(Accessible inside EBB)
(19)	NT_PPC_MMCR0	thread->mmcr0	(Accessible inside EBB)

Ulrich, Mikey, MPE,

	Please do let me know your thoughts on this.

Regards
Anshuman

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

Powered by blists - more mailing lists