Re: [m5-dev] Review Request: ARM: Make sure that software prefetch instructions can't change the state of the TLB

2010-08-19 Thread Ali Saidi 


> On 2010-08-19 10:34:00, Nathan Binkert wrote:
> > src/arch/arm/faults.hh, line 91
> > 
> >
> > Space after // in comments.  I know it's not explicitly stated in the 
> > style guide, but the examples are consistent and it is what almost all 
> > other code does.  We could update the style guide of course.
> > 
> > In the future, to make life easier, I wouldn't mind having the rule 
> > that we follow the Google style guide when our style guide is silent.  (I 
> > don't know the guide myself, but my guess is that it's decent).

fixed.


- Ali


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/192/#review271
---


On 2010-08-13 10:16:34, Ali Saidi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.m5sim.org/r/192/
> ---
> 
> (Updated 2010-08-13 10:16:34)
> 
> 
> Review request for Default.
> 
> 
> Summary
> ---
> 
> ARM: Make sure that software prefetch instructions can't change the state of 
> the TLB
> 
> 
> Diffs
> -
> 
>   src/arch/arm/faults.hh 3c48b2b3cb83 
>   src/arch/arm/table_walker.cc 3c48b2b3cb83 
>   src/arch/arm/tlb.cc 3c48b2b3cb83 
>   src/mem/cache/cache_impl.hh 3c48b2b3cb83 
> 
> Diff: http://reviews.m5sim.org/r/192/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ali
> 
>

___
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev

Re: [m5-dev] Review Request: ARM: Make sure that software prefetch instructions can't change the state of the TLB

2010-08-19 Thread Nathan Binkert 

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/192/#review271
---



src/arch/arm/faults.hh


Space after // in comments.  I know it's not explicitly stated in the style 
guide, but the examples are consistent and it is what almost all other code 
does.  We could update the style guide of course.

In the future, to make life easier, I wouldn't mind having the rule that we 
follow the Google style guide when our style guide is silent.  (I don't know 
the guide myself, but my guess is that it's decent).


- Nathan


On 2010-08-13 10:16:34, Ali Saidi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.m5sim.org/r/192/
> ---
> 
> (Updated 2010-08-13 10:16:34)
> 
> 
> Review request for Default.
> 
> 
> Summary
> ---
> 
> ARM: Make sure that software prefetch instructions can't change the state of 
> the TLB
> 
> 
> Diffs
> -
> 
>   src/arch/arm/faults.hh 3c48b2b3cb83 
>   src/arch/arm/table_walker.cc 3c48b2b3cb83 
>   src/arch/arm/tlb.cc 3c48b2b3cb83 
>   src/mem/cache/cache_impl.hh 3c48b2b3cb83 
> 
> Diff: http://reviews.m5sim.org/r/192/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ali
> 
>

___
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev

Re: [m5-dev] Review Request: ARM: Make sure that software prefetch instructions can't change the state of the TLB

2010-08-17 Thread Ali Saidi 

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/192/#review216
---



src/mem/cache/cache_impl.hh


It's possible to be using the caches for a region of memory and then mark 
in uncachable with ARM. In this case we'l step on this assert. The above change 
causes the cache to invalidate that block when this happens. 


- Ali


On 2010-08-13 10:16:34, Ali Saidi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.m5sim.org/r/192/
> ---
> 
> (Updated 2010-08-13 10:16:34)
> 
> 
> Review request for Default.
> 
> 
> Summary
> ---
> 
> ARM: Make sure that software prefetch instructions can't change the state of 
> the TLB
> 
> 
> Diffs
> -
> 
>   src/arch/arm/faults.hh 3c48b2b3cb83 
>   src/arch/arm/table_walker.cc 3c48b2b3cb83 
>   src/arch/arm/tlb.cc 3c48b2b3cb83 
>   src/mem/cache/cache_impl.hh 3c48b2b3cb83 
> 
> Diff: http://reviews.m5sim.org/r/192/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ali
> 
>

___
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev

Re: [m5-dev] Review Request: ARM: Make sure that software prefetch instructions can't change the state of the TLB

2010-08-14 Thread Steve Reinhardt 

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/192/#review197
---


Is this really desired behavior?  Aren't there cases when having a software 
prefetch prefetching results into the TLB is a good idea?  To be honest I don't 
know what our processors do; I'll try and find out.

- Steve


On 2010-08-13 10:16:34, Ali Saidi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.m5sim.org/r/192/
> ---
> 
> (Updated 2010-08-13 10:16:34)
> 
> 
> Review request for Default.
> 
> 
> Summary
> ---
> 
> ARM: Make sure that software prefetch instructions can't change the state of 
> the TLB
> 
> 
> Diffs
> -
> 
>   src/arch/arm/faults.hh 3c48b2b3cb83 
>   src/arch/arm/table_walker.cc 3c48b2b3cb83 
>   src/arch/arm/tlb.cc 3c48b2b3cb83 
>   src/mem/cache/cache_impl.hh 3c48b2b3cb83 
> 
> Diff: http://reviews.m5sim.org/r/192/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ali
> 
>

___
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev

Re: [m5-dev] Review Request: ARM: Make sure that software prefetch instructions can't change the state of the TLB

2010-08-14 Thread Steve Reinhardt 


> On 2010-08-14 00:33:43, Gabe Black wrote:
> > src/mem/cache/cache_impl.hh, line 561
> > 
> >
> > Don't comment out code, delete it. Why is this assert no longer needed? 
> > I don't know the cache code so I apologize if it's obvious.

This assertion checks that you don't have an uncacheable access to a block 
that's already in your cache, since there's no well-defined general behavior 
for that.  I don't see how it's related to the stated goal of this patch, and I 
don't want to see it removed without some discussion about why it's wrong.


- Steve


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/192/#review190
---


On 2010-08-13 10:16:34, Ali Saidi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.m5sim.org/r/192/
> ---
> 
> (Updated 2010-08-13 10:16:34)
> 
> 
> Review request for Default.
> 
> 
> Summary
> ---
> 
> ARM: Make sure that software prefetch instructions can't change the state of 
> the TLB
> 
> 
> Diffs
> -
> 
>   src/arch/arm/faults.hh 3c48b2b3cb83 
>   src/arch/arm/table_walker.cc 3c48b2b3cb83 
>   src/arch/arm/tlb.cc 3c48b2b3cb83 
>   src/mem/cache/cache_impl.hh 3c48b2b3cb83 
> 
> Diff: http://reviews.m5sim.org/r/192/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ali
> 
>

___
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev

Re: [m5-dev] Review Request: ARM: Make sure that software prefetch instructions can't change the state of the TLB

2010-08-14 Thread Gabe Black 

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/192/#review190
---



src/mem/cache/cache_impl.hh


Don't comment out code, delete it. Why is this assert no longer needed? I 
don't know the cache code so I apologize if it's obvious.


- Gabe


On 2010-08-13 10:16:34, Ali Saidi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.m5sim.org/r/192/
> ---
> 
> (Updated 2010-08-13 10:16:34)
> 
> 
> Review request for Default.
> 
> 
> Summary
> ---
> 
> ARM: Make sure that software prefetch instructions can't change the state of 
> the TLB
> 
> 
> Diffs
> -
> 
>   src/arch/arm/faults.hh 3c48b2b3cb83 
>   src/arch/arm/table_walker.cc 3c48b2b3cb83 
>   src/arch/arm/tlb.cc 3c48b2b3cb83 
>   src/mem/cache/cache_impl.hh 3c48b2b3cb83 
> 
> Diff: http://reviews.m5sim.org/r/192/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ali
> 
>

___
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev