Re: [RFA] MIPS 24K Errata Patch

[Catherine Moore <clm at codesourcery dot com>]

On 04/19/2011 03:41 PM, Richard Sandiford wrote:
> Catherine Moore<clm@codesourcery.com>  writes:
> > I've attached updated patches for the testsuite and the assembler.  I
> > think that I've addressed all of your comments.  Do these new patches
> > now look okay?
>
> Thanks for the updates.  They look good apart from a few minor
> stylistic nits.  I'm still not sure about the last point though:
>
> [ Hmm, I see I got the hex offsets wrong last time, sorry.  I realise
>    it probably didn't make much sense with those mistakes. ]
>
>   The last case is interesting.  Does the errata still trigger if
>   a suxc1 instruction straddles a cache boundary?  E.g., playing
>   battleships for a moment:
>
>       0 2 4 6
>     0 ........
>     8 ........
>    10 ........
>    18 ....XXXX   suxc1 to 0x1C
>    20 XXXX....
>    28 ........
>    30 ..XX....   sh to 0x32
>    38 ......XX   sh to 0x3E
>
>   It looks like the range checks will return 0 in this case, even though
>   all three stores write to different doublewords of the second cache line.

Is there any instruction besides suxc1 that is of concern (ie.  any 
other instruction that allows for unaligned access)?  I've altered the 
original patch to insert nops for the case where the offset field is a 
register.  suxc1 is an instruction in this category.

For this test case:
	.text
	add $4, $4, $4
	add $4, $4, $4
	add $4, $4, $4
	add $4, $4, $4
	suxc1 $f4, $5($2)
	sh    $4, 0x32($2)
	sh    $4, 0x3e($2)

assembled with -mips32r2 -mfix-24k, The current implementation gives me:

00000000 <.text>:
   0:	00842020 	add	a0,a0,a0
   4:	00842020 	add	a0,a0,a0
   8:	00842020 	add	a0,a0,a0
   c:	00842020 	add	a0,a0,a0
  10:	4c45200d 	suxc1	$f4,a1(v0)
  14:	a4440032 	sh	a0,50(v0)
  18:	00000000 	nop
  1c:	a444003e 	sh	a0,62(v0)

negating the need to alter the range checks for all of the unaligned cases.

>   The answer might well be "yes, the errata doesn't trigger for this case".
>   If so, it'd be worth mentioning that in the main comment.  The answer
>   affects what the comments above the remaining 24 and 32 checks should be.
>
> What I meant was that:
>
>    pos[0].off == 0x1C
>    pos[1].off == 0x32
>    pos[2].off == 0x3E
>
> and so:
>
>    if (pos[2].off - pos[1].off>= 24
>        || pos[1].off - pos[0].off>= 24
>        || pos[2].off - pos[0].off>= 32)
>      return 0;
>
> would say that no nops are needed here.  However, all stores are writing
> to different doublewords of the second cache line, and from a naive
> reading of the the errata description, that means that the errata
> could trigger.
>
> Which is right?  If the code is right, then the errata description
> needs a bit more detail.  If the naive reading of the errata is right,
> then the code needs to change.

  I've been told that the naive-reading is correct and that yes, the 
errata should trigger.

Thanks,
Catherine

>