src/arch/arm/isa.cc (Diff revision 2)

I really think we should avoid doing anything along these lines and instead take the plunge and to the CPU -> core switch and then introduce the CPU "box" simobject that is merely a hierarchy with pointers to caches etc.

I really do not like to have assumptions on the connectivity. If we could insert a monitor for example then this code would break. Also, does this work with switching CPUs where the instport won't be connected to start with? Checkers etc? 

Show all issues

1. Geoffrey Blake Aug. 9, 2012, 8:31 a.m. (Aug. 9, 2012, 8:31 a.m.)

   Maybe I'm missing something here, but would it not be better to attempt setting this register at config time with the python code in the same way that the device model registers are handled and have it as a param that is passed to a CPU constructor?

2. Andreas Hansson Aug. 9, 2012, 5 p.m. (Aug. 9, 2012, 5 p.m.)

   I think that sounds like a good idea. I don't know all the details, I am merely keen to arrive at a solution that does not contain a lot of hidden assumptions.

3. Ali Saidi Aug. 12, 2012, 2:42 a.m. (Aug. 12, 2012, 2:42 a.m.)

   The question is how to do it. If you use parent.any it will fail when a the system doesn't have a cache. Otherwise changes to the config files are needed. Maybe teh best thing to do is default it to 64 bytes and allow the parameter to be set , and change the config files appropriately. 
   
   While i'd like to see the cpu container objects, that is going to take a while and we need to get the checker regression going sooner then that. 

src/cpu/checker/cpu.hh (Diff revision 2)

How bad would it be to make the CheckerCPU inherit from something other than a BaseCPU, or insert another class in the hierarchy for "real" CPU vs CPU-like behaviour (the checker)?

It seems we try to squeeze a square peg in a round hole here... and also with some other issues related to the checker.

Show all issues

1. Geoffrey Blake Aug. 9, 2012, 8:31 a.m. (Aug. 9, 2012, 8:31 a.m.)

   I think there are a few areas where instructions pass their ThreadContexts to some component that then expects to get a BaseCPU ptr to set some state that might make this more difficult than expected.  A quick search shows the TLBs for ARM expect this behavior. But, it might be good to separate the checker further away from the CPU classes just to avoid any confusion.

2. Andreas Hansson Aug. 9, 2012, 5:01 p.m. (Aug. 9, 2012, 5:01 p.m.)

   You're the expert Geoff :)
   
   Conceptually I think we should do something along these lines, but where exactly to make the cut I don't know. Do you have an idea of how much work it would be?

3. Geoffrey Blake Aug. 9, 2012, 11:44 p.m. (Aug. 9, 2012, 11:44 p.m.)

   I don't have the bandwidth to volunteer attempting to make the changes is my main objection right now. =)
   
   I think the separation has already been made in the majority of the code that directly interacts with the checker, asking for a CheckerCpuPtr instead of CpuPtr now.  Its the hidden areas I'm not terribly familiar with where I don't have a good grasp of the difficulty to making such changes. I.e. components that interact mainly through ThreadContext's which include the instructions, TLBs, architecture specific handlers etc. 
   
   What other problems were happening with the Checker to push removing the Checker from inheriting from BaseCPU?  I haven't been following close enough obviously?

Any thoughts on this solution?

src/arch/arm/isa.cc (Diff revision 5)

I really think we have to find a better way of doing this. String matching should, in my opinion, never be used to determine the type of a module as it is setting ourselves up for hidden assumptions.

Do you agree?

Show all issues

1. Tony Gutierrez Sept. 1, 2012, 9:58 a.m. (Sept. 1, 2012, 9:58 a.m.)

   I agree. However, there is no current way to determine if we have caches as far as I can tell. Returning a value other than 0 for the cache-related fields here, when there are no caches, is wrong. And it affects the behavior of the kernel at the very least. My previous patch added a cacheline size param to the CPU model. This solved the problem of getting the line size as well as determining if we have caches. I can revert this back to that.

2. Andreas Hansson Sept. 3, 2012, 12:26 a.m. (Sept. 3, 2012, 12:26 a.m.)

   I think what you suggest might be the best option at the moment.
   
   Ultimately, I am a strong believer of the notion of creating a "CPU" wrapper that contains cores (what is called CPU today) and caches, as already mentioned a few times.
   
   Any other ideas?

3. Ali Saidi Sept. 3, 2012, 10:58 a.m. (Sept. 3, 2012, 10:58 a.m.)

   I think that is it, but it's a question of biting the bullet and doing the coding. The other issue here is that if you start a system without caches and then add them later, what happens? I'm not sure if the kernel caches the response of the register or if it reads it every time. If the latter that is fine, but if it's the prior then we have a problem because we need to be able to specify these values even when the cache doesn't exist. Probably we're going to want a default CTR value in the ARM system that is checked for being reasonable against the real cache when it starts up.
   
   Tony,
   
   could you look how this value is read/cached in the kernel?
   
   Thanks,
   Ali
   
   
   

4. Tony Gutierrez Sept. 3, 2012, 11:22 a.m. (Sept. 3, 2012, 11:22 a.m.)

   Well, from my previous work with the invalidation instructions I know that this register can be consulted in some versions of the kernel when doing cache invalidations. E.g., it will read this register to get the line size, then iterate over the range to be invalidated in line-sized steps. Some versions used the CSSIDR. In the use-cases I saw it does not cache the value, it reads the register each time. I don't know where else it could be used, and since the invalidation instructions are not implemented, I don't think it will affect correctness. But, it definitely can skew things like instruction counts. E.g, in the original implementation (where the CTR reg held a constant default value, that also happened to be off because it used values representing bytes instead of words) the cache inv loop was being executed 2-3x less than it should because the line size was 2-3x too large.