[PATCH]: powerpc documentation: Clarify why twi appears in the i/o macros.

[PATCH]: powerpc documentation: Clarify why twi appears in the i/o macros.

[Linas Vepstas]

Paul,
Please apply. This atch resulted from an email discussion
back in Sept.

--linas

Clarify why twi appears in the i/o macros.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Signed-off-by: Segher Boessenkool <segher@kernel.crashing.org>

----
 include/asm-powerpc/io.h |   11 +++++++++++
 1 file changed, 11 insertions(+)

Index: linux-2.6.19-git7/include/asm-powerpc/io.h
===================================================================
--- linux-2.6.19-git7.orig/include/asm-powerpc/io.h	2006-12-05 17:11:02.000000000 -0600
+++ linux-2.6.19-git7/include/asm-powerpc/io.h	2006-12-06 11:48:43.000000000 -0600
@@ -78,6 +78,17 @@ extern unsigned long pci_dram_offset;
  * Note: I might drop the _ns suffix on the stream operations soon as it is
  * simply normal for stream operations to not swap in the first place.
  *
+ * Read operations have additional twi & isync to make sure the read
+ * is actually performed (i.e. the data has come back) before we start
+ * executing any following instructions.
+ *
+ * A data-dependent branch followed by an isync ensures that no
+ * instructions after the isync in program order will be
+ * (speculatively) executed before the isync has completed, and
+ * the isync won't complete until the branch is resolved. The
+ * the load that the twi depends on has to complete before
+ * anything else is executed; in particular, it's a barrier to
+ * keep MMIO reads ordered before main-storage accesses.
  */
 
 #ifdef CONFIG_PPC64

Re: [PATCH]: powerpc documentation: Clarify why twi appears in the i/o macros.

[Sergei Shtylyov]

Hello.

Linas Vepstas wrote:

> Paul,
> Please apply. This atch resulted from an email discussion
> back in Sept.

> --linas

> Clarify why twi appears in the i/o macros.

> Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
> Signed-off-by: Segher Boessenkool <segher@kernel.crashing.org>
> 
> ----
>  include/asm-powerpc/io.h |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> Index: linux-2.6.19-git7/include/asm-powerpc/io.h
> ===================================================================
> --- linux-2.6.19-git7.orig/include/asm-powerpc/io.h	2006-12-05 17:11:02.000000000 -0600
> +++ linux-2.6.19-git7/include/asm-powerpc/io.h	2006-12-06 11:48:43.000000000 -0600
> @@ -78,6 +78,17 @@ extern unsigned long pci_dram_offset;
>   * Note: I might drop the _ns suffix on the stream operations soon as it is
>   * simply normal for stream operations to not swap in the first place.
>   *
> + * Read operations have additional twi & isync to make sure the read
> + * is actually performed (i.e. the data has come back) before we start
> + * executing any following instructions.
> + *
> + * A data-dependent branch followed by an isync ensures that no
> + * instructions after the isync in program order will be
> + * (speculatively) executed before the isync has completed, and
> + * the isync won't complete until the branch is resolved. The
> + * the load that the twi depends on has to complete before

    A minor type here -- double "the".

> + * anything else is executed; in particular, it's a barrier to
> + * keep MMIO reads ordered before main-storage accesses.
>   */
>  
>  #ifdef CONFIG_PPC64

WBR, Sergei

Re: [PATCH]: powerpc documentation: Clarify why twi appears in the i/o macros.

[Linas Vepstas]

Paul, a revised patch below, please apply.

On Wed, Dec 06, 2006 at 09:39:00PM +0300, Sergei Shtylyov wrote:
> 
>    A minor type here -- double "the".

Dohh. New patch below.

> WBR, Sergei

What does "WBR" mean?

--linas


Clarify why twi appears in the i/o macros.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Signed-off-by: Segher Boessenkool <segher@kernel.crashing.org>

----
 include/asm-powerpc/io.h |   11 +++++++++++
 1 file changed, 11 insertions(+)

Index: linux-2.6.19-git7/include/asm-powerpc/io.h
===================================================================
--- linux-2.6.19-git7.orig/include/asm-powerpc/io.h	2006-12-06 13:38:43.000000000 -0600
+++ linux-2.6.19-git7/include/asm-powerpc/io.h	2006-12-06 13:41:50.000000000 -0600
@@ -78,6 +78,17 @@ extern unsigned long pci_dram_offset;
  * Note: I might drop the _ns suffix on the stream operations soon as it is
  * simply normal for stream operations to not swap in the first place.
  *
+ * Read operations have additional twi & isync to make sure the read
+ * is actually performed (i.e. the data has come back) before we start
+ * executing any following instructions.
+ *
+ * A data-dependent branch followed by an isync ensures that
+ * no instructions after the isync in program order will be
+ * (speculatively) executed before the isync has completed,
+ * and the isync won't complete until the branch is resolved.
+ * The load that the twi depends on has to complete before
+ * anything else is executed; in particular, it's a barrier
+ * to keep MMIO reads ordered before main-storage accesses.
  */
 
 #ifdef CONFIG_PPC64

Re: [PATCH]: powerpc documentation: Clarify why twi appears in the i/o macros.

[Linas Vepstas]

Paul,

Please apply.

On Wed, Dec 06, 2006 at 11:24:32PM +0300, Sergei Shtylyov wrote:
> >>  A minor type here -- double "the".
>    And here as well. :-)

I forgot to say "quilt refresh" !? My quality control mechanism
broke.

So again ... 

--linas

Clarify why twi appears in the i/o macros. Shorten some over-length
lines, while we're at it.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
Signed-off-by: Segher Boessenkool <segher@kernel.crashing.org>

----
 include/asm-powerpc/io.h |   33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

Index: linux-2.6.19-git7/include/asm-powerpc/io.h
===================================================================
--- linux-2.6.19-git7.orig/include/asm-powerpc/io.h	2006-12-06 16:11:31.000000000 -0600
+++ linux-2.6.19-git7/include/asm-powerpc/io.h	2006-12-06 16:19:40.000000000 -0600
@@ -62,22 +62,33 @@ extern unsigned long pci_dram_offset;
  *
  * Low level MMIO accessors
  *
- * This provides the non-bus specific accessors to MMIO. Those are PowerPC
- * specific and thus shouldn't be used in generic code. The accessors
- * provided here are:
+ * This provides the non-bus specific accessors to MMIO. Those are 
+ * PowerPC specific and thus shouldn't be used in generic code. The 
+ * accessors provided here are:
  *
  *	in_8, in_le16, in_be16, in_le32, in_be32, in_le64, in_be64
  *	out_8, out_le16, out_be16, out_le32, out_be32, out_le64, out_be64
  *	_insb, _insw_ns, _insl_ns, _outsb, _outsw_ns, _outsl_ns
  *
- * Those operate directly on a kernel virtual address. Note that the prototype
- * for the out_* accessors has the arguments in opposite order from the usual
- * linux PCI accessors. Unlike those, they take the address first and the value
- * next.
- *
- * Note: I might drop the _ns suffix on the stream operations soon as it is
- * simply normal for stream operations to not swap in the first place.
- *
+ * Those operate directly on a kernel virtual address. Note that the 
+ * prototype for the out_* accessors has the arguments in opposite 
+ * order from the usual linux PCI accessors. Unlike those, they take 
+ * the address first and the value next.
+ *
+ * Note: I might drop the _ns suffix on the stream operations soon as it
+ * is simply normal for stream operations to not swap in the first place.
+ *
+ * Read operations have additional twi & isync to make sure the read
+ * is actually performed (i.e. the data has come back) before we start
+ * executing any following instructions.
+ *
+ * A data-dependent branch followed by an isync ensures that no
+ * instructions after the isync in program order will be 
+ * (speculatively) executed before the isync has completed, and the 
+ * isync won't complete until the branch is resolved. The load that 
+ * the twi depends on has to complete before anything else is 
+ * executed; in particular, it's a barrier to keep MMIO reads ordered 
+ * before main-storage accesses.
  */
 
 #ifdef CONFIG_PPC64

Re: [PATCH]: powerpc documentation: Clarify why twi appears in the i/o macros.

[Paul Mackerras]

Linas Vepstas writes:

> Clarify why twi appears in the i/o macros. Shorten some over-length
> lines, while we're at it.

[snip]

> + * A data-dependent branch followed by an isync ensures that no

I think it's potentially confusing to talk about data-dependent
branches when what the code does is a twi instruction.  Even if you
argue that a twi is a data-dependent branch (and I disagree with that,
since a trap is not a branch), that wouldn't be obvious to a casual
reader of the code.  In other words, I don't think the comment
clarifies the situation very much.

Paul.

Re: [PATCH]: powerpc documentation: Clarify why twi appears in the i/o macros.

[Segher Boessenkool]

>> + * A data-dependent branch followed by an isync ensures that no
>
> I think it's potentially confusing to talk about data-dependent
> branches when what the code does is a twi instruction.  Even if you
> argue that a twi is a data-dependent branch (and I disagree with that,
> since a trap is not a branch),

If you argue it is *not* a branch, where in the architecture
documentation can we find any language that gives us the
guarantee we depend on here?

> that wouldn't be obvious to a casual
> reader of the code.

True enough.

> In other words, I don't think the comment
> clarifies the situation very much.

Got anything better?  :-)

The first half of the new comment probably should go in no
matter what (it says what twi;isync do here, not how exactly
that works).


Segher

Re: [PATCH]: powerpc documentation: Clarify why twi appears in the i/o macros.

[Linas Vepstas]

On Fri, Dec 08, 2006 at 10:54:56PM +0100, Segher Boessenkool wrote:
> >>+ * A data-dependent branch followed by an isync ensures that no
> >
> >I think it's potentially confusing to talk about data-dependent
> >branches when what the code does is a twi instruction.  Even if you
> >argue that a twi is a data-dependent branch (and I disagree with that,
> >since a trap is not a branch),
> 
> If you argue it is *not* a branch, where in the architecture
> documentation can we find any language that gives us the
> guarantee we depend on here?

How about this wording: "A data-dependent branch, or trap word, 
followed by an isync ensures that ..."

> >that wouldn't be obvious to a casual
> >reader of the code.

I was trying to understand why the spidernet driver seemed to 
behave in the crazy ways that it does, and I decided to scrutinize 
this code. Even if one is relatively familiar with the overall
powerpc architecture (as I am), the effect of the twi/isync 
was far from obvious, and left me scratching my head.

> >In other words, I don't think the comment
> >clarifies the situation very much.

Later on, I tripped over some text written by Segher, that
made a lightbulb go off in my head ... and thus I was motiviated
to plagiarizingly cut-n-paste it into here.  So this block of 
text clarified things a lot for me, it "hit the spot".

Now, what I don't know is how accurate it really is ... I don't
know if there aren't any strange deviations on 403 or 601 or 
any of the modern embedded processors, or whatever. But the text
certainly makes me beleive I now know what's going on. As a
service to the next person who scratches thier head in wonderment, 
we should have at least something written up here.

--linas

Re: [PATCH]: powerpc documentation: Clarify why twi appears in the i/o macros.

[Paul Mackerras]

Segher Boessenkool writes:

> If you argue it is *not* a branch, where in the architecture
> documentation can we find any language that gives us the
> guarantee we depend on here?

isync is context synchronizing.  In the definition of context
synchronization, it says "2.  The operation is not initiated, or in
the case of isync, does not complete, until all instructions already
in execution have completed to a point at which they have reported all
exceptions they will cause".  The twi conditionally causes an
exception depending on the data from the previous load, therefore it
cannot complete to a point at which is has reported all exceptions it
will cause until it sees the data from the load.  Therefore the isync
cannot complete (and allow following instructions to start) until the
data from the load has returned.

I once asked Ed Silha (one of the PowerPC architects) whether this
reasoning was good, and he agreed it was.

Paul.

Re: [PATCH]: powerpc documentation: Clarify why twi appears in the i/o macros.

[Linas Vepstas]

On Sat, Dec 09, 2006 at 11:19:23AM +1100, Paul Mackerras wrote:
> Segher Boessenkool writes:
> 
> > If you argue it is *not* a branch, where in the architecture
> > documentation can we find any language that gives us the
> > guarantee we depend on here?
> 
> isync is context synchronizing.  In the definition of context
> synchronization, it says "2.  The operation is not initiated, or in
> the case of isync, does not complete, until all instructions already
> in execution have completed to a point at which they have reported all
> exceptions they will cause".  The twi conditionally causes an
> exception depending on the data from the previous load, therefore it
> cannot complete to a point at which is has reported all exceptions it
> will cause until it sees the data from the load.  Therefore the isync
> cannot complete (and allow following instructions to start) until the
> data from the load has returned.

I'd be happy if this text, verbatim or suitably massaged, were 
included in the header file. 

--linas

Re: [PATCH]: powerpc documentation: Clarify why twi appears in the i/o macros.

[Segher Boessenkool]

> isync is context synchronizing.  In the definition of context
> synchronization, it says "2.  The operation is not initiated, or in
> the case of isync, does not complete, until all instructions already
> in execution have completed to a point at which they have reported all
> exceptions they will cause".

> The twi conditionally causes an exception

Except we use a trap with TO=0 so the CPU could optimise the
dependency away -- or where does it say this is prohibited?

For conditional branches, this behaviour (not optimising away
branches, even if they have a result independent on the data)
is explicitly documented.

> depending on the data from the previous load, therefore it
> cannot complete to a point at which is has reported all exceptions it
> will cause until it sees the data from the load.  Therefore the isync
> cannot complete (and allow following instructions to start) until the
> data from the load has returned.
>
> I once asked Ed Silha (one of the PowerPC architects) whether this
> reasoning was good, and he agreed it was.

My only nit is I can't find a guarantee that the trap instruction will
have to get the read data as input before it can "report the exceptions
it caused".  Would you have anything on that?  I'll sleep so much better
if I know it's safe :-)


Segher

Re: [PATCH]: powerpc documentation: Clarify why twi appears in the i/o macros.

[Paul Mackerras]

Segher Boessenkool writes:

> Except we use a trap with TO=0 so the CPU could optimise the
> dependency away -- or where does it say this is prohibited?

I don't know that it explicitly says it is prohibited, but Ed Silha
agreed when I asked him that optimizing this would be against the
spirit of the architecture, at least.  I don't think the cpu designers
would bother optimizing this since it wouldn't get them any extra
performance on any benchmark.

> My only nit is I can't find a guarantee that the trap instruction will
> have to get the read data as input before it can "report the exceptions
> it caused".  Would you have anything on that?  I'll sleep so much better
> if I know it's safe :-)

Well, clearly in general the conditional-trap instructions would have
to see their input operand(s) before they can report an exception,
since reporting an exception before that would be incorrect behaviour
- you would end up taking a 700 interrupt when you shouldn't.  In
other words I am asserting that if an instruction reports a trap
exception, that leads inevitably to a 700 interrupt (in the
architecture sense of "interrupt"), unless of course an earlier
instruction also reports an exception.

Your point about TO=0 is reasonable, and something I also worried
about a little, but Ed Silha didn't seem to think it was a problem.

Paul.

Re: [PATCH]: powerpc documentation: Clarify why twi appears in the i/o macros.

[Segher Boessenkool]

>> Except we use a trap with TO=0 so the CPU could optimise the
>> dependency away -- or where does it say this is prohibited?
>
> I don't know that it explicitly says it is prohibited, but Ed Silha
> agreed when I asked him that optimizing this would be against the
> spirit of the architecture, at least.

Yeah true.  Weirder things that go against "the spirit of the
architecture" have happened though :-)

> I don't think the cpu designers
> would bother optimizing this since it wouldn't get them any extra
> performance on any benchmark.

Not directly for this case -- but it might happen as a side
effect of a more general optimisation perhaps.  Who knows.

>> My only nit is I can't find a guarantee that the trap instruction  
>> will
>> have to get the read data as input before it can "report the  
>> exceptions
>> it caused".  Would you have anything on that?  I'll sleep so much  
>> better
>> if I know it's safe :-)
>
> Well, clearly in general the conditional-trap instructions would have
> to see their input operand(s) before they can report an exception,
> since reporting an exception before that would be incorrect behaviour
> - you would end up taking a 700 interrupt when you shouldn't.

I meant for the case where the exceptions it reports are
"no exceptions".  In some cases (like TO=0) the CPU could
short-circuit the evaluation of what exceptions can be
caused before it has all the input data.  As another
(farfetched but perhaps more realistic) example, a CPU that
does a lot of (memory) speculation could decide that an
instruction will not cause an exception, if it doesn't do
so on any of the possible (speculated) paths.

> Your point about TO=0 is reasonable, and something I also worried
> about a little, but Ed Silha didn't seem to think it was a problem.

It would be lovely to get a guarantee about this in the
architecture.  Failing that, what about this comment (edited
from Linas' patch and your mail):


Read operations have additional twi & isync to make sure the read
is actually performed (i.e. the data has come back) before we start
executing any following instructions.

isync is context synchronizing.  In the definition of context
synchronization, the architecture documents say "2.  The operation
is not initiated, or in the case of isync, does not complete, until
all instructions already in execution have completed to a point at
which they have reported all exceptions they will cause".  The twi
conditionally causes an exception depending on the data from the
previous load; on all current CPUs it will not complete until it
sees the data from the load.  Therefore the isync cannot complete
(and allow following instructions to start) until the data from the
load has returned.


Segher