diff mbox series

Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards

Message ID 1574121497-2433-1-git-send-email-tsimpson@quicinc.com (mailing list archive)
State New, archived
Headers show
Series Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards | expand

Commit Message

Taylor Simpson Nov. 18, 2019, 11:58 p.m. UTC
Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
---
 configure                                   |   9 +
 default-configs/hexagon-linux-user.mak      |   1 +
 include/elf.h                               |   2 +
 linux-user/elfload.c                        |  16 ++
 linux-user/hexagon/cpu_loop.c               | 103 +++++++++
 linux-user/hexagon/signal.c                 | 276 +++++++++++++++++++++++
 linux-user/hexagon/sockbits.h               |   3 +
 linux-user/hexagon/syscall_nr.h             | 331 ++++++++++++++++++++++++++++
 linux-user/hexagon/target_cpu.h             |  35 +++
 linux-user/hexagon/target_elf.h             |  24 ++
 linux-user/hexagon/target_fcntl.h           |   3 +
 linux-user/hexagon/target_signal.h          |  19 ++
 linux-user/hexagon/target_structs.h         |  31 +++
 linux-user/hexagon/target_syscall.h         |  17 ++
 linux-user/hexagon/termbits.h               | 231 +++++++++++++++++++
 linux-user/signal.c                         |   8 +
 linux-user/syscall.c                        |   2 +
 linux-user/syscall_defs.h                   |  33 +++
 target/hexagon/Makefile.objs                |   6 +
 target/hexagon/cpu-param.h                  |  11 +
 target/hexagon/cpu.c                        | 283 ++++++++++++++++++++++++
 target/hexagon/cpu.h                        | 146 ++++++++++++
 target/hexagon/cpu_bits.h                   |  15 ++
 target/hexagon/helper.h                     |   3 +
 target/hexagon/imported/global_types.h      |  25 +++
 target/hexagon/imported/iss_ver_registers.h | 183 +++++++++++++++
 target/hexagon/imported/max.h               |  78 +++++++
 target/hexagon/imported/regs.h              |  19 ++
 target/hexagon/op_helper.c                  |  29 +++
 target/hexagon/translate.c                  | 220 ++++++++++++++++++
 target/hexagon/translate.h                  |  22 ++
 31 files changed, 2184 insertions(+)
 create mode 100644 default-configs/hexagon-linux-user.mak
 create mode 100644 linux-user/hexagon/cpu_loop.c
 create mode 100644 linux-user/hexagon/signal.c
 create mode 100644 linux-user/hexagon/sockbits.h
 create mode 100644 linux-user/hexagon/syscall_nr.h
 create mode 100644 linux-user/hexagon/target_cpu.h
 create mode 100644 linux-user/hexagon/target_elf.h
 create mode 100644 linux-user/hexagon/target_fcntl.h
 create mode 100644 linux-user/hexagon/target_signal.h
 create mode 100644 linux-user/hexagon/target_structs.h
 create mode 100644 linux-user/hexagon/target_syscall.h
 create mode 100644 linux-user/hexagon/termbits.h
 create mode 100644 target/hexagon/Makefile.objs
 create mode 100644 target/hexagon/cpu-param.h
 create mode 100644 target/hexagon/cpu.c
 create mode 100644 target/hexagon/cpu.h
 create mode 100644 target/hexagon/cpu_bits.h
 create mode 100644 target/hexagon/helper.h
 create mode 100644 target/hexagon/imported/global_types.h
 create mode 100644 target/hexagon/imported/iss_ver_registers.h
 create mode 100644 target/hexagon/imported/max.h
 create mode 100644 target/hexagon/imported/regs.h
 create mode 100644 target/hexagon/op_helper.c
 create mode 100644 target/hexagon/translate.c
 create mode 100644 target/hexagon/translate.h

Comments

no-reply@patchew.org Nov. 19, 2019, 1:31 a.m. UTC | #1
Patchew URL: https://patchew.org/QEMU/1574121497-2433-1-git-send-email-tsimpson@quicinc.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards
Type: series
Message-id: 1574121497-2433-1-git-send-email-tsimpson@quicinc.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
7744509 Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards

=== OUTPUT BEGIN ===
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#38: 
new file mode 100644

ERROR: do not use C99 // comments
#1922: FILE: target/hexagon/imported/iss_ver_registers.h:80:
+// New interrupts, keep old defines for the time being

ERROR: trailing whitespace
#1924: FILE: target/hexagon/imported/iss_ver_registers.h:82:
+#define HEX_REG_GSR 81 $

ERROR: do not use C99 // comments
#2003: FILE: target/hexagon/imported/iss_ver_registers.h:161:
+// GREG_AVS renamed to GREG_RGDR2 in v67

ERROR: trailing whitespace
#2038: FILE: target/hexagon/imported/max.h:7:
+ * $

ERROR: code indent should never use tabs
#2046: FILE: target/hexagon/imported/max.h:15:
+#define INSTRUCTIONS_MAX 7^I^I/* 2 pairs + loopend */$

ERROR: code indent should never use tabs
#2051: FILE: target/hexagon/imported/max.h:20:
+#define MAX_CORES 4^I^I^I^I/* Cores per shared L2 */$

ERROR: line over 90 characters
#2054: FILE: target/hexagon/imported/max.h:23:
+#define THREADS_PER_CORE (MAX_CLUSTERS * MAX_THREADS_PER_CLUSTER)              /* HW threads in a core */

ERROR: code indent should never use tabs
#2054: FILE: target/hexagon/imported/max.h:23:
+#define THREADS_PER_CORE (MAX_CLUSTERS * MAX_THREADS_PER_CLUSTER)^I^I/* HW threads in a core */$

ERROR: do not use C99 // comments
#2056: FILE: target/hexagon/imported/max.h:25:
+#define DMA_MAX THREADS_MAX  // DMA: make this independent

ERROR: code indent should never use tabs
#2064: FILE: target/hexagon/imported/max.h:33:
+#define REG_WRITES_MAX 32^I^I/*?? */$

ERROR: code indent should never use tabs
#2065: FILE: target/hexagon/imported/max.h:34:
+#define PRED_WRITES_MAX 5^I^I/* 4 insns + endloop */$

WARNING: line over 80 characters
#2074: FILE: target/hexagon/imported/max.h:43:
+#define MAX_N_GRANULES 4               /* Max of number of granules of all caches */

ERROR: code indent should never use tabs
#2074: FILE: target/hexagon/imported/max.h:43:
+#define MAX_N_GRANULES 4^I^I/* Max of number of granules of all caches */$

ERROR: code indent should never use tabs
#2080: FILE: target/hexagon/imported/max.h:49:
+#define IIC_TAGS_MAX (1024*128)^I/* 1MB = 2^20 bytes */$

ERROR: spaces required around that '*' (ctx:VxV)
#2080: FILE: target/hexagon/imported/max.h:49:
+#define IIC_TAGS_MAX (1024*128)        /* 1MB = 2^20 bytes */
                           ^

ERROR: spaces required around that '*' (ctx:VxV)
#2081: FILE: target/hexagon/imported/max.h:50:
+#define IIC_DATA_IN_BYTES_MAX (1024*1024*4)
                                    ^

ERROR: spaces required around that '*' (ctx:VxV)
#2081: FILE: target/hexagon/imported/max.h:50:
+#define IIC_DATA_IN_BYTES_MAX (1024*1024*4)
                                         ^

ERROR: code indent should never use tabs
#2083: FILE: target/hexagon/imported/max.h:52:
+#define MAX_TLB_GUESS_ENTRIES (1024)^I// power of 2$

ERROR: do not use C99 // comments
#2083: FILE: target/hexagon/imported/max.h:52:
+#define MAX_TLB_GUESS_ENTRIES (1024)   // power of 2

ERROR: spaces required around that '*' (ctx:VxV)
#2088: FILE: target/hexagon/imported/max.h:57:
+#define IICTLB_PAGE_SIZE (1024*4)
                               ^

ERROR: spaces required around that '*' (ctx:VxV)
#2091: FILE: target/hexagon/imported/max.h:60:
+#define MAX_L1S_SIZE (1024*1024)
                           ^

WARNING: line over 80 characters
#2094: FILE: target/hexagon/imported/max.h:63:
+#define SUB_IMMEDS_MAX 4               /* number of immediate fragments within insn */

ERROR: code indent should never use tabs
#2094: FILE: target/hexagon/imported/max.h:63:
+#define SUB_IMMEDS_MAX 4^I^I/* number of immediate fragments within insn */$

ERROR: do not use C99 // comments
#2101: FILE: target/hexagon/imported/max.h:70:
+//#define QTRACE_HEADER_SIZE_MAX 50 /* at most 50 chars in header */

ERROR: code indent should never use tabs
#2109: FILE: target/hexagon/imported/max.h:78:
+#endif^I^I^I^I^I^I^I/* _MAX_H */$

total: 23 errors, 3 warnings, 2232 lines checked

Commit 7744509714d7 (Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/1574121497-2433-1-git-send-email-tsimpson@quicinc.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Laurent Vivier Nov. 19, 2019, 8:39 a.m. UTC | #2
Hi,

thank you for your contribution.

I think we need some little changes:

- fix the errors reported by patchew (you can check your patch with
scripts/checkpatch.pl)

- split the patch in two parts: one to add the target, one to add the
linux-user part,

- add the license header in files where it is missing.
  (you can use the SPDX tag)

- I think you can remove the "DEBUG_HEX" in cpu_loop() as most of the
information can be displayed by QEMU_STRACE env variable. The one in
hexagon_tr_translate_packet() can be done using QEMU_SINGLESTEP (if I
understand the purpose),

- The "[__SIGRTMAX - 1] = __SIGRTMIN + 1" part must be in a separate
patch. As I said at KVM Forum, I agree with this change if it doesn't
break anything else but it can be controversial.

Thanks,
Laurent

Le 19/11/2019 à 00:58, Taylor Simpson a écrit :
> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> ---
>  configure                                   |   9 +
>  default-configs/hexagon-linux-user.mak      |   1 +
>  include/elf.h                               |   2 +
>  linux-user/elfload.c                        |  16 ++
>  linux-user/hexagon/cpu_loop.c               | 103 +++++++++
>  linux-user/hexagon/signal.c                 | 276 +++++++++++++++++++++++
>  linux-user/hexagon/sockbits.h               |   3 +
>  linux-user/hexagon/syscall_nr.h             | 331 ++++++++++++++++++++++++++++
>  linux-user/hexagon/target_cpu.h             |  35 +++
>  linux-user/hexagon/target_elf.h             |  24 ++
>  linux-user/hexagon/target_fcntl.h           |   3 +
>  linux-user/hexagon/target_signal.h          |  19 ++
>  linux-user/hexagon/target_structs.h         |  31 +++
>  linux-user/hexagon/target_syscall.h         |  17 ++
>  linux-user/hexagon/termbits.h               | 231 +++++++++++++++++++
>  linux-user/signal.c                         |   8 +
>  linux-user/syscall.c                        |   2 +
>  linux-user/syscall_defs.h                   |  33 +++
>  target/hexagon/Makefile.objs                |   6 +
>  target/hexagon/cpu-param.h                  |  11 +
>  target/hexagon/cpu.c                        | 283 ++++++++++++++++++++++++
>  target/hexagon/cpu.h                        | 146 ++++++++++++
>  target/hexagon/cpu_bits.h                   |  15 ++
>  target/hexagon/helper.h                     |   3 +
>  target/hexagon/imported/global_types.h      |  25 +++
>  target/hexagon/imported/iss_ver_registers.h | 183 +++++++++++++++
>  target/hexagon/imported/max.h               |  78 +++++++
>  target/hexagon/imported/regs.h              |  19 ++
>  target/hexagon/op_helper.c                  |  29 +++
>  target/hexagon/translate.c                  | 220 ++++++++++++++++++
>  target/hexagon/translate.h                  |  22 ++
>  31 files changed, 2184 insertions(+)
>  create mode 100644 default-configs/hexagon-linux-user.mak
>  create mode 100644 linux-user/hexagon/cpu_loop.c
>  create mode 100644 linux-user/hexagon/signal.c
>  create mode 100644 linux-user/hexagon/sockbits.h
>  create mode 100644 linux-user/hexagon/syscall_nr.h
>  create mode 100644 linux-user/hexagon/target_cpu.h
>  create mode 100644 linux-user/hexagon/target_elf.h
>  create mode 100644 linux-user/hexagon/target_fcntl.h
>  create mode 100644 linux-user/hexagon/target_signal.h
>  create mode 100644 linux-user/hexagon/target_structs.h
>  create mode 100644 linux-user/hexagon/target_syscall.h
>  create mode 100644 linux-user/hexagon/termbits.h
>  create mode 100644 target/hexagon/Makefile.objs
>  create mode 100644 target/hexagon/cpu-param.h
>  create mode 100644 target/hexagon/cpu.c
>  create mode 100644 target/hexagon/cpu.h
>  create mode 100644 target/hexagon/cpu_bits.h
>  create mode 100644 target/hexagon/helper.h
>  create mode 100644 target/hexagon/imported/global_types.h
>  create mode 100644 target/hexagon/imported/iss_ver_registers.h
>  create mode 100644 target/hexagon/imported/max.h
>  create mode 100644 target/hexagon/imported/regs.h
>  create mode 100644 target/hexagon/op_helper.c
>  create mode 100644 target/hexagon/translate.c
>  create mode 100644 target/hexagon/translate.h
>
Philippe Mathieu-Daudé Nov. 19, 2019, 8:51 a.m. UTC | #3
On 11/19/19 2:31 AM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/1574121497-2433-1-git-send-email-tsimpson@quicinc.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Subject: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards
> Type: series
> Message-id: 1574121497-2433-1-git-send-email-tsimpson@quicinc.com
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
> 
> Switched to a new branch 'test'
> 7744509 Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards
> 
> === OUTPUT BEGIN ===
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #38:
> new file mode 100644
> 
> ERROR: do not use C99 // comments
> #1922: FILE: target/hexagon/imported/iss_ver_registers.h:80:
> +// New interrupts, keep old defines for the time being

I tried this quick way to filter some file/directory out of 
checkpatch.pl, and it works nicely:

$ cat .git/hooks/pre-commit
#!/bin/bash
exec git diff --cached -- ':(top)' $(test -e .checkpatchignore && sed 
-ne '/^\(#.*\|$\)/ ! s/.*/:(exclude)\0/p' < .checkpatchignore) | 
scripts/checkpatch.pl --no-signoff -q -

$ cat .checkpatchignore
# A line starting with # serves as a comment.
# A blank line matches no files, so it can serve as a separator for 
readability.
include/standard-headers
target/xtensa/core-*
target/hexagon/imported

Would this be acceptable to reduce patchew false positives?

git exclude pathspec trick from:
https://stackoverflow.com/questions/39931781/git-diff-stat-exclude-certain-files/39937070#39937070
Laurent Vivier Nov. 19, 2019, 9:03 a.m. UTC | #4
Le 19/11/2019 à 09:39, Laurent Vivier a écrit :
> Hi,
> 
> thank you for your contribution.
> 
> I think we need some little changes:
> 
> - fix the errors reported by patchew (you can check your patch with
> scripts/checkpatch.pl)

of course we can ignore the errors for the imported files if you want to
copy them as-is from your reference implementation.

Thanks,
Laurent
Richard Henderson Nov. 19, 2019, 1:33 p.m. UTC | #5
On 11/19/19 9:51 AM, Philippe Mathieu-Daudé wrote:
> I tried this quick way to filter some file/directory out of checkpatch.pl, and
> it works nicely:
> 
> $ cat .git/hooks/pre-commit
> #!/bin/bash
> exec git diff --cached -- ':(top)' $(test -e .checkpatchignore && sed -ne
> '/^\(#.*\|$\)/ ! s/.*/:(exclude)\0/p' < .checkpatchignore) |
> scripts/checkpatch.pl --no-signoff -q -
> 
> $ cat .checkpatchignore
> # A line starting with # serves as a comment.
> # A blank line matches no files, so it can serve as a separator for readability.
> include/standard-headers
> target/xtensa/core-*
> target/hexagon/imported
> 
> Would this be acceptable to reduce patchew false positives?
> 
> git exclude pathspec trick from:
> https://stackoverflow.com/questions/39931781/git-diff-stat-exclude-certain-files/39937070#39937070

Looks like a good solution to me.


r~
Eric Blake Nov. 19, 2019, 2:14 p.m. UTC | #6
On 11/18/19 5:58 PM, Taylor Simpson wrote:

In addition to what others have said, you have an atypical and long 
subject line. You'll want to rewrite it to match more typical subject 
lines (around 65 characters) then a blank line then the additional 
details.  'git shortlog -30' might give you some ideas on how to do it; 
you'll probably want to use 'hexagon:' as your prefix.

> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> ---

>   31 files changed, 2184 insertions(+)

That's a big patch.  How much of it is copied verbatim from elsewhere 
and how much is new code? Splitting the patch along those lines makes 
review of the non-copied portion easier.
Philippe Mathieu-Daudé Nov. 19, 2019, 3:11 p.m. UTC | #7
On 11/19/19 3:14 PM, Eric Blake wrote:
> On 11/18/19 5:58 PM, Taylor Simpson wrote:
> 
> In addition to what others have said, you have an atypical and long 
> subject line. You'll want to rewrite it to match more typical subject 
> lines (around 65 characters) then a blank line then the additional 
> details.  'git shortlog -30' might give you some ideas on how to do it; 
> you'll probably want to use 'hexagon:' as your prefix.
> 
>> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
>> ---
> 
>>   31 files changed, 2184 insertions(+)
> 
> That's a big patch.  How much of it is copied verbatim from elsewhere 
> and how much is new code? Splitting the patch along those lines makes 
> review of the non-copied portion easier.

This is WAY less than the 132114 additions in 2 patches from the first 
round (see 
https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg07188.html)

:)))
Philippe Mathieu-Daudé Nov. 19, 2019, 3:19 p.m. UTC | #8
On 11/19/19 12:58 AM, Taylor Simpson wrote:
> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> ---
[...]
>   target/hexagon/imported/global_types.h      |  25 +++
>   target/hexagon/imported/iss_ver_registers.h | 183 +++++++++++++++
>   target/hexagon/imported/max.h               |  78 +++++++
>   target/hexagon/imported/regs.h              |  19 ++

Maybe you can rename this directory as:

target/hexagon/dsp-sdk/

and add a README "Files under this directory are imported from the SDK 
available once registered on developer.qualcomm.com ..."
Paolo Bonzini Nov. 19, 2019, 3:37 p.m. UTC | #9
On 19/11/19 09:51, Philippe Mathieu-Daudé wrote:
> 
> $ cat .git/hooks/pre-commit
> #!/bin/bash
> exec git diff --cached -- ':(top)' $(test -e .checkpatchignore && sed
> -ne '/^\(#.*\|$\)/ ! s/.*/:(exclude)\0/p' < .checkpatchignore) |
> scripts/checkpatch.pl --no-signoff -q -
> 
> $ cat .checkpatchignore
> # A line starting with # serves as a comment.
> # A blank line matches no files, so it can serve as a separator for
> readability.
> include/standard-headers
> target/xtensa/core-*
> target/hexagon/imported
> 
> Would this be acceptable to reduce patchew false positives?
> 
> git exclude pathspec trick from:
> https://stackoverflow.com/questions/39931781/git-diff-stat-exclude-certain-files/39937070#39937070
> 

Could support for this be added directly to checkpatch, instead?

Paolo
Stefan Hajnoczi Nov. 19, 2019, 4:14 p.m. UTC | #10
On Tue, Nov 19, 2019 at 09:51:00AM +0100, Philippe Mathieu-Daudé wrote:
> On 11/19/19 2:31 AM, no-reply@patchew.org wrote:
> > Patchew URL: https://patchew.org/QEMU/1574121497-2433-1-git-send-email-tsimpson@quicinc.com/
> > 
> > 
> > 
> > Hi,
> > 
> > This series seems to have some coding style problems. See output below for
> > more information:
> > 
> > Subject: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards
> > Type: series
> > Message-id: 1574121497-2433-1-git-send-email-tsimpson@quicinc.com
> > 
> > === TEST SCRIPT BEGIN ===
> > #!/bin/bash
> > git rev-parse base > /dev/null || exit 0
> > git config --local diff.renamelimit 0
> > git config --local diff.renames True
> > git config --local diff.algorithm histogram
> > ./scripts/checkpatch.pl --mailback base..
> > === TEST SCRIPT END ===
> > 
> > Switched to a new branch 'test'
> > 7744509 Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards
> > 
> > === OUTPUT BEGIN ===
> > WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> > #38:
> > new file mode 100644
> > 
> > ERROR: do not use C99 // comments
> > #1922: FILE: target/hexagon/imported/iss_ver_registers.h:80:
> > +// New interrupts, keep old defines for the time being
> 
> I tried this quick way to filter some file/directory out of checkpatch.pl,
> and it works nicely:
> 
> $ cat .git/hooks/pre-commit
> #!/bin/bash
> exec git diff --cached -- ':(top)' $(test -e .checkpatchignore && sed -ne
> '/^\(#.*\|$\)/ ! s/.*/:(exclude)\0/p' < .checkpatchignore) |
> scripts/checkpatch.pl --no-signoff -q -
> 
> $ cat .checkpatchignore
> # A line starting with # serves as a comment.
> # A blank line matches no files, so it can serve as a separator for
> readability.
> include/standard-headers
> target/xtensa/core-*
> target/hexagon/imported
> 
> Would this be acceptable to reduce patchew false positives?
> 
> git exclude pathspec trick from:
> https://stackoverflow.com/questions/39931781/git-diff-stat-exclude-certain-files/39937070#39937070

Seems useful so that patchew/checkpatch output remains high-quality.

An alternative to the git exclude pathspec trick is to build
.checkpatchignore into the checkpatch.pl script so that regular
invocations also use .checkpatchignore.

Stefan
Taylor Simpson Nov. 19, 2019, 5:22 p.m. UTC | #11
Thanks for all the feedback on the patch.  I'll summarize my TODO list here.  Please let me know if there's anything I missed.
- Add a README file in the imported directory to make it clear that the code comes from another project.  Personally, I prefer keeping the name as "imported".  It was suggested by Richard at the meeting.  Also as a heads-up, that is a small subset of the files that will be in that directory eventually.  Right now, it is the minimum needed to build the skeleton target.
- Work on the .checkpatchignore as Philippe suggested.
- Split out the "[__SIGRTMAX - 1] = __SIGRTMIN + 1" into a separate patch.
- Clean up the long subject line.
- Add license text to the new files.
- Remove the DEBUG_HEX blocks.  In general the DEBUG_HEX macro controls a bunch of debugging output as you'll see in later patches.  In the long run, I think it should be replaces with a macro that is defined when configured with --enable-debug and then an additional command-line argument.  I haven't looked into this, so any pointers would be appreciated.
- Laurent suggested I split the patch into two parts: linux-user and target/hexagon.  If I do that, which one should contain the changes to common files (e.g., configure)?  Also, note that we won't be able to build until both patches are merged.  Is that OK?

Thanks,
Taylor


-----Original Message-----
From: Philippe Mathieu-Daudé <philmd@redhat.com>
Sent: Tuesday, November 19, 2019 9:19 AM
To: Taylor Simpson <tsimpson@quicinc.com>; laurent@vivier.eu; riku.voipio@iki.fi; qemu-devel@nongnu.org
Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards

-------------------------------------------------------------------------
CAUTION: This email originated from outside of the organization.
-------------------------------------------------------------------------

On 11/19/19 12:58 AM, Taylor Simpson wrote:
> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> ---
[...]
>   target/hexagon/imported/global_types.h      |  25 +++
>   target/hexagon/imported/iss_ver_registers.h | 183 +++++++++++++++
>   target/hexagon/imported/max.h               |  78 +++++++
>   target/hexagon/imported/regs.h              |  19 ++

Maybe you can rename this directory as:

target/hexagon/dsp-sdk/

and add a README "Files under this directory are imported from the SDK available once registered on developer.qualcomm.com ..."
Peter Maydell Nov. 19, 2019, 5:32 p.m. UTC | #12
On Tue, 19 Nov 2019 at 17:23, Taylor Simpson <tsimpson@quicinc.com> wrote:
> - Laurent suggested I split the patch into two parts: linux-user and target/hexagon.  If I do that, which one should contain the changes to common files (e.g., configure)?  Also, note that we won't be able to build until both patches are merged.  Is that OK?

Rule of thumb -- patches should generally be 200 lines
or less (less is good!), with exceptions where something
is very mechanical/repetitive and there's no point
splitting it up (eg there's no need to have the new
syscall_nr.h additions split into multiple patches).
Judging purely by the "2184 insertions" summary you
should probably be looking to split this into somewhere
between 5 and 10 patches, where each patch is a coherent
chunk with a commit message describing it.

Typically a new port is a complete patchset consisting
of multiple patches which are intended to be applied in
sequence. (They should be sent to the mailing list so
they appear as a cover letter plus a set of patch
emails each of which is a follow-up/reply to the cover
letter. git-send-email can do this kind of thing for you.)

The requirement for a patch series like that is that
at each intermediate point QEMU should still build
and pass 'make check'; but the new port will obviously
not be completely functional until all the parts are
present. You can probably look at how the initial
commits for various other recent ports were structured
to see the general idea.

thanks
-- PMM
Laurent Vivier Nov. 19, 2019, 6:13 p.m. UTC | #13
Le 19/11/2019 à 18:22, Taylor Simpson a écrit :
> Thanks for all the feedback on the patch.  I'll summarize my TODO list here.  Please let me know if there's anything I missed.
> - Add a README file in the imported directory to make it clear that the code comes from another project.  Personally, I prefer keeping the name as "imported".  It was suggested by Richard at the meeting.  Also as a heads-up, that is a small subset of the files that will be in that directory eventually.  Right now, it is the minimum needed to build the skeleton target.
> - Work on the .checkpatchignore as Philippe suggested.
> - Split out the "[__SIGRTMAX - 1] = __SIGRTMIN + 1" into a separate patch.
> - Clean up the long subject line.
> - Add license text to the new files.
> - Remove the DEBUG_HEX blocks.  In general the DEBUG_HEX macro controls a bunch of debugging output as you'll see in later patches.  In the long run, I think it should be replaces with a macro that is defined when configured with --enable-debug and then an additional command-line argument.  I haven't looked into this, so any pointers would be appreciated.

You can have a look to the trace infrastructure
(docs/devel/tracing.txt). We have also some qemu_log() macros for low
level debugging.

> - Laurent suggested I split the patch into two parts: linux-user and target/hexagon.  If I do that, which one should contain the changes to common files (e.g., configure)?  Also, note that we won't be able to build until both patches are merged.  Is that OK?

You should add target/hexagon first, and it should not be build as we
don't have any target (hexagon-linux-user or hexagon-softmmu),
then you can add linux-user part that will be built and use the
target/hexagone CPU. I think the configure part should go to the
linux-user part as it enables the build.

I asked to split the patch for review purpose, but this should not break
anything (to allow bisect).

Thanks,
Laurent

> 
> Thanks,
> Taylor
> 
> 
> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> Sent: Tuesday, November 19, 2019 9:19 AM
> To: Taylor Simpson <tsimpson@quicinc.com>; laurent@vivier.eu; riku.voipio@iki.fi; qemu-devel@nongnu.org
> Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards
> 
> -------------------------------------------------------------------------
> CAUTION: This email originated from outside of the organization.
> -------------------------------------------------------------------------
> 
> On 11/19/19 12:58 AM, Taylor Simpson wrote:
>> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
>> ---
> [...]
>>   target/hexagon/imported/global_types.h      |  25 +++
>>   target/hexagon/imported/iss_ver_registers.h | 183 +++++++++++++++
>>   target/hexagon/imported/max.h               |  78 +++++++
>>   target/hexagon/imported/regs.h              |  19 ++
> 
> Maybe you can rename this directory as:
> 
> target/hexagon/dsp-sdk/
> 
> and add a README "Files under this directory are imported from the SDK available once registered on developer.qualcomm.com ..."
> 
>
Richard Henderson Nov. 19, 2019, 7:33 p.m. UTC | #14
On 11/19/19 12:58 AM, Taylor Simpson wrote:
> +static abi_ulong get_sigframe(struct target_sigaction *ka,
> +                              CPUHexagonState *regs, size_t framesize)
> +{
> +    abi_ulong sp = get_sp_from_cpustate(regs);
> +
> +    /* This is the X/Open sanctioned signal stack switching.  */
> +    sp = target_sigsp(sp, ka) - framesize;
> +
> +    sp &= ~7UL; /* align sp on 8-byte boundary */

QEMU_ALIGN_DOWN.

> diff --git a/linux-user/hexagon/sockbits.h b/linux-user/hexagon/sockbits.h
> new file mode 100644
> index 0000000..85bd64a
> --- /dev/null
> +++ b/linux-user/hexagon/sockbits.h
> @@ -0,0 +1,3 @@
> +/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
> +
> +#include "../generic/sockbits.h"

All new files should have denote their license.

> +static inline const char *cpu_get_model(uint32_t eflags)
> +{
> +    /* For now, treat anything newer than v60 as a v67 */
> +    /* FIXME - Disable instructions that are newer than the specified arch */
> +    if (eflags == 0x04 ||    /* v5  */
> +        eflags == 0x05 ||    /* v55 */
> +        eflags == 0x60 ||    /* v60 */
> +        eflags == 0x61 ||    /* v61 */
> +        eflags == 0x62 ||    /* v62 */
> +        eflags == 0x65 ||    /* v65 */
> +        eflags == 0x66 ||    /* v66 */
> +        eflags == 0x67) {    /* v67 */
> +        return "v67";
> +    }
> +    printf("eflags = 0x%x\n", eflags);

Left over debug.

> diff --git a/target/hexagon/Makefile.objs b/target/hexagon/Makefile.objs
> new file mode 100644
> index 0000000..dfab6ee
> --- /dev/null
> +++ b/target/hexagon/Makefile.objs
> @@ -0,0 +1,6 @@
> +obj-y += \
> +    cpu.o \
> +    translate.o \
> +    op_helper.o
> +
> +CFLAGS += -I$(SRC_PATH)/target/hexagon/imported

Is this really better than

#include "imported/global_types.h"

etc?

> +++ b/target/hexagon/cpu-param.h
> @@ -0,0 +1,11 @@
> +/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
> +
> +
> +#ifndef HEXAGON_CPU_PARAM_H
> +#define HEXAGON_CPU_PARAM_H
> +
> +# define TARGET_PHYS_ADDR_SPACE_BITS 36

Watch your spacing.

Does this really compile without TARGET_VIRT_ADDR_SPACE_BITS?

It's used in linux-user/main.c, but I suppose in a way that
the preprocessor interprets it as 0.

> +const char * const hexagon_prednames[] = {
> +  "p0 ", "p1 ", "p2 ", "p3 "
> +};

Inter-string spacing probably belongs to the format not the name.

> +static inline target_ulong hack_stack_ptrs(CPUHexagonState *env,
> +                                           target_ulong addr)
> +{
> +    target_ulong stack_start = env->stack_start;
> +    target_ulong stack_size = 0x10000;
> +    target_ulong stack_adjust = env->stack_adjust;
> +
> +    if (stack_start + 0x1000 >= addr && addr >= (stack_start - stack_size)) {
> +        return addr - stack_adjust;
> +    }
> +    return addr;
> +}

An explanation would be welcome here.

> +static void hexagon_dump(CPUHexagonState *env, FILE *f)
> +{
> +    static target_ulong last_pc;
> +    int i;
> +
> +    /*
> +     * When comparing with LLDB, it doesn't step through single-cycle
> +     * hardware loops the same way.  So, we just skip them here
> +     */
> +    if (env->gpr[HEX_REG_PC] == last_pc) {
> +        return;
> +    }

This seems mis-placed.

> +#ifdef FIXME
> +    /*
> +     * LLDB bug swaps gp and ugp
> +     * FIXME when the LLDB bug is fixed
> +     */
> +    print_reg(f, env, HEX_REG_GP);
> +    print_reg(f, env, HEX_REG_UGP);
> +#else
> +    fprintf(f, "  %s = 0x" TARGET_FMT_lx "\n",
> +        hexagon_regnames[HEX_REG_GP],
> +        hack_stack_ptrs(env, env->gpr[HEX_REG_UGP]));
> +    fprintf(f, "  %s = 0x" TARGET_FMT_lx "\n",
> +        hexagon_regnames[HEX_REG_UGP],
> +        hack_stack_ptrs(env, env->gpr[HEX_REG_GP]));
> +#endif
> +    print_reg(f, env, HEX_REG_PC);
> +#ifdef FIXME
> +    /*
> +     * Not modelled in qemu, print junk to minimize the diff's
> +     * with LLDB output
> +     */
> +    print_reg(f, env, HEX_REG_CAUSE);
> +    print_reg(f, env, HEX_REG_BADVA);
> +    print_reg(f, env, HEX_REG_CS0);
> +    print_reg(f, env, HEX_REG_CS1);
> +#else
> +    fprintf(f, "  cause = 0x000000db\n");
> +    fprintf(f, "  badva = 0x00000000\n");
> +    fprintf(f, "  cs0 = 0x00000000\n");
> +    fprintf(f, "  cs1 = 0x00000000\n");
> +#endif

Need we retain the fixme?

> +void hexagon_debug(CPUHexagonState *env)
> +{
> +    hexagon_dump(env, stdout);
> +}

Is this to be called from the debugger?  From what location did you find it
useful?  There are only certain locations in tcg that are self-consistent...

> +    DEFINE_CPU(TYPE_HEXAGON_CPU_V67,              hexagon_v67_cpu_init),

Spacing?

> +#ifndef HEXAGON_CPU_H
> +#define HEXAGON_CPU_H
> +
> +/* FIXME - Change this to a command-line option */
> +#ifdef FIXME
> +#define DEBUG_HEX
> +#endif
> +#ifdef FIXME
> +#define COUNT_HEX_HELPERS
> +#endif

Eh?

> +
> +/* Forward declaration needed by some of the header files */
> +typedef struct CPUHexagonState CPUHexagonState;
> +
> +#include <fenv.h>
> +#include "qemu/osdep.h"

osdep.h should already have been included.
Indeed, it must be first for *.c files.

Why do you need fenv.h?

> +#include "global_types.h"
> +#include "max.h"
> +#include "iss_ver_registers.h"
> +
> +#define TARGET_PAGE_BITS 16     /* 64K pages */
> +/*
> + * For experimenting with oslib (4K pages)
> + * #define TARGET_PAGE_BITS 12
> + */
> +#define TARGET_LONG_BITS 32
> +#define TARGET_VIRT_ADDR_SPACE_BITS 32

Ah, to answer my earlier question, these belong to cpu-param.h.
Drop the "experimenting" comment.

> +
> +#include <time.h>

time.h is included by osdep.h.


> +#include "qemu/compiler.h"
> +#include "qemu-common.h"
> +#include "exec/cpu-defs.h"
> +#include "regs.h"
> +
> +#define TYPE_HEXAGON_CPU "hexagon-cpu"
> +
> +#define HEXAGON_CPU_TYPE_SUFFIX "-" TYPE_HEXAGON_CPU
> +#define HEXAGON_CPU_TYPE_NAME(name) (name HEXAGON_CPU_TYPE_SUFFIX)
> +#define CPU_RESOLVING_TYPE TYPE_HEXAGON_CPU
> +
> +#define TYPE_HEXAGON_CPU_V67             HEXAGON_CPU_TYPE_NAME("v67")
> +
> +#define MMU_USER_IDX 0
> +
> +#define TRANSLATE_FAIL 1
> +#define TRANSLATE_SUCCESS 0

What's this for?  This looks like it's cribbed from riscv,
which oddly doesn't match the actual tlb_fill interface,
which uses bool true for success.

> +
> +struct MemLog {
> +    vaddr_t va;
> +    size1u_t width;
> +    size4u_t data32;
> +    size8u_t data64;
> +};

Is this part of translation?  Maybe save this til you
actually use it, and probably place in translate.h.

> +    /* For comparing with LLDB on target - see hack_stack_ptrs function */
> +    target_ulong stack_start;
> +    target_ulong stack_adjust;

Which, as you recall, doesn't actually have any commentary.

> +extern const char * const hexagon_regnames[];
> +extern const char * const hexagon_prednames[];

Do these actually need declaring here?
Let's keep them private to cpu.c otherwise.

> +#define ENV_GET_CPU(e)  CPU(hexagon_env_get_cpu(e))
> +#define ENV_OFFSET      offsetof(HexagonCPU, env)

Obsolete. Remove.

> +int hexagon_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
> +int hexagon_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);

Move these decls to e.g. internal.h?
They're not relevant to generic users of cpu.h

> +void QEMU_NORETURN do_raise_exception_err(CPUHexagonState *env,
> +                                          uint32_t exception, uintptr_t pc);

Likewise.

> +void hexagon_translate_init(void);
> +void hexagon_debug(CPUHexagonState *env);
> +void hexagon_debug_vreg(CPUHexagonState *env, int regnum);
> +void hexagon_debug_qreg(CPUHexagonState *env, int regnum);

Likewise.

> +#ifdef COUNT_HEX_HELPERS
> +extern void print_helper_counts(void);
> +#endif

Likewise.

> +static void decode_packet(CPUHexagonState *env, DisasContext *ctx)
> +{
> +    size4u_t words[4];
> +    int i;
> +    /* Brute force way to make sure current PC is set */
> +    tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->base.pc_next);

What's this for?

> +
> +    for (i = 0; i < 4; i++) {
> +        words[i] = cpu_ldl_code(env, ctx->base.pc_next + i * sizeof(size4u_t));
> +    }

And this?

> +    /*
> +     * Brute force just enough to get the first program to execute.
> +     */
> +    switch (words[0]) {
> +    case 0x7800c806:                                /* r6 = #64 */
> +        tcg_gen_movi_tl(hex_gpr[6], 64);
> +        ctx->base.pc_next += 4;
> +        break;
> +    case 0x7800c020:                                /* r0 = #1 */
> +        tcg_gen_movi_tl(hex_gpr[0], 1);
> +        ctx->base.pc_next += 4;
> +        break;
> +    case 0x00044002:
> +        if (words[1] == 0x7800c001) {               /* r1 = ##0x400080 */
> +            tcg_gen_movi_tl(hex_gpr[1], 0x400080);
> +            ctx->base.pc_next += 8;
> +        } else {
> +            printf("ERROR: Unknown instruction 0x%x\n", words[1]);
> +            g_assert_not_reached();
> +        }
> +        break;
> +    case 0x7800c0e2:                                /* r2 = #7 */
> +        tcg_gen_movi_tl(hex_gpr[2], 7);
> +        ctx->base.pc_next += 4;
> +        break;
> +    case 0x5400c004:                               /* trap0(#1) */
> +    {
> +        TCGv excp_trap0 = tcg_const_tl(HEX_EXCP_TRAP0);
> +        gen_helper_raise_exception(cpu_env, excp_trap0);
> +        tcg_temp_free(excp_trap0);
> +        ctx->base.pc_next += 4;
> +        break;
> +    }
> +    case 0x7800cbc6:                               /* r6 = #94 */
> +        tcg_gen_movi_tl(hex_gpr[6], 94);
> +        ctx->base.pc_next += 4;
> +        break;
> +    case 0x7800cba6:                               /* r6 = #93 */
> +        tcg_gen_movi_tl(hex_gpr[6], 93);
> +        ctx->base.pc_next += 4;
> +        break;
> +    case 0x7800c000:                               /* r0 = #0 */
> +        tcg_gen_movi_tl(hex_gpr[0], 0);
> +        ctx->base.pc_next += 4;
> +        break;
> +    default:
> +        ctx->base.is_jmp = DISAS_TOO_MANY;
> +        ctx->base.pc_next += 4;
> +    }

I'm not especially keen on this, since it will just be removed in subsequent
patches.  The initial patch must compile, but it need not do *anything*
interesting.

C.f. 61766fe9e2d3 which is the initial commit for target/hppa, wherein the
decoder is

> +static ExitStatus translate_one(DisasContext *ctx, uint32_t insn)
> +{
> +    uint32_t opc = extract32(insn, 26, 6);
> +
> +    switch (opc) {
> +    default:
> +        break;
> +    }
> +    return gen_illegal(ctx);
> +}



> +}
> +
> +static void hexagon_tr_init_disas_context(DisasContextBase *dcbase,
> +                                          CPUState *cs)
> +{
> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +
> +    ctx->mem_idx = ctx->base.tb->flags & TB_FLAGS_MMU_MASK;

Since you're not initializing this in cpu_get_tb_cpu_state, you might as well
just use

    ctx->mem_idx = MMU_USER_IDX;

> +static void hexagon_tr_translate_packet(DisasContextBase *dcbase, CPUState *cpu)
> +{
> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +    CPUHexagonState *env = cpu->env_ptr;
> +
> +    decode_packet(env, ctx);
> +
> +    if (ctx->base.is_jmp == DISAS_NEXT) {
> +        target_ulong page_start;
> +
> +        page_start = ctx->base.pc_first & TARGET_PAGE_MASK;
> +        if (ctx->base.pc_next - page_start >= TARGET_PAGE_SIZE) {
> +            ctx->base.is_jmp = DISAS_TOO_MANY;
> +        }
> +
> +#ifdef DEBUG_HEX
> +        /* When debugging, force the end of the TB after each packet */
> +        if (ctx->base.pc_next - ctx->base.pc_first >= 0x04) {
> +            ctx->base.is_jmp = DISAS_TOO_MANY;
> +        }
> +#endif
> +    }

As mentioned elsewhere, this latter should be handled by -singlestep.  The
generic translator loop will have set max_insns to 1.


r~
Richard Henderson Nov. 19, 2019, 7:36 p.m. UTC | #15
On 11/19/19 6:22 PM, Taylor Simpson wrote:
> - Laurent suggested I split the patch into two parts: linux-user and target/hexagon.  If I do that, which one should contain the changes to common files (e.g., configure)?  Also, note that we won't be able to build until both patches are merged.  Is that OK?

The configure parts should be a third, last, patch.

The series is bisectable, because before the configure patch,
none of the hexagon code is compiled at all.


r~
Aleksandar Markovic Nov. 20, 2019, 2:26 a.m. UTC | #16
On Tuesday, November 19, 2019, Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 11/19/19 6:22 PM, Taylor Simpson wrote:
> > - Laurent suggested I split the patch into two parts: linux-user and
> target/hexagon.  If I do that, which one should contain the changes to
> common files (e.g., configure)?  Also, note that we won't be able to build
> until both patches are merged.  Is that OK?
>
> The configure parts should be a third, last, patch.
>
> The series is bisectable, because before the configure patch,
> none of the hexagon code is compiled at all.
>
>
I don't think this is a good advice. Yes, at first glance, that would make
the submitter's job easier - since he could divide the whole code into
parts practicaly arbitrarily - but the resulting series will be of
suboptimal quality. If the submitter was forced from the outset to add
segments of his solution so that each step actually (not only ostensibly)
compiles, he would also be forced to organize patches in much more coherent
way, organize his code in much more modular way, possibly improve initial
organization, additionally making reviews much easier.

Aleksandar


>
> r~
>
>
Taylor Simpson Nov. 20, 2019, 4:48 a.m. UTC | #17
For the general DEBUG_HEX messages, I think the trace infrastructure isn't quite what I'm looking for.

Here's a sample of what it prints
Start packet: pc = 0x4002f0
Packet committed: pc = 0x4002f0
Regs written
        r18 = -69420 (0xfffef0d4)
        r19 = -69412 (0xfffef0dc)
Stores
        memd[0xfffef0b0] = 0 (0x0000000000000000)
Next PC = 0x4002f8
Exec counters: pkt = 17, insn = 24, hvx = 0

For performance, I'd like a way to disable them completely when --enable-debug isn't passed to configure.  It looks like #ifdef CONFIG_DEBUG_TCG will work for this.  Then, I'd like a command-line option to turn them on.  The "-d exec" seems like a reasonable place.  So, the code will look like this

#ifdef CONFIG_DEBUG_TCG
    if (qemu_loglevel_maks(CPU_LOG_EXEC)) {
        qemu_log(...);
    }
#endif

Please let me know if I'm on the right track here.

Thanks,
Taylor


-----Original Message-----
From: Laurent Vivier <laurent@vivier.eu>
Sent: Tuesday, November 19, 2019 12:13 PM
To: Taylor Simpson <tsimpson@quicinc.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; riku.voipio@iki.fi; qemu-devel@nongnu.org
Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards


Le 19/11/2019 à 18:22, Taylor Simpson a écrit :
> Thanks for all the feedback on the patch.  I'll summarize my TODO list here.  Please let me know if there's anything I missed.
> - Add a README file in the imported directory to make it clear that the code comes from another project.  Personally, I prefer keeping the name as "imported".  It was suggested by Richard at the meeting.  Also as a heads-up, that is a small subset of the files that will be in that directory eventually.  Right now, it is the minimum needed to build the skeleton target.
> - Work on the .checkpatchignore as Philippe suggested.
> - Split out the "[__SIGRTMAX - 1] = __SIGRTMIN + 1" into a separate patch.
> - Clean up the long subject line.
> - Add license text to the new files.
> - Remove the DEBUG_HEX blocks.  In general the DEBUG_HEX macro controls a bunch of debugging output as you'll see in later patches.  In the long run, I think it should be replaces with a macro that is defined when configured with --enable-debug and then an additional command-line argument.  I haven't looked into this, so any pointers would be appreciated.

You can have a look to the trace infrastructure (docs/devel/tracing.txt). We have also some qemu_log() macros for low level debugging.

> - Laurent suggested I split the patch into two parts: linux-user and target/hexagon.  If I do that, which one should contain the changes to common files (e.g., configure)?  Also, note that we won't be able to build until both patches are merged.  Is that OK?

You should add target/hexagon first, and it should not be build as we don't have any target (hexagon-linux-user or hexagon-softmmu), then you can add linux-user part that will be built and use the target/hexagone CPU. I think the configure part should go to the linux-user part as it enables the build.

I asked to split the patch for review purpose, but this should not break anything (to allow bisect).

Thanks,
Laurent

>
> Thanks,
> Taylor
>
>
> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> Sent: Tuesday, November 19, 2019 9:19 AM
> To: Taylor Simpson <tsimpson@quicinc.com>; laurent@vivier.eu;
> riku.voipio@iki.fi; qemu-devel@nongnu.org
> Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of
> patches - linux-user changes + linux-user/hexagon + skeleton of
> target/hexagon - Files in target/hexagon/imported are from another
> project and therefore do not conform to qemu coding standards
>
> ----------------------------------------------------------------------
> ---
> CAUTION: This email originated from outside of the organization.
> ----------------------------------------------------------------------
> ---
>
> On 11/19/19 12:58 AM, Taylor Simpson wrote:
>> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
>> ---
> [...]
>>   target/hexagon/imported/global_types.h      |  25 +++
>>   target/hexagon/imported/iss_ver_registers.h | 183 +++++++++++++++
>>   target/hexagon/imported/max.h               |  78 +++++++
>>   target/hexagon/imported/regs.h              |  19 ++
>
> Maybe you can rename this directory as:
>
> target/hexagon/dsp-sdk/
>
> and add a README "Files under this directory are imported from the SDK available once registered on developer.qualcomm.com ..."
>
>
Taylor Simpson Nov. 20, 2019, 5:15 a.m. UTC | #18
Thanks for the feedback Richard.

Responses below ...

Thanks,
Taylor


-----Original Message-----
From: Richard Henderson <richard.henderson@linaro.org>
Sent: Tuesday, November 19, 2019 1:34 PM
To: Taylor Simpson <tsimpson@quicinc.com>; laurent@vivier.eu; riku.voipio@iki.fi; qemu-devel@nongnu.org
Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards

-------------------------------------------------------------------------
CAUTION: This email originated from outside of the organization.
-------------------------------------------------------------------------

On 11/19/19 12:58 AM, Taylor Simpson wrote:
> +static abi_ulong get_sigframe(struct target_sigaction *ka,
> +                              CPUHexagonState *regs, size_t
> +framesize) {
> +    abi_ulong sp = get_sp_from_cpustate(regs);
> +
> +    /* This is the X/Open sanctioned signal stack switching.  */
> +    sp = target_sigsp(sp, ka) - framesize;
> +
> +    sp &= ~7UL; /* align sp on 8-byte boundary */

QEMU_ALIGN_DOWN.

> diff --git a/linux-user/hexagon/sockbits.h
> b/linux-user/hexagon/sockbits.h new file mode 100644 index
> 0000000..85bd64a
> --- /dev/null
> +++ b/linux-user/hexagon/sockbits.h
> @@ -0,0 +1,3 @@
> +/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights
> +Reserved. */
> +
> +#include "../generic/sockbits.h"

All new files should have denote their license.

> +static inline const char *cpu_get_model(uint32_t eflags) {
> +    /* For now, treat anything newer than v60 as a v67 */
> +    /* FIXME - Disable instructions that are newer than the specified arch */
> +    if (eflags == 0x04 ||    /* v5  */
> +        eflags == 0x05 ||    /* v55 */
> +        eflags == 0x60 ||    /* v60 */
> +        eflags == 0x61 ||    /* v61 */
> +        eflags == 0x62 ||    /* v62 */
> +        eflags == 0x65 ||    /* v65 */
> +        eflags == 0x66 ||    /* v66 */
> +        eflags == 0x67) {    /* v67 */
> +        return "v67";
> +    }
> +    printf("eflags = 0x%x\n", eflags);

Left over debug.

> diff --git a/target/hexagon/Makefile.objs
> b/target/hexagon/Makefile.objs new file mode 100644 index
> 0000000..dfab6ee
> --- /dev/null
> +++ b/target/hexagon/Makefile.objs
> @@ -0,0 +1,6 @@
> +obj-y += \
> +    cpu.o \
> +    translate.o \
> +    op_helper.o
> +
> +CFLAGS += -I$(SRC_PATH)/target/hexagon/imported

Is this really better than

#include "imported/global_types.h"

etc?

> +++ b/target/hexagon/cpu-param.h
> @@ -0,0 +1,11 @@
> +/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights
> +Reserved. */
> +
> +
> +#ifndef HEXAGON_CPU_PARAM_H
> +#define HEXAGON_CPU_PARAM_H
> +
> +# define TARGET_PHYS_ADDR_SPACE_BITS 36

Watch your spacing.

Does this really compile without TARGET_VIRT_ADDR_SPACE_BITS?

It's used in linux-user/main.c, but I suppose in a way that the preprocessor interprets it as 0.

> +const char * const hexagon_prednames[] = {
> +  "p0 ", "p1 ", "p2 ", "p3 "
> +};

Inter-string spacing probably belongs to the format not the name.
[Taylor] Could you elaborate?  Should I put spacing  after the commas?

> +static inline target_ulong hack_stack_ptrs(CPUHexagonState *env,
> +                                           target_ulong addr) {
> +    target_ulong stack_start = env->stack_start;
> +    target_ulong stack_size = 0x10000;
> +    target_ulong stack_adjust = env->stack_adjust;
> +
> +    if (stack_start + 0x1000 >= addr && addr >= (stack_start - stack_size)) {
> +        return addr - stack_adjust;
> +    }
> +    return addr;
> +}

An explanation would be welcome here.
[Taylor]  I will add a comment.  One of the main debugging techniques is to use "-d cpu" and compare against LLDB output when single stepping.  However, the target and qemu put the stacks in different locations.  This is used to compensate so the diff is cleaner.

> +static void hexagon_dump(CPUHexagonState *env, FILE *f) {
> +    static target_ulong last_pc;
> +    int i;
> +
> +    /*
> +     * When comparing with LLDB, it doesn't step through single-cycle
> +     * hardware loops the same way.  So, we just skip them here
> +     */
> +    if (env->gpr[HEX_REG_PC] == last_pc) {
> +        return;
> +    }

This seems mis-placed.
[Taylor] Hexagon has hardware controlled loops, so we can have a single packet that executes multiple times.  We don't want the dump to print every time.

> +#ifdef FIXME
> +    /*
> +     * LLDB bug swaps gp and ugp
> +     * FIXME when the LLDB bug is fixed
> +     */
> +    print_reg(f, env, HEX_REG_GP);
> +    print_reg(f, env, HEX_REG_UGP);
> +#else
> +    fprintf(f, "  %s = 0x" TARGET_FMT_lx "\n",
> +        hexagon_regnames[HEX_REG_GP],
> +        hack_stack_ptrs(env, env->gpr[HEX_REG_UGP]));
> +    fprintf(f, "  %s = 0x" TARGET_FMT_lx "\n",
> +        hexagon_regnames[HEX_REG_UGP],
> +        hack_stack_ptrs(env, env->gpr[HEX_REG_GP])); #endif
> +    print_reg(f, env, HEX_REG_PC);
> +#ifdef FIXME
> +    /*
> +     * Not modelled in qemu, print junk to minimize the diff's
> +     * with LLDB output
> +     */
> +    print_reg(f, env, HEX_REG_CAUSE);
> +    print_reg(f, env, HEX_REG_BADVA);
> +    print_reg(f, env, HEX_REG_CS0);
> +    print_reg(f, env, HEX_REG_CS1);
> +#else
> +    fprintf(f, "  cause = 0x000000db\n");
> +    fprintf(f, "  badva = 0x00000000\n");
> +    fprintf(f, "  cs0 = 0x00000000\n");
> +    fprintf(f, "  cs1 = 0x00000000\n"); #endif

Need we retain the fixme?
[Taylor] Those will be needed when we implement system mode.  So, I'll change the macro to CONFIG_USER_ONLY.

> +void hexagon_debug(CPUHexagonState *env) {
> +    hexagon_dump(env, stdout);
> +}

Is this to be called from the debugger?  From what location did you find it useful?  There are only certain locations in tcg that are self-consistent...
[Taylor] Yes, these are useful from the debugger.  I call it from a helper that takes CPUHexagonState as an argument.

> +    DEFINE_CPU(TYPE_HEXAGON_CPU_V67,              hexagon_v67_cpu_init),

Spacing?

> +#ifndef HEXAGON_CPU_H
> +#define HEXAGON_CPU_H
> +
> +/* FIXME - Change this to a command-line option */ #ifdef FIXME
> +#define DEBUG_HEX #endif #ifdef FIXME #define COUNT_HEX_HELPERS
> +#endif

Eh?

> +
> +/* Forward declaration needed by some of the header files */ typedef
> +struct CPUHexagonState CPUHexagonState;
> +
> +#include <fenv.h>
> +#include "qemu/osdep.h"

osdep.h should already have been included.
Indeed, it must be first for *.c files.

Why do you need fenv.h?

> +#include "global_types.h"
> +#include "max.h"
> +#include "iss_ver_registers.h"
> +
> +#define TARGET_PAGE_BITS 16     /* 64K pages */
> +/*
> + * For experimenting with oslib (4K pages)
> + * #define TARGET_PAGE_BITS 12
> + */
> +#define TARGET_LONG_BITS 32
> +#define TARGET_VIRT_ADDR_SPACE_BITS 32

Ah, to answer my earlier question, these belong to cpu-param.h.
Drop the "experimenting" comment.

> +
> +#include <time.h>

time.h is included by osdep.h.


> +#include "qemu/compiler.h"
> +#include "qemu-common.h"
> +#include "exec/cpu-defs.h"
> +#include "regs.h"
> +
> +#define TYPE_HEXAGON_CPU "hexagon-cpu"
> +
> +#define HEXAGON_CPU_TYPE_SUFFIX "-" TYPE_HEXAGON_CPU #define
> +HEXAGON_CPU_TYPE_NAME(name) (name HEXAGON_CPU_TYPE_SUFFIX) #define
> +CPU_RESOLVING_TYPE TYPE_HEXAGON_CPU
> +
> +#define TYPE_HEXAGON_CPU_V67             HEXAGON_CPU_TYPE_NAME("v67")
> +
> +#define MMU_USER_IDX 0
> +
> +#define TRANSLATE_FAIL 1
> +#define TRANSLATE_SUCCESS 0

What's this for?  This looks like it's cribbed from riscv, which oddly doesn't match the actual tlb_fill interface, which uses bool true for success.

> +
> +struct MemLog {
> +    vaddr_t va;
> +    size1u_t width;
> +    size4u_t data32;
> +    size8u_t data64;
> +};

Is this part of translation?  Maybe save this til you actually use it, and probably place in translate.h.
[Taylor] Good catch.  I can save them until they are actually used.  They are part of the execution state, so need to be in cpu.h though.

> +    /* For comparing with LLDB on target - see hack_stack_ptrs function */
> +    target_ulong stack_start;
> +    target_ulong stack_adjust;

Which, as you recall, doesn't actually have any commentary.

> +extern const char * const hexagon_regnames[]; extern const char *
> +const hexagon_prednames[];

Do these actually need declaring here?
Let's keep them private to cpu.c otherwise.

> +#define ENV_GET_CPU(e)  CPU(hexagon_env_get_cpu(e))
> +#define ENV_OFFSET      offsetof(HexagonCPU, env)

Obsolete. Remove.

> +int hexagon_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
> +int hexagon_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);

Move these decls to e.g. internal.h?
They're not relevant to generic users of cpu.h

> +void QEMU_NORETURN do_raise_exception_err(CPUHexagonState *env,
> +                                          uint32_t exception,
> +uintptr_t pc);

Likewise.

> +void hexagon_translate_init(void);
> +void hexagon_debug(CPUHexagonState *env); void
> +hexagon_debug_vreg(CPUHexagonState *env, int regnum); void
> +hexagon_debug_qreg(CPUHexagonState *env, int regnum);

Likewise.

> +#ifdef COUNT_HEX_HELPERS
> +extern void print_helper_counts(void); #endif

Likewise.

> +static void decode_packet(CPUHexagonState *env, DisasContext *ctx) {
> +    size4u_t words[4];
> +    int i;
> +    /* Brute force way to make sure current PC is set */
> +    tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->base.pc_next);

What's this for?
[Taylor] Honestly, I'm not sure.  If I remove it, nothing works - not even "hello world".

> +
> +    for (i = 0; i < 4; i++) {
> +        words[i] = cpu_ldl_code(env, ctx->base.pc_next + i * sizeof(size4u_t));
> +    }

And this?
[Taylor] It's reading from the instruction memory.  The switch statement below uses it.

> +    /*
> +     * Brute force just enough to get the first program to execute.
> +     */
> +    switch (words[0]) {
> +    case 0x7800c806:                                /* r6 = #64 */
> +        tcg_gen_movi_tl(hex_gpr[6], 64);
> +        ctx->base.pc_next += 4;
> +        break;
> +    case 0x7800c020:                                /* r0 = #1 */
> +        tcg_gen_movi_tl(hex_gpr[0], 1);
> +        ctx->base.pc_next += 4;
> +        break;
> +    case 0x00044002:
> +        if (words[1] == 0x7800c001) {               /* r1 = ##0x400080 */
> +            tcg_gen_movi_tl(hex_gpr[1], 0x400080);
> +            ctx->base.pc_next += 8;
> +        } else {
> +            printf("ERROR: Unknown instruction 0x%x\n", words[1]);
> +            g_assert_not_reached();
> +        }
> +        break;
> +    case 0x7800c0e2:                                /* r2 = #7 */
> +        tcg_gen_movi_tl(hex_gpr[2], 7);
> +        ctx->base.pc_next += 4;
> +        break;
> +    case 0x5400c004:                               /* trap0(#1) */
> +    {
> +        TCGv excp_trap0 = tcg_const_tl(HEX_EXCP_TRAP0);
> +        gen_helper_raise_exception(cpu_env, excp_trap0);
> +        tcg_temp_free(excp_trap0);
> +        ctx->base.pc_next += 4;
> +        break;
> +    }
> +    case 0x7800cbc6:                               /* r6 = #94 */
> +        tcg_gen_movi_tl(hex_gpr[6], 94);
> +        ctx->base.pc_next += 4;
> +        break;
> +    case 0x7800cba6:                               /* r6 = #93 */
> +        tcg_gen_movi_tl(hex_gpr[6], 93);
> +        ctx->base.pc_next += 4;
> +        break;
> +    case 0x7800c000:                               /* r0 = #0 */
> +        tcg_gen_movi_tl(hex_gpr[0], 0);
> +        ctx->base.pc_next += 4;
> +        break;
> +    default:
> +        ctx->base.is_jmp = DISAS_TOO_MANY;
> +        ctx->base.pc_next += 4;
> +    }

I'm not especially keen on this, since it will just be removed in subsequent patches.  The initial patch must compile, but it need not do *anything* interesting.

C.f. 61766fe9e2d3 which is the initial commit for target/hppa, wherein the decoder is

> +static ExitStatus translate_one(DisasContext *ctx, uint32_t insn) {
> +    uint32_t opc = extract32(insn, 26, 6);
> +
> +    switch (opc) {
> +    default:
> +        break;
> +    }
> +    return gen_illegal(ctx);
> +}



> +}
> +
> +static void hexagon_tr_init_disas_context(DisasContextBase *dcbase,
> +                                          CPUState *cs) {
> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +
> +    ctx->mem_idx = ctx->base.tb->flags & TB_FLAGS_MMU_MASK;

Since you're not initializing this in cpu_get_tb_cpu_state, you might as well just use

    ctx->mem_idx = MMU_USER_IDX;
[Taylor] Should I be initialize this in cpu_get_bt_cpu_state?

> +static void hexagon_tr_translate_packet(DisasContextBase *dcbase,
> +CPUState *cpu) {
> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +    CPUHexagonState *env = cpu->env_ptr;
> +
> +    decode_packet(env, ctx);
> +
> +    if (ctx->base.is_jmp == DISAS_NEXT) {
> +        target_ulong page_start;
> +
> +        page_start = ctx->base.pc_first & TARGET_PAGE_MASK;
> +        if (ctx->base.pc_next - page_start >= TARGET_PAGE_SIZE) {
> +            ctx->base.is_jmp = DISAS_TOO_MANY;
> +        }
> +
> +#ifdef DEBUG_HEX
> +        /* When debugging, force the end of the TB after each packet */
> +        if (ctx->base.pc_next - ctx->base.pc_first >= 0x04) {
> +            ctx->base.is_jmp = DISAS_TOO_MANY;
> +        }
> +#endif
> +    }

As mentioned elsewhere, this latter should be handled by -singlestep.  The generic translator loop will have set max_insns to 1.


r~
Richard Henderson Nov. 20, 2019, 7:49 a.m. UTC | #19
On 11/20/19 3:26 AM, Aleksandar Markovic wrote:
> 
> 
> On Tuesday, November 19, 2019, Richard Henderson <richard.henderson@linaro.org
> <mailto:richard.henderson@linaro.org>> wrote:
> 
>     On 11/19/19 6:22 PM, Taylor Simpson wrote:
>     > - Laurent suggested I split the patch into two parts: linux-user and
>     target/hexagon.  If I do that, which one should contain the changes to
>     common files (e.g., configure)?  Also, note that we won't be able to build
>     until both patches are merged.  Is that OK?
> 
>     The configure parts should be a third, last, patch.
> 
>     The series is bisectable, because before the configure patch,
>     none of the hexagon code is compiled at all.
> 
> 
> I don't think this is a good advice. Yes, at first glance, that would make the
> submitter's job easier - since he could divide the whole code into parts
> practicaly arbitrarily - but the resulting series will be of suboptimal
> quality.

How's that?  He has been asked to split the linux-user stuff from the target
skeleton stuff.  Neither of these will compile independently.  You could merge
the configure bits artificially into the second of the two patches, or you
could leave it separate.

> If the submitter was forced from the outset to add segments of his
> solution so that each step actually (not only ostensibly) compiles, he would
> also be forced to organize patches in much more coherent way, organize his code
> in much more modular way, possibly improve initial organization, additionally
> making reviews much easier.

This argument would make more sense if there were more present here than a
skeleton.  The vast majority of the work is yet to be submitted, and *will*
have to compile, be coherent, etc, just as you suggest.


r~
Richard Henderson Nov. 20, 2019, 8:06 a.m. UTC | #20
On 11/20/19 6:15 AM, Taylor Simpson wrote:
>> +const char * const hexagon_prednames[] = {
>> +  "p0 ", "p1 ", "p2 ", "p3 "
>> +};
> 
> Inter-string spacing probably belongs to the format not the name.
> [Taylor] Could you elaborate?  Should I put spacing  after the commas?

"p0" not "p0 ".  Typo on my part for "inter-"; sorry for the confusion.

>> +static inline target_ulong hack_stack_ptrs(CPUHexagonState *env,
>> +                                           target_ulong addr) {
>> +    target_ulong stack_start = env->stack_start;
>> +    target_ulong stack_size = 0x10000;
>> +    target_ulong stack_adjust = env->stack_adjust;
>> +
>> +    if (stack_start + 0x1000 >= addr && addr >= (stack_start - stack_size)) {
>> +        return addr - stack_adjust;
>> +    }
>> +    return addr;
>> +}
> 
> An explanation would be welcome here.
> [Taylor]  I will add a comment.  One of the main debugging techniques is to use "-d cpu" and compare against LLDB output when single stepping.  However, the target and qemu put the stacks in different locations.  This is used to compensate so the diff is cleaner.

Ug.  I understand why you want this, but... ug.


>> +static void hexagon_dump(CPUHexagonState *env, FILE *f) {
>> +    static target_ulong last_pc;
>> +    int i;
>> +
>> +    /*
>> +     * When comparing with LLDB, it doesn't step through single-cycle
>> +     * hardware loops the same way.  So, we just skip them here
>> +     */
>> +    if (env->gpr[HEX_REG_PC] == last_pc) {
>> +        return;
>> +    }
> 
> This seems mis-placed.
> [Taylor] Hexagon has hardware controlled loops, so we can have a single packet that executes multiple times.  We don't want the dump to print every time.

Certainly I do.  If I'm single-stepping, I want every packet present.  Just as
if this were written as a traditional loop, with a branch back to the single
packet in the loop body.

Also, you cannot expect a static variable to work in multi-threaded mode, and
you cannot expect a __thread variable to work in single-threaded round-robin mode.

>> +static void decode_packet(CPUHexagonState *env, DisasContext *ctx) {
>> +    size4u_t words[4];
>> +    int i;
>> +    /* Brute force way to make sure current PC is set */
>> +    tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->base.pc_next);
> 
> What's this for?
> [Taylor] Honestly, I'm not sure.  If I remove it, nothing works - not even "hello world".

Weird.  I would have expected that the update you do within hexagon_tr_tb_stop
would be sufficient.  I guess I'll have to peek at it.

I presume your minimal test case is a bit of hand-crafted assembly that issues
a write syscall and exit?

>> +
>> +    for (i = 0; i < 4; i++) {
>> +        words[i] = cpu_ldl_code(env, ctx->base.pc_next + i * sizeof(size4u_t));
>> +    }
> 
> And this?
> [Taylor] It's reading from the instruction memory.  The switch statement below uses it.

I thought packets are variable length and ended by a bit set.  Therefore why
the fixed iteration to 4?  Also...

> 
>> +    /*
>> +     * Brute force just enough to get the first program to execute.
>> +     */
>> +    switch (words[0]) {

... you only use 1 word, but you read 4.

>> +static void hexagon_tr_init_disas_context(DisasContextBase *dcbase,
>> +                                          CPUState *cs) {
>> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
>> +
>> +    ctx->mem_idx = ctx->base.tb->flags & TB_FLAGS_MMU_MASK;
> 
> Since you're not initializing this in cpu_get_tb_cpu_state, you might as well just use
> 
>     ctx->mem_idx = MMU_USER_IDX;
> [Taylor] Should I be initialize this in cpu_get_bt_cpu_state?

Not until there's something more interesting to put there, when you implement
system state.  The constant initialization should be fine.


r~
Laurent Vivier Nov. 20, 2019, 8:33 a.m. UTC | #21
Le 20/11/2019 à 05:48, Taylor Simpson a écrit :
> For the general DEBUG_HEX messages, I think the trace infrastructure isn't quite what I'm looking for.
> 
> Here's a sample of what it prints
> Start packet: pc = 0x4002f0
> Packet committed: pc = 0x4002f0
> Regs written
>         r18 = -69420 (0xfffef0d4)
>         r19 = -69412 (0xfffef0dc)
> Stores
>         memd[0xfffef0b0] = 0 (0x0000000000000000)
> Next PC = 0x4002f8
> Exec counters: pkt = 17, insn = 24, hvx = 0
> 
> For performance, I'd like a way to disable them completely when --enable-debug isn't passed to configure.  It looks like #ifdef CONFIG_DEBUG_TCG will work for this.  Then, I'd like a command-line option to turn them on.  The "-d exec" seems like a reasonable place.  So, the code will look like this
> 
> #ifdef CONFIG_DEBUG_TCG
>     if (qemu_loglevel_maks(CPU_LOG_EXEC)) {
>         qemu_log(...);
>     }
> #endif
> 
> Please let me know if I'm on the right track here.
> 

I'm not sure CONFIG_DEBUG_TCG is the good choice (Richard should
know...), you should keep your DEBUG_HEX in this case.

But the performance impact should not be big without the #ifdef as
qemu_loglevel_maks() is a simple test and the rest of the code is not
executed (Perhaps an "unlikely(qemu_loglevel_maks(CPU_LOG_EXEC))" could
help).

Thanks,
Laurent
Laurent Vivier Nov. 20, 2019, 8:41 a.m. UTC | #22
Le 20/11/2019 à 03:26, Aleksandar Markovic a écrit :
> 
> 
> On Tuesday, November 19, 2019, Richard Henderson
> <richard.henderson@linaro.org <mailto:richard.henderson@linaro.org>> wrote:
> 
>     On 11/19/19 6:22 PM, Taylor Simpson wrote:
>     > - Laurent suggested I split the patch into two parts: linux-user
>     and target/hexagon.  If I do that, which one should contain the
>     changes to common files (e.g., configure)?  Also, note that we won't
>     be able to build until both patches are merged.  Is that OK?
> 
>     The configure parts should be a third, last, patch.
> 
>     The series is bisectable, because before the configure patch,
>     none of the hexagon code is compiled at all.
> 
> 
> I don't think this is a good advice. Yes, at first glance, that would
> make the submitter's job easier - since he could divide the whole code
> into parts practicaly arbitrarily - but the resulting series will be of
> suboptimal quality. If the submitter was forced from the outset to add
> segments of his solution so that each step actually (not only
> ostensibly) compiles, he would also be forced to organize patches in
> much more coherent way, organize his code in much more modular way,
> possibly improve initial organization, additionally making reviews much
> easier.

I totally agree with you but in this case I don't see a simple solution
to be able to compile each part separately.

If there will be problems in the way the code is divided in patches, we
can point out the problem during review.

Thanks,
Laurent
Richard Henderson Nov. 20, 2019, 9:02 a.m. UTC | #23
On 11/20/19 9:33 AM, Laurent Vivier wrote:
> Le 20/11/2019 à 05:48, Taylor Simpson a écrit :
>> For the general DEBUG_HEX messages, I think the trace infrastructure isn't quite what I'm looking for.
>>
>> Here's a sample of what it prints
>> Start packet: pc = 0x4002f0
>> Packet committed: pc = 0x4002f0
>> Regs written
>>         r18 = -69420 (0xfffef0d4)
>>         r19 = -69412 (0xfffef0dc)
>> Stores
>>         memd[0xfffef0b0] = 0 (0x0000000000000000)
>> Next PC = 0x4002f8
>> Exec counters: pkt = 17, insn = 24, hvx = 0

For something like this, I'd keep DEBUG_HEX.

>>     if (qemu_loglevel_maks(CPU_LOG_EXEC)) {

CPU_LOG_EXEC already has a specific structure, listing
the TranslationBlocks that are executed.  It shouldn't
be hijacked for something else.

If you really want a runtime flag for this, we should
add a new CPU_LOG_* flag.


r~
Taylor Simpson Nov. 20, 2019, 12:51 p.m. UTC | #24
Responses inline ...

Thanks,
Taylor


-----Original Message-----
From: Richard Henderson <richard.henderson@linaro.org>
Sent: Wednesday, November 20, 2019 2:07 AM
To: Taylor Simpson <tsimpson@quicinc.com>; laurent@vivier.eu; riku.voipio@iki.fi; qemu-devel@nongnu.org
Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards

-------------------------------------------------------------------------
CAUTION: This email originated from outside of the organization.
-------------------------------------------------------------------------

On 11/20/19 6:15 AM, Taylor Simpson wrote:
>> +const char * const hexagon_prednames[] = {
>> +  "p0 ", "p1 ", "p2 ", "p3 "
>> +};
>
> Inter-string spacing probably belongs to the format not the name.
> [Taylor] Could you elaborate?  Should I put spacing  after the commas?

"p0" not "p0 ".  Typo on my part for "inter-"; sorry for the confusion.

>> +static inline target_ulong hack_stack_ptrs(CPUHexagonState *env,
>> +                                           target_ulong addr) {
>> +    target_ulong stack_start = env->stack_start;
>> +    target_ulong stack_size = 0x10000;
>> +    target_ulong stack_adjust = env->stack_adjust;
>> +
>> +    if (stack_start + 0x1000 >= addr && addr >= (stack_start - stack_size)) {
>> +        return addr - stack_adjust;
>> +    }
>> +    return addr;
>> +}
>
> An explanation would be welcome here.
> [Taylor]  I will add a comment.  One of the main debugging techniques is to use "-d cpu" and compare against LLDB output when single stepping.  However, the target and qemu put the stacks in different locations.  This is used to compensate so the diff is cleaner.

Ug.  I understand why you want this, but... ug.

[Taylor] Not sure what to say - I guess there's a fine line between elegance and mayhem.


>> +static void hexagon_dump(CPUHexagonState *env, FILE *f) {
>> +    static target_ulong last_pc;
>> +    int i;
>> +
>> +    /*
>> +     * When comparing with LLDB, it doesn't step through single-cycle
>> +     * hardware loops the same way.  So, we just skip them here
>> +     */
>> +    if (env->gpr[HEX_REG_PC] == last_pc) {
>> +        return;
>> +    }
>
> This seems mis-placed.
> [Taylor] Hexagon has hardware controlled loops, so we can have a single packet that executes multiple times.  We don't want the dump to print every time.

Certainly I do.  If I'm single-stepping, I want every packet present.  Just as if this were written as a traditional loop, with a branch back to the single packet in the loop body.

Also, you cannot expect a static variable to work in multi-threaded mode, and you cannot expect a __thread variable to work in single-threaded round-robin mode.

[Taylor] This is another place where it's important to match the output of LLDB for a clean diff.  In fact, LLDB uses hardware single step, and that will step to the end of a single-cycle hardware loop.  Also, the technique of comparing with LLDB only works for single threaded examples, so I'm not worried about the thread-safety of this code.

[Taylor] To clarify, this is for a rare case when a hardware loop body is a single packet.  Here's an example.  It's the portion of memset that gets called when the number of bytes is small
  400404:       10 40 02 60     60024010 {      loop0(0x40040c,r2)
  400408:       08 c0 02 10     1002c008        p0 = cmp.eq(r2,#0); if (p0.new) jump:nt 0x400414 <memset+0x34> }
  40040c:       01 81 03 a1     a1038101 {      memb(r3+#1) = r1
  400410:       10 c1 03 ab     ab03c110        memb(r3++#2) = r1 }  :endloop0
The loop0 instruction sets the start address and the iteration count.  The :endloop0 marks the end of the loop and tells the hardware to decrement the counter and go back to the start if it's not zero.  So, the loop at 0x40040c-0x400410 is a single packet.  In this case the hardware single step will execute the entire loop.  Finally, if the loop has more than one packet, the single stepping will go through each iteration as you describe.

>> +static void decode_packet(CPUHexagonState *env, DisasContext *ctx) {
>> +    size4u_t words[4];
>> +    int i;
>> +    /* Brute force way to make sure current PC is set */
>> +    tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->base.pc_next);
>
> What's this for?
> [Taylor] Honestly, I'm not sure.  If I remove it, nothing works - not even "hello world".

Weird.  I would have expected that the update you do within hexagon_tr_tb_stop would be sufficient.  I guess I'll have to peek at it.

I presume your minimal test case is a bit of hand-crafted assembly that issues a write syscall and exit?

[Taylor] The minimal test case is
#define SYS_write 64
#define SYS_exit_group           94
#define SYS_exit                 93

#define FD_STDOUT                1

.typestr,@object
.section.rodata
str:
.string"Hello!\n"
.sizestr, 8

.text
.global _start
_start:
r6 = #SYS_write
r0 = #FD_STDOUT
r1 = ##str
r2 = #7
trap0(#1)

r6 = #SYS_exit_group
trap0(#1)

r6 = #SYS_exit
r0 = #0
trap0(#1)
Without that code, it prints "Hello!" twice.  With the full set of patches, I get segfaults ☹

>> +
>> +    for (i = 0; i < 4; i++) {
>> +        words[i] = cpu_ldl_code(env, ctx->base.pc_next + i * sizeof(size4u_t));
>> +    }
>
> And this?
> [Taylor] It's reading from the instruction memory.  The switch statement below uses it.

I thought packets are variable length and ended by a bit set.  Therefore why the fixed iteration to 4?  Also...


[Taylor] The maximum size of a packet is 4 words, so I go ahead and read that much.  Once the packet is decoded, I set ctx->base.pc_next appropriately.  Note that most of the instructions in the switch add 4, but one of them adds 8.

>
>> +    /*
>> +     * Brute force just enough to get the first program to execute.
>> +     */
>> +    switch (words[0]) {

... you only use 1 word, but you read 4.

>> +static void hexagon_tr_init_disas_context(DisasContextBase *dcbase,
>> +                                          CPUState *cs) {
>> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
>> +
>> +    ctx->mem_idx = ctx->base.tb->flags & TB_FLAGS_MMU_MASK;
>
> Since you're not initializing this in cpu_get_tb_cpu_state, you might
> as well just use
>
>     ctx->mem_idx = MMU_USER_IDX;
> [Taylor] Should I be initialize this in cpu_get_bt_cpu_state?

Not until there's something more interesting to put there, when you implement system state.  The constant initialization should be fine.


r~
Taylor Simpson Nov. 20, 2019, 12:58 p.m. UTC | #25
Is there a precedent for this?  I'm OK with DEBUG_HEX, but I assumed reviewers wouldn't approve
#ifdef FIXME
#define DEBUG_HEX
#endif

Taylor


-----Original Message-----
From: Richard Henderson <richard.henderson@linaro.org>
Sent: Wednesday, November 20, 2019 3:02 AM
To: Laurent Vivier <laurent@vivier.eu>; Taylor Simpson <tsimpson@quicinc.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; riku.voipio@iki.fi; qemu-devel@nongnu.org
Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards


On 11/20/19 9:33 AM, Laurent Vivier wrote:
> Le 20/11/2019 à 05:48, Taylor Simpson a écrit :
>> For the general DEBUG_HEX messages, I think the trace infrastructure isn't quite what I'm looking for.
>>
>> Here's a sample of what it prints
>> Start packet: pc = 0x4002f0
>> Packet committed: pc = 0x4002f0
>> Regs written
>>         r18 = -69420 (0xfffef0d4)
>>         r19 = -69412 (0xfffef0dc)
>> Stores
>>         memd[0xfffef0b0] = 0 (0x0000000000000000) Next PC = 0x4002f8
>> Exec counters: pkt = 17, insn = 24, hvx = 0

For something like this, I'd keep DEBUG_HEX.

>>     if (qemu_loglevel_maks(CPU_LOG_EXEC)) {

CPU_LOG_EXEC already has a specific structure, listing the TranslationBlocks that are executed.  It shouldn't be hijacked for something else.

If you really want a runtime flag for this, we should add a new CPU_LOG_* flag.


r~
Laurent Vivier Nov. 20, 2019, 2:14 p.m. UTC | #26
Le 20/11/2019 à 13:58, Taylor Simpson a écrit :
> Is there a precedent for this?  I'm OK with DEBUG_HEX, but I assumed reviewers wouldn't approve
> #ifdef FIXME
> #define DEBUG_HEX
> #endif


For instance, in target/mips/translate.c you have:

     42
     43 #define MIPS_DEBUG_DISAS 0
     44
...
   2603 #define LOG_DISAS(...)  \
   2604     do { \
   2605         if (MIPS_DEBUG_DISAS) { \
   2606             qemu_log_mask(CPU_LOG_TB_IN_ASM, ## __VA_ARGS__);\
   2607         }  \
   2608     } while (0)
   2609
...

  10121     LOG_DISAS("mftr (reg %d u %d sel %d h %d)\n", rt, u, sel,
h);
...

For the linux-user part, I don't think you need the DEBUG_HEX traces.

Thanks,
Laurent
> 
> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Wednesday, November 20, 2019 3:02 AM
> To: Laurent Vivier <laurent@vivier.eu>; Taylor Simpson <tsimpson@quicinc.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; riku.voipio@iki.fi; qemu-devel@nongnu.org
> Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards
> 
> 
> On 11/20/19 9:33 AM, Laurent Vivier wrote:
>> Le 20/11/2019 à 05:48, Taylor Simpson a écrit :
>>> For the general DEBUG_HEX messages, I think the trace infrastructure isn't quite what I'm looking for.
>>>
>>> Here's a sample of what it prints
>>> Start packet: pc = 0x4002f0
>>> Packet committed: pc = 0x4002f0
>>> Regs written
>>>         r18 = -69420 (0xfffef0d4)
>>>         r19 = -69412 (0xfffef0dc)
>>> Stores
>>>         memd[0xfffef0b0] = 0 (0x0000000000000000) Next PC = 0x4002f8
>>> Exec counters: pkt = 17, insn = 24, hvx = 0
> 
> For something like this, I'd keep DEBUG_HEX.
> 
>>>     if (qemu_loglevel_maks(CPU_LOG_EXEC)) {
> 
> CPU_LOG_EXEC already has a specific structure, listing the TranslationBlocks that are executed.  It shouldn't be hijacked for something else.
> 
> If you really want a runtime flag for this, we should add a new CPU_LOG_* flag.
> 
> 
> r~
>
Richard Henderson Nov. 20, 2019, 2:43 p.m. UTC | #27
On 11/20/19 1:51 PM, Taylor Simpson wrote:
>>> +static inline target_ulong hack_stack_ptrs(CPUHexagonState *env,
>>> +                                           target_ulong addr) {
>>> +    target_ulong stack_start = env->stack_start;
>>> +    target_ulong stack_size = 0x10000;
>>> +    target_ulong stack_adjust = env->stack_adjust;
>>> +
>>> +    if (stack_start + 0x1000 >= addr && addr >= (stack_start - stack_size)) {
>>> +        return addr - stack_adjust;
>>> +    }
>>> +    return addr;
>>> +}
>>
>> An explanation would be welcome here.
>> [Taylor]  I will add a comment.  One of the main debugging techniques is to use "-d cpu" and compare against LLDB output when single stepping.  However, the target and qemu put the stacks in different locations.  This is used to compensate so the diff is cleaner.
> 
> Ug.  I understand why you want this, but... ug.
> 
> [Taylor] Not sure what to say - I guess there's a fine line between elegance and mayhem.

The hack may work for now, but I would certainly expect this code to go away
eventually.  Sooner rather than later.

If you want dumps between LLDB and QEMU to match, then just put the stack at
the same spot.  Messing about with the pointers after the fact is insane.


> [Taylor] To clarify, this is for a rare case when a hardware loop body is a single packet.  Here's an example.  It's the portion of memset that gets called when the number of bytes is small
>   400404:       10 40 02 60     60024010 {      loop0(0x40040c,r2)
>   400408:       08 c0 02 10     1002c008        p0 = cmp.eq(r2,#0); if (p0.new) jump:nt 0x400414 <memset+0x34> }
>   40040c:       01 81 03 a1     a1038101 {      memb(r3+#1) = r1
>   400410:       10 c1 03 ab     ab03c110        memb(r3++#2) = r1 }  :endloop0
> The loop0 instruction sets the start address and the iteration count.  The :endloop0 marks the end of the loop and tells the hardware to decrement the counter and go back to the start if it's not zero.  So, the loop at 0x40040c-0x400410 is a single packet.  In this case the hardware single step will execute the entire loop.  Finally, if the loop has more than one packet, the single stepping will go through each iteration as you describe.

The behaviour inconsistency between a single packet
loop and a two packet loop sounds to me like a HW bug.
Perhaps of the WONTFIX variety, but still...


>>> +static void decode_packet(CPUHexagonState *env, DisasContext *ctx) {
>>> +    size4u_t words[4];
>>> +    int i;
>>> +    /* Brute force way to make sure current PC is set */
>>> +    tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->base.pc_next);
>>
>> What's this for?
>> [Taylor] Honestly, I'm not sure.  If I remove it, nothing works - not even "hello world".

It's because you forgot to update PC before raising the exception for trap #0.
Which also ought to set DISAS_NORETURN to end the TB, since it longjmp's out.


>>> +    for (i = 0; i < 4; i++) {
>>> +        words[i] = cpu_ldl_code(env, ctx->base.pc_next + i * sizeof(size4u_t));
>>> +    }
>>
>> And this?
>> [Taylor] It's reading from the instruction memory.  The switch statement below uses it.
> 
> I thought packets are variable length and ended by a bit set.  Therefore why the fixed iteration to 4?  Also...
> 
> 
> [Taylor] The maximum size of a packet is 4 words, so I go ahead and read that much.  Once the packet is decoded, I set ctx->base.pc_next appropriately.  Note that most of the instructions in the switch add 4, but one of them adds 8.

This is where you will need to take more care wrt the end of the packet and the
end of the page.  I would expect something like

    max = -(dc->base.pc_next | TARGET_PAGE_MASK) / 4;
    if (max < 4) {
        /* this packet *might* cross a page boundary */
        if (dc->base.num_insns == 1) {
            /* this is the first packet in the TB -- allow it */
            max = 4;
        }
    } else {
        max = 4;
    }

    found_end = false;
    for (i = 0; i < 4; i++) {
        words[i] = load(...);
        if (end_of_packet(words[i])) {
            max = i + 1;
            found_end = true;
            break;
        }
    }
    if (!found_end) {
        if (i == 4) {
            /* illegal packet -- read all 4 without an end */
            gen_illegal_insn_or_whatever();
            dc->base.is_jmp = DISAS_NORETURN;
            return;
        }
        /* the packet crosses a page boundary -- defer to next tb */
        dc->base.is_jmp = DISAS_TOO_MANY;
        return;
    }

    ...

    case insn that needs two words:
        assert(i + 2 <= max);
        ...


r~
Taylor Simpson Nov. 20, 2019, 3:17 p.m. UTC | #28
Are you saying there's a way to tell qemu to put the stack at a certain location?  Or are you suggesting we do something on the hardware side?

Taylor


-----Original Message-----
From: Richard Henderson <richard.henderson@linaro.org>
Sent: Wednesday, November 20, 2019 8:43 AM
To: Taylor Simpson <tsimpson@quicinc.com>; laurent@vivier.eu; riku.voipio@iki.fi; qemu-devel@nongnu.org
Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards

-------------------------------------------------------------------------
CAUTION: This email originated from outside of the organization.
-------------------------------------------------------------------------

On 11/20/19 1:51 PM, Taylor Simpson wrote:
>>> +static inline target_ulong hack_stack_ptrs(CPUHexagonState *env,
>>> +                                           target_ulong addr) {
>>> +    target_ulong stack_start = env->stack_start;
>>> +    target_ulong stack_size = 0x10000;
>>> +    target_ulong stack_adjust = env->stack_adjust;
>>> +
>>> +    if (stack_start + 0x1000 >= addr && addr >= (stack_start - stack_size)) {
>>> +        return addr - stack_adjust;
>>> +    }
>>> +    return addr;
>>> +}
>>
>> An explanation would be welcome here.
>> [Taylor]  I will add a comment.  One of the main debugging techniques is to use "-d cpu" and compare against LLDB output when single stepping.  However, the target and qemu put the stacks in different locations.  This is used to compensate so the diff is cleaner.
>
> Ug.  I understand why you want this, but... ug.
>
> [Taylor] Not sure what to say - I guess there's a fine line between elegance and mayhem.

The hack may work for now, but I would certainly expect this code to go away eventually.  Sooner rather than later.

If you want dumps between LLDB and QEMU to match, then just put the stack at the same spot.  Messing about with the pointers after the fact is insane.


> [Taylor] To clarify, this is for a rare case when a hardware loop body is a single packet.  Here's an example.  It's the portion of memset that gets called when the number of bytes is small
>   400404:       10 40 02 60     60024010 {      loop0(0x40040c,r2)
>   400408:       08 c0 02 10     1002c008        p0 = cmp.eq(r2,#0); if (p0.new) jump:nt 0x400414 <memset+0x34> }
>   40040c:       01 81 03 a1     a1038101 {      memb(r3+#1) = r1
>   400410:       10 c1 03 ab     ab03c110        memb(r3++#2) = r1 }  :endloop0
> The loop0 instruction sets the start address and the iteration count.  The :endloop0 marks the end of the loop and tells the hardware to decrement the counter and go back to the start if it's not zero.  So, the loop at 0x40040c-0x400410 is a single packet.  In this case the hardware single step will execute the entire loop.  Finally, if the loop has more than one packet, the single stepping will go through each iteration as you describe.

The behaviour inconsistency between a single packet loop and a two packet loop sounds to me like a HW bug.
Perhaps of the WONTFIX variety, but still...


>>> +static void decode_packet(CPUHexagonState *env, DisasContext *ctx) {
>>> +    size4u_t words[4];
>>> +    int i;
>>> +    /* Brute force way to make sure current PC is set */
>>> +    tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->base.pc_next);
>>
>> What's this for?
>> [Taylor] Honestly, I'm not sure.  If I remove it, nothing works - not even "hello world".

It's because you forgot to update PC before raising the exception for trap #0.
Which also ought to set DISAS_NORETURN to end the TB, since it longjmp's out.


>>> +    for (i = 0; i < 4; i++) {
>>> +        words[i] = cpu_ldl_code(env, ctx->base.pc_next + i * sizeof(size4u_t));
>>> +    }
>>
>> And this?
>> [Taylor] It's reading from the instruction memory.  The switch statement below uses it.
>
> I thought packets are variable length and ended by a bit set.  Therefore why the fixed iteration to 4?  Also...
>
>
> [Taylor] The maximum size of a packet is 4 words, so I go ahead and read that much.  Once the packet is decoded, I set ctx->base.pc_next appropriately.  Note that most of the instructions in the switch add 4, but one of them adds 8.

This is where you will need to take more care wrt the end of the packet and the end of the page.  I would expect something like

    max = -(dc->base.pc_next | TARGET_PAGE_MASK) / 4;
    if (max < 4) {
        /* this packet *might* cross a page boundary */
        if (dc->base.num_insns == 1) {
            /* this is the first packet in the TB -- allow it */
            max = 4;
        }
    } else {
        max = 4;
    }

    found_end = false;
    for (i = 0; i < 4; i++) {
        words[i] = load(...);
        if (end_of_packet(words[i])) {
            max = i + 1;
            found_end = true;
            break;
        }
    }
    if (!found_end) {
        if (i == 4) {
            /* illegal packet -- read all 4 without an end */
            gen_illegal_insn_or_whatever();
            dc->base.is_jmp = DISAS_NORETURN;
            return;
        }
        /* the packet crosses a page boundary -- defer to next tb */
        dc->base.is_jmp = DISAS_TOO_MANY;
        return;
    }

    ...

    case insn that needs two words:
        assert(i + 2 <= max);
        ...


r~
Taylor Simpson Nov. 20, 2019, 3:19 p.m. UTC | #29
OK, I'll modify to follow the mips approach.

Taylor


-----Original Message-----
From: Laurent Vivier <laurent@vivier.eu>
Sent: Wednesday, November 20, 2019 8:15 AM
To: Taylor Simpson <tsimpson@quicinc.com>; Richard Henderson <richard.henderson@linaro.org>; Philippe Mathieu-Daudé <philmd@redhat.com>; riku.voipio@iki.fi; qemu-devel@nongnu.org
Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards

-------------------------------------------------------------------------
CAUTION: This email originated from outside of the organization.
-------------------------------------------------------------------------

Le 20/11/2019 à 13:58, Taylor Simpson a écrit :
> Is there a precedent for this?  I'm OK with DEBUG_HEX, but I assumed
> reviewers wouldn't approve #ifdef FIXME #define DEBUG_HEX #endif


For instance, in target/mips/translate.c you have:

     42
     43 #define MIPS_DEBUG_DISAS 0
     44
...
   2603 #define LOG_DISAS(...)  \
   2604     do { \
   2605         if (MIPS_DEBUG_DISAS) { \
   2606             qemu_log_mask(CPU_LOG_TB_IN_ASM, ## __VA_ARGS__);\
   2607         }  \
   2608     } while (0)
   2609
...

  10121     LOG_DISAS("mftr (reg %d u %d sel %d h %d)\n", rt, u, sel,
h);
...

For the linux-user part, I don't think you need the DEBUG_HEX traces.

Thanks,
Laurent
>
> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Wednesday, November 20, 2019 3:02 AM
> To: Laurent Vivier <laurent@vivier.eu>; Taylor Simpson
> <tsimpson@quicinc.com>; Philippe Mathieu-Daudé <philmd@redhat.com>;
> riku.voipio@iki.fi; qemu-devel@nongnu.org
> Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of
> patches - linux-user changes + linux-user/hexagon + skeleton of
> target/hexagon - Files in target/hexagon/imported are from another
> project and therefore do not conform to qemu coding standards
>
>
> On 11/20/19 9:33 AM, Laurent Vivier wrote:
>> Le 20/11/2019 à 05:48, Taylor Simpson a écrit :
>>> For the general DEBUG_HEX messages, I think the trace infrastructure isn't quite what I'm looking for.
>>>
>>> Here's a sample of what it prints
>>> Start packet: pc = 0x4002f0
>>> Packet committed: pc = 0x4002f0
>>> Regs written
>>>         r18 = -69420 (0xfffef0d4)
>>>         r19 = -69412 (0xfffef0dc)
>>> Stores
>>>         memd[0xfffef0b0] = 0 (0x0000000000000000) Next PC = 0x4002f8
>>> Exec counters: pkt = 17, insn = 24, hvx = 0
>
> For something like this, I'd keep DEBUG_HEX.
>
>>>     if (qemu_loglevel_maks(CPU_LOG_EXEC)) {
>
> CPU_LOG_EXEC already has a specific structure, listing the TranslationBlocks that are executed.  It shouldn't be hijacked for something else.
>
> If you really want a runtime flag for this, we should add a new CPU_LOG_* flag.
>
>
> r~
>
Alex Bennée Nov. 20, 2019, 4:40 p.m. UTC | #30
Taylor Simpson <tsimpson@quicinc.com> writes:

> Is there a precedent for this?  I'm OK with DEBUG_HEX, but I assumed reviewers wouldn't approve
> #ifdef FIXME
> #define DEBUG_HEX
> #endif

We are trying to phase out this style of #define as it's prone to
bitrot. What you can do if define a debug helper like:

#define D(fmt, ..) \
  if (DEBUG_HEXAGON) { \
      qemu_log(fmt, ## __VA_ARGS__); \
  }

So the debug will still be built (and format strings checked etc) but
with DEBUG_HEXAGON as 0 things get dead coded away.

>
> Taylor
>
>
> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Wednesday, November 20, 2019 3:02 AM
> To: Laurent Vivier <laurent@vivier.eu>; Taylor Simpson <tsimpson@quicinc.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; riku.voipio@iki.fi; qemu-devel@nongnu.org
> Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards
>
>
> On 11/20/19 9:33 AM, Laurent Vivier wrote:
>> Le 20/11/2019 à 05:48, Taylor Simpson a écrit :
>>> For the general DEBUG_HEX messages, I think the trace infrastructure isn't quite what I'm looking for.
>>>
>>> Here's a sample of what it prints
>>> Start packet: pc = 0x4002f0
>>> Packet committed: pc = 0x4002f0
>>> Regs written
>>>         r18 = -69420 (0xfffef0d4)
>>>         r19 = -69412 (0xfffef0dc)
>>> Stores
>>>         memd[0xfffef0b0] = 0 (0x0000000000000000) Next PC = 0x4002f8
>>> Exec counters: pkt = 17, insn = 24, hvx = 0
>
> For something like this, I'd keep DEBUG_HEX.
>
>>>     if (qemu_loglevel_maks(CPU_LOG_EXEC)) {
>
> CPU_LOG_EXEC already has a specific structure, listing the TranslationBlocks that are executed.  It shouldn't be hijacked for something else.
>
> If you really want a runtime flag for this, we should add a new CPU_LOG_* flag.
>
>
> r~


--
Alex Bennée
Philippe Mathieu-Daudé Nov. 20, 2019, 5:09 p.m. UTC | #31
On 11/19/19 7:13 PM, Laurent Vivier wrote:
> Le 19/11/2019 à 18:22, Taylor Simpson a écrit :
>> Thanks for all the feedback on the patch.  I'll summarize my TODO list here.  Please let me know if there's anything I missed.
>> - Add a README file in the imported directory to make it clear that the code comes from another project.  Personally, I prefer keeping the name as "imported".  It was suggested by Richard at the meeting.  Also as a heads-up, that is a small subset of the files that will be in that directory eventually.  Right now, it is the minimum needed to build the skeleton target.
>> - Work on the .checkpatchignore as Philippe suggested.
>> - Split out the "[__SIGRTMAX - 1] = __SIGRTMIN + 1" into a separate patch.
>> - Clean up the long subject line.
>> - Add license text to the new files.
>> - Remove the DEBUG_HEX blocks.  In general the DEBUG_HEX macro controls a bunch of debugging output as you'll see in later patches.  In the long run, I think it should be replaces with a macro that is defined when configured with --enable-debug and then an additional command-line argument.  I haven't looked into this, so any pointers would be appreciated.
> 
> You can have a look to the trace infrastructure
> (docs/devel/tracing.txt). We have also some qemu_log() macros for low
> level debugging.
> 
>> - Laurent suggested I split the patch into two parts: linux-user and target/hexagon.  If I do that, which one should contain the changes to common files (e.g., configure)?  Also, note that we won't be able to build until both patches are merged.  Is that OK?
> 
> You should add target/hexagon first, and it should not be build as we
> don't have any target (hexagon-linux-user or hexagon-softmmu),
> then you can add linux-user part that will be built and use the
> target/hexagone CPU. I think the configure part should go to the
> linux-user part as it enables the build.

If you have a CPU in target/hexagone, you should be able to link 
hexagon-softmmu and run 'hexagon-softmmu -M none -m $SIZE -S -s', this 
starts a CPU with $SIZE MiB of ram mapped at 0, and wait for gdb on port 
1234. From here you can use 'load binary' to load some code in memory, 
set breakpoints, and test your CPU (and TCG).

Then I'd continue with linux-user. If you already have it, that's easier.

> 
> I asked to split the patch for review purpose, but this should not break
> anything (to allow bisect).
> 
> Thanks,
> Laurent
Alex Bennée Nov. 20, 2019, 5:34 p.m. UTC | #32
Aleksandar Markovic <aleksandar.m.mail@gmail.com> writes:

> On Tuesday, November 19, 2019, Richard Henderson <
> richard.henderson@linaro.org> wrote:
>
>> On 11/19/19 6:22 PM, Taylor Simpson wrote:
>> > - Laurent suggested I split the patch into two parts: linux-user and
>> target/hexagon.  If I do that, which one should contain the changes to
>> common files (e.g., configure)?  Also, note that we won't be able to build
>> until both patches are merged.  Is that OK?
>>
>> The configure parts should be a third, last, patch.
>>
>> The series is bisectable, because before the configure patch,
>> none of the hexagon code is compiled at all.
>>
>>
> I don't think this is a good advice. Yes, at first glance, that would make
> the submitter's job easier - since he could divide the whole code into
> parts practicaly arbitrarily - but the resulting series will be of
> suboptimal quality. If the submitter was forced from the outset to add
> segments of his solution so that each step actually (not only ostensibly)
> compiles, he would also be forced to organize patches in much more coherent
> way, organize his code in much more modular way, possibly improve initial
> organization, additionally making reviews much easier.

You can do what I did for the TCG plugins which is put the machinery
patches at the start (or near the start) while developing but move it to
the end when it comes to the final series and PR.

It's fairly easy to move patches around.

>
> Aleksandar
>
>
>>
>> r~
>>
>>


--
Alex Bennée
Aleksandar Markovic Nov. 21, 2019, 6:01 a.m. UTC | #33
On Wed, Nov 20, 2019 at 8:49 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> How's that?  He has been asked to split the linux-user stuff from the target
> skeleton stuff.

...

> This argument would make more sense if there were more present here than a
> skeleton.

Speaking about anatomy, I am opposed to upstreaming any "skeletons".
The other month, another community was dead serious wanting to
upstream code based on "proposal of the draft" (or was it "draft of
the proposal"), and now we want to upstream "skeletons"??

And even that "skeleton" can't be regularly built stage by stage, but
must resort to "enable configure at the end" cheap tricks?

What happened to QEMU upstream?

If this is really just a skeleton that can't be organized in a decent
patch series that actually builds, my recommendation to Taylor is
simply to postpone upstreaming until the skeleton is made stronger,
all bones are in their right place, and the full body is ready - what
is the point/purpose of having such a skeleton in QEMU upstream?

I am slightly disappointed that after a slick presentation on KVM
Forum, we now talk about a "skeleton".

Yours,
Aleksandar
Richard Henderson Nov. 21, 2019, 8:55 a.m. UTC | #34
On 11/21/19 7:01 AM, Aleksandar Markovic wrote:
> On Wed, Nov 20, 2019 at 8:49 AM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
> 
>> How's that?  He has been asked to split the linux-user stuff from the target
>> skeleton stuff.
> 
> ...
> 
>> This argument would make more sense if there were more present here than a
>> skeleton.
> 
> Speaking about anatomy, I am opposed to upstreaming any "skeletons".
> The other month, another community was dead serious wanting to
> upstream code based on "proposal of the draft" (or was it "draft of
> the proposal"), and now we want to upstream "skeletons"??
> 
> And even that "skeleton" can't be regularly built stage by stage, but
> must resort to "enable configure at the end" cheap tricks?
> 
> What happened to QEMU upstream?

You are over reacting.

We're also talking about *2* patches before the "cheap trick" of enable at the
end.  The only alternative to the "cheap trick" is *1* patch, which no one
wants.  There really is a minimum amount of code that is required before
*anything* will compile.

We're not talking about applying anything at this stage.  We're talking about
patch review, and making the pieces small enough to actually review.

I assume that Taylor and the revng guys are coordinating the actual meat of the
port.  I assume those will be presented in reviewable chunks, each of which
will compile.  Only when we see all of those will the whole thing be applied.


r~
Richard Henderson Nov. 21, 2019, 9 a.m. UTC | #35
On 11/20/19 4:17 PM, Taylor Simpson wrote:
> Are you saying there's a way to tell qemu to put the stack at a certain location?  Or are you suggesting we do something on the hardware side?

There's obviously nothing to be done on the hardware side.  Presumably this is
all about the kernel, running on the hardware, and the default memory map that
it creates for the application.

There's certainly a way to influence where the stack gets put, via
ELF_START_MMAP.  We could even add another define to force the stack to a given
location if required.  I don't know enough about the kernel's default memory
map, and what's different here, to answer that question.


r~
Aleksandar Markovic Nov. 21, 2019, 7:20 p.m. UTC | #36
>  create mode 100644 target/hexagon/imported/global_types.h
>  create mode 100644 target/hexagon/imported/iss_ver_registers.h
>  create mode 100644 target/hexagon/imported/max.h
>  create mode 100644 target/hexagon/imported/regs.h

Taylor, if I understood you well, these files don't confirm to QEMU
coding standard, because they are imported. But, from where? And what
is the reason they need to be imported (and not created independently
by you or somebody else, but within QEMU code style guidelines) ?
Their content looks fairly simple to me.

Thanks,
Aleksandar
Taylor Simpson Nov. 21, 2019, 7:52 p.m. UTC | #37
They are imported from the existing Hexagon simulator.  Please understand that this patch is the first in a series.  Later patches will contain more elaborate contents in that directory.  The reason I don't want to reformat them is to stay in sync with the other simulator in the future.  When the other team makes changes to the code (either to fix bugs or add features), it will be easier to identify the changes and bring them into qemu.

Taylor

-----Original Message-----
From: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
Sent: Thursday, November 21, 2019 1:20 PM
To: Taylor Simpson <tsimpson@quicinc.com>
Cc: Laurent Vivier <laurent@vivier.eu>; Riku Voipio <riku.voipio@iki.fi>; QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards


>  create mode 100644 target/hexagon/imported/global_types.h
>  create mode 100644 target/hexagon/imported/iss_ver_registers.h
>  create mode 100644 target/hexagon/imported/max.h  create mode 100644
> target/hexagon/imported/regs.h

Taylor, if I understood you well, these files don't confirm to QEMU coding standard, because they are imported. But, from where? And what is the reason they need to be imported (and not created independently by you or somebody else, but within QEMU code style guidelines) ?
Their content looks fairly simple to me.

Thanks,
Aleksandar
Aleksandar Markovic Nov. 21, 2019, 8:44 p.m. UTC | #38
On Thu, Nov 21, 2019 at 8:52 PM Taylor Simpson <tsimpson@quicinc.com> wrote:
>
> They are imported from the existing Hexagon simulator.  Please understand that this patch is the first in a series.  Later patches will contain more elaborate contents in that directory.  The reason I don't want to reformat them is to stay in sync with the other simulator in the future.  When the other team makes changes to the code (either to fix bugs or add features), it will be easier to identify the changes and bring them into qemu.
>
> Taylor
>

Taylor,

Please understand that this patch can't remain a single patch. It
can't remain even a set of 2 or 3 patches as others suggested. A patch
is a logically connected unit of code whose typical size is less than
200 lines. There are lots of such logical units in this single path
that you sent, and you should not have sent it in its present form,
even if you wanted just comments to it. You should have submitted a
series rather than a single patch. And you should have said this is v1
of my series that I will expand later on. Guidelines for submissions
are here:

https://wiki.qemu.org/Contribute/SubmitAPatch

As far as "imported" files, frankly, I dislike the fact that you are
willing to sacrifice our coding style guidelines in favor to your
convenience. But, more than this, I also find very problematic that
you practically create a dependency between QEMU and another
simulator. QEMU implementation should rely on specifications, and only
on specifications, and certainly should not depend on another
simulator. Currently, in QEMU, there are some cases of imported
disassemblers or similar relatively unimportant tools, but those
imports change very rarely, and are modified to comply to QEMU coding
style. I am not aware on dependency of QEMU on another simulator in
the form you want to do for Hexagon. My strong impression is that you
will create more problems than benefits with such dependency, both for
you and for QEMU in general.

Once a CPU or any other device is specified though documentation,
these specs don't change. Consequently, their emulation does not
change too, in functional sense. The fact that you anticipate changes
in these files imported from another simulator, leaves me with a
(possibly wrong) perception that neither Hexagon internal simulator
nor QEMU implementation you are trying to integrate are complete. If
that is not true, can you explain what exactly you expect to be
changing in imported files?

Yours,
Aleksandar


> -----Original Message-----
> From: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
> Sent: Thursday, November 21, 2019 1:20 PM
> To: Taylor Simpson <tsimpson@quicinc.com>
> Cc: Laurent Vivier <laurent@vivier.eu>; Riku Voipio <riku.voipio@iki.fi>; QEMU Developers <qemu-devel@nongnu.org>
> Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards
>
>
> >  create mode 100644 target/hexagon/imported/global_types.h
> >  create mode 100644 target/hexagon/imported/iss_ver_registers.h
> >  create mode 100644 target/hexagon/imported/max.h  create mode 100644
> > target/hexagon/imported/regs.h
>
> Taylor, if I understood you well, these files don't confirm to QEMU coding standard, because they are imported. But, from where? And what is the reason they need to be imported (and not created independently by you or somebody else, but within QEMU code style guidelines) ?
> Their content looks fairly simple to me.
>
> Thanks,
> Aleksandar
Taylor Simpson Nov. 21, 2019, 11:51 p.m. UTC | #39
Hi Aleksandar,

The complete qemu-hexagon implementation can be found here
https://github.com/quic/qemu

From the beginning, I knew I would have to divide it up into smaller patches for review.  The discussion on how divide it was here
https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg01971.html

I'll summarize the proposal
    1) linux-user changes + linux-user/hexagon + skeleton of target/hexagon
    2) Add the code this imported from the Hexagon simulator and the qemu helper generator
    3) Add support for packet semantics
    4) Add support for wide vector extensions
    5) Add the helper overrides for performance optimization

My assumption in the proposal was that each phase of the submission should compile and execute, and each phase should expand the capabilities of the Hexagon target.  I apologize that it wasn't clear how large each of the patches would be.

I greatly appreciate all of the feedback on the proposal and the patch, and I am making the revisions.  In several cases, the feedback also applies to the remainder of the code.  So, I am making the changes there as well.  From that perspective, I think it will reduce the overall burden on the reviewers.

As far as what might change in the imported files goes, the current simulator (hexagon-sim) has been used in production as part of the toolchain for over a decade and has been used to verify every version of the core that we have shipped.  Since it is used in verification, it is considered the gold standard - more so than the PDF manual.  Any changes to that code, including reformatting, would put qemu at risk of not accurately emulating the processor.

Taylor


-----Original Message-----
From: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
Sent: Thursday, November 21, 2019 2:45 PM
To: Taylor Simpson <tsimpson@quicinc.com>
Cc: Laurent Vivier <laurent@vivier.eu>; Riku Voipio <riku.voipio@iki.fi>; QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards

On Thu, Nov 21, 2019 at 8:52 PM Taylor Simpson <tsimpson@quicinc.com> wrote:
>
> They are imported from the existing Hexagon simulator.  Please understand that this patch is the first in a series.  Later patches will contain more elaborate contents in that directory.  The reason I don't want to reformat them is to stay in sync with the other simulator in the future.  When the other team makes changes to the code (either to fix bugs or add features), it will be easier to identify the changes and bring them into qemu.
>
> Taylor
>

Taylor,

Please understand that this patch can't remain a single patch. It can't remain even a set of 2 or 3 patches as others suggested. A patch is a logically connected unit of code whose typical size is less than
200 lines. There are lots of such logical units in this single path that you sent, and you should not have sent it in its present form, even if you wanted just comments to it. You should have submitted a series rather than a single patch. And you should have said this is v1 of my series that I will expand later on. Guidelines for submissions are here:

https://wiki.qemu.org/Contribute/SubmitAPatch

As far as "imported" files, frankly, I dislike the fact that you are willing to sacrifice our coding style guidelines in favor to your convenience. But, more than this, I also find very problematic that you practically create a dependency between QEMU and another simulator. QEMU implementation should rely on specifications, and only on specifications, and certainly should not depend on another simulator. Currently, in QEMU, there are some cases of imported disassemblers or similar relatively unimportant tools, but those imports change very rarely, and are modified to comply to QEMU coding style. I am not aware on dependency of QEMU on another simulator in the form you want to do for Hexagon. My strong impression is that you will create more problems than benefits with such dependency, both for you and for QEMU in general.

Once a CPU or any other device is specified though documentation, these specs don't change. Consequently, their emulation does not change too, in functional sense. The fact that you anticipate changes in these files imported from another simulator, leaves me with a (possibly wrong) perception that neither Hexagon internal simulator nor QEMU implementation you are trying to integrate are complete. If that is not true, can you explain what exactly you expect to be changing in imported files?

Yours,
Aleksandar


> -----Original Message-----
> From: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
> Sent: Thursday, November 21, 2019 1:20 PM
> To: Taylor Simpson <tsimpson@quicinc.com>
> Cc: Laurent Vivier <laurent@vivier.eu>; Riku Voipio
> <riku.voipio@iki.fi>; QEMU Developers <qemu-devel@nongnu.org>
> Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of
> patches - linux-user changes + linux-user/hexagon + skeleton of
> target/hexagon - Files in target/hexagon/imported are from another
> project and therefore do not conform to qemu coding standards
>
>
> >  create mode 100644 target/hexagon/imported/global_types.h
> >  create mode 100644 target/hexagon/imported/iss_ver_registers.h
> >  create mode 100644 target/hexagon/imported/max.h  create mode
> > 100644 target/hexagon/imported/regs.h
>
> Taylor, if I understood you well, these files don't confirm to QEMU coding standard, because they are imported. But, from where? And what is the reason they need to be imported (and not created independently by you or somebody else, but within QEMU code style guidelines) ?
> Their content looks fairly simple to me.
>
> Thanks,
> Aleksandar
Aleksandar Markovic Nov. 22, 2019, 9:33 a.m. UTC | #40
On Friday, November 22, 2019, Taylor Simpson <tsimpson@quicinc.com> wrote:

> Hi Aleksandar,
>
> The complete qemu-hexagon implementation can be found here
> https://github.com/quic/qemu
>
> From the beginning, I knew I would have to divide it up into smaller
> patches for review.  The discussion on how divide it was here
> https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg01971.html
>
I'll summarize the proposal
>     1) linux-user changes + linux-user/hexagon + skeleton of target/hexagon
>     2) Add the code this imported from the Hexagon simulator and the qemu
> helper generator
>     3) Add support for packet semantics
>     4) Add support for wide vector extensions
>     5) Add the helper overrides for performance optimization
>
>
I think you, for some reason, mix and keep mixing two really different
things:

- outline of your changes (as seen as points 1-5 above)

- a patch

None of points 1-5 could be a patch. All of them should be patch series, or
part of patch series.

I am surprised that it seems as if you did not take a look at the previous
cases of platform introductions to QEMU. Or you did, but decided you don't
need to bother creating patch series, you don't need to follow QEMU
submission guidelines - you apparently think you can simply send the
changes as several huge patches.


> My assumption in the proposal was that each phase of the submission should
> compile and execute, and each phase should expand the capabilities of the
> Hexagon target.  I apologize that it wasn't clear how large each of the
> patches would be.
>
> I greatly appreciate all of the feedback on the proposal and the patch,
> and I am making the revisions.  In several cases, the feedback also applies
> to the remainder of the code.  So, I am making the changes there as well.
> From that perspective, I think it will reduce the overall burden on the
> reviewers.
>
> As far as what might change in the imported files goes, the current
> simulator (hexagon-sim) has been used in production as part of the
> toolchain for over a decade and has been used to verify every version of
> the core that we have shipped.  Since it is used in verification, it is
> considered the gold standard - more so than the PDF manual.  Any changes to
> that code, including reformatting, would put qemu at risk of not accurately
> emulating the processor.
>
>
We in MIPS also have ISA verification software that can be viewed as "gold
standard", but we don't burden QEMU with any imported files - and we
certainly could, using the same rationale as you do.

I am opposed to considering the output (or a part) of an internally
developed simulator more important than (or on the same level as)
documentation, and calling it a "gold standard".

A gold-standard-related question arises here:

If something is called a "gold standard", should it be a constant, by its
very definition?

Also, you did not answer my question from my previous resoonse, which was:

What do you expect to be changing in imported-from-another-simulator code?

And I add another one:

What exactly components of QEMU for Hexagon are contained in
imported-from-another-simulator code?

Thanks,
Aleksandar


> Taylor
>
>
> -----Original Message-----
> From: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
> Sent: Thursday, November 21, 2019 2:45 PM
> To: Taylor Simpson <tsimpson@quicinc.com>
> Cc: Laurent Vivier <laurent@vivier.eu>; Riku Voipio <riku.voipio@iki.fi>;
> QEMU Developers <qemu-devel@nongnu.org>
> Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of
> patches - linux-user changes + linux-user/hexagon + skeleton of
> target/hexagon - Files in target/hexagon/imported are from another project
> and therefore do not conform to qemu coding standards
>
> On Thu, Nov 21, 2019 at 8:52 PM Taylor Simpson <tsimpson@quicinc.com>
> wrote:
> >
> > They are imported from the existing Hexagon simulator.  Please
> understand that this patch is the first in a series.  Later patches will
> contain more elaborate contents in that directory.  The reason I don't want
> to reformat them is to stay in sync with the other simulator in the
> future.  When the other team makes changes to the code (either to fix bugs
> or add features), it will be easier to identify the changes and bring them
> into qemu.
> >
> > Taylor
> >
>
> Taylor,
>
> Please understand that this patch can't remain a single patch. It can't
> remain even a set of 2 or 3 patches as others suggested. A patch is a
> logically connected unit of code whose typical size is less than
> 200 lines. There are lots of such logical units in this single path that
> you sent, and you should not have sent it in its present form, even if you
> wanted just comments to it. You should have submitted a series rather than
> a single patch. And you should have said this is v1 of my series that I
> will expand later on. Guidelines for submissions are here:
>
> https://wiki.qemu.org/Contribute/SubmitAPatch
>
> As far as "imported" files, frankly, I dislike the fact that you are
> willing to sacrifice our coding style guidelines in favor to your
> convenience. But, more than this, I also find very problematic that you
> practically create a dependency between QEMU and another simulator. QEMU
> implementation should rely on specifications, and only on specifications,
> and certainly should not depend on another simulator. Currently, in QEMU,
> there are some cases of imported disassemblers or similar relatively
> unimportant tools, but those imports change very rarely, and are modified
> to comply to QEMU coding style. I am not aware on dependency of QEMU on
> another simulator in the form you want to do for Hexagon. My strong
> impression is that you will create more problems than benefits with such
> dependency, both for you and for QEMU in general.
>
> Once a CPU or any other device is specified though documentation, these
> specs don't change. Consequently, their emulation does not change too, in
> functional sense. The fact that you anticipate changes in these files
> imported from another simulator, leaves me with a (possibly wrong)
> perception that neither Hexagon internal simulator nor QEMU implementation
> you are trying to integrate are complete. If that is not true, can you
> explain what exactly you expect to be changing in imported files?
>
> Yours,
> Aleksandar
>
>
> > -----Original Message-----
> > From: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
> > Sent: Thursday, November 21, 2019 1:20 PM
> > To: Taylor Simpson <tsimpson@quicinc.com>
> > Cc: Laurent Vivier <laurent@vivier.eu>; Riku Voipio
> > <riku.voipio@iki.fi>; QEMU Developers <qemu-devel@nongnu.org>
> > Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of
> > patches - linux-user changes + linux-user/hexagon + skeleton of
> > target/hexagon - Files in target/hexagon/imported are from another
> > project and therefore do not conform to qemu coding standards
> >
> >
> > >  create mode 100644 target/hexagon/imported/global_types.h
> > >  create mode 100644 target/hexagon/imported/iss_ver_registers.h
> > >  create mode 100644 target/hexagon/imported/max.h  create mode
> > > 100644 target/hexagon/imported/regs.h
> >
> > Taylor, if I understood you well, these files don't confirm to QEMU
> coding standard, because they are imported. But, from where? And what is
> the reason they need to be imported (and not created independently by you
> or somebody else, but within QEMU code style guidelines) ?
> > Their content looks fairly simple to me.
> >
> > Thanks,
> > Aleksandar
>
diff mbox series

Patch

diff --git a/configure b/configure
index efe165e..e15ef8c 100755
--- a/configure
+++ b/configure
@@ -7739,6 +7739,12 @@  case "$target_name" in
     bflt="yes"
     mttcg="yes"
   ;;
+  hexagon)
+    TARGET_BASE_ARCH=hexagon
+    TARGET_ABI_DIR=hexagon
+    mttcg=yes
+    target_compiler=$cross_cc_hexagon
+  ;;
   *)
     error_exit "Unsupported target CPU"
   ;;
@@ -7902,6 +7908,9 @@  for i in $ARCH $TARGET_BASE_ARCH ; do
   xtensa*)
     disas_config "XTENSA"
   ;;
+  hexagon)
+    disas_config "HEXAGON"
+  ;;
   esac
 done
 if test "$tcg_interpreter" = "yes" ; then
diff --git a/default-configs/hexagon-linux-user.mak b/default-configs/hexagon-linux-user.mak
new file mode 100644
index 0000000..ad55af0
--- /dev/null
+++ b/default-configs/hexagon-linux-user.mak
@@ -0,0 +1 @@ 
+# Default configuration for hexagon-linux-user
diff --git a/include/elf.h b/include/elf.h
index 3501e0c..efd1ad2 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -170,6 +170,8 @@  typedef struct mips_elf_abiflags_v0 {
 
 #define EM_UNICORE32    110     /* UniCore32 */
 
+#define EM_HEXAGON     164     /* Qualcomm Hexagon */
+
 #define EM_RISCV        243     /* RISC-V */
 
 #define EM_NANOMIPS     249     /* Wave Computing nanoMIPS */
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index f6693e5..9d9542a 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1504,6 +1504,22 @@  static void elf_core_copy_regs(target_elf_gregset_t *regs,
 
 #endif /* TARGET_XTENSA */
 
+#ifdef TARGET_HEXAGON
+
+#define ELF_START_MMAP 0x20000000
+
+#define ELF_CLASS       ELFCLASS32
+#define ELF_ARCH        EM_HEXAGON
+
+static inline void init_thread(struct target_pt_regs *regs,
+                               struct image_info *infop)
+{
+    regs->sepc = infop->entry;
+    regs->sp = infop->start_stack;
+}
+
+#endif /* TARGET_HEXAGON */
+
 #ifndef ELF_PLATFORM
 #define ELF_PLATFORM (NULL)
 #endif
diff --git a/linux-user/hexagon/cpu_loop.c b/linux-user/hexagon/cpu_loop.c
new file mode 100644
index 0000000..ad5977d
--- /dev/null
+++ b/linux-user/hexagon/cpu_loop.c
@@ -0,0 +1,103 @@ 
+/*
+ *  qemu user cpu loop
+ *
+ *  Copyright (c) 2003-2008 Fabrice Bellard
+ *  Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu.h"
+#include "cpu_loop-common.h"
+#include "translate.h"
+
+void cpu_loop(CPUHexagonState *env)
+{
+    CPUState *cs = CPU(hexagon_env_get_cpu(env));
+    int trapnr;
+    target_ulong syscallnum;
+    target_ulong ret;
+
+    for (;;) {
+        cpu_exec_start(cs);
+        trapnr = cpu_exec(cs);
+        cpu_exec_end(cs);
+        process_queued_cpu_work(cs);
+
+        switch (trapnr) {
+        case EXCP_INTERRUPT:
+            /* just indicate that signals should be handled asap */
+            break;
+        case HEX_EXCP_TRAP0:
+            syscallnum = env->gpr[6];
+            env->gpr[HEX_REG_PC] += 4;
+#ifdef DEBUG_HEX
+            printf("syscall %d (%d, %d, %d, %d, %d, %d)\n",
+                             syscallnum,
+                             env->gpr[0],
+                             env->gpr[1],
+                             env->gpr[2],
+                             env->gpr[3],
+                             env->gpr[4],
+                             env->gpr[5]);
+#endif
+            ret = do_syscall(env,
+                             syscallnum,
+                             env->gpr[0],
+                             env->gpr[1],
+                             env->gpr[2],
+                             env->gpr[3],
+                             env->gpr[4],
+                             env->gpr[5],
+                             0, 0);
+            if (ret == -TARGET_ERESTARTSYS) {
+#ifdef DEBUG_HEX
+                printf("\tSyscall %d returned -TARGET_ERESTARTSYS\n",
+                    syscallnum);
+#endif
+                env->gpr[HEX_REG_PC] -= 4;
+            } else if (ret != -TARGET_QEMU_ESIGRETURN) {
+#ifdef DEBUG_HEX
+                printf("\tSyscall %d returned %d\n", syscallnum, ret);
+#endif
+                env->gpr[0] = ret;
+            }
+#ifdef DEBUG_HEX
+            else {
+                printf("\tSyscall %d returned -TARGET_QEMU_ESIGRETURN\n",
+                       syscallnum);
+            }
+#endif
+            break;
+        case HEX_EXCP_TRAP1:
+            EXCP_DUMP(env, "\nqemu: trap1 exception %#x - aborting\n",
+                     trapnr);
+            exit(EXIT_FAILURE);
+            break;
+        default:
+            EXCP_DUMP(env, "\nqemu: unhandled CPU exception %#x - aborting\n",
+                     trapnr);
+            exit(EXIT_FAILURE);
+        }
+
+        process_pending_signals(env);
+    }
+}
+
+void target_cpu_copy_regs(CPUArchState *env, struct target_pt_regs *regs)
+{
+    env->gpr[HEX_REG_PC] = regs->sepc;
+    env->gpr[HEX_REG_SP] = regs->sp;
+}
diff --git a/linux-user/hexagon/signal.c b/linux-user/hexagon/signal.c
new file mode 100644
index 0000000..e67c085
--- /dev/null
+++ b/linux-user/hexagon/signal.c
@@ -0,0 +1,276 @@ 
+/*
+ *  Emulation of Linux signals
+ *
+ *  Copyright (c) 2003 Fabrice Bellard
+ *  Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#include "qemu/osdep.h"
+#include "qemu.h"
+#include "signal-common.h"
+#include "linux-user/trace.h"
+
+struct target_sigcontext {
+    target_ulong r0,  r1,  r2,  r3;
+    target_ulong r4,  r5,  r6,  r7;
+    target_ulong r8,  r9, r10, r11;
+    target_ulong r12, r13, r14, r15;
+    target_ulong r16, r17, r18, r19;
+    target_ulong r20, r21, r22, r23;
+    target_ulong r24, r25, r26, r27;
+    target_ulong r28, r29, r30, r31;
+    target_ulong sa0;
+    target_ulong lc0;
+    target_ulong sa1;
+    target_ulong lc1;
+    target_ulong m0;
+    target_ulong m1;
+    target_ulong usr;
+    target_ulong p3_0;
+    target_ulong gp;
+    target_ulong ugp;
+    target_ulong pc;
+    target_ulong cause;
+    target_ulong badva;
+    target_ulong pad1;
+    target_ulong pad2;
+    target_ulong pad3;
+};
+
+struct target_ucontext {
+    unsigned long uc_flags;
+    target_ulong uc_link; /* target pointer */
+    target_stack_t uc_stack;
+    struct target_sigcontext uc_mcontext;
+    target_sigset_t uc_sigmask;
+};
+
+struct target_rt_sigframe {
+    uint32_t tramp[2];
+    struct target_siginfo info;
+    struct target_ucontext uc;
+};
+
+static abi_ulong get_sigframe(struct target_sigaction *ka,
+                              CPUHexagonState *regs, size_t framesize)
+{
+    abi_ulong sp = get_sp_from_cpustate(regs);
+
+    /* This is the X/Open sanctioned signal stack switching.  */
+    sp = target_sigsp(sp, ka) - framesize;
+
+    sp &= ~7UL; /* align sp on 8-byte boundary */
+
+    return sp;
+}
+
+static void setup_sigcontext(struct target_sigcontext *sc, CPUHexagonState *env)
+{
+    __put_user(env->gpr[HEX_REG_R00], &sc->r0);
+    __put_user(env->gpr[HEX_REG_R01], &sc->r1);
+    __put_user(env->gpr[HEX_REG_R02], &sc->r2);
+    __put_user(env->gpr[HEX_REG_R03], &sc->r3);
+    __put_user(env->gpr[HEX_REG_R04], &sc->r4);
+    __put_user(env->gpr[HEX_REG_R05], &sc->r5);
+    __put_user(env->gpr[HEX_REG_R06], &sc->r6);
+    __put_user(env->gpr[HEX_REG_R07], &sc->r7);
+    __put_user(env->gpr[HEX_REG_R08], &sc->r8);
+    __put_user(env->gpr[HEX_REG_R09], &sc->r9);
+    __put_user(env->gpr[HEX_REG_R10], &sc->r10);
+    __put_user(env->gpr[HEX_REG_R11], &sc->r11);
+    __put_user(env->gpr[HEX_REG_R12], &sc->r12);
+    __put_user(env->gpr[HEX_REG_R13], &sc->r13);
+    __put_user(env->gpr[HEX_REG_R14], &sc->r14);
+    __put_user(env->gpr[HEX_REG_R15], &sc->r15);
+    __put_user(env->gpr[HEX_REG_R16], &sc->r16);
+    __put_user(env->gpr[HEX_REG_R17], &sc->r17);
+    __put_user(env->gpr[HEX_REG_R18], &sc->r18);
+    __put_user(env->gpr[HEX_REG_R19], &sc->r19);
+    __put_user(env->gpr[HEX_REG_R20], &sc->r20);
+    __put_user(env->gpr[HEX_REG_R21], &sc->r21);
+    __put_user(env->gpr[HEX_REG_R22], &sc->r22);
+    __put_user(env->gpr[HEX_REG_R23], &sc->r23);
+    __put_user(env->gpr[HEX_REG_R24], &sc->r24);
+    __put_user(env->gpr[HEX_REG_R25], &sc->r25);
+    __put_user(env->gpr[HEX_REG_R26], &sc->r26);
+    __put_user(env->gpr[HEX_REG_R27], &sc->r27);
+    __put_user(env->gpr[HEX_REG_R28], &sc->r28);
+    __put_user(env->gpr[HEX_REG_R29], &sc->r29);
+    __put_user(env->gpr[HEX_REG_R30], &sc->r30);
+    __put_user(env->gpr[HEX_REG_R31], &sc->r31);
+    __put_user(env->gpr[HEX_REG_SA0], &sc->sa0);
+    __put_user(env->gpr[HEX_REG_LC0], &sc->lc0);
+    __put_user(env->gpr[HEX_REG_SA1], &sc->sa1);
+    __put_user(env->gpr[HEX_REG_LC1], &sc->lc1);
+    __put_user(env->gpr[HEX_REG_M0], &sc->m0);
+    __put_user(env->gpr[HEX_REG_M1], &sc->m1);
+    __put_user(env->gpr[HEX_REG_USR], &sc->usr);
+    __put_user(env->gpr[HEX_REG_P3_0], &sc->p3_0);
+    __put_user(env->gpr[HEX_REG_GP], &sc->gp);
+    __put_user(env->gpr[HEX_REG_UGP], &sc->ugp);
+    __put_user(env->gpr[HEX_REG_PC], &sc->pc);
+}
+
+static void setup_ucontext(struct target_ucontext *uc,
+                           CPUHexagonState *env, target_sigset_t *set)
+{
+    __put_user(0,    &(uc->uc_flags));
+    __put_user(0,    &(uc->uc_link));
+
+    target_save_altstack(&uc->uc_stack, env);
+
+    int i;
+    for (i = 0; i < TARGET_NSIG_WORDS; i++) {
+        __put_user(set->sig[i], &(uc->uc_sigmask.sig[i]));
+    }
+
+    setup_sigcontext(&uc->uc_mcontext, env);
+}
+
+static inline void install_sigtramp(uint32_t *tramp)
+{
+    __put_user(0x7800d166, tramp + 0); /*  { r6=#__NR_rt_sigreturn } */
+    __put_user(0x5400c004, tramp + 1); /*  { trap0(#1) } */
+}
+
+void setup_rt_frame(int sig, struct target_sigaction *ka,
+                    target_siginfo_t *info,
+                    target_sigset_t *set, CPUHexagonState *env)
+{
+    abi_ulong frame_addr;
+    struct target_rt_sigframe *frame;
+
+    frame_addr = get_sigframe(ka, env, sizeof(*frame));
+    trace_user_setup_rt_frame(env, frame_addr);
+
+    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
+        goto badframe;
+    }
+
+    setup_ucontext(&frame->uc, env, set);
+    tswap_siginfo(&frame->info, info);
+    install_sigtramp(frame->tramp);
+
+    env->gpr[HEX_REG_PC] = ka->_sa_handler;
+    env->gpr[HEX_REG_SP] = frame_addr;
+    env->gpr[HEX_REG_R00] = sig;
+    env->gpr[HEX_REG_R01] =
+        frame_addr + offsetof(struct target_rt_sigframe, info);
+    env->gpr[HEX_REG_R02] =
+        frame_addr + offsetof(struct target_rt_sigframe, uc);
+    env->gpr[HEX_REG_LR] =
+        frame_addr + offsetof(struct target_rt_sigframe, tramp);
+
+    return;
+
+badframe:
+    unlock_user_struct(frame, frame_addr, 1);
+    if (sig == TARGET_SIGSEGV) {
+        ka->_sa_handler = TARGET_SIG_DFL;
+    }
+    force_sig(TARGET_SIGSEGV);
+}
+
+static void restore_sigcontext(CPUHexagonState *env,
+                               struct target_sigcontext *sc)
+{
+    __get_user(env->gpr[HEX_REG_R00], &sc->r0);
+    __get_user(env->gpr[HEX_REG_R01], &sc->r1);
+    __get_user(env->gpr[HEX_REG_R02], &sc->r2);
+    __get_user(env->gpr[HEX_REG_R03], &sc->r3);
+    __get_user(env->gpr[HEX_REG_R04], &sc->r4);
+    __get_user(env->gpr[HEX_REG_R05], &sc->r5);
+    __get_user(env->gpr[HEX_REG_R06], &sc->r6);
+    __get_user(env->gpr[HEX_REG_R07], &sc->r7);
+    __get_user(env->gpr[HEX_REG_R08], &sc->r8);
+    __get_user(env->gpr[HEX_REG_R09], &sc->r9);
+    __get_user(env->gpr[HEX_REG_R10], &sc->r10);
+    __get_user(env->gpr[HEX_REG_R11], &sc->r11);
+    __get_user(env->gpr[HEX_REG_R12], &sc->r12);
+    __get_user(env->gpr[HEX_REG_R13], &sc->r13);
+    __get_user(env->gpr[HEX_REG_R14], &sc->r14);
+    __get_user(env->gpr[HEX_REG_R15], &sc->r15);
+    __get_user(env->gpr[HEX_REG_R16], &sc->r16);
+    __get_user(env->gpr[HEX_REG_R17], &sc->r17);
+    __get_user(env->gpr[HEX_REG_R18], &sc->r18);
+    __get_user(env->gpr[HEX_REG_R19], &sc->r19);
+    __get_user(env->gpr[HEX_REG_R20], &sc->r20);
+    __get_user(env->gpr[HEX_REG_R21], &sc->r21);
+    __get_user(env->gpr[HEX_REG_R22], &sc->r22);
+    __get_user(env->gpr[HEX_REG_R23], &sc->r23);
+    __get_user(env->gpr[HEX_REG_R24], &sc->r24);
+    __get_user(env->gpr[HEX_REG_R25], &sc->r25);
+    __get_user(env->gpr[HEX_REG_R26], &sc->r26);
+    __get_user(env->gpr[HEX_REG_R27], &sc->r27);
+    __get_user(env->gpr[HEX_REG_R28], &sc->r28);
+    __get_user(env->gpr[HEX_REG_R29], &sc->r29);
+    __get_user(env->gpr[HEX_REG_R30], &sc->r30);
+    __get_user(env->gpr[HEX_REG_R31], &sc->r31);
+    __get_user(env->gpr[HEX_REG_SA0], &sc->sa0);
+    __get_user(env->gpr[HEX_REG_LC0], &sc->lc0);
+    __get_user(env->gpr[HEX_REG_SA1], &sc->sa1);
+    __get_user(env->gpr[HEX_REG_LC1], &sc->lc1);
+    __get_user(env->gpr[HEX_REG_M0], &sc->m0);
+    __get_user(env->gpr[HEX_REG_M1], &sc->m1);
+    __get_user(env->gpr[HEX_REG_USR], &sc->usr);
+    __get_user(env->gpr[HEX_REG_P3_0], &sc->p3_0);
+    __get_user(env->gpr[HEX_REG_GP], &sc->gp);
+    __get_user(env->gpr[HEX_REG_UGP], &sc->ugp);
+    __get_user(env->gpr[HEX_REG_PC], &sc->pc);
+}
+
+static void restore_ucontext(CPUHexagonState *env, struct target_ucontext *uc)
+{
+    sigset_t blocked;
+    target_sigset_t target_set;
+    int i;
+
+    target_sigemptyset(&target_set);
+    for (i = 0; i < TARGET_NSIG_WORDS; i++) {
+        __get_user(target_set.sig[i], &(uc->uc_sigmask.sig[i]));
+    }
+
+    target_to_host_sigset_internal(&blocked, &target_set);
+    set_sigmask(&blocked);
+
+    restore_sigcontext(env, &uc->uc_mcontext);
+}
+
+long do_rt_sigreturn(CPUHexagonState *env)
+{
+    struct target_rt_sigframe *frame;
+    abi_ulong frame_addr;
+
+    frame_addr = env->gpr[HEX_REG_SP];
+    trace_user_do_sigreturn(env, frame_addr);
+    if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1)) {
+        goto badframe;
+    }
+
+    restore_ucontext(env, &frame->uc);
+
+    if (do_sigaltstack(frame_addr + offsetof(struct target_rt_sigframe,
+            uc.uc_stack), 0, get_sp_from_cpustate(env)) == -EFAULT) {
+        goto badframe;
+    }
+
+    unlock_user_struct(frame, frame_addr, 0);
+    return -TARGET_QEMU_ESIGRETURN;
+
+badframe:
+    unlock_user_struct(frame, frame_addr, 0);
+    force_sig(TARGET_SIGSEGV);
+    return 0;
+}
diff --git a/linux-user/hexagon/sockbits.h b/linux-user/hexagon/sockbits.h
new file mode 100644
index 0000000..85bd64a
--- /dev/null
+++ b/linux-user/hexagon/sockbits.h
@@ -0,0 +1,3 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+#include "../generic/sockbits.h"
diff --git a/linux-user/hexagon/syscall_nr.h b/linux-user/hexagon/syscall_nr.h
new file mode 100644
index 0000000..1587cbd
--- /dev/null
+++ b/linux-user/hexagon/syscall_nr.h
@@ -0,0 +1,331 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+/*
+ * Syscall numbers
+ */
+
+#define  TARGET_NR_io_setup 0
+#define  TARGET_NR_io_destroy 1
+#define  TARGET_NR_io_submit 2
+#define  TARGET_NR_io_cancel 3
+#define  TARGET_NR_io_getevents 4
+#define  TARGET_NR_setxattr 5
+#define  TARGET_NR_lsetxattr 6
+#define  TARGET_NR_fsetxattr 7
+#define  TARGET_NR_getxattr 8
+#define  TARGET_NR_lgetxattr 9
+#define  TARGET_NR_fgetxattr 10
+#define  TARGET_NR_listxattr 11
+#define  TARGET_NR_llistxattr 12
+#define  TARGET_NR_flistxattr 13
+#define  TARGET_NR_removexattr 14
+#define  TARGET_NR_lremovexattr 15
+#define  TARGET_NR_fremovexattr 16
+#define  TARGET_NR_getcwd 17
+#define  TARGET_NR_lookup_dcookie 18
+#define  TARGET_NR_eventfd2 19
+#define  TARGET_NR_epoll_create1 20
+#define  TARGET_NR_epoll_ctl 21
+#define  TARGET_NR_epoll_pwait 22
+#define  TARGET_NR_dup 23
+#define  TARGET_NR_dup3 24
+#define  TARGET_NR3264_fcntl 25
+#define  TARGET_NR_inotify_init1 26
+#define  TARGET_NR_inotify_add_watch 27
+#define  TARGET_NR_inotify_rm_watch 28
+#define  TARGET_NR_ioctl 29
+#define  TARGET_NR_ioprio_set 30
+#define  TARGET_NR_ioprio_get 31
+#define  TARGET_NR_flock 32
+#define  TARGET_NR_mknodat 33
+#define  TARGET_NR_mkdirat 34
+#define  TARGET_NR_unlinkat 35
+#define  TARGET_NR_symlinkat 36
+#define  TARGET_NR_linkat 37
+#define  TARGET_NR_renameat 38
+#define  TARGET_NR_umount2 39
+#define  TARGET_NR_mount 40
+#define  TARGET_NR_pivot_root 41
+#define  TARGET_NR_nfsservctl 42
+#define  TARGET_NR3264_statfs 43
+#define  TARGET_NR_statfs TARGET_NR3264_statfs
+#define  TARGET_NR3264_fstatfs 44
+#define  TARGET_NR3264_truncate 45
+#define  TARGET_NR3264_ftruncate 46
+#define  TARGET_NR_fallocate 47
+#define  TARGET_NR_faccessat 48
+#define  TARGET_NR_chdir 49
+#define  TARGET_NR_fchdir 50
+#define  TARGET_NR_chroot 51
+#define  TARGET_NR_fchmod 52
+#define  TARGET_NR_fchmodat 53
+#define  TARGET_NR_fchownat 54
+#define  TARGET_NR_fchown 55
+#define  TARGET_NR_openat 56
+#define  TARGET_NR_close 57
+#define  TARGET_NR_vhangup 58
+#define  TARGET_NR_pipe2 59
+#define  TARGET_NR_quotactl 60
+#define  TARGET_NR_getdents64 61
+#define  TARGET_NR3264_lseek 62
+#define  TARGET_NR_read 63
+#define  TARGET_NR_write 64
+#define  TARGET_NR_readv 65
+#define  TARGET_NR_writev 66
+#define  TARGET_NR_pread64 67
+#define  TARGET_NR_pwrite64 68
+#define  TARGET_NR_preadv 69
+#define  TARGET_NR_pwritev 70
+#define  TARGET_NR3264_sendfile 71
+#define  TARGET_NR_pselect6 72
+#define  TARGET_NR_ppoll 73
+#define  TARGET_NR_signalfd4 74
+#define  TARGET_NR_vmsplice 75
+#define  TARGET_NR_splice 76
+#define  TARGET_NR_tee 77
+#define  TARGET_NR_readlinkat 78
+#define  TARGET_NR3264_fstatat 79
+#define  TARGET_NR3264_fstat 80
+#define  TARGET_NR_fstat  TARGET_NR3264_fstat
+#define  TARGET_NR_sync 81
+#define  TARGET_NR_fsync 82
+#define  TARGET_NR_fdatasync 83
+#define  TARGET_NR_sync_file_range 84
+#define  TARGET_NR_timerfd_create 85
+#define  TARGET_NR_timerfd_settime 86
+#define  TARGET_NR_timerfd_gettime 87
+#define  TARGET_NR_utimensat 88
+#define  TARGET_NR_acct 89
+#define  TARGET_NR_capget 90
+#define  TARGET_NR_capset 91
+#define  TARGET_NR_personality 92
+#define  TARGET_NR_exit 93
+#define  TARGET_NR_exit_group 94
+#define  TARGET_NR_waitid 95
+#define  TARGET_NR_set_tid_address 96
+#define  TARGET_NR_unshare 97
+#define  TARGET_NR_futex 98
+#define  TARGET_NR_set_robust_list 99
+#define  TARGET_NR_get_robust_list 100
+#define  TARGET_NR_nanosleep 101
+#define  TARGET_NR_getitimer 102
+#define  TARGET_NR_setitimer 103
+#define  TARGET_NR_kexec_load 104
+#define  TARGET_NR_init_module 105
+#define  TARGET_NR_delete_module 106
+#define  TARGET_NR_timer_create 107
+#define  TARGET_NR_timer_gettime 108
+#define  TARGET_NR_timer_getoverrun 109
+#define  TARGET_NR_timer_settime 110
+#define  TARGET_NR_timer_delete 111
+#define  TARGET_NR_clock_settime 112
+#define  TARGET_NR_clock_gettime 113
+#define  TARGET_NR_clock_getres 114
+#define  TARGET_NR_clock_nanosleep 115
+#define  TARGET_NR_syslog 116
+#define  TARGET_NR_ptrace 117
+#define  TARGET_NR_sched_setparam 118
+#define  TARGET_NR_sched_setscheduler 119
+#define  TARGET_NR_sched_getscheduler 120
+#define  TARGET_NR_sched_getparam 121
+#define  TARGET_NR_sched_setaffinity 122
+#define  TARGET_NR_sched_getaffinity 123
+#define  TARGET_NR_sched_yield 124
+#define  TARGET_NR_sched_get_priority_max 125
+#define  TARGET_NR_sched_get_priority_min 126
+#define  TARGET_NR_sched_rr_get_interval 127
+#define  TARGET_NR_restart_syscall 128
+#define  TARGET_NR_kill 129
+#define  TARGET_NR_tkill 130
+#define  TARGET_NR_tgkill 131
+#define  TARGET_NR_sigaltstack 132
+#define  TARGET_NR_rt_sigsuspend 133
+#define  TARGET_NR_rt_sigaction 134
+#define  TARGET_NR_rt_sigprocmask 135
+#define  TARGET_NR_rt_sigpending 136
+#define  TARGET_NR_rt_sigtimedwait 137
+#define  TARGET_NR_rt_sigqueueinfo 138
+#define  TARGET_NR_rt_sigreturn 139
+#define  TARGET_NR_setpriority 140
+#define  TARGET_NR_getpriority 141
+#define  TARGET_NR_reboot 142
+#define  TARGET_NR_setregid 143
+#define  TARGET_NR_setgid 144
+#define  TARGET_NR_setreuid 145
+#define  TARGET_NR_setuid 146
+#define  TARGET_NR_setresuid 147
+#define  TARGET_NR_getresuid 148
+#define  TARGET_NR_setresgid 149
+#define  TARGET_NR_getresgid 150
+#define  TARGET_NR_setfsuid 151
+#define  TARGET_NR_setfsgid 152
+#define  TARGET_NR_times 153
+#define  TARGET_NR_setpgid 154
+#define  TARGET_NR_getpgid 155
+#define  TARGET_NR_getsid 156
+#define  TARGET_NR_setsid 157
+#define  TARGET_NR_getgroups 158
+#define  TARGET_NR_setgroups 159
+#define  TARGET_NR_uname 160
+#define  TARGET_NR_sethostname 161
+#define  TARGET_NR_setdomainname 162
+#define  TARGET_NR_getrlimit 163
+#define  TARGET_NR_setrlimit 164
+#define  TARGET_NR_getrusage 165
+#define  TARGET_NR_umask 166
+#define  TARGET_NR_prctl 167
+#define  TARGET_NR_getcpu 168
+#define  TARGET_NR_gettimeofday 169
+#define  TARGET_NR_settimeofday 170
+#define  TARGET_NR_adjtimex 171
+#define  TARGET_NR_getpid 172
+#define  TARGET_NR_getppid 173
+#define  TARGET_NR_getuid 174
+#define  TARGET_NR_geteuid 175
+#define  TARGET_NR_getgid 176
+#define  TARGET_NR_getegid 177
+#define  TARGET_NR_gettid 178
+#define  TARGET_NR_sysinfo 179
+#define  TARGET_NR_mq_open 180
+#define  TARGET_NR_mq_unlink 181
+#define  TARGET_NR_mq_timedsend 182
+#define  TARGET_NR_mq_timedreceive 183
+#define  TARGET_NR_mq_notify 184
+#define  TARGET_NR_mq_getsetattr 185
+#define  TARGET_NR_msgget 186
+#define  TARGET_NR_msgctl 187
+#define  TARGET_NR_msgrcv 188
+#define  TARGET_NR_msgsnd 189
+#define  TARGET_NR_semget 190
+#define  TARGET_NR_semctl 191
+#define  TARGET_NR_semtimedop 192
+#define  TARGET_NR_semop 193
+#define  TARGET_NR_shmget 194
+#define  TARGET_NR_shmctl 195
+#define  TARGET_NR_shmat 196
+#define  TARGET_NR_shmdt 197
+#define  TARGET_NR_socket 198
+#define  TARGET_NR_socketpair 199
+#define  TARGET_NR_bind 200
+#define  TARGET_NR_listen 201
+#define  TARGET_NR_accept 202
+#define  TARGET_NR_connect 203
+#define  TARGET_NR_getsockname 204
+#define  TARGET_NR_getpeername 205
+#define  TARGET_NR_sendto 206
+#define  TARGET_NR_recvfrom 207
+#define  TARGET_NR_setsockopt 208
+#define  TARGET_NR_getsockopt 209
+#define  TARGET_NR_shutdown 210
+#define  TARGET_NR_sendmsg 211
+#define  TARGET_NR_recvmsg 212
+#define  TARGET_NR_readahead 213
+#define  TARGET_NR_brk 214
+#define  TARGET_NR_munmap 215
+#define  TARGET_NR_mremap 216
+#define  TARGET_NR_add_key 217
+#define  TARGET_NR_request_key 218
+#define  TARGET_NR_keyctl 219
+#define  TARGET_NR_clone 220
+#define  TARGET_NR_execve 221
+#define  TARGET_NR3264_mmap 222
+#define  TARGET_NR3264_fadvise64 223
+#define  TARGET_NR_swapon 224
+#define  TARGET_NR_swapoff 225
+#define  TARGET_NR_mprotect 226
+#define  TARGET_NR_msync 227
+#define  TARGET_NR_mlock 228
+#define  TARGET_NR_munlock 229
+#define  TARGET_NR_mlockall 230
+#define  TARGET_NR_munlockall 231
+#define  TARGET_NR_mincore 232
+#define  TARGET_NR_madvise 233
+#define  TARGET_NR_remap_file_pages 234
+#define  TARGET_NR_mbind 235
+#define  TARGET_NR_get_mempolicy 236
+#define  TARGET_NR_set_mempolicy 237
+#define  TARGET_NR_migrate_pages 238
+#define  TARGET_NR_move_pages 239
+#define  TARGET_NR_rt_tgsigqueueinfo 240
+#define  TARGET_NR_perf_event_open 241
+#define  TARGET_NR_accept4 242
+#define  TARGET_NR_recvmmsg 243
+#define  TARGET_NR_arch_specific_syscall 244
+#define  TARGET_NR_wait4 260
+#define  TARGET_NR_prlimit64 261
+#define  TARGET_NR_fanotify_init 262
+#define  TARGET_NR_fanotify_mark 263
+#define  TARGET_NR_name_to_handle_at         264
+#define  TARGET_NR_open_by_handle_at         265
+#define  TARGET_NR_clock_adjtime 266
+#define  TARGET_NR_syncfs 267
+#define  TARGET_NR_setns 268
+#define  TARGET_NR_sendmmsg 269
+#define  TARGET_NR_process_vm_readv 270
+#define  TARGET_NR_process_vm_writev 271
+#define  TARGET_NR_kcmp 272
+#define  TARGET_NR_finit_module 273
+#define  TARGET_NR_sched_setattr 274
+#define  TARGET_NR_sched_getattr 275
+#define  TARGET_NR_renameat2 276
+#define  TARGET_NR_seccomp 277
+#define  TARGET_NR_getrandom 278
+#define  TARGET_NR_memfd_create 279
+#define  TARGET_NR_bpf 280
+#define  TARGET_NR_execveat 281
+#define  TARGET_NR_open 1024
+#define  TARGET_NR_link 1025
+#define  TARGET_NR_unlink 1026
+#define  TARGET_NR_mknod 1027
+#define  TARGET_NR_chmod 1028
+#define  TARGET_NR_chown 1029
+#define  TARGET_NR_mkdir 1030
+#define  TARGET_NR_rmdir 1031
+#define  TARGET_NR_lchown 1032
+#define  TARGET_NR_access 1033
+#define  TARGET_NR_rename 1034
+#define  TARGET_NR_readlink 1035
+#define  TARGET_NR_symlink 1036
+#define  TARGET_NR_utimes 1037
+#define  TARGET_NR3264_stat 1038
+#define  TARGET_NR3264_lstat 1039
+#define  TARGET_NR_pipe 1040
+#define  TARGET_NR_dup2 1041
+#define  TARGET_NR_epoll_create 1042
+#define  TARGET_NR_inotify_init 1043
+#define  TARGET_NR_eventfd 1044
+#define  TARGET_NR_signalfd 1045
+#define  TARGET_NR_alarm 1059
+#define  TARGET_NR_getpgrp 1060
+#define  TARGET_NR_pause 1061
+#define  TARGET_NR_time 1062
+#define  TARGET_NR_utime 1063
+#define  TARGET_NR_creat 1064
+#define  TARGET_NR_getdents 1065
+#define  TARGET_NR_futimesat 1066
+#define  TARGET_NR_select 1067
+#define  TARGET_NR_poll 1068
+#define  TARGET_NR_epoll_wait 1069
+#define  TARGET_NR_ustat 1070
+#define  TARGET_NR_vfork 1071
+#define  TARGET_NR_oldwait4 1072
+#define  TARGET_NR_recv 1073
+#define  TARGET_NR_send 1074
+#define  TARGET_NR_bdflush 1075
+#define  TARGET_NR_umount 1076
+#define  TARGET_NR_uselib 1077
+#define  TARGET_NR__sysctl 1078
+#define  TARGET_NR_fork 1079
+#define  TARGET_NR_syscalls (TARGET_NR_fork + 1)
+#define  TARGET_NR_fstatfs  TARGET_NR3264_fstatfs
+#define  TARGET_NR_truncate64  TARGET_NR3264_truncate
+#define  TARGET_NR_ftruncate64  TARGET_NR3264_ftruncate
+#define  TARGET_NR_sendfile  TARGET_NR3264_sendfile
+#define  TARGET_NR_newfstatat  TARGET_NR3264_fstatat
+#define  TARGET_NR_lstat  TARGET_NR3264_lstat
+#define  TARGET_NR_fcntl64  TARGET_NR3264_fcntl
+#define  TARGET_NR__llseek  TARGET_NR3264_lseek
+#define  TARGET_NR_mmap2  TARGET_NR3264_mmap
+#define  TARGET_NR_fadvise64_64  TARGET_NR3264_fadvise64
+#define  TARGET_NR_stat64  TARGET_NR3264_stat
+
diff --git a/linux-user/hexagon/target_cpu.h b/linux-user/hexagon/target_cpu.h
new file mode 100644
index 0000000..c151363
--- /dev/null
+++ b/linux-user/hexagon/target_cpu.h
@@ -0,0 +1,35 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+#ifndef TARGET_CPU_H
+#define TARGET_CPU_H
+
+static inline void cpu_clone_regs_child(CPUHexagonState *env,
+                                        target_ulong newsp, unsigned flags)
+{
+#ifdef DEBUG_HEX
+    printf("cpu_clone_regs 0x%x\n", newsp);
+#endif
+    if (newsp) {
+        env->gpr[HEX_REG_SP] = newsp;
+    }
+    env->gpr[0] = 0;
+}
+
+static inline void cpu_clone_regs_parent(CPUHexagonState *env, unsigned flags)
+{
+}
+
+static inline void cpu_set_tls(CPUHexagonState *env, target_ulong newtls)
+{
+#ifdef DEBUG_HEX
+    printf("cpu_set_tls 0x%x\n", newtls);
+#endif
+    env->gpr[HEX_REG_UGP] = newtls;
+}
+
+static inline abi_ulong get_sp_from_cpustate(CPUHexagonState *state)
+{
+    return state->gpr[HEX_REG_SP];
+}
+
+#endif
diff --git a/linux-user/hexagon/target_elf.h b/linux-user/hexagon/target_elf.h
new file mode 100644
index 0000000..6466e0a
--- /dev/null
+++ b/linux-user/hexagon/target_elf.h
@@ -0,0 +1,24 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+#ifndef HEXAGON_TARGET_ELF_H
+#define HEXAGON_TARGET_ELF_H
+
+static inline const char *cpu_get_model(uint32_t eflags)
+{
+    /* For now, treat anything newer than v60 as a v67 */
+    /* FIXME - Disable instructions that are newer than the specified arch */
+    if (eflags == 0x04 ||    /* v5  */
+        eflags == 0x05 ||    /* v55 */
+        eflags == 0x60 ||    /* v60 */
+        eflags == 0x61 ||    /* v61 */
+        eflags == 0x62 ||    /* v62 */
+        eflags == 0x65 ||    /* v65 */
+        eflags == 0x66 ||    /* v66 */
+        eflags == 0x67) {    /* v67 */
+        return "v67";
+    }
+    printf("eflags = 0x%x\n", eflags);
+    return "unknown";
+}
+
+#endif
diff --git a/linux-user/hexagon/target_fcntl.h b/linux-user/hexagon/target_fcntl.h
new file mode 100644
index 0000000..a2961fd
--- /dev/null
+++ b/linux-user/hexagon/target_fcntl.h
@@ -0,0 +1,3 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+#include "../generic/fcntl.h"
diff --git a/linux-user/hexagon/target_signal.h b/linux-user/hexagon/target_signal.h
new file mode 100644
index 0000000..add626d
--- /dev/null
+++ b/linux-user/hexagon/target_signal.h
@@ -0,0 +1,19 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+#ifndef TARGET_SIGNAL_H
+#define TARGET_SIGNAL_H
+
+typedef struct target_sigaltstack {
+    abi_ulong ss_sp;
+    abi_int ss_flags;
+    abi_ulong ss_size;
+} target_stack_t;
+
+#define TARGET_SS_ONSTACK 1
+#define TARGET_SS_DISABLE 2
+
+#define TARGET_MINSIGSTKSZ 2048
+
+#include "../generic/signal.h"
+
+#endif /* TARGET_SIGNAL_H */
diff --git a/linux-user/hexagon/target_structs.h b/linux-user/hexagon/target_structs.h
new file mode 100644
index 0000000..8ac5838
--- /dev/null
+++ b/linux-user/hexagon/target_structs.h
@@ -0,0 +1,31 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+/*
+ * Hexagon specific structures for linux-user
+ */
+#ifndef TARGET_STRUCTS_H
+#define TARGET_STRUCTS_H
+
+struct target_ipc_perm {
+    abi_int __key;
+    abi_int uid;
+    abi_int gid;
+    abi_int cuid;
+    abi_int cgid;
+    abi_ushort mode;
+    abi_ushort __pad1;
+    abi_ushort __seq;
+};
+
+struct target_shmid_ds {
+    struct target_ipc_perm shm_perm;
+    abi_long shm_segsz;
+    abi_ulong shm_atime;
+    abi_ulong shm_dtime;
+    abi_ulong shm_ctime;
+    abi_int shm_cpid;
+    abi_int shm_lpid;
+    abi_ulong shm_nattch;
+};
+
+#endif
diff --git a/linux-user/hexagon/target_syscall.h b/linux-user/hexagon/target_syscall.h
new file mode 100644
index 0000000..1d8de9a
--- /dev/null
+++ b/linux-user/hexagon/target_syscall.h
@@ -0,0 +1,17 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+#ifndef TARGET_SYSCALL_H
+#define TARGET_SYSCALL_H
+
+struct target_pt_regs {
+    abi_long sepc;
+    abi_long sp;
+};
+
+#define UNAME_MACHINE "hexagon"
+#define UNAME_MINIMUM_RELEASE "4.15.0"
+
+#define TARGET_MLOCKALL_MCL_CURRENT 1
+#define TARGET_MLOCKALL_MCL_FUTURE  2
+
+#endif
diff --git a/linux-user/hexagon/termbits.h b/linux-user/hexagon/termbits.h
new file mode 100644
index 0000000..6ac6581
--- /dev/null
+++ b/linux-user/hexagon/termbits.h
@@ -0,0 +1,231 @@ 
+/* from asm/termbits.h */
+/* Copied/modified from i386 */
+
+#define TARGET_NCCS 32
+
+struct target_termios {
+    unsigned int c_iflag;               /* input mode flags */
+    unsigned int c_oflag;               /* output mode flags */
+    unsigned int c_cflag;               /* control mode flags */
+    unsigned int c_lflag;               /* local mode flags */
+    unsigned char c_line;                    /* line discipline */
+    unsigned char c_cc[TARGET_NCCS];                /* control characters */
+};
+
+/* c_iflag bits */
+#define TARGET_IGNBRK  0000001
+#define TARGET_BRKINT  0000002
+#define TARGET_IGNPAR  0000004
+#define TARGET_PARMRK  0000010
+#define TARGET_INPCK   0000020
+#define TARGET_ISTRIP  0000040
+#define TARGET_INLCR   0000100
+#define TARGET_IGNCR   0000200
+#define TARGET_ICRNL   0000400
+#define TARGET_IUCLC   0001000
+#define TARGET_IXON    0002000
+#define TARGET_IXANY   0004000
+#define TARGET_IXOFF   0010000
+#define TARGET_IMAXBEL 0020000
+#define TARGET_IUTF8   0040000
+
+/* c_oflag bits */
+#define TARGET_OPOST   0000001
+#define TARGET_OLCUC   0000002
+#define TARGET_ONLCR   0000004
+#define TARGET_OCRNL   0000010
+#define TARGET_ONOCR   0000020
+#define TARGET_ONLRET  0000040
+#define TARGET_OFILL   0000100
+#define TARGET_OFDEL   0000200
+#define TARGET_NLDLY   0000400
+#define   TARGET_NL0   0000000
+#define   TARGET_NL1   0000400
+#define TARGET_CRDLY   0003000
+#define   TARGET_CR0   0000000
+#define   TARGET_CR1   0001000
+#define   TARGET_CR2   0002000
+#define   TARGET_CR3   0003000
+#define TARGET_TABDLY  0014000
+#define   TARGET_TAB0  0000000
+#define   TARGET_TAB1  0004000
+#define   TARGET_TAB2  0010000
+#define   TARGET_TAB3  0014000
+#define   TARGET_XTABS 0014000
+#define TARGET_BSDLY   0020000
+#define   TARGET_BS0   0000000
+#define   TARGET_BS1   0020000
+#define TARGET_VTDLY   0040000
+#define   TARGET_VT0   0000000
+#define   TARGET_VT1   0040000
+#define TARGET_FFDLY   0100000
+#define   TARGET_FF0   0000000
+#define   TARGET_FF1   0100000
+
+/* c_cflag bit meaning */
+#define TARGET_CBAUD   0010017
+#define  TARGET_B0     0000000         /* hang up */
+#define  TARGET_B50    0000001
+#define  TARGET_B75    0000002
+#define  TARGET_B110   0000003
+#define  TARGET_B134   0000004
+#define  TARGET_B150   0000005
+#define  TARGET_B200   0000006
+#define  TARGET_B300   0000007
+#define  TARGET_B600   0000010
+#define  TARGET_B1200  0000011
+#define  TARGET_B1800  0000012
+#define  TARGET_B2400  0000013
+#define  TARGET_B4800  0000014
+#define  TARGET_B9600  0000015
+#define  TARGET_B19200 0000016
+#define  TARGET_B38400 0000017
+#define TARGET_EXTA B19200
+#define TARGET_EXTB B38400
+#define TARGET_CSIZE   0000060
+#define   TARGET_CS5   0000000
+#define   TARGET_CS6   0000020
+#define   TARGET_CS7   0000040
+#define   TARGET_CS8   0000060
+#define TARGET_CSTOPB  0000100
+#define TARGET_CREAD   0000200
+#define TARGET_PARENB  0000400
+#define TARGET_PARODD  0001000
+#define TARGET_HUPCL   0002000
+#define TARGET_CLOCAL  0004000
+#define TARGET_CBAUDEX 0010000
+#define  TARGET_B57600  0010001
+#define  TARGET_B115200 0010002
+#define  TARGET_B230400 0010003
+#define  TARGET_B460800 0010004
+#define  TARGET_B500000 0010005
+#define  TARGET_B576000 0010006
+#define  TARGET_B921600 0010007
+#define  TARGET_B1000000 0010010
+#define  TARGET_B1152000 0010011
+#define  TARGET_B1500000 0010012
+#define  TARGET_B2000000 0010013
+#define  TARGET_B2500000 0010014
+#define  TARGET_B3000000 0010015
+#define  TARGET_B3500000 0010016
+#define  TARGET_B4000000 0010017
+#define TARGET_CIBAUD    002003600000  /* input baud rate (not used) */
+#define TARGET_CMSPAR    010000000000  /* mark or space (stick) parity */
+#define TARGET_CRTSCTS   020000000000  /* flow control */
+
+/* c_lflag bits */
+#define TARGET_ISIG    0000001
+#define TARGET_ICANON  0000002
+#define TARGET_XCASE   0000004
+#define TARGET_ECHO    0000010
+#define TARGET_ECHOE   0000020
+#define TARGET_ECHOK   0000040
+#define TARGET_ECHONL  0000100
+#define TARGET_NOFLSH  0000200
+#define TARGET_TOSTOP  0000400
+#define TARGET_ECHOCTL 0001000
+#define TARGET_ECHOPRT 0002000
+#define TARGET_ECHOKE  0004000
+#define TARGET_FLUSHO  0010000
+#define TARGET_PENDIN  0040000
+#define TARGET_IEXTEN  0100000
+
+/* c_cc character offsets */
+#define TARGET_VINTR    0
+#define TARGET_VQUIT    1
+#define TARGET_VERASE   2
+#define TARGET_VKILL    3
+#define TARGET_VEOF     4
+#define TARGET_VTIME    5
+#define TARGET_VMIN     6
+#define TARGET_VSWTC    7
+#define TARGET_VSTART   8
+#define TARGET_VSTOP    9
+#define TARGET_VSUSP    10
+#define TARGET_VEOL     11
+#define TARGET_VREPRINT 12
+#define TARGET_VDISCARD 13
+#define TARGET_VWERASE  14
+#define TARGET_VLNEXT   15
+#define TARGET_VEOL2    16
+
+/* ioctls */
+
+#define TARGET_TCGETS           0x5401
+#define TARGET_TCSETS           0x5402
+#define TARGET_TCSETSW          0x5403
+#define TARGET_TCSETSF          0x5404
+#define TARGET_TCGETA           0x5405
+#define TARGET_TCSETA           0x5406
+#define TARGET_TCSETAW          0x5407
+#define TARGET_TCSETAF          0x5408
+#define TARGET_TCSBRK           0x5409
+#define TARGET_TCXONC           0x540A
+#define TARGET_TCFLSH           0x540B
+
+#define TARGET_TIOCEXCL         0x540C
+#define TARGET_TIOCNXCL         0x540D
+#define TARGET_TIOCSCTTY        0x540E
+#define TARGET_TIOCGPGRP        0x540F
+#define TARGET_TIOCSPGRP        0x5410
+#define TARGET_TIOCOUTQ         0x5411
+#define TARGET_TIOCSTI          0x5412
+#define TARGET_TIOCGWINSZ       0x5413
+#define TARGET_TIOCSWINSZ       0x5414
+#define TARGET_TIOCMGET         0x5415
+#define TARGET_TIOCMBIS         0x5416
+#define TARGET_TIOCMBIC         0x5417
+#define TARGET_TIOCMSET         0x5418
+#define TARGET_TIOCGSOFTCAR     0x5419
+#define TARGET_TIOCSSOFTCAR     0x541A
+#define TARGET_FIONREAD         0x541B
+#define TARGET_TIOCINQ          TARGET_FIONREAD
+#define TARGET_TIOCLINUX        0x541C
+#define TARGET_TIOCCONS         0x541D
+#define TARGET_TIOCGSERIAL      0x541E
+#define TARGET_TIOCSSERIAL      0x541F
+#define TARGET_TIOCPKT          0x5420
+#define TARGET_FIONBIO          0x5421
+#define TARGET_TIOCNOTTY        0x5422
+#define TARGET_TIOCSETD         0x5423
+#define TARGET_TIOCGETD         0x5424
+#define TARGET_TCSBRKP          0x5425     /* Needed for POSIX tcsendbreak() */
+#define TARGET_TIOCTTYGSTRUCT   0x5426     /* For debugging only */
+#define TARGET_TIOCSBRK         0x5427     /* BSD compatibility */
+#define TARGET_TIOCCBRK         0x5428     /* BSD compatibility */
+#define TARGET_TIOCGSID         0x5429     /* Return the session ID of FD */
+/* Get Pty Number (of pty-mux device) */
+#define TARGET_TIOCGPTN         TARGET_IOR('T', 0x30, unsigned int)
+#define TARGET_TIOCSPTLCK       TARGET_IOW('T', 0x31, int) /* Lock/unlock Pty */
+#define TARGET_TIOCGPTPEER      TARGET_IO('T', 0x41) /* Safely open the slave */
+
+#define TARGET_FIONCLEX         0x5450  /* these numbers need to be adjusted. */
+#define TARGET_FIOCLEX          0x5451
+#define TARGET_FIOASYNC         0x5452
+#define TARGET_TIOCSERCONFIG    0x5453
+#define TARGET_TIOCSERGWILD     0x5454
+#define TARGET_TIOCSERSWILD     0x5455
+#define TARGET_TIOCGLCKTRMIOS   0x5456
+#define TARGET_TIOCSLCKTRMIOS   0x5457
+#define TARGET_TIOCSERGSTRUCT   0x5458 /* For debugging only */
+#define TARGET_TIOCSERGETLSR   0x5459 /* Get line status register */
+#define TARGET_TIOCSERGETMULTI 0x545A /* Get multiport config  */
+#define TARGET_TIOCSERSETMULTI 0x545B /* Set multiport config */
+
+/* wait for a change on serial input line(s) */
+#define TARGET_TIOCMIWAIT      0x545C
+/* read serial port inline interrupt counts */
+#define TARGET_TIOCGICOUNT     0x545D
+#define TARGET_TIOCGHAYESESP   0x545E  /* Get Hayes ESP configuration */
+#define TARGET_TIOCSHAYESESP   0x545F  /* Set Hayes ESP configuration */
+
+/* Used for packet mode */
+#define TARGET_TIOCPKT_DATA              0
+#define TARGET_TIOCPKT_FLUSHREAD         1
+#define TARGET_TIOCPKT_FLUSHWRITE        2
+#define TARGET_TIOCPKT_STOP              4
+#define TARGET_TIOCPKT_START             8
+#define TARGET_TIOCPKT_NOSTOP           16
+#define TARGET_TIOCPKT_DOSTOP           32
+
+#define TARGET_TIOCSER_TEMT    0x01    /* Transmitter physically empty */
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 5ca6d62..ce3d27f 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -72,6 +72,14 @@  static uint8_t host_to_target_signal_table[_NSIG] = {
        over a single host signal.  */
     [__SIGRTMIN] = __SIGRTMAX,
     [__SIGRTMAX] = __SIGRTMIN,
+#ifdef TARGET_HEXAGON
+    /*
+     * Hexagon uses the same signal for pthread cancel as the host pthreads,
+     * so cannot be overridden.
+     * Therefore, we map Hexagon signal to a different host signal.
+     */
+    [__SIGRTMAX - 1] = __SIGRTMIN + 1,
+#endif
 };
 static uint8_t target_to_host_signal_table[_NSIG];
 
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ab9d933..a5ca2ec 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -507,6 +507,8 @@  static inline int regpairs_aligned(void *cpu_env, int num)
 }
 #elif defined(TARGET_XTENSA)
 static inline int regpairs_aligned(void *cpu_env, int num) { return 1; }
+#elif defined(TARGET_HEXAGON)
+static inline int regpairs_aligned(void *cpu_env, int num) { return 1; }
 #else
 static inline int regpairs_aligned(void *cpu_env, int num) { return 0; }
 #endif
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 98c2119..1c80773 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -102,6 +102,14 @@ 
 #define TARGET_IOC_WRITE  2U
 #define TARGET_IOC_READ   1U
 
+#elif defined(TARGET_HEXAGON)
+
+#define TARGET_IOC_SIZEBITS     14
+
+#define TARGET_IOC_NONE   0U
+#define TARGET_IOC_WRITE  1U
+#define TARGET_IOC_READ          2U
+
 #else
 #error unsupported CPU
 #endif
@@ -2085,6 +2093,31 @@  struct target_stat64 {
     uint64_t   st_ino;
 };
 
+#elif defined(TARGET_HEXAGON)
+
+struct target_stat {
+    unsigned long long st_dev;
+    unsigned long long st_ino;
+    unsigned int st_mode;
+    unsigned int st_nlink;
+    unsigned int st_uid;
+    unsigned int st_gid;
+    unsigned long long st_rdev;
+    target_ulong __pad1;
+    long long st_size;
+    target_long st_blksize;
+    int __pad2;
+    long long st_blocks;
+
+    target_long target_st_atime;
+    target_long target_st_atime_nsec;
+    target_long target_st_mtime;
+    target_long target_st_mtime_nsec;
+    target_long target_st_ctime;
+    target_long target_st_ctime_nsec;
+    int __unused[2];
+};
+
 #else
 #error unsupported CPU
 #endif
diff --git a/target/hexagon/Makefile.objs b/target/hexagon/Makefile.objs
new file mode 100644
index 0000000..dfab6ee
--- /dev/null
+++ b/target/hexagon/Makefile.objs
@@ -0,0 +1,6 @@ 
+obj-y += \
+    cpu.o \
+    translate.o \
+    op_helper.o
+
+CFLAGS += -I$(SRC_PATH)/target/hexagon/imported
diff --git a/target/hexagon/cpu-param.h b/target/hexagon/cpu-param.h
new file mode 100644
index 0000000..6cfe93f
--- /dev/null
+++ b/target/hexagon/cpu-param.h
@@ -0,0 +1,11 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+
+#ifndef HEXAGON_CPU_PARAM_H
+#define HEXAGON_CPU_PARAM_H
+
+# define TARGET_PHYS_ADDR_SPACE_BITS 36
+
+#define NB_MMU_MODES 1
+
+#endif
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
new file mode 100644
index 0000000..03e989c
--- /dev/null
+++ b/target/hexagon/cpu.c
@@ -0,0 +1,283 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+/*
+ * QEMU Hexagon
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "cpu.h"
+#include "exec/exec-all.h"
+#include "qapi/error.h"
+#include "migration/vmstate.h"
+
+static void hexagon_v67_cpu_init(Object *obj)
+{
+}
+
+static ObjectClass *hexagon_cpu_class_by_name(const char *cpu_model)
+{
+    ObjectClass *oc;
+    char *typename;
+    char **cpuname;
+
+    cpuname = g_strsplit(cpu_model, ",", 1);
+    typename = g_strdup_printf(HEXAGON_CPU_TYPE_NAME("%s"), cpuname[0]);
+    oc = object_class_by_name(typename);
+    g_strfreev(cpuname);
+    g_free(typename);
+    if (!oc || !object_class_dynamic_cast(oc, TYPE_HEXAGON_CPU) ||
+        object_class_is_abstract(oc)) {
+        return NULL;
+    }
+    return oc;
+}
+
+const char * const hexagon_regnames[] = {
+   "r0", "r1",  "r2",  "r3",  "r4",   "r5",  "r6",  "r7",
+   "r8", "r9",  "r10", "r11", "r12",  "r13", "r14", "r15",
+  "r16", "r17", "r18", "r19", "r20",  "r21", "r22", "r23",
+  "r24", "r25", "r26", "r27", "r28",  "r29", "r30", "r31",
+  "sa0", "lc0", "sa1", "lc1", "p3_0", "c5",  "m0",  "m1",
+  "usr", "pc",  "ugp", "gp",  "cs0",  "cs1", "c14", "c15",
+  "c16", "c17", "c18", "c19", "c20",  "c21", "c22", "c23",
+  "c24", "c25", "c26", "c27", "c28",  "c29", "c30", "c31",
+};
+
+const char * const hexagon_prednames[] = {
+  "p0 ", "p1 ", "p2 ", "p3 "
+};
+
+static inline target_ulong hack_stack_ptrs(CPUHexagonState *env,
+                                           target_ulong addr)
+{
+    target_ulong stack_start = env->stack_start;
+    target_ulong stack_size = 0x10000;
+    target_ulong stack_adjust = env->stack_adjust;
+
+    if (stack_start + 0x1000 >= addr && addr >= (stack_start - stack_size)) {
+        return addr - stack_adjust;
+    }
+    return addr;
+}
+
+static void print_reg(FILE *f, CPUHexagonState *env, int regnum)
+{
+    fprintf(f, "  %s = 0x" TARGET_FMT_lx "\n",
+        hexagon_regnames[regnum],
+        regnum < 32 ? hack_stack_ptrs(env, env->gpr[regnum])
+                    : env->gpr[regnum]);
+}
+
+static void hexagon_dump(CPUHexagonState *env, FILE *f)
+{
+    static target_ulong last_pc;
+    int i;
+
+    /*
+     * When comparing with LLDB, it doesn't step through single-cycle
+     * hardware loops the same way.  So, we just skip them here
+     */
+    if (env->gpr[HEX_REG_PC] == last_pc) {
+        return;
+    }
+    last_pc = env->gpr[HEX_REG_PC];
+    fprintf(f, "General Purpose Registers = {\n");
+    for (i = 0; i < 32; i++) {
+        print_reg(f, env, i);
+    }
+    print_reg(f, env, HEX_REG_SA0);
+    print_reg(f, env, HEX_REG_LC0);
+    print_reg(f, env, HEX_REG_SA1);
+    print_reg(f, env, HEX_REG_LC1);
+    print_reg(f, env, HEX_REG_M0);
+    print_reg(f, env, HEX_REG_M1);
+    print_reg(f, env, HEX_REG_USR);
+    print_reg(f, env, HEX_REG_P3_0);
+#ifdef FIXME
+    /*
+     * LLDB bug swaps gp and ugp
+     * FIXME when the LLDB bug is fixed
+     */
+    print_reg(f, env, HEX_REG_GP);
+    print_reg(f, env, HEX_REG_UGP);
+#else
+    fprintf(f, "  %s = 0x" TARGET_FMT_lx "\n",
+        hexagon_regnames[HEX_REG_GP],
+        hack_stack_ptrs(env, env->gpr[HEX_REG_UGP]));
+    fprintf(f, "  %s = 0x" TARGET_FMT_lx "\n",
+        hexagon_regnames[HEX_REG_UGP],
+        hack_stack_ptrs(env, env->gpr[HEX_REG_GP]));
+#endif
+    print_reg(f, env, HEX_REG_PC);
+#ifdef FIXME
+    /*
+     * Not modelled in qemu, print junk to minimize the diff's
+     * with LLDB output
+     */
+    print_reg(f, env, HEX_REG_CAUSE);
+    print_reg(f, env, HEX_REG_BADVA);
+    print_reg(f, env, HEX_REG_CS0);
+    print_reg(f, env, HEX_REG_CS1);
+#else
+    fprintf(f, "  cause = 0x000000db\n");
+    fprintf(f, "  badva = 0x00000000\n");
+    fprintf(f, "  cs0 = 0x00000000\n");
+    fprintf(f, "  cs1 = 0x00000000\n");
+#endif
+    fprintf(f, "}\n");
+}
+
+static void hexagon_dump_state(CPUState *cs, FILE *f, int flags)
+{
+    HexagonCPU *cpu = HEXAGON_CPU(cs);
+    CPUHexagonState *env = &cpu->env;
+
+    hexagon_dump(env, f);
+}
+
+void hexagon_debug(CPUHexagonState *env)
+{
+    hexagon_dump(env, stdout);
+}
+
+static void hexagon_cpu_set_pc(CPUState *cs, vaddr value)
+{
+    HexagonCPU *cpu = HEXAGON_CPU(cs);
+    CPUHexagonState *env = &cpu->env;
+    env->gpr[HEX_REG_PC] = value;
+}
+
+static void hexagon_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb)
+{
+    HexagonCPU *cpu = HEXAGON_CPU(cs);
+    CPUHexagonState *env = &cpu->env;
+    env->gpr[HEX_REG_PC] = tb->pc;
+}
+
+static bool hexagon_cpu_has_work(CPUState *cs)
+{
+    return true;
+}
+
+void restore_state_to_opc(CPUHexagonState *env, TranslationBlock *tb,
+                          target_ulong *data)
+{
+    env->gpr[HEX_REG_PC] = data[0];
+}
+
+static void hexagon_cpu_reset(CPUState *cs)
+{
+    HexagonCPU *cpu = HEXAGON_CPU(cs);
+    HexagonCPUClass *mcc = HEXAGON_CPU_GET_CLASS(cpu);
+
+    mcc->parent_reset(cs);
+}
+
+static void hexagon_cpu_disas_set_info(CPUState *s, disassemble_info *info)
+{
+    printf("ERROR: disas_set_info not implemented\n");
+    g_assert_not_reached();
+}
+
+static void hexagon_cpu_realize(DeviceState *dev, Error **errp)
+{
+    CPUState *cs = CPU(dev);
+    HexagonCPUClass *mcc = HEXAGON_CPU_GET_CLASS(dev);
+    Error *local_err = NULL;
+
+    cpu_exec_realizefn(cs, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    qemu_init_vcpu(cs);
+    cpu_reset(cs);
+
+    mcc->parent_realize(dev, errp);
+}
+
+static void hexagon_cpu_init(Object *obj)
+{
+    HexagonCPU *cpu = HEXAGON_CPU(obj);
+
+    cpu_set_cpustate_pointers(cpu);
+}
+
+static bool hexagon_tlb_fill(CPUState *cs, vaddr address, int size,
+                             MMUAccessType access_type, int mmu_idx,
+                             bool probe, uintptr_t retaddr)
+{
+#ifdef CONFIG_USER_ONLY
+    switch (access_type) {
+    case MMU_INST_FETCH:
+        cs->exception_index = HEX_EXCP_FETCH_NO_UPAGE;
+        break;
+    case MMU_DATA_LOAD:
+        cs->exception_index = HEX_EXCP_PRIV_NO_UREAD;
+        break;
+    case MMU_DATA_STORE:
+        cs->exception_index = HEX_EXCP_PRIV_NO_UWRITE;
+        break;
+    }
+    cpu_loop_exit_restore(cs, retaddr);
+#else
+#error System mode not implemented for Hexagon
+#endif
+}
+
+static const VMStateDescription vmstate_hexagon_cpu = {
+    .name = "cpu",
+    .unmigratable = 1,
+};
+
+static void hexagon_cpu_class_init(ObjectClass *c, void *data)
+{
+    HexagonCPUClass *mcc = HEXAGON_CPU_CLASS(c);
+    CPUClass *cc = CPU_CLASS(c);
+    DeviceClass *dc = DEVICE_CLASS(c);
+
+    mcc->parent_realize = dc->realize;
+    dc->realize = hexagon_cpu_realize;
+
+    mcc->parent_reset = cc->reset;
+    cc->reset = hexagon_cpu_reset;
+
+    cc->class_by_name = hexagon_cpu_class_by_name;
+    cc->has_work = hexagon_cpu_has_work;
+    cc->dump_state = hexagon_dump_state;
+    cc->set_pc = hexagon_cpu_set_pc;
+    cc->synchronize_from_tb = hexagon_cpu_synchronize_from_tb;
+    cc->gdb_core_xml_file = "hexagon-core.xml";
+    cc->gdb_stop_before_watchpoint = true;
+    cc->disas_set_info = hexagon_cpu_disas_set_info;
+#ifdef CONFIG_TCG
+    cc->tcg_initialize = hexagon_translate_init;
+    cc->tlb_fill = hexagon_tlb_fill;
+#endif
+    /* For now, mark unmigratable: */
+    cc->vmsd = &vmstate_hexagon_cpu;
+}
+
+#define DEFINE_CPU(type_name, initfn)      \
+    {                                      \
+        .name = type_name,                 \
+        .parent = TYPE_HEXAGON_CPU,        \
+        .instance_init = initfn            \
+    }
+
+static const TypeInfo hexagon_cpu_type_infos[] = {
+    {
+        .name = TYPE_HEXAGON_CPU,
+        .parent = TYPE_CPU,
+        .instance_size = sizeof(HexagonCPU),
+        .instance_init = hexagon_cpu_init,
+        .abstract = true,
+        .class_size = sizeof(HexagonCPUClass),
+        .class_init = hexagon_cpu_class_init,
+    },
+    DEFINE_CPU(TYPE_HEXAGON_CPU_V67,              hexagon_v67_cpu_init),
+};
+
+DEFINE_TYPES(hexagon_cpu_type_infos)
diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h
new file mode 100644
index 0000000..35dddf6
--- /dev/null
+++ b/target/hexagon/cpu.h
@@ -0,0 +1,146 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+/*
+ * HEXAGON CPU
+ *
+ */
+
+#ifndef HEXAGON_CPU_H
+#define HEXAGON_CPU_H
+
+/* FIXME - Change this to a command-line option */
+#ifdef FIXME
+#define DEBUG_HEX
+#endif
+#ifdef FIXME
+#define COUNT_HEX_HELPERS
+#endif
+
+/* Forward declaration needed by some of the header files */
+typedef struct CPUHexagonState CPUHexagonState;
+
+#include <fenv.h>
+#include "qemu/osdep.h"
+#include "global_types.h"
+#include "max.h"
+#include "iss_ver_registers.h"
+
+#define TARGET_PAGE_BITS 16     /* 64K pages */
+/*
+ * For experimenting with oslib (4K pages)
+ * #define TARGET_PAGE_BITS 12
+ */
+#define TARGET_LONG_BITS 32
+#define TARGET_VIRT_ADDR_SPACE_BITS 32
+
+#include <time.h>
+#include "qemu/compiler.h"
+#include "qemu-common.h"
+#include "exec/cpu-defs.h"
+#include "regs.h"
+
+#define TYPE_HEXAGON_CPU "hexagon-cpu"
+
+#define HEXAGON_CPU_TYPE_SUFFIX "-" TYPE_HEXAGON_CPU
+#define HEXAGON_CPU_TYPE_NAME(name) (name HEXAGON_CPU_TYPE_SUFFIX)
+#define CPU_RESOLVING_TYPE TYPE_HEXAGON_CPU
+
+#define TYPE_HEXAGON_CPU_V67             HEXAGON_CPU_TYPE_NAME("v67")
+
+#define MMU_USER_IDX 0
+
+#define TRANSLATE_FAIL 1
+#define TRANSLATE_SUCCESS 0
+
+struct MemLog {
+    vaddr_t va;
+    size1u_t width;
+    size4u_t data32;
+    size8u_t data64;
+};
+
+struct CPUHexagonState {
+    target_ulong gpr[TOTAL_PER_THREAD_REGS];
+    target_ulong pred[NUM_PREGS];
+
+    /* For comparing with LLDB on target - see hack_stack_ptrs function */
+    target_ulong stack_start;
+    target_ulong stack_adjust;
+};
+
+#define HEXAGON_CPU_CLASS(klass) \
+    OBJECT_CLASS_CHECK(HexagonCPUClass, (klass), TYPE_HEXAGON_CPU)
+#define HEXAGON_CPU(obj) \
+    OBJECT_CHECK(HexagonCPU, (obj), TYPE_HEXAGON_CPU)
+#define HEXAGON_CPU_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(HexagonCPUClass, (obj), TYPE_HEXAGON_CPU)
+
+typedef struct HexagonCPUClass {
+    /*< private >*/
+    CPUClass parent_class;
+    /*< public >*/
+    DeviceRealize parent_realize;
+    void (*parent_reset)(CPUState *cpu);
+} HexagonCPUClass;
+
+typedef struct HexagonCPU {
+    /*< private >*/
+    CPUState parent_obj;
+    /*< public >*/
+    CPUNegativeOffsetState neg;
+    CPUHexagonState env;
+} HexagonCPU;
+
+static inline HexagonCPU *hexagon_env_get_cpu(CPUHexagonState *env)
+{
+    return container_of(env, HexagonCPU, env);
+}
+
+
+#include "cpu_bits.h"
+
+extern const char * const hexagon_regnames[];
+extern const char * const hexagon_prednames[];
+
+#define ENV_GET_CPU(e)  CPU(hexagon_env_get_cpu(e))
+#define ENV_OFFSET      offsetof(HexagonCPU, env)
+
+int hexagon_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
+int hexagon_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
+
+#define cpu_signal_handler cpu_hexagon_signal_handler
+
+int cpu_hexagon_signal_handler(int host_signum, void *pinfo, void *puc);
+
+void QEMU_NORETURN do_raise_exception_err(CPUHexagonState *env,
+                                          uint32_t exception, uintptr_t pc);
+
+#define TB_FLAGS_MMU_MASK  3
+
+static inline void cpu_get_tb_cpu_state(CPUHexagonState *env, target_ulong *pc,
+                                        target_ulong *cs_base, uint32_t *flags)
+{
+    *pc = env->gpr[HEX_REG_PC];
+    *cs_base = 0;
+#ifdef CONFIG_USER_ONLY
+    *flags = 0;
+#else
+#error System mode not supported on Hexagon yet
+#endif
+}
+
+void hexagon_translate_init(void);
+void hexagon_debug(CPUHexagonState *env);
+void hexagon_debug_vreg(CPUHexagonState *env, int regnum);
+void hexagon_debug_qreg(CPUHexagonState *env, int regnum);
+
+typedef struct CPUHexagonState CPUArchState;
+typedef HexagonCPU ArchCPU;
+
+#ifdef COUNT_HEX_HELPERS
+extern void print_helper_counts(void);
+#endif
+
+#include "exec/cpu-all.h"
+
+#endif /* HEXAGON_CPU_H */
diff --git a/target/hexagon/cpu_bits.h b/target/hexagon/cpu_bits.h
new file mode 100644
index 0000000..043f2b5
--- /dev/null
+++ b/target/hexagon/cpu_bits.h
@@ -0,0 +1,15 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+#ifndef HEXAGON_CPU_BITS_H
+#define HEXAGON_CPU_BITS_H
+
+#define HEX_EXCP_FETCH_NO_UPAGE  0x012
+#define HEX_EXCP_PRIV_NO_UREAD   0x024
+#define HEX_EXCP_PRIV_NO_UWRITE  0x025
+
+#define HEX_EXCP_TRAP0           0x172
+#define HEX_EXCP_TRAP1           0x173
+#define HEX_EXCP_SC4             0x100
+#define HEX_EXCP_SC8             0x200
+
+#endif
diff --git a/target/hexagon/helper.h b/target/hexagon/helper.h
new file mode 100644
index 0000000..76e34f8
--- /dev/null
+++ b/target/hexagon/helper.h
@@ -0,0 +1,3 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+DEF_HELPER_2(raise_exception, noreturn, env, i32)
diff --git a/target/hexagon/imported/global_types.h b/target/hexagon/imported/global_types.h
new file mode 100644
index 0000000..b4f4371
--- /dev/null
+++ b/target/hexagon/imported/global_types.h
@@ -0,0 +1,25 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+
+#ifndef _GLOBAL_TYPES_H_
+#define _GLOBAL_TYPES_H_ 1
+
+
+typedef unsigned char size1u_t;
+typedef char size1s_t;
+typedef unsigned short int size2u_t;
+typedef short size2s_t;
+typedef unsigned int size4u_t;
+typedef int size4s_t;
+typedef unsigned long long int size8u_t;
+typedef long long int size8s_t;
+typedef size8u_t paddr_t;
+typedef size4u_t vaddr_t;
+typedef size8u_t pcycles_t;
+
+typedef struct size16s {
+    size8s_t hi;
+    size8u_t lo;
+} size16s_t;
+
+#endif /* _GLOBAL_TYPES_H_ */
diff --git a/target/hexagon/imported/iss_ver_registers.h b/target/hexagon/imported/iss_ver_registers.h
new file mode 100644
index 0000000..08e34d2
--- /dev/null
+++ b/target/hexagon/imported/iss_ver_registers.h
@@ -0,0 +1,183 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+#define HEX_REG_R00 0
+#define HEX_REG_R01 1
+#define HEX_REG_R02 2
+#define HEX_REG_R03 3
+#define HEX_REG_R04 4
+#define HEX_REG_R05 5
+#define HEX_REG_R06 6
+#define HEX_REG_R07 7
+#define HEX_REG_R08 8
+#define HEX_REG_R09 9
+#define HEX_REG_R10 10
+#define HEX_REG_R11 11
+#define HEX_REG_R12 12
+#define HEX_REG_R13 13
+#define HEX_REG_R14 14
+#define HEX_REG_R15 15
+#define HEX_REG_R16 16
+#define HEX_REG_R17 17
+#define HEX_REG_R18 18
+#define HEX_REG_R19 19
+#define HEX_REG_R20 20
+#define HEX_REG_R21 21
+#define HEX_REG_R22 22
+#define HEX_REG_R23 23
+#define HEX_REG_R24 24
+#define HEX_REG_R25 25
+#define HEX_REG_R26 26
+#define HEX_REG_R27 27
+#define HEX_REG_R28 28
+#define HEX_REG_R29 29
+#define HEX_REG_SP 29
+#define HEX_REG_FP 30
+#define HEX_REG_R30 30
+#define HEX_REG_LR 31
+#define HEX_REG_R31 31
+#define HEX_REG_SA0 32
+#define HEX_REG_LC0 33
+#define HEX_REG_SA1 34
+#define HEX_REG_LC1 35
+#define HEX_REG_P3_0 36
+#define HEX_REG_M0 38
+#define HEX_REG_M1 39
+#define HEX_REG_USR 40
+#define HEX_REG_PC 41
+#define HEX_REG_UGP 42
+#define HEX_REG_GP 43
+#define HEX_REG_CS0 44
+#define HEX_REG_CS1 45
+#define HEX_REG_UPCYCLELO 46
+#define HEX_REG_UPCYCLEHI 47
+#define HEX_REG_FRAMELIMIT 48
+#define HEX_REG_FRAMEKEY 49
+#define HEX_REG_PKTCNTLO 50
+#define HEX_REG_PKTCNTHI 51
+/* Use reserved control registers for qemu execution counts */
+#define HEX_REG_QEMU_PKT_CNT 52
+#define HEX_REG_QEMU_INSN_CNT 53
+#define HEX_REG_QEMU_HVX_CNT 54
+#define HEX_REG_UTIMERLO 62
+#define HEX_REG_UTIMERHI 63
+#define HEX_REG_SGP0 64
+#define HEX_REG_SGP1 65
+#define HEX_REG_STID 66
+#define HEX_REG_ELR 67
+#define HEX_REG_BADVA0 68
+#define HEX_REG_BADVA1 69
+#define HEX_REG_SSR 70
+#define HEX_REG_CCR 71
+#define HEX_REG_HTID 72
+#define HEX_REG_BADVA 73
+#define HEX_REG_IMASK 74
+#define HEX_REG_GEVB 75
+
+/*
+ * Not used in qemu
+ */
+#ifdef FIXME
+// New interrupts, keep old defines for the time being
+#define HEX_REG_GELR 80
+#define HEX_REG_GSR 81 
+#define HEX_REG_GOSP 82
+#define HEX_REG_GBADVA 83
+
+#define HEX_REG_G0 80
+#define HEX_REG_G1 81
+#define HEX_REG_G2 82
+#define HEX_REG_G3 83
+
+#define HEX_REG_G4 84
+#define HEX_REG_G5 85
+#define HEX_REG_G6 86
+#define HEX_REG_G7 87
+#define HEX_REG_G8 88
+#define HEX_REG_G9 89
+#define HEX_REG_G10 90
+#define HEX_REG_G11 91
+#define HEX_REG_G12 92
+#define HEX_REG_G13 93
+#define HEX_REG_G14 94
+#define HEX_REG_G15 95
+#define HEX_REG_G16 96
+#define HEX_REG_G17 97
+#define HEX_REG_G18 98
+#define HEX_REG_G19 99
+#define HEX_REG_G20 100
+#define HEX_REG_G21 101
+#define HEX_REG_G22 102
+#define HEX_REG_G23 103
+#define HEX_REG_G24 104
+#define HEX_REG_G25 105
+#define HEX_REG_G26 106
+#define HEX_REG_G27 107
+#define HEX_REG_G28 108
+#define HEX_REG_G29 109
+#define HEX_REG_G30 110
+#define HEX_REG_G31 111
+#define GREG_EVB 0
+#define GREG_MODECTL 1
+#define GREG_SYSCFG 2
+#define GREG_EVB1 3
+#define GREG_IPENDAD 4
+#define GREG_VID 5
+#define GREG_VID1 6
+#define GREG_BESTWAIT 7
+#define GREG_SCHEDCFG 9
+#define GREG_CFGBASE 11
+#define GREG_DIAG 12
+#define GREG_REV 13
+#define GREG_PCYCLELO 14
+#define GREG_PCYCLEHI 15
+#define GREG_ISDBST 16
+#define GREG_ISDBCFG0 17
+#define GREG_ISDBCFG1 18
+#define GREG_LIVELOCK 19
+#define GREG_BRKPTPC0 20
+#define GREG_BRKPTCFG0 21
+#define GREG_BRKPTPC1 22
+#define GREG_BRKPTCFG1 23
+#define GREG_ISDBMBXIN 24
+#define GREG_ISDBMBXOUT 25
+#define GREG_ISDBEN 26
+#define GREG_ISDBGPR 27
+#define GREG_PMUCNT4 28
+#define GREG_PMUCNT5 29
+#define GREG_PMUCNT6 30
+#define GREG_PMUCNT7 31
+#define GREG_PMUCNT0 32
+#define GREG_PMUCNT1 33
+#define GREG_PMUCNT2 34
+#define GREG_PMUCNT3 35
+#define GREG_PMUEVTCFG 36
+#define GREG_PMUSTID0 37
+#define GREG_PMUEVTCFG1 38
+#define GREG_PMUSTID1 39
+#define GREG_TIMERLO 40
+#define GREG_TIMERHI 41
+#define GREG_PMUCFG 42
+#define GREG_AVS 43
+// GREG_AVS renamed to GREG_RGDR2 in v67
+#define GREG_RGDR2 43
+#define GREG_RGDR 44
+#define GREG_TURKEY 45
+#define GREG_DUCK 46
+#define GREG_CHICKEN 47
+#define GREG_COMMIT1T 48
+#define GREG_COMMIT2T 49
+#define GREG_COMMIT3T 50
+#define GREG_COMMIT4T 51
+#define GREG_COMMIT5T 52
+#define GREG_COMMIT6T 53
+#define GREG_PCYCLE1T 54
+#define GREG_PCYCLE2T 55
+#define GREG_PCYCLE3T 56
+#define GREG_PCYCLE4T 57
+#define GREG_PCYCLE5T 58
+#define GREG_PCYCLE6T 59
+#define GREG_STFINST  60
+#define GREG_ISDBCMD  61
+#define GREG_ISDBVER  62
+#define GREG_BRKPTINFO 63
+#endif
diff --git a/target/hexagon/imported/max.h b/target/hexagon/imported/max.h
new file mode 100644
index 0000000..62af24d
--- /dev/null
+++ b/target/hexagon/imported/max.h
@@ -0,0 +1,78 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+/*
+ *
+ * This file contains architecture parameters
+ *
+ * 
+ *
+ */
+
+#ifndef _MAX_H
+#define _MAX_H
+
+
+#define INSTRUCTIONS_MAX 7		/* 2 pairs + loopend */
+#define LOOPNEST_MAX 2
+#define IMMEDS_MAX 2
+
+
+#define MAX_CORES 4				/* Cores per shared L2 */
+#define MAX_CLUSTERS 2
+#define MAX_THREADS_PER_CLUSTER 4
+#define THREADS_PER_CORE (MAX_CLUSTERS * MAX_THREADS_PER_CLUSTER)		/* HW threads in a core */
+#define THREADS_MAX (MAX_CORES * THREADS_PER_CORE)
+#define DMA_MAX THREADS_MAX  // DMA: make this independent
+#define MAX_EXT_CONTEXTS 4
+#define MAX_L2_INTERLEAVES 2
+#define MAX_VFIFO_COUNT 4
+
+#define SLOTS_MAX 4
+
+/* #define PROCESSORS_MAX 1 */
+#define REG_WRITES_MAX 32		/*?? */
+#define PRED_WRITES_MAX 5		/* 4 insns + endloop */
+#define STORES_MAX 2
+#define LOADS_MAX 2
+#define MAX_PRED 4
+
+#define PACKET_BYTES_MAX 16
+#define MAX_TLB_ENTRIES 320
+#define DTLB_ENTRIES 16
+#define ITLB_ENTRIES 16
+#define MAX_N_GRANULES 4		/* Max of number of granules of all caches */
+
+/* FMC */
+#define FAST_MEM_CACHE_SIZE (256)
+
+/* IIC stuff */
+#define IIC_TAGS_MAX (1024*128)	/* 1MB = 2^20 bytes */
+#define IIC_DATA_IN_BYTES_MAX (1024*1024*4)
+
+#define MAX_TLB_GUESS_ENTRIES (1024)	// power of 2
+
+/* IICTLB */
+#define IICTLB_PAGE_BITS 12
+#define IICTLB_ENTRIES 64
+#define IICTLB_PAGE_SIZE (1024*4)
+#define NUM_MEM_UTLB_ENTRIES 8
+
+#define MAX_L1S_SIZE (1024*1024)
+
+#define REG_OPERANDS_MAX 5
+#define SUB_IMMEDS_MAX 4		/* number of immediate fragments within insn */
+
+#define MAX_SUPPORTED_BUSES 4
+
+#define MAX_HISTO_BUCKETS 256
+
+/* Trace input format */
+//#define QTRACE_HEADER_SIZE_MAX 50 /* at most 50 chars in header */
+
+#define MAX_PMU_REGISTERS 8
+#define EXEC_PIPE_DEPTH 3
+#define MAX_L2_INTERLEAVE 2
+#define MAX_BP_QUEUE_SIZE 16
+#define MAX_CMT_WINDOW 10
+#define MAX_BRANCHES_PER_PACKET 2
+#endif							/* _MAX_H */
diff --git a/target/hexagon/imported/regs.h b/target/hexagon/imported/regs.h
new file mode 100644
index 0000000..89d4f57
--- /dev/null
+++ b/target/hexagon/imported/regs.h
@@ -0,0 +1,19 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+
+/*
+ * There are 32 general user regs and up to 32 user control regs.
+ */
+
+
+#ifndef _REGS_H
+#define _REGS_H
+
+#define NUM_GEN_REGS 32
+#define NUM_PREGS 4
+/* user + guest + per-thread supervisor + A regs */
+#define NUM_PER_THREAD_CR (32 + 32 + 16 + 48)
+#define TOTAL_PER_THREAD_REGS 64
+#define NUM_GLOBAL_REGS (128 + 32) /* + A regs */
+
+#endif
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
new file mode 100644
index 0000000..d26ba66
--- /dev/null
+++ b/target/hexagon/op_helper.c
@@ -0,0 +1,29 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+/*
+ * Hexagon helpers
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "tcg-op.h"
+#include "cpu.h"
+#include "qemu/main-loop.h"
+#include "exec/exec-all.h"
+#include "exec/helper-proto.h"
+
+/* Exceptions processing helpers */
+void QEMU_NORETURN do_raise_exception_err(CPUHexagonState *env,
+                                          uint32_t exception, uintptr_t pc)
+{
+    CPUState *cs = CPU(hexagon_env_get_cpu(env));
+    qemu_log_mask(CPU_LOG_INT, "%s: %d\n", __func__, exception);
+    cs->exception_index = exception;
+    cpu_loop_exit_restore(cs, pc);
+}
+
+void helper_raise_exception(CPUHexagonState *env, uint32_t exception)
+{
+    do_raise_exception_err(env, exception, 0);
+}
+
diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
new file mode 100644
index 0000000..26e6c4a
--- /dev/null
+++ b/target/hexagon/translate.c
@@ -0,0 +1,220 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+/*
+ * Hexagon translation
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "cpu.h"
+#include "tcg-op.h"
+#include "disas/disas.h"
+#include "exec/cpu_ldst.h"
+#include "exec/exec-all.h"
+#include "exec/helper-proto.h"
+#include "exec/helper-gen.h"
+#include "translate.h"
+#include "regs.h"
+
+#include "exec/translator.h"
+#include "exec/log.h"
+
+TCGv hex_gpr[TOTAL_PER_THREAD_REGS];
+TCGv hex_pred[NUM_PREGS];
+
+
+#include "exec/gen-icount.h"
+
+void gen_exception(int excp)
+{
+    TCGv_i32 helper_tmp = tcg_const_i32(excp);
+    gen_helper_raise_exception(cpu_env, helper_tmp);
+    tcg_temp_free_i32(helper_tmp);
+}
+
+void gen_exception_debug(void)
+{
+    gen_exception(EXCP_DEBUG);
+}
+
+static void decode_packet(CPUHexagonState *env, DisasContext *ctx)
+{
+    size4u_t words[4];
+    int i;
+    /* Brute force way to make sure current PC is set */
+    tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->base.pc_next);
+
+    for (i = 0; i < 4; i++) {
+        words[i] = cpu_ldl_code(env, ctx->base.pc_next + i * sizeof(size4u_t));
+    }
+    /*
+     * Brute force just enough to get the first program to execute.
+     */
+    switch (words[0]) {
+    case 0x7800c806:                                /* r6 = #64 */
+        tcg_gen_movi_tl(hex_gpr[6], 64);
+        ctx->base.pc_next += 4;
+        break;
+    case 0x7800c020:                                /* r0 = #1 */
+        tcg_gen_movi_tl(hex_gpr[0], 1);
+        ctx->base.pc_next += 4;
+        break;
+    case 0x00044002:
+        if (words[1] == 0x7800c001) {               /* r1 = ##0x400080 */
+            tcg_gen_movi_tl(hex_gpr[1], 0x400080);
+            ctx->base.pc_next += 8;
+        } else {
+            printf("ERROR: Unknown instruction 0x%x\n", words[1]);
+            g_assert_not_reached();
+        }
+        break;
+    case 0x7800c0e2:                                /* r2 = #7 */
+        tcg_gen_movi_tl(hex_gpr[2], 7);
+        ctx->base.pc_next += 4;
+        break;
+    case 0x5400c004:                               /* trap0(#1) */
+    {
+        TCGv excp_trap0 = tcg_const_tl(HEX_EXCP_TRAP0);
+        gen_helper_raise_exception(cpu_env, excp_trap0);
+        tcg_temp_free(excp_trap0);
+        ctx->base.pc_next += 4;
+        break;
+    }
+    case 0x7800cbc6:                               /* r6 = #94 */
+        tcg_gen_movi_tl(hex_gpr[6], 94);
+        ctx->base.pc_next += 4;
+        break;
+    case 0x7800cba6:                               /* r6 = #93 */
+        tcg_gen_movi_tl(hex_gpr[6], 93);
+        ctx->base.pc_next += 4;
+        break;
+    case 0x7800c000:                               /* r0 = #0 */
+        tcg_gen_movi_tl(hex_gpr[0], 0);
+        ctx->base.pc_next += 4;
+        break;
+    default:
+        ctx->base.is_jmp = DISAS_TOO_MANY;
+        ctx->base.pc_next += 4;
+    }
+}
+
+static void hexagon_tr_init_disas_context(DisasContextBase *dcbase,
+                                          CPUState *cs)
+{
+    DisasContext *ctx = container_of(dcbase, DisasContext, base);
+
+    ctx->mem_idx = ctx->base.tb->flags & TB_FLAGS_MMU_MASK;
+}
+
+static void hexagon_tr_tb_start(DisasContextBase *db, CPUState *cpu)
+{
+}
+
+static void hexagon_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
+{
+    DisasContext *ctx = container_of(dcbase, DisasContext, base);
+
+    tcg_gen_insn_start(ctx->base.pc_next);
+}
+
+static bool hexagon_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
+                                        const CPUBreakpoint *bp)
+{
+    DisasContext *ctx = container_of(dcbase, DisasContext, base);
+
+    tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->base.pc_next);
+    ctx->base.is_jmp = DISAS_NORETURN;
+    gen_exception_debug();
+    /*
+     * The address covered by the breakpoint must be included in
+     * [tb->pc, tb->pc + tb->size) in order to for it to be
+     * properly cleared -- thus we increment the PC here so that
+     * the logic setting tb->size below does the right thing.
+     */
+    ctx->base.pc_next += 4;
+    return true;
+}
+
+
+static void hexagon_tr_translate_packet(DisasContextBase *dcbase, CPUState *cpu)
+{
+    DisasContext *ctx = container_of(dcbase, DisasContext, base);
+    CPUHexagonState *env = cpu->env_ptr;
+
+    decode_packet(env, ctx);
+
+    if (ctx->base.is_jmp == DISAS_NEXT) {
+        target_ulong page_start;
+
+        page_start = ctx->base.pc_first & TARGET_PAGE_MASK;
+        if (ctx->base.pc_next - page_start >= TARGET_PAGE_SIZE) {
+            ctx->base.is_jmp = DISAS_TOO_MANY;
+        }
+
+#ifdef DEBUG_HEX
+        /* When debugging, force the end of the TB after each packet */
+        if (ctx->base.pc_next - ctx->base.pc_first >= 0x04) {
+            ctx->base.is_jmp = DISAS_TOO_MANY;
+        }
+#endif
+    }
+}
+
+static void hexagon_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
+{
+    DisasContext *ctx = container_of(dcbase, DisasContext, base);
+
+    switch (ctx->base.is_jmp) {
+    case DISAS_TOO_MANY:
+        tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->base.pc_next);
+        if (ctx->base.singlestep_enabled) {
+            gen_exception_debug();
+        } else {
+            tcg_gen_exit_tb(NULL, 0);
+        }
+        break;
+    case DISAS_NORETURN:
+        break;
+    default:
+        g_assert_not_reached();
+    }
+}
+
+static void hexagon_tr_disas_log(const DisasContextBase *dcbase, CPUState *cpu)
+{
+    qemu_log("IN: %s\n", lookup_symbol(dcbase->pc_first));
+    log_target_disas(cpu, dcbase->pc_first, dcbase->tb->size);
+}
+
+
+static const TranslatorOps hexagon_tr_ops = {
+    .init_disas_context = hexagon_tr_init_disas_context,
+    .tb_start           = hexagon_tr_tb_start,
+    .insn_start         = hexagon_tr_insn_start,
+    .breakpoint_check   = hexagon_tr_breakpoint_check,
+    .translate_insn     = hexagon_tr_translate_packet,
+    .tb_stop            = hexagon_tr_tb_stop,
+    .disas_log          = hexagon_tr_disas_log,
+};
+
+void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
+{
+    DisasContext ctx;
+
+    translator_loop(&hexagon_tr_ops, &ctx.base, cs, tb, max_insns);
+}
+
+void hexagon_translate_init(void)
+{
+    int i;
+
+    for (i = 0; i < TOTAL_PER_THREAD_REGS; i++) {
+        hex_gpr[i] = tcg_global_mem_new(cpu_env,
+            offsetof(CPUHexagonState, gpr[i]), hexagon_regnames[i]);
+    }
+    for (i = 0; i < NUM_PREGS; i++) {
+        hex_pred[i] = tcg_global_mem_new(cpu_env,
+            offsetof(CPUHexagonState, pred[i]), hexagon_prednames[i]);
+    }
+}
+
diff --git a/target/hexagon/translate.h b/target/hexagon/translate.h
new file mode 100644
index 0000000..c63421e
--- /dev/null
+++ b/target/hexagon/translate.h
@@ -0,0 +1,22 @@ 
+/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights Reserved. */
+
+#ifndef TRANSLATE_H
+#define TRANSLATE_H
+
+#include "cpu.h"
+#include "exec/translator.h"
+#include "exec/helper-proto.h"
+#include "exec/helper-gen.h"
+
+typedef struct DisasContext {
+    DisasContextBase base;
+    uint32_t mem_idx;
+} DisasContext;
+
+extern TCGv hex_gpr[TOTAL_PER_THREAD_REGS];
+extern TCGv hex_pred[NUM_PREGS];
+
+extern void gen_exception(int excp);
+extern void gen_exception_debug(void);
+
+#endif