Re: [gem5-dev] Review Request 3793: x86: Fix implicit stack addressing in 64-bit mode

2017-01-27 Thread Sanchayan Maity 

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

Ship it!


Ship It!

- Sanchayan Maity


On Jan. 28, 2017, 4:29 a.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3793/
> ---
> 
> (Updated Jan. 28, 2017, 4:29 a.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> x86: Fix implicit stack addressing in 64-bit mode
> 
> When in 64-bit mode, if the stack is accessed implicitly by an instruction,
> the alternate address prefix should be ignored if present.
> 
> This patch adds an extra flag to the ldstop which signifies when the
> address override should be ignored. Then, for all of the affected
> instructions, this patch adds two options to the ld and st opcode to
> use the current stack addressing mode for all addresses and to ignore the
> AddressSizeFlagBit.
> Finally, this patch updates the x86 TLB to not truncate the address if it
> is in 64-bit mode and the IgnoreAddrSizeFlagBit is set.
> 
> This fixes a problem when calling __libc_start_main with a binary that
> is linked with a recent version of ld. This version of ld uses the
> address override prefix (0x67) on the call instruction instead of a nop.
> 
> Note: This has not been tested in compatibility mode and only the call
> instruction with the address override prefix has been tested.
> 
> See [1] page 9 (pdf page 45)
> 
> For instructions that are affected see [1] page 519 (pdf page 555).
> 
> [1] http://support.amd.com/TechDocs/24594.pdf
> 
> 
> Diffs
> -
> 
>   src/arch/x86/isa/insts/general_purpose/control_transfer/call.py 
> cd7f3a1dbf55 
>   src/arch/x86/isa/insts/general_purpose/data_transfer/stack_operations.py 
> cd7f3a1dbf55 
>   src/arch/x86/isa/microops/ldstop.isa cd7f3a1dbf55 
> 
> Diff: http://reviews.gem5.org/r/3793/diff/
> 
> 
> Testing
> ---
> 
> Runs "hello" compiled with gcc version "gcc (GCC) 6.3.1 20170109". Also runs 
> hello compiled with earlier gcc in test-progs, and hello32 from test-progs.
> Currently testing Linux boot.
> 
> Testing done on arch docker image. See 
> https://hub.docker.com/r/powerjg/arch-dev/.
> Run "docker run -v `pwd`:/gem5/ -it powerjg/arch-dev /bin/bash" to 
> build/run/test gem5.
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

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

Re: [gem5-dev] Review Request 3793: x86: Fix implicit stack addressing in 64-bit mode

2017-01-27 Thread Jason Lowe-Power 


> On Jan. 27, 2017, 10:37 p.m., Steve Reinhardt wrote:
> > Nice! I suggest somewhat more obvious mnemonics (e.g., ld_stack/st_stack, 
> > if '_' is allowed), and just a couple more comments, but that's all 
> > optional. Feel free to submit w/o reposting on my account.

Might as well do it right while I'm knee-deep in this code.


- Jason


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


On Jan. 27, 2017, 10:59 p.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3793/
> ---
> 
> (Updated Jan. 27, 2017, 10:59 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> x86: Fix implicit stack addressing in 64-bit mode
> 
> When in 64-bit mode, if the stack is accessed implicitly by an instruction,
> the alternate address prefix should be ignored if present.
> 
> This patch adds an extra flag to the ldstop which signifies when the
> address override should be ignored. Then, for all of the affected
> instructions, this patch adds two options to the ld and st opcode to
> use the current stack addressing mode for all addresses and to ignore the
> AddressSizeFlagBit.
> Finally, this patch updates the x86 TLB to not truncate the address if it
> is in 64-bit mode and the IgnoreAddrSizeFlagBit is set.
> 
> This fixes a problem when calling __libc_start_main with a binary that
> is linked with a recent version of ld. This version of ld uses the
> address override prefix (0x67) on the call instruction instead of a nop.
> 
> Note: This has not been tested in compatibility mode and only the call
> instruction with the address override prefix has been tested.
> 
> See [1] page 9 (pdf page 45)
> 
> For instructions that are affected see [1] page 519 (pdf page 555).
> 
> [1] http://support.amd.com/TechDocs/24594.pdf
> 
> 
> Diffs
> -
> 
>   src/arch/x86/isa/insts/general_purpose/control_transfer/call.py 
> cd7f3a1dbf55 
>   src/arch/x86/isa/insts/general_purpose/data_transfer/stack_operations.py 
> cd7f3a1dbf55 
>   src/arch/x86/isa/microops/ldstop.isa cd7f3a1dbf55 
> 
> Diff: http://reviews.gem5.org/r/3793/diff/
> 
> 
> Testing
> ---
> 
> Runs "hello" compiled with gcc version "gcc (GCC) 6.3.1 20170109". Also runs 
> hello compiled with earlier gcc in test-progs, and hello32 from test-progs.
> Currently testing Linux boot.
> 
> Testing done on arch docker image. See 
> https://hub.docker.com/r/powerjg/arch-dev/.
> Run "docker run -v `pwd`:/gem5/ -it powerjg/arch-dev /bin/bash" to 
> build/run/test gem5.
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

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

Re: [gem5-dev] Review Request 3793: x86: Fix implicit stack addressing in 64-bit mode

2017-01-27 Thread Jason Lowe-Power 

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

(Updated Jan. 27, 2017, 10:59 p.m.)


Review request for Default.


Repository: gem5


Description
---

x86: Fix implicit stack addressing in 64-bit mode

When in 64-bit mode, if the stack is accessed implicitly by an instruction,
the alternate address prefix should be ignored if present.

This patch adds an extra flag to the ldstop which signifies when the
address override should be ignored. Then, for all of the affected
instructions, this patch adds two options to the ld and st opcode to
use the current stack addressing mode for all addresses and to ignore the
AddressSizeFlagBit.
Finally, this patch updates the x86 TLB to not truncate the address if it
is in 64-bit mode and the IgnoreAddrSizeFlagBit is set.

This fixes a problem when calling __libc_start_main with a binary that
is linked with a recent version of ld. This version of ld uses the
address override prefix (0x67) on the call instruction instead of a nop.

Note: This has not been tested in compatibility mode and only the call
instruction with the address override prefix has been tested.

See [1] page 9 (pdf page 45)

For instructions that are affected see [1] page 519 (pdf page 555).

[1] http://support.amd.com/TechDocs/24594.pdf


Diffs (updated)
-

  src/arch/x86/isa/insts/general_purpose/control_transfer/call.py cd7f3a1dbf55 
  src/arch/x86/isa/insts/general_purpose/data_transfer/stack_operations.py 
cd7f3a1dbf55 
  src/arch/x86/isa/microops/ldstop.isa cd7f3a1dbf55 

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


Testing
---

Runs "hello" compiled with gcc version "gcc (GCC) 6.3.1 20170109". Also runs 
hello compiled with earlier gcc in test-progs, and hello32 from test-progs.
Currently testing Linux boot.

Testing done on arch docker image. See 
https://hub.docker.com/r/powerjg/arch-dev/.
Run "docker run -v `pwd`:/gem5/ -it powerjg/arch-dev /bin/bash" to 
build/run/test gem5.


Thanks,

Jason Lowe-Power

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

Re: [gem5-dev] Review Request 3793: x86: Fix implicit stack addressing in 64-bit mode

2017-01-27 Thread Steve Reinhardt 

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

Ship it!


Nice! I suggest somewhat more obvious mnemonics (e.g., ld_stack/st_stack, if 
'_' is allowed), and just a couple more comments, but that's all optional. Feel 
free to submit w/o reposting on my account.


src/arch/x86/isa/microops/ldstop.isa (line 501)


could use a comment here



src/arch/x86/isa/microops/ldstop.isa (line 644)


comment here too


- Steve Reinhardt


On Jan. 27, 2017, 9:40 a.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3793/
> ---
> 
> (Updated Jan. 27, 2017, 9:40 a.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> x86: Fix implicit stack addressing in 64-bit mode
> 
> When in 64-bit mode, if the stack is accessed implicitly by an instruction,
> the alternate address prefix should be ignored if present.
> 
> This patch adds an extra flag to the ldstop which signifies when the
> address override should be ignored. Then, for all of the affected
> instructions, this patch adds two options to the ld and st opcode to
> use the current stack addressing mode for all addresses and to ignore the
> AddressSizeFlagBit.
> Finally, this patch updates the x86 TLB to not truncate the address if it
> is in 64-bit mode and the IgnoreAddrSizeFlagBit is set.
> 
> This fixes a problem when calling __libc_start_main with a binary that
> is linked with a recent version of ld. This version of ld uses the
> address override prefix (0x67) on the call instruction instead of a nop.
> 
> Note: This has not been tested in compatibility mode and only the call
> instruction with the address override prefix has been tested.
> 
> See [1] page 9 (pdf page 45)
> 
> For instructions that are affected see [1] page 519 (pdf page 555).
> 
> [1] http://support.amd.com/TechDocs/24594.pdf
> 
> 
> Diffs
> -
> 
>   src/arch/x86/isa/insts/general_purpose/control_transfer/call.py 
> cd7f3a1dbf55 
>   src/arch/x86/isa/insts/general_purpose/data_transfer/stack_operations.py 
> cd7f3a1dbf55 
>   src/arch/x86/isa/microops/ldstop.isa cd7f3a1dbf55 
> 
> Diff: http://reviews.gem5.org/r/3793/diff/
> 
> 
> Testing
> ---
> 
> Runs "hello" compiled with gcc version "gcc (GCC) 6.3.1 20170109". Also runs 
> hello compiled with earlier gcc in test-progs, and hello32 from test-progs.
> Currently testing Linux boot.
> 
> Testing done on arch docker image. See 
> https://hub.docker.com/r/powerjg/arch-dev/.
> Run "docker run -v `pwd`:/gem5/ -it powerjg/arch-dev /bin/bash" to 
> build/run/test gem5.
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

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

Re: [gem5-dev] Review Request 3793: x86: Fix implicit stack addressing in 64-bit mode

2017-01-27 Thread Jason Lowe-Power 

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

(Updated Jan. 27, 2017, 5:40 p.m.)


Review request for Default.


Repository: gem5


Description
---

x86: Fix implicit stack addressing in 64-bit mode

When in 64-bit mode, if the stack is accessed implicitly by an instruction,
the alternate address prefix should be ignored if present.

This patch adds an extra flag to the ldstop which signifies when the
address override should be ignored. Then, for all of the affected
instructions, this patch adds two options to the ld and st opcode to
use the current stack addressing mode for all addresses and to ignore the
AddressSizeFlagBit.
Finally, this patch updates the x86 TLB to not truncate the address if it
is in 64-bit mode and the IgnoreAddrSizeFlagBit is set.

This fixes a problem when calling __libc_start_main with a binary that
is linked with a recent version of ld. This version of ld uses the
address override prefix (0x67) on the call instruction instead of a nop.

Note: This has not been tested in compatibility mode and only the call
instruction with the address override prefix has been tested.

See [1] page 9 (pdf page 45)

For instructions that are affected see [1] page 519 (pdf page 555).

[1] http://support.amd.com/TechDocs/24594.pdf


Diffs (updated)
-

  src/arch/x86/isa/insts/general_purpose/control_transfer/call.py cd7f3a1dbf55 
  src/arch/x86/isa/insts/general_purpose/data_transfer/stack_operations.py 
cd7f3a1dbf55 
  src/arch/x86/isa/microops/ldstop.isa cd7f3a1dbf55 

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


Testing
---

Runs "hello" compiled with gcc version "gcc (GCC) 6.3.1 20170109". Also runs 
hello compiled with earlier gcc in test-progs, and hello32 from test-progs.
Currently testing Linux boot.

Testing done on arch docker image. See 
https://hub.docker.com/r/powerjg/arch-dev/.
Run "docker run -v `pwd`:/gem5/ -it powerjg/arch-dev /bin/bash" to 
build/run/test gem5.


Thanks,

Jason Lowe-Power

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

Re: [gem5-dev] Review Request 3793: x86: Fix implicit stack addressing in 64-bit mode

2017-01-27 Thread Jason Lowe-Power 


> On Jan. 26, 2017, 6:47 p.m., Steve Reinhardt wrote:
> > src/arch/x86/isa/microops/ldstop.isa, line 382
> > 
> >
> > why not just put 'if not implicitStack:' in front of the previous line 
> > (that adds the legacy.addr check to self.memFlags)? If that works, then we 
> > could get rid of IgnoreAddrSizeFlagBit.

I guess I was worried about what would happen if the prefix was included when 
in 32-bit mode. But I think I've reasoned through this and it seems like it 
would be handled correctly.


On Jan. 26, 2017, 6:47 p.m., Jason Lowe-Power wrote:
> > If we were trying to keep things clean, I'd suggest adding new micro-ops 
> > like ld.ss and st.ss that would wrap ld an st and automatically set 
> > segment=ss, implicitStack=True, addressSize=ssz, and maybe dataSize=ssz 
> > (looks like we usually set this, but not always, and I'm not sure if the 
> > exceptions are significant) and, for st.ss, disp=-env.dataSize.
> > 
> > I can totally understand if you don't want to mess with that at this point 
> > though, so I won't press you on it.

I tried this method. It was worth it for me to learn how to do this. I'm not 
sure if '.' is allowed in microp names. Whenver I tried to include a '.' I got 
an error "*** Error creating microop object with mnemonic st."


- Jason


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


On Jan. 27, 2017, 5:40 p.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3793/
> ---
> 
> (Updated Jan. 27, 2017, 5:40 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> x86: Fix implicit stack addressing in 64-bit mode
> 
> When in 64-bit mode, if the stack is accessed implicitly by an instruction,
> the alternate address prefix should be ignored if present.
> 
> This patch adds an extra flag to the ldstop which signifies when the
> address override should be ignored. Then, for all of the affected
> instructions, this patch adds two options to the ld and st opcode to
> use the current stack addressing mode for all addresses and to ignore the
> AddressSizeFlagBit.
> Finally, this patch updates the x86 TLB to not truncate the address if it
> is in 64-bit mode and the IgnoreAddrSizeFlagBit is set.
> 
> This fixes a problem when calling __libc_start_main with a binary that
> is linked with a recent version of ld. This version of ld uses the
> address override prefix (0x67) on the call instruction instead of a nop.
> 
> Note: This has not been tested in compatibility mode and only the call
> instruction with the address override prefix has been tested.
> 
> See [1] page 9 (pdf page 45)
> 
> For instructions that are affected see [1] page 519 (pdf page 555).
> 
> [1] http://support.amd.com/TechDocs/24594.pdf
> 
> 
> Diffs
> -
> 
>   src/arch/x86/isa/insts/general_purpose/control_transfer/call.py 
> cd7f3a1dbf55 
>   src/arch/x86/isa/insts/general_purpose/data_transfer/stack_operations.py 
> cd7f3a1dbf55 
>   src/arch/x86/isa/microops/ldstop.isa cd7f3a1dbf55 
> 
> Diff: http://reviews.gem5.org/r/3793/diff/
> 
> 
> Testing
> ---
> 
> Runs "hello" compiled with gcc version "gcc (GCC) 6.3.1 20170109". Also runs 
> hello compiled with earlier gcc in test-progs, and hello32 from test-progs.
> Currently testing Linux boot.
> 
> Testing done on arch docker image. See 
> https://hub.docker.com/r/powerjg/arch-dev/.
> Run "docker run -v `pwd`:/gem5/ -it powerjg/arch-dev /bin/bash" to 
> build/run/test gem5.
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

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

Re: [gem5-dev] Review Request 3793: x86: Fix implicit stack addressing in 64-bit mode

2017-01-26 Thread Steve Reinhardt 

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



src/arch/x86/isa/microops/ldstop.isa (line 382)


why not just put 'if not implicitStack:' in front of the previous line 
(that adds the legacy.addr check to self.memFlags)? If that works, then we 
could get rid of IgnoreAddrSizeFlagBit.


If we were trying to keep things clean, I'd suggest adding new micro-ops like 
ld.ss and st.ss that would wrap ld an st and automatically set segment=ss, 
implicitStack=True, addressSize=ssz, and maybe dataSize=ssz (looks like we 
usually set this, but not always, and I'm not sure if the exceptions are 
significant) and, for st.ss, disp=-env.dataSize.

I can totally understand if you don't want to mess with that at this point 
though, so I won't press you on it.

- Steve Reinhardt


On Jan. 26, 2017, 8:51 a.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3793/
> ---
> 
> (Updated Jan. 26, 2017, 8:51 a.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> x86: Fix implicit stack addressing in 64-bit mode
> 
> When in 64-bit mode, if the stack is accessed implicitly by an instruction,
> the alternate address prefix should be ignored if present.
> 
> This patch adds an extra flag to the ldstop which signifies when the
> address override should be ignored. Then, for all of the affected
> instructions, this patch adds two options to the ld and st opcode to
> use the current stack addressing mode for all addresses and to ignore the
> AddressSizeFlagBit.
> Finally, this patch updates the x86 TLB to not truncate the address if it
> is in 64-bit mode and the IgnoreAddrSizeFlagBit is set.
> 
> This fixes a problem when calling __libc_start_main with a binary that
> is linked with a recent version of ld. This version of ld uses the
> address override prefix (0x67) on the call instruction instead of a nop.
> 
> Note: This has not been tested in compatibility mode and only the call
> instruction with the address override prefix has been tested.
> 
> See [1] page 9 (pdf page 45)
> 
> For instructions that are affected see [1] page 519 (pdf page 555).
> 
> [1] http://support.amd.com/TechDocs/24594.pdf
> 
> 
> Diffs
> -
> 
>   src/arch/x86/isa/insts/general_purpose/control_transfer/call.py 
> cd7f3a1dbf55 
>   src/arch/x86/isa/insts/general_purpose/data_transfer/stack_operations.py 
> cd7f3a1dbf55 
>   src/arch/x86/isa/microops/ldstop.isa cd7f3a1dbf55 
>   src/arch/x86/ldstflags.hh cd7f3a1dbf55 
>   src/arch/x86/tlb.cc cd7f3a1dbf55 
> 
> Diff: http://reviews.gem5.org/r/3793/diff/
> 
> 
> Testing
> ---
> 
> Runs "hello" compiled with gcc version "gcc (GCC) 6.3.1 20170109". Also runs 
> hello compiled with earlier gcc in test-progs, and hello32 from test-progs.
> Currently testing Linux boot.
> 
> Testing done on arch docker image. See 
> https://hub.docker.com/r/powerjg/arch-dev/.
> Run "docker run -v `pwd`:/gem5/ -it powerjg/arch-dev /bin/bash" to 
> build/run/test gem5.
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

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