RFA: dcache corruption bug

[jtc at redback dot com (J.T. Conklin)]

While adopting dcache for memory region attributes, I discovered what
appears to be a potential cache corruption, plus a nit that the 'refs'
field is never zeroed when a cache line is re-cycled, which makes it 
somewhat less than useful.

The comment for dcache_alloc() states its caller should remove the
returned dcache_block from the free list and place it on the valid
list to prevent cache corruption.  Good idea, but the code actually
removes a dcache_block from the free list (or recycles one from the
valid list) and inserts it on the valid list.  Because none of the 
fields are re-initialized, if the caller does not do it the contents
of the cache block could be written to memory.

This was the case for dcache_peek_byte().  If a new block was
allocated, and for some reason the read failed, the block would have
the contents of the previous cache line that used the block.

I don't think that bad values would be ever be written; the line would
have been flushed before the block was re-cycled. But bytes with state
ENTRY_OK would return the old values in subsequent reads.

To fix this problem, I changed dcache_alloc() to initialize the
address and invalidate the cache line before returning a new block.

Other related issues:

* dcache makes no attempt to remove the least-recently-used block when
  a cache line is recycled.  When a new cache-line is allocated it is
  added to the tail of the valid list.  If there aren't any cache-lines
  in the free list, the one from the head of the valid list is recycled.
  Anyone have any reasons why making this a LRU cache is a bad idea?

* dcache has a free list and a valid list.  Why not just have one list 
  and invalidate unused cache lines.  When we need a new block, we can
  just grab a line from the pool.  With a LRU cache, the unused blocks
  will be the ones subject for re-cycling.

* Is there a generic set of list manipulation macros in libiberty?  I
  thought there was, but couldn't find anything.


        --jtc

2000-08-09  J.T. Conklin  <jtc@redback.com>

        * dcache.c (dcache_alloc): Changed to take address of line as an
        argument, and to invalidate cache line before returning.
        (dcache_peek_byte): Updated.
        (dcache_poke_byte): Updated.


Index: dcache.c
===================================================================
RCS file: /cvs/src/src/gdb/dcache.c,v
retrieving revision 1.5
diff -c -r1.5 dcache.c
*** dcache.c	2000/07/30 01:48:25	1.5
--- dcache.c	2000/08/09 18:17:28
***************
*** 157,163 ****
  
  static int dcache_write_line (DCACHE * dcache, struct dcache_block *db);
  
! static struct dcache_block *dcache_alloc (DCACHE * dcache);
  
  static int dcache_writeback (DCACHE * dcache);
  
--- 157,163 ----
  
  static int dcache_write_line (DCACHE * dcache, struct dcache_block *db);
  
! static struct dcache_block *dcache_alloc (DCACHE * dcache, CORE_ADDR addr);
  
  static int dcache_writeback (DCACHE * dcache);
  
***************
*** 267,281 ****
  
  
  /* Get a free cache block, put or keep it on the valid list,
!    and return its address.  The caller should store into the block
!    the address and data that it describes, then remque it from the
!    free list and insert it into the valid list.  This procedure
!    prevents errors from creeping in if a memory retrieval is
!    interrupted (which used to put garbage blocks in the valid
!    list...).  */
  
  static struct dcache_block *
! dcache_alloc (DCACHE *dcache)
  {
    register struct dcache_block *db;
  
--- 267,276 ----
  
  
  /* Get a free cache block, put or keep it on the valid list,
!    and return its address.  */
  
  static struct dcache_block *
! dcache_alloc (DCACHE *dcache, CORE_ADDR addr)
  {
    register struct dcache_block *db;
  
***************
*** 297,302 ****
--- 292,302 ----
        dcache_write_line (dcache, db);
      }
  
+   db->addr = MASK(addr);
+   db->refs = 0;
+   db->anydirty = 0;
+   memset (db->state, ENTRY_BAD, sizeof (db->data));
+ 
    /* append this line to end of valid list */
    if (!dcache->valid_head)
      dcache->valid_head = db;
***************
*** 327,335 ****
  	  dcache_write_line (dcache, db);
  	}
        else
! 	db = dcache_alloc (dcache);
        immediate_quit++;
-       db->addr = MASK (addr);
        while (done < LINE_SIZE)
  	{
  	  int try =
--- 327,335 ----
  	  dcache_write_line (dcache, db);
  	}
        else
! 	db = dcache_alloc (dcache, addr);
! 
        immediate_quit++;
        while (done < LINE_SIZE)
  	{
  	  int try =
***************
*** 379,387 ****
  
    if (!db)
      {
!       db = dcache_alloc (dcache);
!       db->addr = MASK (addr);
!       memset (db->state, ENTRY_BAD, sizeof (db->data));
      }
  
    db->data[XFORM (addr)] = *ptr;
--- 379,385 ----
  
    if (!db)
      {
!       db = dcache_alloc (dcache, addr);
      }
  
    db->data[XFORM (addr)] = *ptr;



-- 
J.T. Conklin
RedBack Networks