* Re: [PATCH 1/3] sparc64: correctly recognise M7 cpu type
2014-08-19 4:53 [PATCH 1/3] sparc64: correctly recognise M7 cpu type Allen Pais
@ 2014-08-19 15:21 ` Sam Ravnborg
2014-08-21 10:15 ` Allen Pais
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Sam Ravnborg @ 2014-08-19 15:21 UTC (permalink / raw)
To: sparclinux
On Tue, Aug 19, 2014 at 10:23:32AM +0530, Allen Pais wrote:
> The following patch adds support for correctly
> recognising M7 cpu type.
>
> Signed-off-by: Allen Pais <allen.pais@oracle.com>
> ---
> arch/sparc/include/asm/spitfire.h | 1 +
> arch/sparc/kernel/cpu.c | 6 ++++++
> arch/sparc/kernel/head_64.S | 13 +++++++++++++
> 3 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/arch/sparc/include/asm/spitfire.h b/arch/sparc/include/asm/spitfire.h
> index 3fc5869..9aec17b 100644
> --- a/arch/sparc/include/asm/spitfire.h
> +++ b/arch/sparc/include/asm/spitfire.h
> @@ -45,6 +45,7 @@
> #define SUN4V_CHIP_NIAGARA3 0x03
> #define SUN4V_CHIP_NIAGARA4 0x04
> #define SUN4V_CHIP_NIAGARA5 0x05
> +#define SUN4V_CHIP_SPARC_M7 0x08
> #define SUN4V_CHIP_SPARC64X 0x8a
> #define SUN4V_CHIP_UNKNOWN 0xff
>
> diff --git a/arch/sparc/kernel/cpu.c b/arch/sparc/kernel/cpu.c
> index 82a3a71..55dfb62 100644
> --- a/arch/sparc/kernel/cpu.c
> +++ b/arch/sparc/kernel/cpu.c
> @@ -494,6 +494,12 @@ static void __init sun4v_cpu_probe(void)
> sparc_pmu_type = "niagara5";
> break;
>
> + case SUN4V_CHIP_SPARC_M7:
> + sparc_cpu_type = "SPARC-M7";
> + sparc_fpu_type = "SPARC-M7 integrated FPU";
> + sparc_pmu_type = "sparc-m7";
> + break;
> +
> case SUN4V_CHIP_SPARC64X:
> sparc_cpu_type = "SPARC64-X";
> sparc_fpu_type = "SPARC64-X integrated FPU";
> diff --git a/arch/sparc/kernel/head_64.S b/arch/sparc/kernel/head_64.S
> index 452f04f..508a542 100644
> --- a/arch/sparc/kernel/head_64.S
> +++ b/arch/sparc/kernel/head_64.S
> @@ -414,6 +414,7 @@ sun4v_chip_type:
> cmp %g2, 'T'
> be,pt %xcc, 70f
> cmp %g2, 'M'
> + be,pt %xcc, 71f
> bne,pn %xcc, 49f
Looks like you are missing a nop in the delay slot?
Sam
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] sparc64: correctly recognise M7 cpu type
2014-08-19 4:53 [PATCH 1/3] sparc64: correctly recognise M7 cpu type Allen Pais
2014-08-19 15:21 ` Sam Ravnborg
@ 2014-08-21 10:15 ` Allen Pais
2014-08-21 12:14 ` Sam Ravnborg
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Allen Pais @ 2014-08-21 10:15 UTC (permalink / raw)
To: sparclinux
>> diff --git a/arch/sparc/kernel/head_64.S b/arch/sparc/kernel/head_64.S
>> index 452f04f..508a542 100644
>> --- a/arch/sparc/kernel/head_64.S
>> +++ b/arch/sparc/kernel/head_64.S
>> @@ -414,6 +414,7 @@ sun4v_chip_type:
>> cmp %g2, 'T'
>> be,pt %xcc, 70f
>> cmp %g2, 'M'
>> + be,pt %xcc, 71f
>> bne,pn %xcc, 49f
> Looks like you are missing a nop in the delay slot?
I don't think so. I have tested the patch on M7 and I have had no issues with it.
- Allen
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] sparc64: correctly recognise M7 cpu type
2014-08-19 4:53 [PATCH 1/3] sparc64: correctly recognise M7 cpu type Allen Pais
2014-08-19 15:21 ` Sam Ravnborg
2014-08-21 10:15 ` Allen Pais
@ 2014-08-21 12:14 ` Sam Ravnborg
2014-08-21 12:29 ` Richard Mortimer
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Sam Ravnborg @ 2014-08-21 12:14 UTC (permalink / raw)
To: sparclinux
On Thu, Aug 21, 2014 at 03:33:27PM +0530, Allen Pais wrote:
>
> >> diff --git a/arch/sparc/kernel/head_64.S b/arch/sparc/kernel/head_64.S
> >> index 452f04f..508a542 100644
> >> --- a/arch/sparc/kernel/head_64.S
> >> +++ b/arch/sparc/kernel/head_64.S
> >> @@ -414,6 +414,7 @@ sun4v_chip_type:
> >> cmp %g2, 'T'
> >> be,pt %xcc, 70f
> >> cmp %g2, 'M'
> >> + be,pt %xcc, 71f
> >> bne,pn %xcc, 49f
> > Looks like you are missing a nop in the delay slot?
>
> I don't think so. I have tested the patch on M7 and I have had no issues with it.
Even if it works as expected then it is confusing.
I read the code like this:
if xcc equals pt then jump to 71 but if pn is not equal to xcc then jump to 49
Maybe the second opcode is ignored because it is a conditional jump or maybe
they are always equal.
But whatever it is confusing.
Adding an extra nop would help the readability.
Sam
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] sparc64: correctly recognise M7 cpu type
2014-08-19 4:53 [PATCH 1/3] sparc64: correctly recognise M7 cpu type Allen Pais
` (2 preceding siblings ...)
2014-08-21 12:14 ` Sam Ravnborg
@ 2014-08-21 12:29 ` Richard Mortimer
2014-08-21 13:52 ` Allen Pais
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Richard Mortimer @ 2014-08-21 12:29 UTC (permalink / raw)
To: sparclinux
On 21/08/2014 13:14, Sam Ravnborg wrote:
> On Thu, Aug 21, 2014 at 03:33:27PM +0530, Allen Pais wrote:
>>
>>>> diff --git a/arch/sparc/kernel/head_64.S b/arch/sparc/kernel/head_64.S
>>>> index 452f04f..508a542 100644
>>>> --- a/arch/sparc/kernel/head_64.S
>>>> +++ b/arch/sparc/kernel/head_64.S
>>>> @@ -414,6 +414,7 @@ sun4v_chip_type:
>>>> cmp %g2, 'T'
>>>> be,pt %xcc, 70f
>>>> cmp %g2, 'M'
>>>> + be,pt %xcc, 71f
>>>> bne,pn %xcc, 49f
>>> Looks like you are missing a nop in the delay slot?
>>
>> I don't think so. I have tested the patch on M7 and I have had no issues with it.
> Even if it works as expected then it is confusing.
> I read the code like this:
>
> if xcc equals pt then jump to 71 but if pn is not equal to xcc then jump to 49
>
> Maybe the second opcode is ignored because it is a conditional jump or maybe
> they are always equal.
The two branches are the logical opposite of each other so only one is
ever taken and that is why the code "works".
> But whatever it is confusing.
>
> Adding an extra nop would help the readability.
>
Agreed. This isn't performance sensitive code.
Regards
Richard
P.S. Historical note - SPARC V8 didn't define what happens in the above
case as noted on page 77 of the SPARC v9 manual)
"V8 Compatibility Note:
SPARC-V8 left as undefined the result of executing a delayed conditional
branch that had a delayed control transfer in its delay slot. For this
reason, programmers should avoid such constructs when
backwards compatibility is an issue."
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] sparc64: correctly recognise M7 cpu type
2014-08-19 4:53 [PATCH 1/3] sparc64: correctly recognise M7 cpu type Allen Pais
` (3 preceding siblings ...)
2014-08-21 12:29 ` Richard Mortimer
@ 2014-08-21 13:52 ` Allen Pais
2014-08-22 5:09 ` David Miller
2014-08-22 5:10 ` David Miller
6 siblings, 0 replies; 8+ messages in thread
From: Allen Pais @ 2014-08-21 13:52 UTC (permalink / raw)
To: sparclinux
Sam, Richard,
>>>>> @@ -414,6 +414,7 @@ sun4v_chip_type:
>>>>> cmp %g2, 'T'
>>>>> be,pt %xcc, 70f
>>>>> cmp %g2, 'M'
>>>>> + be,pt %xcc, 71f
>>>>> bne,pn %xcc, 49f
>>>> Looks like you are missing a nop in the delay slot?
>>>
>>> I don't think so. I have tested the patch on M7 and I have had no issues with it.
>> Even if it works as expected then it is confusing.
>> I read the code like this:
>>
>> if xcc equals pt then jump to 71 but if pn is not equal to xcc then jump to 49
>>
>> Maybe the second opcode is ignored because it is a conditional jump or maybe
>> they are always equal.
> The two branches are the logical opposite of each other so only one is ever taken and that is why the code "works".
I'll go ahead and add the delay slot for better readability. It'll look something like this,
cmp %g2, 'T'
be,pt %xcc, 70f
cmp %g2, 'M'
+ be,pt %xcc, 71f
+ nop
bne,pn %xcc, 49f
nop
- ALlen
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] sparc64: correctly recognise M7 cpu type
2014-08-19 4:53 [PATCH 1/3] sparc64: correctly recognise M7 cpu type Allen Pais
` (4 preceding siblings ...)
2014-08-21 13:52 ` Allen Pais
@ 2014-08-22 5:09 ` David Miller
2014-08-22 5:10 ` David Miller
6 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2014-08-22 5:09 UTC (permalink / raw)
To: sparclinux
From: Sam Ravnborg <sam@ravnborg.org>
Date: Thu, 21 Aug 2014 14:14:50 +0200
> Maybe the second opcode is ignored because it is a conditional jump
> or maybe they are always equal. But whatever it is confusing.
I totally agree that the patch should use a nop in the delay slot so
that it's clear and easier to understand, however I wanted to show how
the behavior is well defined and can even be used intentionally for
useful purposes. :-)
Branches in a delay slot a long time ago were marked as undefined
behavior, but for some time now they are expected to behave in
a certain way, consider:
ba label_a
ba label_b
this is a neat way to execute one instruction at label_a then go
to label_b.
More generically:
jmpl %reg
ba interpreter_continue
interpreter_continue:
which you could use to build an instruction at a time execution
engine, which only must elide emulate instructions which use registers
in this special code sequence.
Gordan Irlam used such a technique for his sun4d tracer named SKY.
Unfortunately the SKY web page no longer exists, but this tracer led
to discoveries that brough about things such as the SLAB allocator.
Anyways, all delayed control transfer instructions do is set NPC. So:
0x00 ba 0x20
0x04 ba 0x30
...
0x20 nop
...
0x30 nop
...
At the first instruction, PC=0x00 and NPC=0x04
PC will be set to NPC at the end of execution.
For non-branches NPC is set to NPC + 0x4 but for this
delayed control transfer NPC will be set to 0x20 instead.
So PC=0x04 and NPC=0x20 when we get to the second instruction
in the first instruction's delay slot.
PC will be set to 0x20 and NPC will be set to 0x30
The nop at 0x20 will be executed, PC will be set to 0x30 and NPC
will be set to 0x34.
Finally we execute the nop at 0x30 and continue to linearly execute.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] sparc64: correctly recognise M7 cpu type
2014-08-19 4:53 [PATCH 1/3] sparc64: correctly recognise M7 cpu type Allen Pais
` (5 preceding siblings ...)
2014-08-22 5:09 ` David Miller
@ 2014-08-22 5:10 ` David Miller
6 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2014-08-22 5:10 UTC (permalink / raw)
To: sparclinux
From: Richard Mortimer <richm@oldelvet.org.uk>
Date: Thu, 21 Aug 2014 13:29:36 +0100
> On 21/08/2014 13:14, Sam Ravnborg wrote:
>> Adding an extra nop would help the readability.
>>
> Agreed. This isn't performance sensitive code.
Alan, please resubmit your patch series with the nop added, thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread