[PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[Edgar E. Iglesias]

Hi,

Commit 362e696428590f7d0a5d0971a2d04b0372a761b8
reorders a bunch of insns to improve the flow of the pipeline but
for MT_SMTC kernels, AFAICT, the saving of CP0_STATUS seems wrong.

Am I missing something?

If not here is a patch, tested with qemu.

Cheers

commit 1a72dd0ed0a238007d3d67c1e3a634d477527ce2
Author: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Date:   Tue Aug 30 01:07:15 2011 +0200

    MIPS: SMTC: Correct saving of CP0_STATUS
    
    Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>

diff --git a/arch/mips/include/asm/stackframe.h b/arch/mips/include/asm/stackframe.h
index b4ba244..1b8d9a0 100644
--- a/arch/mips/include/asm/stackframe.h
+++ b/arch/mips/include/asm/stackframe.h
@@ -189,6 +189,9 @@
 		LONG_S	$0, PT_R0(sp)
 		mfc0	v1, CP0_STATUS
 		LONG_S	$2, PT_R2(sp)
+		LONG_S	$4, PT_R4(sp)
+		LONG_S	$5, PT_R5(sp)
+		LONG_S	v1, PT_STATUS(sp)
 #ifdef CONFIG_MIPS_MT_SMTC
 		/*
 		 * Ideally, these instructions would be shuffled in
@@ -199,9 +202,6 @@
 		.set	mips0
 		LONG_S	v1, PT_TCSTATUS(sp)
 #endif /* CONFIG_MIPS_MT_SMTC */
-		LONG_S	$4, PT_R4(sp)
-		LONG_S	$5, PT_R5(sp)
-		LONG_S	v1, PT_STATUS(sp)
 		mfc0	v1, CP0_CAUSE
 		LONG_S	$6, PT_R6(sp)
 		LONG_S	$7, PT_R7(sp)

Re: [PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[David Daney]

[-- Attachment #1: Type: text/plain, Size: 481 bytes --]

On 08/29/2011 04:20 PM, Edgar E. Iglesias wrote:
> Hi,
>
> Commit 362e696428590f7d0a5d0971a2d04b0372a761b8
> reorders a bunch of insns to improve the flow of the pipeline but
> for MT_SMTC kernels, AFAICT, the saving of CP0_STATUS seems wrong.

Indeed.

>
> Am I missing something?
>

It does look like in the MIPS_MT_SMTC case we are clobbering the value 
in v1.

> If not here is a patch, tested with qemu.
>

How about the attached completely untested one instead?

David Daney

[-- Attachment #2: 0001-MIPS-Don-t-clobber-CP0_STATUS-vaue-for-CONFIG_MIPS_M.patch --]
[-- Type: text/plain, Size: 967 bytes --]

>From d0035295ae34bcf84d601b1e25e2642fe0802752 Mon Sep 17 00:00:00 2001
From: David Daney <david.daney@cavium.com>
Date: Mon, 29 Aug 2011 16:42:12 -0700
Subject: [PATCH] MIPS: Don't clobber CP0_STATUS vaue for CONFIG_MIPS_MT_SMTC

Untested, but it looks nice.

Reported-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Signed-off-by: David Daney <david.daney@cavium.com>
---
 arch/mips/include/asm/stackframe.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/stackframe.h b/arch/mips/include/asm/stackframe.h
index 569681e..51a3a0c 100644
--- a/arch/mips/include/asm/stackframe.h
+++ b/arch/mips/include/asm/stackframe.h
@@ -195,9 +195,9 @@
 		 * to cover the pipeline delay.
 		 */
 		.set	mips32
-		mfc0	v1, CP0_TCSTATUS
+		mfc0	k0, CP0_TCSTATUS
 		.set	mips0
-		LONG_S	v1, PT_TCSTATUS(sp)
+		LONG_S	k0, PT_TCSTATUS(sp)
 #endif /* CONFIG_MIPS_MT_SMTC */
 		LONG_S	$4, PT_R4(sp)
 		LONG_S	$5, PT_R5(sp)
-- 
1.7.2.3

Re: [PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[David Daney]

[-- Attachment #1: Type: text/plain, Size: 481 bytes --]

On 08/29/2011 04:20 PM, Edgar E. Iglesias wrote:
> Hi,
>
> Commit 362e696428590f7d0a5d0971a2d04b0372a761b8
> reorders a bunch of insns to improve the flow of the pipeline but
> for MT_SMTC kernels, AFAICT, the saving of CP0_STATUS seems wrong.

Indeed.

>
> Am I missing something?
>

It does look like in the MIPS_MT_SMTC case we are clobbering the value 
in v1.

> If not here is a patch, tested with qemu.
>

How about the attached completely untested one instead?

David Daney

[-- Attachment #2: 0001-MIPS-Don-t-clobber-CP0_STATUS-vaue-for-CONFIG_MIPS_M.patch --]
[-- Type: text/plain, Size: 966 bytes --]

From d0035295ae34bcf84d601b1e25e2642fe0802752 Mon Sep 17 00:00:00 2001
From: David Daney <david.daney@cavium.com>
Date: Mon, 29 Aug 2011 16:42:12 -0700
Subject: [PATCH] MIPS: Don't clobber CP0_STATUS vaue for CONFIG_MIPS_MT_SMTC

Untested, but it looks nice.

Reported-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Signed-off-by: David Daney <david.daney@cavium.com>
---
 arch/mips/include/asm/stackframe.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/stackframe.h b/arch/mips/include/asm/stackframe.h
index 569681e..51a3a0c 100644
--- a/arch/mips/include/asm/stackframe.h
+++ b/arch/mips/include/asm/stackframe.h
@@ -195,9 +195,9 @@
 		 * to cover the pipeline delay.
 		 */
 		.set	mips32
-		mfc0	v1, CP0_TCSTATUS
+		mfc0	k0, CP0_TCSTATUS
 		.set	mips0
-		LONG_S	v1, PT_TCSTATUS(sp)
+		LONG_S	k0, PT_TCSTATUS(sp)
 #endif /* CONFIG_MIPS_MT_SMTC */
 		LONG_S	$4, PT_R4(sp)
 		LONG_S	$5, PT_R5(sp)
-- 
1.7.2.3

Re: [PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[David Daney]

On 08/29/2011 04:45 PM, David Daney wrote:
> On 08/29/2011 04:20 PM, Edgar E. Iglesias wrote:
>> Hi,
>>
>> Commit 362e696428590f7d0a5d0971a2d04b0372a761b8
>> reorders a bunch of insns to improve the flow of the pipeline but
>> for MT_SMTC kernels, AFAICT, the saving of CP0_STATUS seems wrong.
>
> Indeed.
>
>>
>> Am I missing something?
>>
>
> It does look like in the MIPS_MT_SMTC case we are clobbering the value
> in v1.
>
>> If not here is a patch, tested with qemu.
>>
>
> How about the attached completely untested one instead?
>

Ralf,

I didn't mean to imply that Edgar's patch was not correct, it seems 
likely that it is.  Really either approach is fine with me.

David Daney.

.
.
.

>
>  From d0035295ae34bcf84d601b1e25e2642fe0802752 Mon Sep 17 00:00:00 2001
> From: David Daney<david.daney@cavium.com>
> Date: Mon, 29 Aug 2011 16:42:12 -0700
> Subject: [PATCH] MIPS: Don't clobber CP0_STATUS vaue for CONFIG_MIPS_MT_SMTC
>
> Untested, but it looks nice.
>
> Reported-by: Edgar E. Iglesias<edgar.iglesias@gmail.com>
> Signed-off-by: David Daney<david.daney@cavium.com>
> ---
>   arch/mips/include/asm/stackframe.h |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/include/asm/stackframe.h b/arch/mips/include/asm/stackframe.h
> index 569681e..51a3a0c 100644
> --- a/arch/mips/include/asm/stackframe.h
> +++ b/arch/mips/include/asm/stackframe.h
> @@ -195,9 +195,9 @@
>   		 * to cover the pipeline delay.
>   		 */
>   		.set	mips32
> -		mfc0	v1, CP0_TCSTATUS
> +		mfc0	k0, CP0_TCSTATUS
>   		.set	mips0
> -		LONG_S	v1, PT_TCSTATUS(sp)
> +		LONG_S	k0, PT_TCSTATUS(sp)
>   #endif /* CONFIG_MIPS_MT_SMTC */
>   		LONG_S	$4, PT_R4(sp)
>   		LONG_S	$5, PT_R5(sp)

Re: [PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[Kevin D. Kissell]

I submitted that exact patch (David's more minimal version) in December
2010 (and I did test it).  Did it not take?  See the thread "SMTC
support status in latest git head".  The patch went out on December 24
(why I was spending my Christmas Eve fixing MIPS Linux is another
question... :op )

            Kevin K.

On 08/29/11 16:45, David Daney wrote:
> On 08/29/2011 04:20 PM, Edgar E. Iglesias wrote:
>> Hi,
>>
>> Commit 362e696428590f7d0a5d0971a2d04b0372a761b8
>> reorders a bunch of insns to improve the flow of the pipeline but
>> for MT_SMTC kernels, AFAICT, the saving of CP0_STATUS seems wrong.
>
> Indeed.
>
>>
>> Am I missing something?
>>
>
> It does look like in the MIPS_MT_SMTC case we are clobbering the value
> in v1.
>
>> If not here is a patch, tested with qemu.
>>
>
> How about the attached completely untested one instead?
>
> David Daney

Re: [PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[David Daney]

On 08/29/2011 04:55 PM, Kevin D. Kissell wrote:
> I submitted that exact patch (David's more minimal version) in December
> 2010 (and I did test it).  Did it not take?

Evidently not.  Perhaps Ralf will see fit to commit it this time.

>  See the thread "SMTC
> support status in latest git head".  The patch went out on December 24
> (why I was spending my Christmas Eve fixing MIPS Linux is another
> question... :op )
>

... one which I will not attempt to address.

David Daney.


>              Kevin K.
>
> On 08/29/11 16:45, David Daney wrote:
>> On 08/29/2011 04:20 PM, Edgar E. Iglesias wrote:
>>> Hi,
>>>
>>> Commit 362e696428590f7d0a5d0971a2d04b0372a761b8
>>> reorders a bunch of insns to improve the flow of the pipeline but
>>> for MT_SMTC kernels, AFAICT, the saving of CP0_STATUS seems wrong.
>>
>> Indeed.
>>
>>>
>>> Am I missing something?
>>>
>>
>> It does look like in the MIPS_MT_SMTC case we are clobbering the value
>> in v1.
>>
>>> If not here is a patch, tested with qemu.
>>>
>>
>> How about the attached completely untested one instead?
>>
>> David Daney
>
>

Re: [PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[Kevin D. Kissell]

On 08/29/11 17:14, David Daney wrote:
> On 08/29/2011 04:55 PM, Kevin D. Kissell wrote:
>> I submitted that exact patch (David's more minimal version) in December
>> 2010 (and I did test it).  Did it not take?
>
> Evidently not.  Perhaps Ralf will see fit to commit it this time.

Looking back over the exchange and thinking about the date,
in all fairness to Ralf, when I submitted it, it was a diff -u and
not a git diff, because I was at my brother-in-law's house for
the holidays and had no access to my MIPS Linux git machine.
It got picked up by patchwork.linux-mips.org ( patch/1896 ),
but was only actually tested by Anoop a couple of days
later.  He confirmed that it solved the problem, but also that
there were other problems with SMTC in the then-current
head, apparently relating to timekeeping_notify().  Those
problems weren't resolved in the context of that email thread,
so the patch - something along the lines of which really and
truly is necessary - may have been ignored as a partial fix,
even though it is a self-contained solution to a self-contained bug.

            Regards,

            Kevin K.

Re: [PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[Edgar E. Iglesias]

On Mon, Aug 29, 2011 at 04:45:20PM -0700, David Daney wrote:
> On 08/29/2011 04:20 PM, Edgar E. Iglesias wrote:
> >Hi,
> >
> >Commit 362e696428590f7d0a5d0971a2d04b0372a761b8
> >reorders a bunch of insns to improve the flow of the pipeline but
> >for MT_SMTC kernels, AFAICT, the saving of CP0_STATUS seems wrong.
> 
> Indeed.
> 
> >
> >Am I missing something?
> >
> 
> It does look like in the MIPS_MT_SMTC case we are clobbering the
> value in v1.
> 
> >If not here is a patch, tested with qemu.
> >
> 
> How about the attached completely untested one instead?
> 
> David Daney

> From d0035295ae34bcf84d601b1e25e2642fe0802752 Mon Sep 17 00:00:00 2001
> From: David Daney <david.daney@cavium.com>
> Date: Mon, 29 Aug 2011 16:42:12 -0700
> Subject: [PATCH] MIPS: Don't clobber CP0_STATUS vaue for CONFIG_MIPS_MT_SMTC
> 
> Untested, but it looks nice.


Thanks, I like you patch better. But whichever version will do.

Cheers

Re: [PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[Edgar E. Iglesias]

On Mon, Aug 29, 2011 at 05:35:44PM -0700, Kevin D. Kissell wrote:
> On 08/29/11 17:14, David Daney wrote:
> > On 08/29/2011 04:55 PM, Kevin D. Kissell wrote:
> >> I submitted that exact patch (David's more minimal version) in December
> >> 2010 (and I did test it).  Did it not take?
> >
> > Evidently not.  Perhaps Ralf will see fit to commit it this time.
> 
> Looking back over the exchange and thinking about the date,
> in all fairness to Ralf, when I submitted it, it was a diff -u and
> not a git diff, because I was at my brother-in-law's house for
> the holidays and had no access to my MIPS Linux git machine.
> It got picked up by patchwork.linux-mips.org ( patch/1896 ),
> but was only actually tested by Anoop a couple of days
> later.  He confirmed that it solved the problem, but also that
> there were other problems with SMTC in the then-current
> head, apparently relating to timekeeping_notify().  Those
> problems weren't resolved in the context of that email thread,
> so the patch - something along the lines of which really and
> truly is necessary - may have been ignored as a partial fix,
> even though it is a self-contained solution to a self-contained bug.

Interesting. Coincidentally, after getting pass the CP0_Status issue, I ran
into some issues related to time-keeping, but I got the impression I was
seeing a mix of problems with QEMU and possibly the kernel. I never hunted
these issues down though. Maybe I'll give it a go some other time..

It makes me wonder, what is the state of SMTC kernels? Are they widely
used and considered stable?
Or is the SMP mode (1 TC per VPE) the common choice?

Cheers

Re: [PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[Kevin D. Kissell]

On 08/30/11 04:16, Edgar E. Iglesias wrote:
> On Mon, Aug 29, 2011 at 05:35:44PM -0700, Kevin D. Kissell wrote:
>> On 08/29/11 17:14, David Daney wrote:
>>> On 08/29/2011 04:55 PM, Kevin D. Kissell wrote:
>>>> I submitted that exact patch (David's more minimal version) in December
>>>> 2010 (and I did test it).  Did it not take?
>>> Evidently not.  Perhaps Ralf will see fit to commit it this time.
>> Looking back over the exchange and thinking about the date,
>> in all fairness to Ralf, when I submitted it, it was a diff -u and
>> not a git diff, because I was at my brother-in-law's house for
>> the holidays and had no access to my MIPS Linux git machine.
>> It got picked up by patchwork.linux-mips.org ( patch/1896 ),
>> but was only actually tested by Anoop a couple of days
>> later.  He confirmed that it solved the problem, but also that
>> there were other problems with SMTC in the then-current
>> head, apparently relating to timekeeping_notify().  Those
>> problems weren't resolved in the context of that email thread,
>> so the patch - something along the lines of which really and
>> truly is necessary - may have been ignored as a partial fix,
>> even though it is a self-contained solution to a self-contained bug.
> Interesting. Coincidentally, after getting pass the CP0_Status issue, I ran
> into some issues related to time-keeping, but I got the impression I was
> seeing a mix of problems with QEMU and possibly the kernel. I never hunted
> these issues down though. Maybe I'll give it a go some other time..
It could very well have been a QEMU issue.  At the time, I did spend
a while staring at the diffs between the working and non-working
kernel sources and I was unable to spot anything obviously suspect.
> It makes me wonder, what is the state of SMTC kernels? Are they widely
> used and considered stable?
> Or is the SMP mode (1 TC per VPE) the common choice?
The virtual SMP mode is far more common.  SMTC has the advantage
that it allows the maximum throughput to be extracted from a 34K
core - depending on the application/benchmark, the "sweet spot"
may be more than 2 concurrent threads - but it's less well maintained.

            Regards,

            Kevin K.

Re: [PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[Ralf Baechle]

On Tue, Aug 30, 2011 at 09:54:53AM -0700, Kevin D. Kissell wrote:

> It could very well have been a QEMU issue.  At the time, I did spend
> a while staring at the diffs between the working and non-working
> kernel sources and I was unable to spot anything obviously suspect.
> > It makes me wonder, what is the state of SMTC kernels? Are they widely
> > used and considered stable?
> > Or is the SMP mode (1 TC per VPE) the common choice?
> The virtual SMP mode is far more common.  SMTC has the advantage
> that it allows the maximum throughput to be extracted from a 34K
> core - depending on the application/benchmark, the "sweet spot"
> may be more than 2 concurrent threads - but it's less well maintained.

Not to mention that SMTC was developed for a single 34K core.  It has
never been pimped up to support multi-core systems such as the 1004K
which would add some considerable complexity.

  Ralf

Re: [PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[Kevin D. Kissell]

On 09/14/11 08:12, Ralf Baechle wrote:
> On Tue, Aug 30, 2011 at 09:54:53AM -0700, Kevin D. Kissell wrote:
>
>> It could very well have been a QEMU issue.  At the time, I did spend
>> a while staring at the diffs between the working and non-working
>> kernel sources and I was unable to spot anything obviously suspect.
>>> It makes me wonder, what is the state of SMTC kernels? Are they widely
>>> used and considered stable?
>>> Or is the SMP mode (1 TC per VPE) the common choice?
>> The virtual SMP mode is far more common.  SMTC has the advantage
>> that it allows the maximum throughput to be extracted from a 34K
>> core - depending on the application/benchmark, the "sweet spot"
>> may be more than 2 concurrent threads - but it's less well maintained.
> Not to mention that SMTC was developed for a single 34K core.  It has
> never been pimped up to support multi-core systems such as the 1004K
> which would add some considerable complexity.
SMTC was only *run* on single 34K cores, but the design was
done keeping in mind the possibility that a multi-MT-core
configuration might be built.  The main thing that would need
to be done would be to add startup code that would make the
first TC/VPE of the second (and subsequent) cores recognize
that they are secondary CPUs and follow the same code path
as the non-zeroth TC/VPE in the single-core configuration.  One
would also need to do "real" IPIs between cores, which is anyway
what the actual production parts with 34Ks in them do (as opposed
to what we had to do on the FPGA prototypes).

The 1004K, as I understand it, was designed with exactly 2 VPEs
and 2 TCs, and unless there's an option in the core configuration
to synthesize it with more TCs per VPE (>2 TCs or <2 VPEs), SMTC
would not be meaningful.  Even if you could build a single-VPE
1004K with 2 TCs, the area savings would be relatively small
and would have to be balanced against the small per-TC
kernel performance hit of going to SMTC.  SMTC rocks when
you  can put 3 or 4 TCs in a single VPE, so that the better
utilization of functional units has a chance to more than
make up for the SMTC efficiency hit in kernel mode.

            Regards,

            Kevin K.

Re: [PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[Ralf Baechle]

On Mon, Aug 29, 2011 at 04:45:20PM -0700, David Daney wrote:

> How about the attached completely untested one instead?

Applied.  I like it more than than Edgar's patch because of the better
scheduling.

I didn't apply Kevin's original patch because a followup to the patch
mentioned there were still problems remaining - but it seems they were
unrelated.

Thanks everybody,

  Ralf

Re: [PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[Edgar E. Iglesias]

On Thu, Sep 15, 2011 at 06:00:54PM +0200, Ralf Baechle wrote:
> On Mon, Aug 29, 2011 at 04:45:20PM -0700, David Daney wrote:
> 
> > How about the attached completely untested one instead?
> 
> Applied.  I like it more than than Edgar's patch because of the better
> scheduling.

I agree, thanks!

BTW, in case anyone is intersted, it is now possible to boot malta
boards with SMP with the latest QEMU. The neat thing is that if you
debug the kernel with GDB, you'll see the different cores execution
contexts as differten threads and can singletep them individually.

You need a QEMU from latest git.

Cheers

Re: [PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[Ralf Baechle]

On Sat, Sep 17, 2011 at 03:23:15AM +0200, Edgar E. Iglesias wrote:

> I agree, thanks!

You're welcome.

> BTW, in case anyone is intersted, it is now possible to boot malta
> boards with SMP with the latest QEMU. The neat thing is that if you
> debug the kernel with GDB, you'll see the different cores execution
> contexts as differten threads and can singletep them individually.

Very interesting.  What type of SMP system does it emulate?

  Ralf

Re: [PATCH] MIPS: SMTC: Correct saving of CP0_STATUS

[Edgar E. Iglesias]

On Sat, Sep 17, 2011 at 04:00:32AM +0200, Ralf Baechle wrote:
> On Sat, Sep 17, 2011 at 03:23:15AM +0200, Edgar E. Iglesias wrote:
> 
> > I agree, thanks!
> 
> You're welcome.
> 
> > BTW, in case anyone is intersted, it is now possible to boot malta
> > boards with SMP with the latest QEMU. The neat thing is that if you
> > debug the kernel with GDB, you'll see the different cores execution
> > contexts as differten threads and can singletep them individually.
> 
> Very interesting.  What type of SMP system does it emulate?

I'm not all familiar with the terminology, but it works with the kind
where you've got multiple VPEs with one TC per VPE, vSMP IIUC.

You need to pass these options to QEMU, -cpu 34Kf -smp 2, where 2 maybe
anything up to 16.

We've (AXIS) got models for the GIC aswell, but I need to check with MTI 
before publishing those.  IMO these models would help everybody cause
it's, to say the leaast, a huge pain to setup the GIC without having QEMU
or any emulator to check the interrupt routing.

Cheers