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

On 04/10/2015 04:03 PM, Ulrich Weigand wrote:
> Anshuman Khandual <khandual@...ux.vnet.ibm.com> wrote on 10.04.2015
> 11:10:35:
> 
>> I had posted a newer version [V7] of this patch series couple of months
> back
>> which got ignored while the discussion continued in this version.
>>
>> V7: https://lkml.org/lkml/2015/1/14/19
> 
> Ah, with all the back-and-forth on the checkpointed state, I never looked
> at this.  Unfortunately, there's still a number of issues with this, I
> think:
> 
> - 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.

> 
> - We may have had this discussion in the past, but I still do not like the
>   notion of a "misc" register set, in particular since the three registers
>   in it are available at different architecture levels and categories.

Misc category as always stands for registers which can not be easily classified
into any meaningful categories.

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

> 
>   If we do have a single regset, at the very least a "get" operation should
>   set registers unvailable on the machine to a defined state (zero?)
>   instead of simply leaving memory uninitialized.

Yeah sure, we can do that.

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

> 
>   For example, if we have NT_PPC_DSCR, there should be a NT_PPC_CDSCR for
>   the checkpointed version etc.  (If we do stay with MISC, there should
> then
>   be a CMISC).

But then NT_PPC_MISC and NT_PPC_CMISC can contain different set of registers.
NT_PPC_CMISC will contain the orig_msr for now which the other elf core note
does not have and NT_PPC_MISC will grow to have lot more registers in the
future leaving behind NT_PPC_CMISC as it is. Need to take care of these.

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

> 
>   I may be misreading kernel code, but it seems the kernel does not
> actually
>   use the ckpt_regs.msr slot at all, and therefore the corresponding slot
> of
>   the NT_PPC_TM_CGPR regset is likewise undefined/unused.  Would it not be
>   more consistent to use that slot to pass the checkpointed MSR?

Hmm. Its a valid point. Would like to get some more clarity on this from
Mikey whether that slot can be used for check pointed MSR value or not.
Then why did we have these two slots to hold the same check pointed MSR
value in the first place at all. Getting confused a bit.

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

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