Re: [gem5-dev] Review Request 3806: sparc: fix bugs caused by cd7f3a1dbf55

[Brandon Potter]

> On Feb. 16, 2017, 10:44 a.m., Andreas Hansson wrote:
> > I would argue that this is a prime example why removing SPARC is a good 
> > idea.

SPARC, ALPHA, and MIPS?


- Brandon


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3806/#review9433
---


On Feb. 10, 2017, 8:31 p.m., Brandon Potter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3806/
> ---
> 
> (Updated Feb. 10, 2017, 8:31 p.m.)
> 
> 
> Review request for Default, Ali Saidi, Gabe Black, and Steve Reinhardt.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11803:0ab0146d777a
> ---
> sparc: fix bugs caused by cd7f3a1dbf55
> 
> Turns out that SPARC SE mode relied on M5_pid being "0" in
> all cases. The entries in the SPARC TLBs are accessed with
> M5_pid as their context. This is buggy in the sense that it
> will never work with more than one process or any
> initialization that doesn't have the M5_pid value passed in
> as "0".
> 
> cd7f3a1dbf55 broke the SPARC build because it deletes M5_pid
> and uses a _pid with a default of "100" instead. This caused
> the SPARC TLB to never return any valid lookups for any
> request; the program never moved past the first instruction
> with SPARC SE in the regression tester.
> 
> The solution proposed in this changeset is to initialize
> the address space identification register with the PID value
> that is passed into the process class as a parameter from
> Python. This should return the correct responses from the TLB
> since the insertions and lookups into the page table will be
> using the same PID.
> 
> Furthermore, there are corner cases in the code which elevate
> privileges and revert to using context "0" as the context in
> the TLB. I believe that these are related to kernel level
> traps and hypervisor privilege escalations, but I'm not
> completely sure. I've tried to address the corner cases
> properly, but it would be beneficial to have someone who is
> familiar with the SPARC architecture to take a look at this
> fix.
> 
> 
> Diffs
> -
> 
>   src/arch/sparc/process.cc cd7f3a1dbf55bfa03b436c8cde51ebda515fbdbc 
>   src/arch/sparc/faults.cc cd7f3a1dbf55bfa03b436c8cde51ebda515fbdbc 
> 
> Diff: http://reviews.gem5.org/r/3806/diff/
> 
> 
> Testing
> ---
> 
> util/regress all
> 
> 
> Thanks,
> 
> Brandon Potter
> 
>

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

Re: [gem5-dev] Review Request 3806: sparc: fix bugs caused by cd7f3a1dbf55

[Andreas Hansson]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3806/#review9433
---

Ship it!


I would argue that this is a prime example why removing SPARC is a good idea.

- Andreas Hansson


On Feb. 10, 2017, 8:31 p.m., Brandon Potter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3806/
> ---
> 
> (Updated Feb. 10, 2017, 8:31 p.m.)
> 
> 
> Review request for Default, Ali Saidi, Gabe Black, and Steve Reinhardt.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11803:0ab0146d777a
> ---
> sparc: fix bugs caused by cd7f3a1dbf55
> 
> Turns out that SPARC SE mode relied on M5_pid being "0" in
> all cases. The entries in the SPARC TLBs are accessed with
> M5_pid as their context. This is buggy in the sense that it
> will never work with more than one process or any
> initialization that doesn't have the M5_pid value passed in
> as "0".
> 
> cd7f3a1dbf55 broke the SPARC build because it deletes M5_pid
> and uses a _pid with a default of "100" instead. This caused
> the SPARC TLB to never return any valid lookups for any
> request; the program never moved past the first instruction
> with SPARC SE in the regression tester.
> 
> The solution proposed in this changeset is to initialize
> the address space identification register with the PID value
> that is passed into the process class as a parameter from
> Python. This should return the correct responses from the TLB
> since the insertions and lookups into the page table will be
> using the same PID.
> 
> Furthermore, there are corner cases in the code which elevate
> privileges and revert to using context "0" as the context in
> the TLB. I believe that these are related to kernel level
> traps and hypervisor privilege escalations, but I'm not
> completely sure. I've tried to address the corner cases
> properly, but it would be beneficial to have someone who is
> familiar with the SPARC architecture to take a look at this
> fix.
> 
> 
> Diffs
> -
> 
>   src/arch/sparc/process.cc cd7f3a1dbf55bfa03b436c8cde51ebda515fbdbc 
>   src/arch/sparc/faults.cc cd7f3a1dbf55bfa03b436c8cde51ebda515fbdbc 
> 
> Diff: http://reviews.gem5.org/r/3806/diff/
> 
> 
> Testing
> ---
> 
> util/regress all
> 
> 
> Thanks,
> 
> Brandon Potter
> 
>

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

Re: [gem5-dev] Review Request 3806: sparc: fix bugs caused by cd7f3a1dbf55

[Tony Gutierrez]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3806/#review9428
---

Ship it!


Ship It!

- Tony Gutierrez


On Feb. 10, 2017, 12:31 p.m., Brandon Potter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3806/
> ---
> 
> (Updated Feb. 10, 2017, 12:31 p.m.)
> 
> 
> Review request for Default, Ali Saidi, Gabe Black, and Steve Reinhardt.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11803:0ab0146d777a
> ---
> sparc: fix bugs caused by cd7f3a1dbf55
> 
> Turns out that SPARC SE mode relied on M5_pid being "0" in
> all cases. The entries in the SPARC TLBs are accessed with
> M5_pid as their context. This is buggy in the sense that it
> will never work with more than one process or any
> initialization that doesn't have the M5_pid value passed in
> as "0".
> 
> cd7f3a1dbf55 broke the SPARC build because it deletes M5_pid
> and uses a _pid with a default of "100" instead. This caused
> the SPARC TLB to never return any valid lookups for any
> request; the program never moved past the first instruction
> with SPARC SE in the regression tester.
> 
> The solution proposed in this changeset is to initialize
> the address space identification register with the PID value
> that is passed into the process class as a parameter from
> Python. This should return the correct responses from the TLB
> since the insertions and lookups into the page table will be
> using the same PID.
> 
> Furthermore, there are corner cases in the code which elevate
> privileges and revert to using context "0" as the context in
> the TLB. I believe that these are related to kernel level
> traps and hypervisor privilege escalations, but I'm not
> completely sure. I've tried to address the corner cases
> properly, but it would be beneficial to have someone who is
> familiar with the SPARC architecture to take a look at this
> fix.
> 
> 
> Diffs
> -
> 
>   src/arch/sparc/process.cc cd7f3a1dbf55bfa03b436c8cde51ebda515fbdbc 
>   src/arch/sparc/faults.cc cd7f3a1dbf55bfa03b436c8cde51ebda515fbdbc 
> 
> Diff: http://reviews.gem5.org/r/3806/diff/
> 
> 
> Testing
> ---
> 
> util/regress all
> 
> 
> Thanks,
> 
> Brandon Potter
> 
>

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

Re: [gem5-dev] Review Request 3806: sparc: fix bugs caused by cd7f3a1dbf55

[Steve Reinhardt]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3806/#review9427
---

Ship it!


I don't remember much about SPARC address translation, but this looks 
sufficiently complicated that it's quite believable.  Also thanks for the 
thorough comments; at least if there ever are any issues with or questions 
about the code, there won't be much doubt about what you intended.

- Steve Reinhardt


On Feb. 10, 2017, 12:31 p.m., Brandon Potter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3806/
> ---
> 
> (Updated Feb. 10, 2017, 12:31 p.m.)
> 
> 
> Review request for Default, Ali Saidi, Gabe Black, and Steve Reinhardt.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11803:0ab0146d777a
> ---
> sparc: fix bugs caused by cd7f3a1dbf55
> 
> Turns out that SPARC SE mode relied on M5_pid being "0" in
> all cases. The entries in the SPARC TLBs are accessed with
> M5_pid as their context. This is buggy in the sense that it
> will never work with more than one process or any
> initialization that doesn't have the M5_pid value passed in
> as "0".
> 
> cd7f3a1dbf55 broke the SPARC build because it deletes M5_pid
> and uses a _pid with a default of "100" instead. This caused
> the SPARC TLB to never return any valid lookups for any
> request; the program never moved past the first instruction
> with SPARC SE in the regression tester.
> 
> The solution proposed in this changeset is to initialize
> the address space identification register with the PID value
> that is passed into the process class as a parameter from
> Python. This should return the correct responses from the TLB
> since the insertions and lookups into the page table will be
> using the same PID.
> 
> Furthermore, there are corner cases in the code which elevate
> privileges and revert to using context "0" as the context in
> the TLB. I believe that these are related to kernel level
> traps and hypervisor privilege escalations, but I'm not
> completely sure. I've tried to address the corner cases
> properly, but it would be beneficial to have someone who is
> familiar with the SPARC architecture to take a look at this
> fix.
> 
> 
> Diffs
> -
> 
>   src/arch/sparc/process.cc cd7f3a1dbf55bfa03b436c8cde51ebda515fbdbc 
>   src/arch/sparc/faults.cc cd7f3a1dbf55bfa03b436c8cde51ebda515fbdbc 
> 
> Diff: http://reviews.gem5.org/r/3806/diff/
> 
> 
> Testing
> ---
> 
> util/regress all
> 
> 
> Thanks,
> 
> Brandon Potter
> 
>

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

Re: [gem5-dev] Review Request 3806: sparc: fix bugs caused by cd7f3a1dbf55

[Brandon Potter]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3806/
---

(Updated Feb. 10, 2017, 8:31 p.m.)


Review request for Default, Ali Saidi, Gabe Black, and Steve Reinhardt.


Repository: gem5


Description (updated)
---

Changeset 11803:0ab0146d777a
---
sparc: fix bugs caused by cd7f3a1dbf55

Turns out that SPARC SE mode relied on M5_pid being "0" in
all cases. The entries in the SPARC TLBs are accessed with
M5_pid as their context. This is buggy in the sense that it
will never work with more than one process or any
initialization that doesn't have the M5_pid value passed in
as "0".

cd7f3a1dbf55 broke the SPARC build because it deletes M5_pid
and uses a _pid with a default of "100" instead. This caused
the SPARC TLB to never return any valid lookups for any
request; the program never moved past the first instruction
with SPARC SE in the regression tester.

The solution proposed in this changeset is to initialize
the address space identification register with the PID value
that is passed into the process class as a parameter from
Python. This should return the correct responses from the TLB
since the insertions and lookups into the page table will be
using the same PID.

Furthermore, there are corner cases in the code which elevate
privileges and revert to using context "0" as the context in
the TLB. I believe that these are related to kernel level
traps and hypervisor privilege escalations, but I'm not
completely sure. I've tried to address the corner cases
properly, but it would be beneficial to have someone who is
familiar with the SPARC architecture to take a look at this
fix.


Diffs (updated)
-

  src/arch/sparc/process.cc cd7f3a1dbf55bfa03b436c8cde51ebda515fbdbc 
  src/arch/sparc/faults.cc cd7f3a1dbf55bfa03b436c8cde51ebda515fbdbc 

Diff: http://reviews.gem5.org/r/3806/diff/


Testing
---

util/regress all


Thanks,

Brandon Potter

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

Re: [gem5-dev] Review Request 3806: sparc: fix bugs caused by cd7f3a1dbf55

[Tony Gutierrez]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3806/#review9407
---



src/arch/sparc/faults.cc (line 641)


more comments here



src/arch/sparc/faults.cc (line 668)


Some more comments here would be useful



src/arch/sparc/faults.cc (line 674)


If you want just a single bit you can use the 2 arg variant of bits(T val, 
int bit), which will call the 3 arg version with fist == last.


- Tony Gutierrez


On Feb. 3, 2017, 10:48 a.m., Brandon Potter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3806/
> ---
> 
> (Updated Feb. 3, 2017, 10:48 a.m.)
> 
> 
> Review request for Default, Ali Saidi, Gabe Black, and Steve Reinhardt.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11803:28dec3e7b84b
> ---
> sparc: fix bugs caused by cd7f3a1dbf55
> 
> Turns out that SPARC SE mode relied on M5_pid being "0" in
> all cases. The entries in the SPARC TLBs are accessed with
> M5_pid as their context. This is buggy in the sense that it
> will never work with more than one process or any
> initialization that doesn't have the M5_pid value passed in
> as "0".
> 
> cd7f3a1dbf55 broke the SPARC build because it deletes M5_pid
> and uses a _pid with a default of "100" instead. This caused
> the SPARC TLB to never return any valid lookups for any
> request; the program never moved past the first instruction
> with SPARC SE in the regression tester.
> 
> The solution proposed in this changeset is to initialize
> the address space identification register with the PID value
> that is passed into the process class as a parameter from
> Python. This should return the correct responses from the TLB
> since the insertions and lookups into the page table will be
> using the same PID.
> 
> Furthermore, there are corner cases in the code which elevate
> privileges and revert to using context "0" as the context in
> the TLB. I believe that these are related to kernel level
> traps and hypervisor privilege escalations, but I'm not
> completely sure. I've tried to address the corner cases
> properly, but it would be beneficial to have someone who is
> familiar with the SPARC architecture to take a look at this
> fix.
> 
> 
> Diffs
> -
> 
>   src/arch/sparc/faults.cc cd7f3a1dbf55bfa03b436c8cde51ebda515fbdbc 
>   src/arch/sparc/process.cc cd7f3a1dbf55bfa03b436c8cde51ebda515fbdbc 
> 
> Diff: http://reviews.gem5.org/r/3806/diff/
> 
> 
> Testing
> ---
> 
> util/regress all
> 
> 
> Thanks,
> 
> Brandon Potter
> 
>

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

Re: [gem5-dev] Review Request 3806: sparc: fix bugs caused by cd7f3a1dbf55

[Brandon Potter]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3806/
---

(Updated Feb. 3, 2017, 6:48 p.m.)


Review request for Default, Ali Saidi, Gabe Black, and Steve Reinhardt.


Repository: gem5


Description
---

Changeset 11803:28dec3e7b84b
---
sparc: fix bugs caused by cd7f3a1dbf55

Turns out that SPARC SE mode relied on M5_pid being "0" in
all cases. The entries in the SPARC TLBs are accessed with
M5_pid as their context. This is buggy in the sense that it
will never work with more than one process or any
initialization that doesn't have the M5_pid value passed in
as "0".

cd7f3a1dbf55 broke the SPARC build because it deletes M5_pid
and uses a _pid with a default of "100" instead. This caused
the SPARC TLB to never return any valid lookups for any
request; the program never moved past the first instruction
with SPARC SE in the regression tester.

The solution proposed in this changeset is to initialize
the address space identification register with the PID value
that is passed into the process class as a parameter from
Python. This should return the correct responses from the TLB
since the insertions and lookups into the page table will be
using the same PID.

Furthermore, there are corner cases in the code which elevate
privileges and revert to using context "0" as the context in
the TLB. I believe that these are related to kernel level
traps and hypervisor privilege escalations, but I'm not
completely sure. I've tried to address the corner cases
properly, but it would be beneficial to have someone who is
familiar with the SPARC architecture to take a look at this
fix.


Diffs
-

  src/arch/sparc/faults.cc cd7f3a1dbf55bfa03b436c8cde51ebda515fbdbc 
  src/arch/sparc/process.cc cd7f3a1dbf55bfa03b436c8cde51ebda515fbdbc 

Diff: http://reviews.gem5.org/r/3806/diff/


Testing
---

util/regress all


Thanks,

Brandon Potter

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