diff mbox

[V3,1/5] powerpc, perf: Add new BHRB related instructions for POWER8

Message ID 1366287976-3900-2-git-send-email-khandual@linux.vnet.ibm.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Anshuman Khandual April 18, 2013, 12:26 p.m. UTC 
This patch adds new POWER8 instruction encoding for reading
the BHRB buffer entries and also clearing it. Encoding for
"clrbhrb" instruction is straight forward. But "mfbhrbe"
encoding involves reading a certain index of BHRB buffer
into a particular GPR register.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/ppc-opcode.h | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Michael Ellerman April 21, 2013, 11:41 p.m. UTC | #1 
On Thu, Apr 18, 2013 at 05:56:12PM +0530, Anshuman Khandual wrote:
> This patch adds new POWER8 instruction encoding for reading
> the BHRB buffer entries and also clearing it. Encoding for
> "clrbhrb" instruction is straight forward.

Which is "clear branch history rolling buffer" ?

> But "mfbhrbe"
> encoding involves reading a certain index of BHRB buffer
> into a particular GPR register.

And "Move from branch history rolling buffer entry" ?

> diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
> index 8752bc8..93ae5a1 100644
> --- a/arch/powerpc/include/asm/ppc-opcode.h
> +++ b/arch/powerpc/include/asm/ppc-opcode.h
> @@ -82,6 +82,7 @@
>  #define	__REGA0_R31	31
>  
>  /* sorted alphabetically */
> +#define PPC_INST_BHRBE			0x7c00025c

I don't think you really need this, just use the literal value below.

> @@ -297,6 +298,12 @@
>  #define PPC_NAP			stringify_in_c(.long PPC_INST_NAP)
>  #define PPC_SLEEP		stringify_in_c(.long PPC_INST_SLEEP)
>  
> +/* BHRB instructions */
> +#define PPC_CLRBHRB		stringify_in_c(.long 0x7c00035c)
> +#define PPC_MFBHRBE(r, n)	stringify_in_c(.long PPC_INST_BHRBE | \
> +						__PPC_RS(r) | \
> +							(((n) & 0x1f) << 11))

Why are you not using ___PPC_RB(n) here ?

cheers
Michael Neuling April 22, 2013, 1:13 a.m. UTC | #2 
Michael Ellerman <michael@ellerman.id.au> wrote:

> On Thu, Apr 18, 2013 at 05:56:12PM +0530, Anshuman Khandual wrote:
> > This patch adds new POWER8 instruction encoding for reading
> > the BHRB buffer entries and also clearing it. Encoding for
> > "clrbhrb" instruction is straight forward.
> 
> Which is "clear branch history rolling buffer" ?
> 
> > But "mfbhrbe"
> > encoding involves reading a certain index of BHRB buffer
> > into a particular GPR register.
> 
> And "Move from branch history rolling buffer entry" ?
> 
> > diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
> > index 8752bc8..93ae5a1 100644
> > --- a/arch/powerpc/include/asm/ppc-opcode.h
> > +++ b/arch/powerpc/include/asm/ppc-opcode.h
> > @@ -82,6 +82,7 @@
> >  #define	__REGA0_R31	31
> >  
> >  /* sorted alphabetically */
> > +#define PPC_INST_BHRBE			0x7c00025c
> 
> I don't think you really need this, just use the literal value below.

The rest of the defines in this file do this, so Anshuman's right. 

> > @@ -297,6 +298,12 @@
> >  #define PPC_NAP			stringify_in_c(.long PPC_INST_NAP)
> >  #define PPC_SLEEP		stringify_in_c(.long PPC_INST_SLEEP)
> >  
> > +/* BHRB instructions */
> > +#define PPC_CLRBHRB		stringify_in_c(.long 0x7c00035c)
> > +#define PPC_MFBHRBE(r, n)	stringify_in_c(.long PPC_INST_BHRBE | \
> > +						__PPC_RS(r) | \
> > +							(((n) & 0x1f) << 11))
> 
> Why are you not using ___PPC_RB(n) here ?

Actually, this is wrong.  The number field should be 10 bits (0x3ff),
not 5 (0x1f)  Anshuman please fix.  

Mikey
Michael Ellerman April 22, 2013, 2:45 a.m. UTC | #3 
On Mon, Apr 22, 2013 at 11:13:43AM +1000, Michael Neuling wrote:
> Michael Ellerman <michael@ellerman.id.au> wrote:
> 
> > On Thu, Apr 18, 2013 at 05:56:12PM +0530, Anshuman Khandual wrote:
> > > This patch adds new POWER8 instruction encoding for reading
> > > the BHRB buffer entries and also clearing it. Encoding for
> > > "clrbhrb" instruction is straight forward.
> > 
> > Which is "clear branch history rolling buffer" ?
> > 
> > > But "mfbhrbe"
> > > encoding involves reading a certain index of BHRB buffer
> > > into a particular GPR register.
> > 
> > And "Move from branch history rolling buffer entry" ?
> > 
> > > diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
> > > index 8752bc8..93ae5a1 100644
> > > --- a/arch/powerpc/include/asm/ppc-opcode.h
> > > +++ b/arch/powerpc/include/asm/ppc-opcode.h
> > > @@ -82,6 +82,7 @@
> > >  #define	__REGA0_R31	31
> > >  
> > >  /* sorted alphabetically */
> > > +#define PPC_INST_BHRBE			0x7c00025c
> > 
> > I don't think you really need this, just use the literal value below.
> 
> The rest of the defines in this file do this, so Anshuman's right. 

I don't see the point, but sure let's be consistent. Though in that case
he should do the same for PPC_CLRBHRB below.

> > > @@ -297,6 +298,12 @@
> > >  #define PPC_NAP			stringify_in_c(.long PPC_INST_NAP)
> > >  #define PPC_SLEEP		stringify_in_c(.long PPC_INST_SLEEP)
> > >  
> > > +/* BHRB instructions */
> > > +#define PPC_CLRBHRB		stringify_in_c(.long 0x7c00035c)
> > > +#define PPC_MFBHRBE(r, n)	stringify_in_c(.long PPC_INST_BHRBE | \
> > > +						__PPC_RS(r) | \
> > > +							(((n) & 0x1f) << 11))
> > 
> > Why are you not using ___PPC_RB(n) here ?
> 
> Actually, this is wrong.  The number field should be 10 bits (0x3ff),
> not 5 (0x1f)  Anshuman please fix.

ACK.

cheers
Michael Neuling April 22, 2013, 2:50 a.m. UTC | #4 
Michael Ellerman <michael@ellerman.id.au> wrote:

> On Mon, Apr 22, 2013 at 11:13:43AM +1000, Michael Neuling wrote:
> > Michael Ellerman <michael@ellerman.id.au> wrote:
> > 
> > > On Thu, Apr 18, 2013 at 05:56:12PM +0530, Anshuman Khandual wrote:
> > > > This patch adds new POWER8 instruction encoding for reading
> > > > the BHRB buffer entries and also clearing it. Encoding for
> > > > "clrbhrb" instruction is straight forward.
> > > 
> > > Which is "clear branch history rolling buffer" ?
> > > 
> > > > But "mfbhrbe"
> > > > encoding involves reading a certain index of BHRB buffer
> > > > into a particular GPR register.
> > > 
> > > And "Move from branch history rolling buffer entry" ?
> > > 
> > > > diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
> > > > index 8752bc8..93ae5a1 100644
> > > > --- a/arch/powerpc/include/asm/ppc-opcode.h
> > > > +++ b/arch/powerpc/include/asm/ppc-opcode.h
> > > > @@ -82,6 +82,7 @@
> > > >  #define	__REGA0_R31	31
> > > >  
> > > >  /* sorted alphabetically */
> > > > +#define PPC_INST_BHRBE			0x7c00025c
> > > 
> > > I don't think you really need this, just use the literal value below.
> > 
> > The rest of the defines in this file do this, so Anshuman's right. 
> 
> I don't see the point, but sure let's be consistent. Though in that case
> he should do the same for PPC_CLRBHRB below.

Agreed.

Mikey

> 
> > > > @@ -297,6 +298,12 @@
> > > >  #define PPC_NAP			stringify_in_c(.long PPC_INST_NAP)
> > > >  #define PPC_SLEEP		stringify_in_c(.long PPC_INST_SLEEP)
> > > >  
> > > > +/* BHRB instructions */
> > > > +#define PPC_CLRBHRB		stringify_in_c(.long 0x7c00035c)
> > > > +#define PPC_MFBHRBE(r, n)	stringify_in_c(.long PPC_INST_BHRBE | \
> > > > +						__PPC_RS(r) | \
> > > > +							(((n) & 0x1f) << 11))
> > > 
> > > Why are you not using ___PPC_RB(n) here ?
> > 
> > Actually, this is wrong.  The number field should be 10 bits (0x3ff),
> > not 5 (0x1f)  Anshuman please fix.
> 
> ACK.
> 
> cheers
>
Anshuman Khandual April 22, 2013, 6:57 a.m. UTC | #5 
On 04/22/2013 05:11 AM, Michael Ellerman wrote:
> On Thu, Apr 18, 2013 at 05:56:12PM +0530, Anshuman Khandual wrote:
>> This patch adds new POWER8 instruction encoding for reading
>> the BHRB buffer entries and also clearing it. Encoding for
>> "clrbhrb" instruction is straight forward.
> 
> Which is "clear branch history rolling buffer" ?
> 
>> But "mfbhrbe"
>> encoding involves reading a certain index of BHRB buffer
>> into a particular GPR register.
> 
> And "Move from branch history rolling buffer entry" ?
> 

Sure, would add these descriptions in the change log.

>> diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
>> index 8752bc8..93ae5a1 100644
>> --- a/arch/powerpc/include/asm/ppc-opcode.h
>> +++ b/arch/powerpc/include/asm/ppc-opcode.h
>> @@ -82,6 +82,7 @@
>>  #define	__REGA0_R31	31
>>  
>>  /* sorted alphabetically */
>> +#define PPC_INST_BHRBE			0x7c00025c
> 
> I don't think you really need this, just use the literal value below.
> 
>> @@ -297,6 +298,12 @@
>>  #define PPC_NAP			stringify_in_c(.long PPC_INST_NAP)
>>  #define PPC_SLEEP		stringify_in_c(.long PPC_INST_SLEEP)
>>  
>> +/* BHRB instructions */
>> +#define PPC_CLRBHRB		stringify_in_c(.long 0x7c00035c)
>> +#define PPC_MFBHRBE(r, n)	stringify_in_c(.long PPC_INST_BHRBE | \
>> +						__PPC_RS(r) | \
>> +							(((n) & 0x1f) << 11))
> 
> Why are you not using ___PPC_RB(n) here ?
> 

I would replace __PPC_RS(r) with __PPC_RT(r) which makes more sense from
instruction encoding point of view.

Regards
Anshuman
Anshuman Khandual April 22, 2013, 7:03 a.m. UTC | #6 
On 04/22/2013 08:20 AM, Michael Neuling wrote:
> Michael Ellerman <michael@ellerman.id.au> wrote:
> 
>> On Mon, Apr 22, 2013 at 11:13:43AM +1000, Michael Neuling wrote:
>>> Michael Ellerman <michael@ellerman.id.au> wrote:
>>>
>>>> On Thu, Apr 18, 2013 at 05:56:12PM +0530, Anshuman Khandual wrote:
>>>>> This patch adds new POWER8 instruction encoding for reading
>>>>> the BHRB buffer entries and also clearing it. Encoding for
>>>>> "clrbhrb" instruction is straight forward.
>>>>
>>>> Which is "clear branch history rolling buffer" ?
>>>>
>>>>> But "mfbhrbe"
>>>>> encoding involves reading a certain index of BHRB buffer
>>>>> into a particular GPR register.
>>>>
>>>> And "Move from branch history rolling buffer entry" ?
>>>>
>>>>> diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
>>>>> index 8752bc8..93ae5a1 100644
>>>>> --- a/arch/powerpc/include/asm/ppc-opcode.h
>>>>> +++ b/arch/powerpc/include/asm/ppc-opcode.h
>>>>> @@ -82,6 +82,7 @@
>>>>>  #define	__REGA0_R31	31
>>>>>  
>>>>>  /* sorted alphabetically */
>>>>> +#define PPC_INST_BHRBE			0x7c00025c
>>>>
>>>> I don't think you really need this, just use the literal value below.
>>>
>>> The rest of the defines in this file do this, so Anshuman's right. 
>>
>> I don't see the point, but sure let's be consistent. Though in that case
>> he should do the same for PPC_CLRBHRB below.
> 
> Agreed.
> 

Sure, would define a new macro (PPC_INST_CLRBHRB) to encode 0x7c00035c
before using it for PPC_CLRBHRB.

> Mikey
> 
>>
>>>>> @@ -297,6 +298,12 @@
>>>>>  #define PPC_NAP			stringify_in_c(.long PPC_INST_NAP)
>>>>>  #define PPC_SLEEP		stringify_in_c(.long PPC_INST_SLEEP)
>>>>>  
>>>>> +/* BHRB instructions */
>>>>> +#define PPC_CLRBHRB		stringify_in_c(.long 0x7c00035c)
>>>>> +#define PPC_MFBHRBE(r, n)	stringify_in_c(.long PPC_INST_BHRBE | \
>>>>> +						__PPC_RS(r) | \
>>>>> +							(((n) & 0x1f) << 11))
>>>>
>>>> Why are you not using ___PPC_RB(n) here ?
>>>
>>> Actually, this is wrong.  The number field should be 10 bits (0x3ff),
>>> not 5 (0x1f)  Anshuman please fix.
>>
>> ACK.

I got it wrong, thought this as 32 instead of 1024. Would fix it.

Regards
Anshuman
Anshuman Khandual April 22, 2013, 7:03 a.m. UTC | #7 
On 04/22/2013 08:20 AM, Michael Neuling wrote:
> Michael Ellerman <michael@ellerman.id.au> wrote:
> 
>> On Mon, Apr 22, 2013 at 11:13:43AM +1000, Michael Neuling wrote:
>>> Michael Ellerman <michael@ellerman.id.au> wrote:
>>>
>>>> On Thu, Apr 18, 2013 at 05:56:12PM +0530, Anshuman Khandual wrote:
>>>>> This patch adds new POWER8 instruction encoding for reading
>>>>> the BHRB buffer entries and also clearing it. Encoding for
>>>>> "clrbhrb" instruction is straight forward.
>>>>
>>>> Which is "clear branch history rolling buffer" ?
>>>>
>>>>> But "mfbhrbe"
>>>>> encoding involves reading a certain index of BHRB buffer
>>>>> into a particular GPR register.
>>>>
>>>> And "Move from branch history rolling buffer entry" ?
>>>>
>>>>> diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
>>>>> index 8752bc8..93ae5a1 100644
>>>>> --- a/arch/powerpc/include/asm/ppc-opcode.h
>>>>> +++ b/arch/powerpc/include/asm/ppc-opcode.h
>>>>> @@ -82,6 +82,7 @@
>>>>>  #define	__REGA0_R31	31
>>>>>  
>>>>>  /* sorted alphabetically */
>>>>> +#define PPC_INST_BHRBE			0x7c00025c
>>>>
>>>> I don't think you really need this, just use the literal value below.
>>>
>>> The rest of the defines in this file do this, so Anshuman's right. 
>>
>> I don't see the point, but sure let's be consistent. Though in that case
>> he should do the same for PPC_CLRBHRB below.
> 
> Agreed.
> 

Sure, would define a new macro (PPC_INST_CLRBHRB) to encode 0x7c00035c
before using it for PPC_CLRBHRB.

> Mikey
> 
>>
>>>>> @@ -297,6 +298,12 @@
>>>>>  #define PPC_NAP			stringify_in_c(.long PPC_INST_NAP)
>>>>>  #define PPC_SLEEP		stringify_in_c(.long PPC_INST_SLEEP)
>>>>>  
>>>>> +/* BHRB instructions */
>>>>> +#define PPC_CLRBHRB		stringify_in_c(.long 0x7c00035c)
>>>>> +#define PPC_MFBHRBE(r, n)	stringify_in_c(.long PPC_INST_BHRBE | \
>>>>> +						__PPC_RS(r) | \
>>>>> +							(((n) & 0x1f) << 11))
>>>>
>>>> Why are you not using ___PPC_RB(n) here ?
>>>
>>> Actually, this is wrong.  The number field should be 10 bits (0x3ff),
>>> not 5 (0x1f)  Anshuman please fix.
>>
>> ACK.

I got it wrong, thought this as 32 instead of 1024. Would fix it.

Regards
Anshuman
diff mbox

Patch

diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
index 8752bc8..93ae5a1 100644
--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h
@@ -82,6 +82,7 @@ 
 #define	__REGA0_R31	31
 
 /* sorted alphabetically */
+#define PPC_INST_BHRBE			0x7c00025c
 #define PPC_INST_DCBA			0x7c0005ec
 #define PPC_INST_DCBA_MASK		0xfc0007fe
 #define PPC_INST_DCBAL			0x7c2005ec
@@ -297,6 +298,12 @@ 
 #define PPC_NAP			stringify_in_c(.long PPC_INST_NAP)
 #define PPC_SLEEP		stringify_in_c(.long PPC_INST_SLEEP)
 
+/* BHRB instructions */
+#define PPC_CLRBHRB		stringify_in_c(.long 0x7c00035c)
+#define PPC_MFBHRBE(r, n)	stringify_in_c(.long PPC_INST_BHRBE | \
+						__PPC_RS(r) | \
+							(((n) & 0x1f) << 11))
+
 /* Transactional memory instructions */
 #define TRECHKPT		stringify_in_c(.long PPC_INST_TRECHKPT)
 #define TRECLAIM(r)		stringify_in_c(.long PPC_INST_TRECLAIM \