src/mem/packet.hh (Diff revision 1)

Doxygen comments for this function?

just fix up the doxygen comment

src/mem/packet.hh (Diff revision 1)

In most on-chip protocols there is a notion of beats (i.e. multiples of the physical word width) in a burst, and a mask of byte enables. The latter are usually per word and have to be the same for all beats, e.g. forming a burst of half words.

Is this bytes_valid to be used similar to a burst length? In other words, is it always a multiple of the interface width? Is there a way to avoid the confusion? Would the problem with dynamic allocation go away if we introduced a maximum burst length?

1. Geoffrey Blake Nov. 28, 2011, 1:33 a.m. (Nov. 28, 2011, 1:33 a.m.)

   This fix is for functional accesses so it doesn't need to be aware of the memory system architecture. Dynamic allocation is needed as the accesses can be any size when dealing with functional accesses.  I'll try to think of something to remove any potential ambiguity with that variable name.

src/mem/packet.hh (Diff revision 1)

Ok maybe I got things confused here. Is the idea that you can have an address that is not aligned to any word (e.g. only byte aligned) and that any bytes could be valid within the burst (hence the mask)? Do the bytes have to come in whole words or can they be partial? Do the valid bytes all reside at the beginning of the burst, i.e. from the start address or could they be anywhere?

I am merely trying to understand if this is indeed a byte enable (and if so how organised) or if it is more a way of having smaller than "getBlockSize()" bursts.

Does that make sense?

1. Ali Saidi Nov. 28, 2011, 11:26 p.m. (Nov. 28, 2011, 11:26 p.m.)

   This has nothing to do with byte enables, bursts, or masks in the ense you're thinking. The issue is that functional accesses go hunting in the memory system for a version of the block that they can use to satisfy a request. Up to this point, if a block with some of the request was found, but not all of it, that would cause the simulator to panic, because it could only satisfy parts of the request and there was no way to indicate that it old dome but not all the bytes. You couldn't just satisfy the whole thing at the next level, because it might have stale data. This change adds a status per byte of functional accesses ich fixes that issue. 

2. Andreas Hansson Nov. 28, 2011, 11:40 p.m. (Nov. 28, 2011, 11:40 p.m.)

   Sorry for the misunderstanding. So a functional request is made to an address that is not aligned to the "system" block size boundary, or not of the "system" block size? Should this not be addressed with a ChunkGenerator at the source port making the request? Could there be non-functional requests that have these properties?

3. Geoffrey Blake Nov. 29, 2011, 12:42 a.m. (Nov. 29, 2011, 12:42 a.m.)

   Added the changes requested: Doxygen comment and some clarification to the bytes_valid variable with a comment.
   Also updated the code in src/mem/packet.cc to behave as intended, it could gather stale data because the did not cross-reference the bytes_valid bitvector while copying data, now it does do the cross-reference and only copy data that hasn't been found yet.
   
   To the best of my knowledge all requests that are not of the magic functional variety are broken up into multiple requests if they need to be block aligned or are bigger than the block size. Functional accesses don't need this as they are a software interface into the memory state of M5. 

Functionally it looks fine to me.  My only concern is that we're adding some (small) overhead to every single Packet just to deal with this one extremely rare corner case.  Packets get dynamically allocated a *lot* in timing simulation, so it's painful to me to see us adding any overhead there.  Is there a way we can at least avoid calling bytes_valid.resize() in every constructor?  I'm not sure how much space a std::vector<bool> takes, or how much the default constructor costs; if either one is not negligible, it might even be worth just putting a pointer in the Packet and dynamically allocating the vector itself only when needed.  I don't know if that is really worthwhile or not though.

Made a derived class FunctionalPacket to enable partial functional reads to not induce more overhead in the Packet class as requested by Steve, Nate and Ali.  Modified blobHelper() in src/mem/port.cc to use FunctionalPacket instead of Packet.  Over areas could use functional packets (src/arch/x86/pagetable_walker.cc and src/cpu/testers/memtest/memtest.cc) but have left them alone because they do not really need to be fixed.  Tested by compiling and running using the CheckerCPU that exercises this code path heavily.  No bugs found.

Looks good to me, assuming it works.  Thanks for making the separate class.

Removed derived class.  Added two uint16_t's: bytesValidStart and bytesValidEnd to Packet class to represent the range of values copied in a partial read.  Added back functionality in Packet class to satisfy partial functional read.  Tested with CheckerCPU to see if I hit any dis-contiguous byte ranges.  Nothing triggered the panic for this case.  

Anybody have recommendations for the variables I used to construct the range checks to figure out which bytes to copy instead of a,b,c, and d?  I did put extensive comments in the code to explain what they were being used for.