RE: [PATCH 2/9] Hexagon (target/hexagon) Mark new_read_idx in trans functions
> -Original Message-
> From: Matheus Tavares Bernardino
> Sent: Tuesday, February 27, 2024 8:20 AM
> To: Taylor Simpson
> Cc: qemu-devel@nongnu.org; bc...@quicinc.com;
> quic_mathb...@quicinc.com; sidn...@quicinc.com;
> quic_mlie...@quicinc.com; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: Re: [PATCH 2/9] Hexagon (target/hexagon) Mark new_read_idx in
> trans functions
>
> On Mon, 26 Feb 2024 13:17:15 -0700 Taylor Simpson
> wrote:
> >
> > diff --git a/target/hexagon/gen_trans_funcs.py
> > b/target/hexagon/gen_trans_funcs.py
> > index 53e844a44b..79475b2946 100755
> > --- a/target/hexagon/gen_trans_funcs.py
> > +++ b/target/hexagon/gen_trans_funcs.py
> > @@ -84,14 +84,15 @@ def gen_trans_funcs(f):
> > insn->opcode = {tag};
> > """))
> >
> > -regno = 0
> > -for reg in regs:
> > -reg_type = reg[0]
> > -reg_id = reg[1]
> > +new_read_idx = -1
> > +for regno, regstruct in enumerate(regs):
> > +reg_type, reg_id, _, _ = regstruct
> > +reg = hex_common.get_register(tag, reg_type, reg_id)
>
> Nit: since we don't care about the remaining elements of regstruct, we
could
> simplify (and future-proof) this even further to:
>
> reg_type, reg_id, *_ = regstruct
>
> Or perhaps even eliminate the variable entirely:
>
> for regno, (reg_type, reg_id, *_) in enumerate(regs):
> ...
Let's go with the second option. I'll make the change.
Thanks,
Taylor
RE: [PATCH 4/9] Hexagon (target/hexagon) Mark has_pred_dest in trans functions
> -Original Message- > From: Matheus Tavares Bernardino > Sent: Tuesday, February 27, 2024 8:21 AM > To: Taylor Simpson > Cc: qemu-devel@nongnu.org; bc...@quicinc.com; > quic_mathb...@quicinc.com; sidn...@quicinc.com; > quic_mlie...@quicinc.com; richard.hender...@linaro.org; > phi...@linaro.org; a...@rev.ng; a...@rev.ng > Subject: Re: [PATCH 4/9] Hexagon (target/hexagon) Mark has_pred_dest in > trans functions > > On Mon, 26 Feb 2024 13:17:17 -0700 Taylor Simpson > wrote: > > > > diff --git a/target/hexagon/gen_trans_funcs.py > > b/target/hexagon/gen_trans_funcs.py > > index 07292e0170..f1972fd2dd 100755 > > --- a/target/hexagon/gen_trans_funcs.py > > +++ b/target/hexagon/gen_trans_funcs.py > > @@ -86,6 +86,7 @@ def gen_trans_funcs(f): > > > > new_read_idx = -1 > > dest_idx = -1 > > +has_pred_dest = "false" > > for regno, regstruct in enumerate(regs): > > reg_type, reg_id, _, _ = regstruct > > reg = hex_common.get_register(tag, reg_type, reg_id) @@ > > -96,6 +97,8 @@ def gen_trans_funcs(f): > > new_read_idx = regno > > if reg.is_written() and dest_idx == -1: > > dest_idx = regno > > +if reg_type == "P" and not reg.is_read(): > > +has_pred_dest = "true" > > I got a bit confused here. Why do we use "not reg.is_read()"? I though this > would be "reg.is_written()". The original C code is if ((strstr(opcode_wregs[opcode], "Pd4") || strstr(opcode_wregs[opcode], "Pe4")) && Checking for reg.is_written() would also match Px4, which is read-write. Would it be more clear if we checked for reg.is_written() and non reg.is_read()? Thanks, Taylor
RE: [PATCH] Hexagon: add PC alignment check and exception
> -Original Message- > From: Matheus Tavares Bernardino > Sent: Friday, April 26, 2024 1:16 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; sidn...@quicinc.com; a...@rev.ng; a...@rev.ng; > ltaylorsimp...@gmail.com > Subject: [PATCH] Hexagon: add PC alignment check and exception > > The Hexagon Programmer's Reference Manual says that the exception 0x1e > should be raised upon an unaligned program counter. Let's implement that > and also add tests for both the most common case as well as packets with > multiple change-of-flow instructions. > > Signed-off-by: Matheus Tavares Bernardino > --- > --- a/target/hexagon/genptr.c > +++ b/target/hexagon/genptr.c > @@ -473,6 +473,7 @@ static void gen_write_new_pc_addr(DisasContext You haven't added the check to gen_write_new_pc_pcrel. It's not needed there because the encoding guarantees the target is always aligned - right? However, there is a call to gen_write_new_pc_addr inside that function. In this case, we'll add a check that isn't necessary. Consider adding a parameter to indicate if the check can be avoided. > a/tests/tcg/hexagon/Makefile.target b/tests/tcg/hexagon/Makefile.target > index f839b2c0d5..02d7fff34c 100644 > --- a/tests/tcg/hexagon/Makefile.target > +++ b/tests/tcg/hexagon/Makefile.target > @@ -51,6 +51,19 @@ HEX_TESTS += scatter_gather HEX_TESTS += hvx_misc > HEX_TESTS += hvx_histogram HEX_TESTS += invalid-slots > +HEX_TESTS += unaligned_pc > +HEX_TESTS += unaligned_pc_multi_cof > + > +run-unaligned_pc: unaligned_pc > +run-unaligned_pc_multi_cof: unaligned_pc_multi_cof run-unaligned_pc > +run-unaligned_pc_multi_cof: > + $(call run-test, $<, $(QEMU) $< 2> $<.stderr,"$< on > $(TARGET_NAME)"); \ > + if [ $$? -ne 1 ] ; then \ > + return 1; \ > + fi > + $(call quiet-command, \ > + grep -q "exception 0x1e" $<.stderr, \ > + "GREP", "exception 0x1e"); We should also test endloop instructions. Thanks, Taylor
RE: [PATCH] Hexagon: add PC alignment check and exception
PS You should also update the pkt_raises_exception function in translate.c to return true for packets that contain these instructions. This will ensure that none of the machine state is changed before the check is complete. Taylor > -Original Message- > From: ltaylorsimp...@gmail.com > Sent: Monday, April 29, 2024 9:41 AM > To: 'Matheus Tavares Bernardino' ; qemu- > de...@nongnu.org > Cc: bc...@quicinc.com; sidn...@quicinc.com; a...@rev.ng; a...@rev.ng > Subject: RE: [PATCH] Hexagon: add PC alignment check and exception > > > > > -Original Message- > > From: Matheus Tavares Bernardino > > Sent: Friday, April 26, 2024 1:16 PM > > To: qemu-devel@nongnu.org > > Cc: bc...@quicinc.com; sidn...@quicinc.com; a...@rev.ng; a...@rev.ng; > > ltaylorsimp...@gmail.com > > Subject: [PATCH] Hexagon: add PC alignment check and exception > > > > The Hexagon Programmer's Reference Manual says that the exception > 0x1e > > should be raised upon an unaligned program counter. Let's implement > > that and also add tests for both the most common case as well as > > packets with multiple change-of-flow instructions. > > > > Signed-off-by: Matheus Tavares Bernardino > > > --- > > > > --- a/target/hexagon/genptr.c > > +++ b/target/hexagon/genptr.c > > @@ -473,6 +473,7 @@ static void gen_write_new_pc_addr(DisasContext > > You haven't added the check to gen_write_new_pc_pcrel. It's not needed > there because the encoding guarantees the target is always aligned - right? > However, there is a call to gen_write_new_pc_addr inside that function. In > this case, we'll add a check that isn't necessary. Consider adding a parameter > to indicate if the check can be avoided. > > > > a/tests/tcg/hexagon/Makefile.target > > b/tests/tcg/hexagon/Makefile.target > > index f839b2c0d5..02d7fff34c 100644 > > --- a/tests/tcg/hexagon/Makefile.target > > +++ b/tests/tcg/hexagon/Makefile.target > > @@ -51,6 +51,19 @@ HEX_TESTS += scatter_gather HEX_TESTS += > hvx_misc > > HEX_TESTS += hvx_histogram HEX_TESTS += invalid-slots > > +HEX_TESTS += unaligned_pc > > +HEX_TESTS += unaligned_pc_multi_cof > > + > > +run-unaligned_pc: unaligned_pc > > +run-unaligned_pc_multi_cof: unaligned_pc_multi_cof run-unaligned_pc > > +run-unaligned_pc_multi_cof: > > + $(call run-test, $<, $(QEMU) $< 2> $<.stderr,"$< on > > $(TARGET_NAME)"); \ > > + if [ $$? -ne 1 ] ; then \ > > + return 1; \ > > + fi > > + $(call quiet-command, \ > > + grep -q "exception 0x1e" $<.stderr, \ > > + "GREP", "exception 0x1e"); > > We should also test endloop instructions. > > Thanks, > Taylor >
RE: [PATCH v3] Hexagon: add PC alignment check and exception
> -Original Message-
> From: Matheus Tavares Bernardino
> Sent: Tuesday, April 30, 2024 9:25 AM
> To: qemu-devel@nongnu.org
> Cc: bc...@quicinc.com; sidn...@quicinc.com; a...@rev.ng; a...@rev.ng;
> ltaylorsimp...@gmail.com; richard.hender...@linaro.org; Laurent Vivier
>
> Subject: [PATCH v3] Hexagon: add PC alignment check and exception
>
> The Hexagon Programmer's Reference Manual says that the exception 0x1e
> should be raised upon an unaligned program counter. Let's implement that
> and also add some tests.
>
> Signed-off-by: Matheus Tavares Bernardino
> ---
> v2: https://lore.kernel.org/qemu-
> devel/e559b521d1920f804df10244c8c07564431aeba5.1714419461.git.quic_ma
> thb...@quicinc.com/
>
> Thanks for the comments, Richard and Taylor!
>
> Changed in v3:
> - Removed now unnecessary pkt_raises_exception addition.
> - Added HEX_EXCP_PC_NOT_ALIGNED handling at
> linux-user/hexagon/cpu_loop.c.
> - Merged all tests into a C file that uses signal handler to check
> that the exception was raised.
>
> target/hexagon/cpu.h | 7 ++
> target/hexagon/cpu_bits.h | 4 +
> target/hexagon/macros.h| 3 -
> linux-user/hexagon/cpu_loop.c | 4 +
> target/hexagon/op_helper.c | 9 +--
> tests/tcg/hexagon/unaligned_pc.c | 85 ++
> tests/tcg/hexagon/Makefile.target | 4 +
> tests/tcg/hexagon/unaligned_pc_multi_cof.S | 5 ++
> 8 files changed, 113 insertions(+), 8 deletions(-) create mode 100644
> tests/tcg/hexagon/unaligned_pc.c create mode 100644
> tests/tcg/hexagon/unaligned_pc_multi_cof.S
>
> a/tests/tcg/hexagon/unaligned_pc.c b/tests/tcg/hexagon/unaligned_pc.c
> new file mode 100644
> index 00..1add2d0d99
> --- /dev/null
> +++ b/tests/tcg/hexagon/unaligned_pc.c
> @@ -0,0 +1,85 @@
> +#include
> +#include
> +#include
> +#include
> +
> +/* will be changed in signal handler */ volatile sig_atomic_t
> +completed_tests; static jmp_buf after_test; static int nr_tests;
> +
> +void __attribute__((naked)) test_return(void) {
> +asm volatile(
> +"allocframe(#0x8)\n"
> +"r0 = #0x\n"
> +"framekey = r0\n"
> +"dealloc_return\n"
> +: : : "r0");
Add r29, r30, r31 to clobbers list.
Add framekey to clobbers list (assuming the compiler will take it).
> +}
> +
> +void test_endloop(void)
> +{
> +asm volatile(
> +"loop0(1f, #2)\n"
> +"1: r0 = #0x3\n"
> +"sa0 = r0\n"
> +"{ nop }:endloop0\n"
> +: : : "r0");
> +}
Add sa0, lc0, usr to the clobbers list.
> +
> +void test_multi_cof(void)
> +{
> +asm volatile(
> +"p0 = cmp.eq(r0, r0)\n"
> +"{\n"
> +"if (p0) jump test_multi_cof_unaligned\n"
> +"jump 1f\n"
> +"}\n"
> +"1: nop\n"
> +: : : "p0");
> +}
> +
> +void sigbus_handler(int signum)
> +{
> +/* retore framekey after test_return */
> +asm volatile(
> +"r0 = #0\n"
> +"framekey = r0\n"
> +: : : "r0");
Add framekey to the clobbers list.
> +printf("Test %d complete\n", completed_tests);
> +completed_tests++;
> +siglongjmp(after_test, 1);
> +}
> +
> +void test_done(void)
> +{
> +int err = (completed_tests != nr_tests);
> +puts(err ? "FAIL" : "PASS");
> +exit(err);
> +}
> +
> +typedef void (*test_fn)(void);
> +
> +int main()
> +{
> +test_fn tests[] = { test_return, test_endloop, test_multi_cof,
test_done
> };
> +nr_tests = (sizeof(tests) / sizeof(tests[0])) - 1;
> +
> +struct sigaction sa = {
> +.sa_sigaction = sigbus_handler,
> +.sa_flags = SA_SIGINFO
> +};
> +
> +if (sigaction(SIGBUS, &sa, NULL) < 0) {
> +perror("sigaction");
> +return EXIT_FAILURE;
> +}
> +
> +sigsetjmp(after_test, 1);
> +tests[completed_tests]();
> +
> +/* should never get here */
> +puts("FAIL");
> +return 1;
> +}
> diff --git a/tests/tcg/hexagon/Makefile.target
> b/tests/tcg/hexagon/Makefile.target
> index f839b2c0d5..75139e731c 100644
> --- a/tests/tcg/hexagon/Makefile.target
> +++ b/tests/tcg/hexagon/Makefile.target
> @@ -51,6 +51,7 @@ HEX_TESTS += scatter_gather HEX_TESTS += hvx_misc
> HEX_TESTS += hvx_histogram HEX_TESTS += invalid-slots
> +HEX_TESTS += unaligned_pc
>
> run-and-check-exception = $(call run-test,$2,$3 2>$2.stderr; \
> test $$? -eq 1 && grep -q "exception $(strip $1)" $2.stderr) @@ -
> 108,6 +109,9 @@ preg_alias: preg_alias.c hex_test.h
> read_write_overlap: read_write_overlap.c hex_test.h
> reg_mut: reg_mut.c hex_test.h
>
> +unaligned_pc: unaligned_pc.c unaligned_pc_multi_cof.S
> + $(CC) $(CFLAGS) $(CROSS_CC_GUEST_CFLAGS) -mv73 $^ -o $@
> $(LDFLAGS)
> +
> # This test has to be compiled for the -mv67t target
> usr: usr.c hex_test.h
> $(CC) $(CFLAGS) -mv67t -O2 -Wno-inline-asm -Wno-expansion-to-
> defined $< -o $@ $(LDFLAGS) diff --git
> a/tests/tcg/hexagon/unaligned_pc_multi
RE: [PATCH v4] Hexagon: add PC alignment check and exception
> -Original Message- > From: Matheus Tavares Bernardino > Sent: Thursday, May 2, 2024 9:55 AM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; sidn...@quicinc.com; a...@rev.ng; a...@rev.ng; > ltaylorsimp...@gmail.com; richard.hender...@linaro.org; Laurent Vivier > > Subject: [PATCH v4] Hexagon: add PC alignment check and exception > > The Hexagon Programmer's Reference Manual says that the exception 0x1e > should be raised upon an unaligned program counter. Let's implement that > and also add some tests. > > Signed-off-by: Matheus Tavares Bernardino > Reviewed-by: Richard Henderson > --- > v3: https://lore.kernel.org/qemu- > devel/5c90567ec28723865e144f386b36f5b676b7a5d3.1714486874.git.quic_ma > thb...@quicinc.com/ > > Changes in v4: > - Added missing regs to clobber list as mentioned by Taylor. > - Avoided undefined behavior on package with multiple branches > (at test_multi_cof), as suggested offline by Brian. > > a/tests/tcg/hexagon/unaligned_pc_multi_cof.S > b/tests/tcg/hexagon/unaligned_pc_multi_cof.S > new file mode 100644 > index 00..10accd0057 > --- /dev/null > +++ b/tests/tcg/hexagon/unaligned_pc_multi_cof.S > @@ -0,0 +1,5 @@ > +.org 0x3 > +.global test_multi_cof_unaligned > +test_multi_cof_unaligned: > + nop > + jumpr r31 You should be able to put this as an inline asm block with the .org directive in unaligned_pc.c (outside of any function). Otherwise Reviewed-by: Taylor Simpson
RE: [PATCH 1/4] target/hexagon: idef-parser remove unused defines
> -Original Message- > From: Anton Johansson > Sent: Monday, May 6, 2024 1:31 PM > To: qemu-devel@nongnu.org > Cc: a...@rev.ng; ltaylorsimp...@gmail.com; bc...@quicinc.com > Subject: [PATCH 1/4] target/hexagon: idef-parser remove unused defines > > Before switching to GArray/g_string_printf we used fixed size arrays for > output buffers and instructions arguments among other things. > > Macros defining the sizes of these buffers were left behind, remove them. > > Signed-off-by: Anton Johansson > --- > target/hexagon/idef-parser/idef-parser.h | 10 -- > 1 file changed, 10 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH 4/4] target/hexagon: idef-parser simplify predicate init
> -Original Message-
> From: Anton Johansson
> Sent: Monday, May 6, 2024 1:31 PM
> To: qemu-devel@nongnu.org
> Cc: a...@rev.ng; ltaylorsimp...@gmail.com; bc...@quicinc.com
> Subject: [PATCH 4/4] target/hexagon: idef-parser simplify predicate init
>
> Only predicate instruction arguments need to be initialized by
idef-parser.
> This commit removes registers from the init_list and simplifies
> gen_inst_init_args() slightly.
>
> Signed-off-by: Anton Johansson
> ---
> target/hexagon/idef-parser/idef-parser.y| 2 --
> target/hexagon/idef-parser/parser-helpers.c | 20 +---
> 2 files changed, 9 insertions(+), 13 deletions(-)
> diff --git a/target/hexagon/idef-parser/parser-helpers.c
> b/target/hexagon/idef-parser/parser-helpers.c
> index bae01c2bb8..33e8f82007 100644
> --- a/target/hexagon/idef-parser/parser-helpers.c
> +++ b/target/hexagon/idef-parser/parser-helpers.c
> @@ -1652,26 +1652,24 @@ void gen_inst(Context *c, GString *iname)
>
> void gen_inst_init_args(Context *c, YYLTYPE *locp) {
> +/* If init_list is NULL arguments have already been initialized */
> if (!c->inst.init_list) {
> return;
> }
>
> for (unsigned i = 0; i < c->inst.init_list->len; i++) {
> HexValue *val = &g_array_index(c->inst.init_list, HexValue, i);
> -if (val->type == REGISTER_ARG) {
> -/* Nothing to do here */
> -} else if (val->type == PREDICATE) {
> -char suffix = val->is_dotnew ? 'N' : 'V';
> -EMIT_HEAD(c, "tcg_gen_movi_i%u(P%c%c, 0);\n", val->bit_width,
> - val->pred.id, suffix);
> -} else {
> -yyassert(c, locp, false, "Invalid arg type!");
> -}
> +yyassert(c, locp, val->type == PREDICATE,
> + "Only predicates need to be initialized!");
> +char suffix = val->is_dotnew ? 'N' : 'V';
Declarations should be at the beginning of the function per QEMU coding
standards.
> +EMIT_HEAD(c, "tcg_gen_movi_i%u(P%c%c, 0);\n", val->bit_width,
Since you know this is a predicate, the bit_width will always be 32. You
can hard-code that instead of using %u.
> + val->pred.id, suffix);
> }
>
> /* Free argument init list once we have initialized everything */
Taylor
RE: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
> -Original Message-
> From: Anton Johansson
> Sent: Monday, May 6, 2024 1:31 PM
> To: qemu-devel@nongnu.org
> Cc: a...@rev.ng; ltaylorsimp...@gmail.com; bc...@quicinc.com
> Subject: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
>
> gen_inst_init_args() is called for instructions using a predicate as an
rvalue.
> Upon first call, the list of arguments which might need initialization
init_list is
> freed to indicate that they have been processed. For instructions without
an
> rvalue predicate,
> gen_inst_init_args() isn't called and init_list will never be freed.
>
> Free init_list from free_instruction() if it hasn't already been freed.
>
> Signed-off-by: Anton Johansson
> ---
> target/hexagon/idef-parser/parser-helpers.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/target/hexagon/idef-parser/parser-helpers.c
> b/target/hexagon/idef-parser/parser-helpers.c
> index 95f2b43076..bae01c2bb8 100644
> --- a/target/hexagon/idef-parser/parser-helpers.c
> +++ b/target/hexagon/idef-parser/parser-helpers.c
> @@ -2121,6 +2121,13 @@ void free_instruction(Context *c)
> g_string_free(g_array_index(c->inst.strings, GString*, i), TRUE);
> }
> g_array_free(c->inst.strings, TRUE);
> +/*
> + * Free list of arguments that might need initialization, if they
haven't
> + * already been free'd.
> + */
> +if (c->inst.init_list) {
> +g_array_free(c->inst.init_list, TRUE);
> +}
> /* Free INAME token value */
> g_string_free(c->inst.name, TRUE);
> /* Free variables and registers */
Why not do this in gen_inst_init_args just before this?
/* Free argument init list once we have initialized everything */
g_array_free(c->inst.init_list, TRUE);
c->inst.init_list = NULL;
Taylor
RE: [PATCH 2/4] target/hexagon: idef-parser remove undefined functions
> -Original Message- > From: Anton Johansson > Sent: Monday, May 6, 2024 1:31 PM > To: qemu-devel@nongnu.org > Cc: a...@rev.ng; ltaylorsimp...@gmail.com; bc...@quicinc.com > Subject: [PATCH 2/4] target/hexagon: idef-parser remove undefined > functions > > Signed-off-by: Anton Johansson > --- > target/hexagon/idef-parser/parser-helpers.h | 13 - > 1 file changed, 13 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
> -Original Message-
> From: 'Anton Johansson'
> Sent: Tuesday, May 7, 2024 4:47 AM
> To: ltaylorsimp...@gmail.com
> Cc: qemu-devel@nongnu.org; a...@rev.ng; bc...@quicinc.com
> Subject: Re: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
>
> On 06/05/24, ltaylorsimp...@gmail.com wrote:
> >
> >
> > > -Original Message-
> > > From: Anton Johansson
> > > Sent: Monday, May 6, 2024 1:31 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: a...@rev.ng; ltaylorsimp...@gmail.com; bc...@quicinc.com
> > > Subject: [PATCH 3/4] target/hexagon: idef-parser fix leak of
> > > init_list
> > >
> > > gen_inst_init_args() is called for instructions using a predicate as
> > > an
> > rvalue.
> > > Upon first call, the list of arguments which might need
> > > initialization
> > init_list is
> > > freed to indicate that they have been processed. For instructions
> > > without
> > an
> > > rvalue predicate,
> > > gen_inst_init_args() isn't called and init_list will never be freed.
> > >
> > > Free init_list from free_instruction() if it hasn't already been freed.
> > >
> > > Signed-off-by: Anton Johansson
> > > ---
> > > target/hexagon/idef-parser/parser-helpers.c | 7 +++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/target/hexagon/idef-parser/parser-helpers.c
> > > b/target/hexagon/idef-parser/parser-helpers.c
> > > index 95f2b43076..bae01c2bb8 100644
> > > --- a/target/hexagon/idef-parser/parser-helpers.c
> > > +++ b/target/hexagon/idef-parser/parser-helpers.c
> > > @@ -2121,6 +2121,13 @@ void free_instruction(Context *c)
> > > g_string_free(g_array_index(c->inst.strings, GString*, i), TRUE);
> > > }
> > > g_array_free(c->inst.strings, TRUE);
> > > +/*
> > > + * Free list of arguments that might need initialization, if
> > > + they
> > haven't
> > > + * already been free'd.
> > > + */
> > > +if (c->inst.init_list) {
> > > +g_array_free(c->inst.init_list, TRUE);
> > > +}
> > > /* Free INAME token value */
> > > g_string_free(c->inst.name, TRUE);
> > > /* Free variables and registers */
> >
> > Why not do this in gen_inst_init_args just before this?
> >/* Free argument init list once we have initialized everything */
> > g_array_free(c->inst.init_list, TRUE);
> > c->inst.init_list = NULL;
>
> Thanks for the reviews Taylor! I'm not sure I understand what you mean
> here, we already free init_list in gen_inst_init_args, since
> gen_inst_init_args
> won't be called for all instructions we need to also free from a common
> place.
>
> //Anton
It just seems more natural to free the elements of the array at the same place
you free the array itself. If there are valid reasons for doing it elsewhere,
I'm OK with that.
Taylor
RE: [PATCH 1/3] target/hexagon: Rename HEX_EXCP_ => HEX_EVENT_
> -Original Message- > From: Brian Cain > Sent: Friday, August 16, 2024 1:06 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; quic_mathb...@quicinc.com; > sidn...@quicinc.com; quic_mlie...@quicinc.com; > ltaylorsimp...@gmail.com; Laurent Vivier > Subject: [PATCH 1/3] target/hexagon: Rename HEX_EXCP_ => HEX_EVENT_ > > Change the name of these definitions to reflect that they correspond to the > event code for the exception. > > Signed-off-by: Brian Cain > --- > linux-user/hexagon/cpu_loop.c | 4 ++-- > target/hexagon/cpu.h | 2 +- > target/hexagon/cpu_bits.h | 14 +++--- > target/hexagon/gen_tcg.h | 2 +- > target/hexagon/translate.c| 6 +++--- > 5 files changed, 14 insertions(+), 14 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH 2/3] target/hexagon: rename HEX_EVENT_TRAP0=>HEX_CAUSE_TRAP0
> -Original Message- > From: Brian Cain > Sent: Friday, August 16, 2024 1:06 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; quic_mathb...@quicinc.com; > sidn...@quicinc.com; quic_mlie...@quicinc.com; > ltaylorsimp...@gmail.com > Subject: [PATCH 2/3] target/hexagon: rename > HEX_EVENT_TRAP0=>HEX_CAUSE_TRAP0 > > The value previously used for "HEX_EVENT_TRAP0" was the cause code > definition and not the event number. So in this commit, we update the > name to reflect the cause code and add a new "HEX_EVENT_TRAP0" > with the correct event number. > > Signed-off-by: Brian Cain > --- > target/hexagon/cpu_bits.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Taylor Simpson
RE: [PATCH 3/3] target/hexagon: add enums for event, cause
> -Original Message- > From: Brian Cain > Sent: Friday, August 16, 2024 1:06 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; quic_mathb...@quicinc.com; > sidn...@quicinc.com; quic_mlie...@quicinc.com; > ltaylorsimp...@gmail.com > Subject: [PATCH 3/3] target/hexagon: add enums for event, cause > > Signed-off-by: Brian Cain > --- > target/hexagon/cpu_bits.h | 22 ++ > 1 file changed, 14 insertions(+), 8 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH v2 1/3] Hexagon (target/hexagon) Use QEMU decodetree (32-bit instructions)
> -Original Message-
> From: Brian Cain
> Sent: Sunday, January 14, 2024 5:21 PM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: Matheus Bernardino (QUIC) ; Sid
> Manning ; Marco Liebel (QUIC)
> ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: RE: [PATCH v2 1/3] Hexagon (target/hexagon) Use QEMU
> decodetree (32-bit instructions)
>
>
>
> > -Original Message-
> > From: Taylor Simpson
> > Sent: Monday, January 8, 2024 4:49 PM
> > To: qemu-devel@nongnu.org
> > Cc: Brian Cain ; Matheus Bernardino (QUIC)
> > ; Sid Manning ;
> Marco
> > Liebel (QUIC) ;
> > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > a...@rev.ng; ltaylorsimp...@gmail.com
> > Subject: [PATCH v2 1/3] Hexagon (target/hexagon) Use QEMU decodetree
> > (32- bit instructions)
> >
> >
> > The Decodetree Specification can be found here
> > https://www.qemu.org/docs/master/devel/decodetree.html
> >
> > Covers all 32-bit instructions, including HVX
> >
> > We generate separate decoders for each instruction class. The reason
> > will be more apparent in the next patch in this series.
> >
> > We add 2 new scripts
> > gen_decodetree.pyGenerate the input to decodetree.py
> > gen_trans_funcs.py Generate the trans_* functions used by the
> > output of decodetree.py
> >
> > Since the functions generated by decodetree.py take DisasContext * as
> > an argument, we add the argument to a couple of functions that didn't
> > need it previously. We also set the insn field in DisasContext during
> > decode because it is used by the trans_* functions.
> >
> > There is a g_assert_not_reached() in decode_insns() in decode.c to
> > verify we never try to use the old decoder on 32-bit instructions
> >
> > Signed-off-by: Taylor Simpson
> > ---
> > target/hexagon/decode.h | 5 +-
> > target/hexagon/decode.c | 54 -
> > target/hexagon/translate.c| 4 +-
> > target/hexagon/README | 13 +-
> > target/hexagon/gen_decodetree.py | 193
> > ++
> target/hexagon/gen_trans_funcs.py | 132
> > target/hexagon/meson.build| 55 +
> > 7 files changed, 442 insertions(+), 14 deletions(-) create mode
> > 100755 target/hexagon/gen_decodetree.py create mode 100755
> > target/hexagon/gen_trans_funcs.py
> >
>
> LGTM, but some nitpicky suggestions:
>
> diff --git a/target/hexagon/gen_decodetree.py
> b/target/hexagon/gen_decodetree.py
> index 2dff975f55..62bd8a62b6 100755
> --- a/target/hexagon/gen_decodetree.py
> +++ b/target/hexagon/gen_decodetree.py
> @@ -57,7 +57,7 @@ def ordered_unique(l):
> "d",
> "e",
> "f",
> -"g"
> +"g",
> }
>
> #
> @@ -104,9 +104,6 @@ def gen_decodetree_file(f, class_to_decode):
> if skip_tag(tag, class_to_decode):
> continue
>
> -f.write("")
> -f.write("\n")
> -
> enc = encs[tag]
> enc_str = "".join(reversed(encs[tag]))
>
> @@ -115,21 +112,21 @@ def gen_decodetree_file(f, class_to_decode):
> if is_subinsn:
> enc_str = "---" + enc_str
>
> -f.write(f"## {tag}:\t{enc_str}\n")
> -f.write("##\n")
> +f.write(("#" * 80) + "\n"
> +f"## {tag}:\t{enc_str}\n"
> +"##\n")
>
> regs = ordered_unique(regre.findall(iset.iset[tag]["syntax"]))
> imms = ordered_unique(immre.findall(iset.iset[tag]["syntax"]))
>
> # Write the field definitions for the registers
> -regno = 0
> -for reg in regs:
> -reg_type = reg[0]
> -reg_id = reg[1]
> +for regno, reg in enumerate(regs):
> +reg_type, reg_id, _, reg_enc_size = reg
> reg_letter = reg_id[0]
> -reg_num_choices = int(reg[3].rstrip("S"))
> -reg_mapping = reg[0] + "".join(["_" for letter in reg[1]]) +
> reg[3]
> +reg_num_choices = int(reg_enc_size.rstrip("S"))
> +reg_mapping = reg_type + "".join("_" for letter in reg_id)
> + + reg_enc_size
> reg_enc_fields = re.findall(reg_letter + "+", enc)
> +print(f'{reg} -> {reg_enc_fields}')
>
> # Check for some errors
> if len(reg_enc_fields) == 0:
> @@ -140,13 +137,12 @@ def gen_decodetree_file(f, class_to_decode):
> if 2 ** len(reg_enc_field) != reg_num_choices:
> raise Exception(f"{tag} has incorrect register field width!")
>
> -f.write(f"%{tag}_{reg_type}{reg_id}\t")
> -f.write(f"{enc.index(reg_enc_field)}:{len(reg_enc_field)}")
> +f.write(f"%{tag}_{reg_type}{reg_id}\t"
> +f"{enc.index(reg_enc_field)}:{len(reg_enc_field)}")
> if (reg_type in num_registers and
> reg_num_choices != num_registers[re
RE: [PATCH] Hexagon: fix HVX store new
> -Original Message- > From: Matheus Tavares Bernardino > Sent: Monday, May 20, 2024 10:53 AM > To: qemu-devel@nongnu.org > Cc: ltaylorsimp...@gmail.com; sidn...@quicinc.com; bc...@quicinc.com; > richard.hender...@linaro.org; a...@rev.ng; a...@rev.ng > Subject: [PATCH] Hexagon: fix HVX store new > > At 09a7e7db0f (Hexagon (target/hexagon) Remove uses of > op_regs_generated.h.inc, 2024-03-06), we've changed the logic of > check_new_value() to use the new pre-calculated > packet->insn[...].dest_idx instead of calculating the index on the fly > using opcode_reginfo[...]. The dest_idx index is calculated roughly like the > following: > > for reg in iset[tag]["syntax"]: > if reg.is_written(): > dest_idx = regno > break > > Thus, we take the first register that is writtable. Before that, however, we > also used to follow an alphabetical order on the register > type: 'd', 'e', 'x', and 'y'. No longer following that makes us select the wrong > register index and the HVX store new instruction does not update the > memory like expected. > > Signed-off-by: Matheus Tavares Bernardino Reviewed-by: Taylor Simpson
RE: [PATCH v2 4/4] target/hexagon: idef-parser simplify predicate init
> -Original Message- > From: Anton Johansson > Sent: Friday, May 10, 2024 9:53 AM > To: qemu-devel@nongnu.org > Cc: a...@rev.ng; ltaylorsimp...@gmail.com; bc...@quicinc.com > Subject: [PATCH v2 4/4] target/hexagon: idef-parser simplify predicate init > > Only predicate instruction arguments need to be initialized by idef-parser. > This commit removes registers from the init_list and simplifies > gen_inst_init_args() slightly. > > Signed-off-by: Anton Johansson Reviewed-by: Taylor Simpson
RE: [PATCH v2 3/4] target/hexagon: idef-parser fix leak of init_list
> -Original Message- > From: Anton Johansson > Sent: Friday, May 10, 2024 9:53 AM > To: qemu-devel@nongnu.org > Cc: a...@rev.ng; ltaylorsimp...@gmail.com; bc...@quicinc.com > Subject: [PATCH v2 3/4] target/hexagon: idef-parser fix leak of init_list > > gen_inst_init_args() is called for instructions using a predicate as an rvalue. > Upon first call, the list of arguments which might need initialization init_list is > freed to indicate that they have been processed. For instructions without an > rvalue predicate, > gen_inst_init_args() isn't called and init_list will never be freed. > > Free init_list from free_instruction() if it hasn't already been freed. > A comment in free_instruction is also updated. > > Signed-off-by: Anton Johansson Reviewed-by: Taylor Simpson
RE: [PATCH] Hexagon: lldb read/write predicate registers p0/p1/p2/p3
> -Original Message-
> From: Matheus Tavares Bernardino
> Sent: Wednesday, June 12, 2024 12:30 PM
> To: ltaylorsimp...@gmail.com
> Cc: qemu-devel@nongnu.org; bc...@quicinc.com; tedw...@quicinc.com;
> alex.ben...@linaro.org; quic_mathb...@quicinc.com;
> sidn...@quicinc.com; quic_mlie...@quicinc.com;
> richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: Re: [PATCH] Hexagon: lldb read/write predicate registers
> p0/p1/p2/p3
>
> On Wed, 12 Jun 2024 10:42:39 -0600 Taylor Simpson
> wrote:
> >
> > diff --git a/target/hexagon/gdbstub.c b/target/hexagon/gdbstub.c index
> > 502c6987f0..e67e627fc9 100644
> > --- a/target/hexagon/gdbstub.c
> > +++ b/target/hexagon/gdbstub.c
> > @@ -56,6 +64,15 @@ int hexagon_gdb_write_register(CPUState *cs,
> uint8_t *mem_buf, int n)
> > return sizeof(target_ulong);
> > }
> >
> > +n -= TOTAL_PER_THREAD_REGS;
> > +
> > +if (n < NUM_PREGS) {
> > +env->pred[n] = ldtul_p(mem_buf);
> > +return sizeof(uint8_t);
>
> I wonder, shouldn't this be sizeof(target_ulong) since we wrote a
> target_ulong?
Good question.
>From the architecture point of view, predicates are 8 bits (Section 2.2.5 of
the v73 Hexagon PRM). However, we model them in QEMU as target_ulong
because TCG variables must be either 32 bits or 64 bits. There isn't an
option for 8 bits. Whenever we write to a predicate, do "and" with 0xff
first to ensure there are only 8 bits written (see gen_log_pred_write in
target/hexagon/genptr.c).
I did some more digging and here is what I found:
- Since we have bitsize="8" in hexagon-core.xml, lldb will reject any
attempt to write something larger.
(lldb) reg write p1 0x1ff
error: Failed to write register 'p1' with value '0x1ff': value 0x1ff is
too large to fit in a 1 byte unsigned integer value
- For the lldb "reg write" command, the return value from
hexagon_gdb_write_register isn't used.
- The only place the return value is used is in handle_write_all_regs. This
function is called in response to a "G" packet from the debugger. I don't
know if/when lldb uses this packet, but it seems like it would count on it
being 8 bits since that's what is in hexagon-core.xml.
Ted , when would lldb generate a "G" packet, and what
assumptions will it make about the size of predicate registers?
Thanks,
Taylor
RE: [PATCH] Hexagon: lldb read/write predicate registers p0/p1/p2/p3
> -Original Message-
> From: Ted Woodward
> Sent: Thursday, June 13, 2024 9:03 AM
> To: ltaylorsimp...@gmail.com; Matheus Bernardino (QUIC)
>
> Cc: qemu-devel@nongnu.org; Brian Cain ;
> alex.ben...@linaro.org; Sid Manning ; Marco
> Liebel (QUIC) ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: RE: [PATCH] Hexagon: lldb read/write predicate registers
> p0/p1/p2/p3
>
>
>
> > -Original Message-
> > From: ltaylorsimp...@gmail.com
> > Sent: Wednesday, June 12, 2024 9:49 PM
> > To: Matheus Bernardino (QUIC)
> > Cc: qemu-devel@nongnu.org; Brian Cain ; Ted
> > Woodward ; alex.ben...@linaro.org; Sid
> Manning
> > ; Marco Liebel (QUIC)
> ;
> > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > a...@rev.ng
> > Subject: RE: [PATCH] Hexagon: lldb read/write predicate registers
> > p0/p1/p2/p3
> >
> > > -Original Message-
> > > From: Matheus Tavares Bernardino
> > > Sent: Wednesday, June 12, 2024 12:30 PM
> > > To: ltaylorsimp...@gmail.com
> > > Cc: qemu-devel@nongnu.org; bc...@quicinc.com;
> tedw...@quicinc.com;
> > > alex.ben...@linaro.org; quic_mathb...@quicinc.com;
> > > sidn...@quicinc.com; quic_mlie...@quicinc.com;
> > > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > > a...@rev.ng
> > > Subject: Re: [PATCH] Hexagon: lldb read/write predicate registers
> > > p0/p1/p2/p3
> > >
> > > On Wed, 12 Jun 2024 10:42:39 -0600 Taylor Simpson
> > > wrote:
> > > >
> > > > diff --git a/target/hexagon/gdbstub.c b/target/hexagon/gdbstub.c
> > > > index
> > > > 502c6987f0..e67e627fc9 100644
> > > > --- a/target/hexagon/gdbstub.c
> > > > +++ b/target/hexagon/gdbstub.c
> > > > @@ -56,6 +64,15 @@ int hexagon_gdb_write_register(CPUState *cs,
> > > uint8_t *mem_buf, int n)
> > > > return sizeof(target_ulong);
> > > > }
> > > >
> > > > +n -= TOTAL_PER_THREAD_REGS;
> > > > +
> > > > +if (n < NUM_PREGS) {
> > > > +env->pred[n] = ldtul_p(mem_buf);
> > > > +return sizeof(uint8_t);
> > >
> > > I wonder, shouldn't this be sizeof(target_ulong) since we wrote a
> > > target_ulong?
> >
> > Good question.
> >
> > From the architecture point of view, predicates are 8 bits (Section
> > 2.2.5 of the
> > v73 Hexagon PRM). However, we model them in QEMU as target_ulong
> > because TCG variables must be either 32 bits or 64 bits. There isn't
> > an option for 8 bits. Whenever we write to a predicate, do "and" with
> > 0xff first to ensure there are only 8 bits written (see
> > gen_log_pred_write in target/hexagon/genptr.c).
> >
> > I did some more digging and here is what I found:
> > - Since we have bitsize="8" in hexagon-core.xml, lldb will reject any
> > attempt to write something larger.
> > (lldb) reg write p1 0x1ff
> > error: Failed to write register 'p1' with value '0x1ff': value 0x1ff
> > is too large to fit in a 1 byte unsigned integer value
> > - For the lldb "reg write" command, the return value from
> > hexagon_gdb_write_register isn't used.
> > - The only place the return value is used is in handle_write_all_regs.
> > This function is called in response to a "G" packet from the debugger.
> > I don't know if/when lldb uses this packet, but it seems like it would
> > count on it being 8 bits since that's what is in hexagon-core.xml.
> >
> > Ted , when would lldb generate a "G" packet, and
> > what assumptions will it make about the size of predicate registers?
>
> When you use the expression parser to call a function, lldb will save the
> current state, set up the function call, set a breakpoint on a return (by
> changing the lr register and setting a breakpoint on the new address), set
the
> PC to the function address, and resume. After the breakpoint is hit, lldb
will
> restore the saved state.
>
> Since QEMU doesn't support the lldb RSP extension
> QSaveRegisterState/QRestoreRegisterState,
> lldb will use G/g packets to save and restore the register state.
>
> lldb doesn't interpret the values from the G/g packets. It just saves and
> restores them, so I don't think the new predicate definitions will matter
for
> that. You can test this out by changing the predicate registers, then
calling a
> function with the expression parser. Not a varargs function, since the IR
> interpreter doesn't handle those.
>
> Ted
Thanks Ted! We do indeed execute handle_write_all_regs when we print the
result of a function call in lldb.
So, the answer to Metheus' question is "no". We should return sizeof
uint8_t. However, we should also mask off the high bits from the value
returned from ldtul_p before assigning to the predicate register. This
avoids putting bits from subsequent items in the buffer into the register.
env->pred[n] = ldtul_p(mem_buf) & 0xff;
I'll send v2 of the patch with this change shortly.
Taylor
RE: [PATCH v2 1/2] target/hexagon: rename HEX_EXCP_*=>HEX_CAUSE_*
> -Original Message- > From: Brian Cain > Sent: Monday, August 26, 2024 6:27 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; quic_mathb...@quicinc.com; > sidn...@quicinc.com; quic_mlie...@quicinc.com; > ltaylorsimp...@gmail.com; Laurent Vivier > Subject: [PATCH v2 1/2] target/hexagon: rename > HEX_EXCP_*=>HEX_CAUSE_* > > The values previously used for "HEX_EXCP_*" were the cause code > definitions and not the event numbers. So in this commit, we update the > names to reflect the cause codes. In HEX_EVENT_TRAP0's case, we add a > new "HEX_EVENT_*" with the correct event number. > > Signed-off-by: Brian Cain > --- > linux-user/hexagon/cpu_loop.c | 4 ++-- > target/hexagon/cpu.h | 2 +- > target/hexagon/cpu_bits.h | 15 --- > target/hexagon/gen_tcg.h | 2 +- > target/hexagon/translate.c| 6 +++--- > 5 files changed, 15 insertions(+), 14 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH v2 2/2] target/hexagon: add enums for event, cause
> -Original Message- > From: Brian Cain > Sent: Monday, August 26, 2024 6:27 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; quic_mathb...@quicinc.com; > sidn...@quicinc.com; quic_mlie...@quicinc.com; > ltaylorsimp...@gmail.com > Subject: [PATCH v2 2/2] target/hexagon: add enums for event, cause > > Signed-off-by: Brian Cain > --- > target/hexagon/cpu_bits.h | 22 ++ > 1 file changed, 14 insertions(+), 8 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH v2 2/3] target/hexagon: fix some occurrences of -Wshadow=local
> -Original Message-
> From: Brian Cain
> Sent: Thursday, October 5, 2023 4:22 PM
> To: qemu-devel@nongnu.org
> Cc: bc...@quicinc.com; arm...@redhat.com; richard.hender...@linaro.org;
> phi...@linaro.org; peter.mayd...@linaro.org; quic_mathb...@quicinc.com;
> stefa...@redhat.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com
> Subject: [PATCH v2 2/3] target/hexagon: fix some occurrences of -
> Wshadow=local
>
> Of the changes in this commit, the changes in
> `HELPER(commit_hvx_stores)()` are less obvious. They are required because
> of some macro invocations like SCATTER_OP_WRITE_TO_MEM().
>
> e.g.:
>
> In file included from ../target/hexagon/op_helper.c:31:
> ../target/hexagon/mmvec/macros.h:205:18: error: declaration of ‘i’
> shadows a previous local [-Werror=shadow=compatible-local]
> 205 | for (int i = 0; i < sizeof(MMVector); i += sizeof(TYPE))
> { \
> | ^
> ../target/hexagon/op_helper.c:157:17: note: in expansion of macro
> ‘SCATTER_OP_WRITE_TO_MEM’
> 157 | SCATTER_OP_WRITE_TO_MEM(uint16_t);
> | ^~~
> ../target/hexagon/op_helper.c:135:9: note: shadowed declaration is here
> 135 | int i;
> | ^
> In file included from ../target/hexagon/op_helper.c:31:
> ../target/hexagon/mmvec/macros.h:204:19: error: declaration of ‘ra’
> shadows a previous local [-Werror=shadow=compatible-local]
> 204 | uintptr_t ra = GETPC(); \
> | ^~
> ../target/hexagon/op_helper.c:160:17: note: in expansion of macro
> ‘SCATTER_OP_WRITE_TO_MEM’
> 160 | SCATTER_OP_WRITE_TO_MEM(uint32_t);
> | ^~~
> ../target/hexagon/op_helper.c:134:15: note: shadowed declaration is here
> 134 | uintptr_t ra = GETPC();
> | ^~
>
> Reviewed-by: Matheus Tavares Bernardino
> Signed-off-by: Brian Cain
> ---
> target/hexagon/imported/alu.idef | 6 +++---
> target/hexagon/mmvec/macros.h| 6 +++---
> target/hexagon/op_helper.c | 9 +++--
> target/hexagon/translate.c | 9 -
> 4 files changed, 13 insertions(+), 17 deletions(-)
Reviewed-by: Taylor Simpson
RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> -Original Message- > From: Brian Cain > Sent: Thursday, October 5, 2023 4:22 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; arm...@redhat.com; richard.hender...@linaro.org; > phi...@linaro.org; peter.mayd...@linaro.org; quic_mathb...@quicinc.com; > stefa...@redhat.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com > Subject: [PATCH v2 3/3] target/hexagon: avoid shadowing globals > > The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the > identifiers to avoid shadowing the type name. > > The global `cpu_env` is shadowed by local `cpu_env` arguments, so we > rename the function arguments to avoid shadowing the global. > > Signed-off-by: Brian Cain > --- > target/hexagon/genptr.c | 56 - > target/hexagon/genptr.h | 18 > target/hexagon/mmvec/system_ext_mmvec.c | 4 +- > target/hexagon/mmvec/system_ext_mmvec.h | 2 +- > target/hexagon/op_helper.c | 4 +- > 5 files changed, 42 insertions(+), 42 deletions(-) > > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index > 217bc7bb5a..11377ac92b 100644 > --- a/target/hexagon/genptr.c > +++ b/target/hexagon/genptr.c > @@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result, TCGv > src) > tcg_gen_deposit_i64(result, result, src64, N * 8, 8); } > > -static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int > mem_index) > +static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int > +mem_index) I'd recommend moving both the type and the arg name to the new line, also indent the new line. static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int mem_index) > > -static inline void gen_load_locked8u(TCGv_i64 dest, TCGv vaddr, int > mem_index) > +static inline void gen_load_locked8u(TCGv_i64 dest, TCGv v_addr, int > +mem_index) Ditto > static inline void gen_store_conditional4(DisasContext *ctx, > - TCGv pred, TCGv vaddr, TCGv src) > + TCGv pred, TCGv v_addr, TCGv > + src) Ditto > zero = tcg_constant_tl(0); > @@ -374,13 +374,13 @@ static inline void > gen_store_conditional4(DisasContext *ctx, } > > static inline void gen_store_conditional8(DisasContext *ctx, > - TCGv pred, TCGv vaddr, TCGv_i64 > src) > + TCGv pred, TCGv v_addr, > + TCGv_i64 src) Indent > -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int > slot) > +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int > +slot) Ditto > -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int > slot); > +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int > +slot); Ditto Otherwise, Reviewed-by: Taylor Simpson
RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> -Original Message- > From: Brian Cain > Sent: Sunday, October 8, 2023 7:50 AM > To: ltaylorsimp...@gmail.com; qemu-devel@nongnu.org > Cc: arm...@redhat.com; richard.hender...@linaro.org; phi...@linaro.org; > peter.mayd...@linaro.org; Matheus Bernardino (QUIC) > ; stefa...@redhat.com; a...@rev.ng; > a...@rev.ng; Marco Liebel (QUIC) > Subject: RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals > > > > > -Original Message- > > From: ltaylorsimp...@gmail.com > > Sent: Friday, October 6, 2023 11:01 AM > > To: Brian Cain ; qemu-devel@nongnu.org > > Cc: arm...@redhat.com; richard.hender...@linaro.org; > > phi...@linaro.org; peter.mayd...@linaro.org; Matheus Bernardino (QUIC) > > ; stefa...@redhat.com; a...@rev.ng; > > a...@rev.ng; Marco Liebel (QUIC) > > Subject: RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals > > > > WARNING: This email originated from outside of Qualcomm. Please be > > wary of any links or attachments, and do not enable macros. > > > > > -Original Message- > > > From: Brian Cain > > > Sent: Thursday, October 5, 2023 4:22 PM > > > To: qemu-devel@nongnu.org > > > Cc: bc...@quicinc.com; arm...@redhat.com; > > > richard.hender...@linaro.org; phi...@linaro.org; > > > peter.mayd...@linaro.org; quic_mathb...@quicinc.com; > > > stefa...@redhat.com; a...@rev.ng; a...@rev.ng; > > > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com > > > Subject: [PATCH v2 3/3] target/hexagon: avoid shadowing globals > > > > > > The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename > > > the identifiers to avoid shadowing the type name. > > > > > > The global `cpu_env` is shadowed by local `cpu_env` arguments, so we > > > rename the function arguments to avoid shadowing the global. > > > > > > Signed-off-by: Brian Cain > > > --- > > > target/hexagon/genptr.c | 56 - > > > target/hexagon/genptr.h | 18 > > > target/hexagon/mmvec/system_ext_mmvec.c | 4 +- > > > target/hexagon/mmvec/system_ext_mmvec.h | 2 +- > > > target/hexagon/op_helper.c | 4 +- > > > 5 files changed, 42 insertions(+), 42 deletions(-) > > > > > > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index > > > 217bc7bb5a..11377ac92b 100644 > > > --- a/target/hexagon/genptr.c > > > +++ b/target/hexagon/genptr.c > > > @@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result, > > TCGv > > > src) > > > tcg_gen_deposit_i64(result, result, src64, N * 8, 8); } > > > > > > -static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int > > > mem_index) > > > +static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int > > > +mem_index) > > > > I'd recommend moving both the type and the arg name to the new line, > > also indent the new line. > > static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, > > int > > mem_index) > > > > > I could be mistaken but AFAICT none of these lines are wrapped in the way > they're quoted above in my patch (nor the baseline). I don't think any of > these lines exceed 80 columns, so they shouldn't need wrapping, either. > > I double checked how it's displayed at the archive > https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg01667.html to > make sure that it wasn't a misconfiguration of my mailer. For another > perspective - refer to the commit used to create this patch: > https://github.com/quic/qemu/commit/7f20565d403d16337ab6d69ee663121 > a3eef71e6 > > Is your review comment that "these lines should be wrapped and when you > do, make sure you do it like this"? Or "if you are going to wrap them, wrap > them like this"? Or something else? Yes. It looked like some adding the v_ would sometimes put the line over the 80 character size. If so, wrap them as described. If not, no wrapping is needed. > > > Otherwise, > > Reviewed-by: Taylor Simpson > >
RE: [PATCH v2] Hexagon: move GETPC() calls to top level helpers
> -Original Message-
> From: Matheus Tavares Bernardino
> Sent: Wednesday, July 5, 2023 12:35 PM
> To: qemu-devel@nongnu.org
> Cc: quic_mathb...@quicinc.com; bc...@quicinc.com;
> ltaylorsimp...@gmail.com; quic_mlie...@quicinc.com;
> richard.hender...@linaro.org
> Subject: [PATCH v2] Hexagon: move GETPC() calls to top level helpers
>
> As docs/devel/loads-stores.rst states:
>
> ``GETPC()`` should be used with great care: calling
> it in other functions that are *not* the top level
> ``HELPER(foo)`` will cause unexpected behavior. Instead, the
> value of ``GETPC()`` should be read from the helper and passed
> if needed to the functions that the helper calls.
>
> Let's fix the GETPC() usage in Hexagon, making sure it's always called
from
> top level helpers and passed down to the places where it's needed. There
> are two snippets where that is not currently the case:
>
> - probe_store(), which is only called from two helpers, so it's easy to
> move GETPC() up.
>
> - mem_load*() functions, which are also called directly from helpers,
> but through the MEM_LOAD*() set of macros. Note that this are only
> used when compiling with --disable-hexagon-idef-parser.
>
> In this case, we also take this opportunity to simplify the code,
> unifying the mem_load*() functions.
>
> Signed-off-by: Matheus Tavares Bernardino
> ---
> v1:
> d40fabcf9d6e92e4cd8d6a144e9b2a9acf4580dc.1688420966.git.quic_mathber
> n...@quicinc.com
>
> Changes in v2:
> - Fixed wrong cpu_ld* unification from previous version.
> - Passed retaddr down to check_noshuf() and further, as Taylor
> suggested.
> - Reorganized macros for simplification.
>
> target/hexagon/macros.h| 19 ++--
> target/hexagon/op_helper.h | 11 ++- target/hexagon/op_helper.c | 62
> +++---
> 3 files changed, 29 insertions(+), 63 deletions(-)
>
> diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h index
> 5451b061ee..e44a932434 100644
> --- a/target/hexagon/macros.h
> +++ b/target/hexagon/macros.h
> @@ -173,15 +173,6 @@
> #define fLOAD(NUM, SIZE, SIGN, EA, DST) \
> -DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE##SIGN(EA)
> +DST = (size##SIZE##SIGN##_t)({ \
> +check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \
> +MEM_LOAD##SIZE(env, EA, GETPC()); \
> +})
> #endif
This should be formatted as
#define fLOAD(...) \
do { \
check_noshuf(...); \
DST = ...; \
} while (0)
> a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h index
> 8f3764d15e..7744e819ef 100644
> --- a/target/hexagon/op_helper.h
> +++ b/target/hexagon/op_helper.h
> +void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
> + uint32_t slot, target_ulong vaddr, int size,
> +uintptr_t ra);
Are you sure this needs to be non-static?
Othersiwe
Reviewed-by: Taylor Simpson
RE: [PATCH v2] Hexagon: move GETPC() calls to top level helpers
> -Original Message- > From: Matheus Tavares Bernardino > Sent: Thursday, July 6, 2023 5:23 AM > To: ltaylorsimp...@gmail.com > Cc: bc...@quicinc.com; qemu-devel@nongnu.org; > quic_mathb...@quicinc.com; quic_mlie...@quicinc.com; > richard.hender...@linaro.org > Subject: Re: [PATCH v2] Hexagon: move GETPC() calls to top level helpers > > > > ltaylorsimp...@gmail.com wrote: > > > > > -Original Message- > > > From: Matheus Tavares Bernardino > > > Sent: Wednesday, July 5, 2023 12:35 PM > > > To: qemu-devel@nongnu.org > > > Cc: quic_mathb...@quicinc.com; bc...@quicinc.com; > > > ltaylorsimp...@gmail.com; quic_mlie...@quicinc.com; > > > richard.hender...@linaro.org > > > Subject: [PATCH v2] Hexagon: move GETPC() calls to top level helpers > > > > > > a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h index > > > 8f3764d15e..7744e819ef 100644 > > > --- a/target/hexagon/op_helper.h > > > +++ b/target/hexagon/op_helper.h > > > +void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1, > > > + uint32_t slot, target_ulong vaddr, int size, > > > +uintptr_t ra); > > > > Are you sure this needs to be non-static? > > Yeah, since we remove the mem_load*() functions, check_noshuf() must > now be visible to the other compilation units that include macros.h, as we will > expand the fLOAD() macro to call it. Since the generated helper functions are included at the bottom of op_helper.c #include "helper_funcs_generated.c.inc" it can be static. It needs to be guarded with #ifndef CONFIG_HEXAGON_IDEF_PARSER because it is not used when by any of the idef-parser functions. Thanks, Taylor
RE: [PATCH] Hexagon: move GETPC() calls to top level helpers
-Original Message-
From: Matheus Tavares Bernardino
Sent: Monday, July 3, 2023 3:50 PM
To: qemu-devel@nongnu.org
Cc: bc...@quicinc.com; quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com
Subject: [PATCH] Hexagon: move GETPC() calls to top level helpers
As docs/devel/loads-stores.rst states:
``GETPC()`` should be used with great care: calling
it in other functions that are *not* the top level
``HELPER(foo)`` will cause unexpected behavior. Instead, the
value of ``GETPC()`` should be read from the helper and passed
if needed to the functions that the helper calls.
Let's fix the GETPC() usage in Hexagon, making sure it's always called from
top level helpers and passed down to the places where it's needed. There are
two snippets where that is not currently the case:
- probe_store(), which is only called from two helpers, so it's easy to
move GETPC() up.
- mem_load*() functions, which are also called directly from helpers,
but through the MEM_LOAD*() set of macros. Note that this are only
used when compiling with --disable-hexagon-idef-parser.
In this case, we also take this opportunity to simplify the code,
unifying the mem_load*() functions.
Signed-off-by: Matheus Tavares Bernardino
---
target/hexagon/macros.h| 22 ++---
target/hexagon/op_helper.h | 11 ++--- target/hexagon/op_helper.c | 49
+++---
3 files changed, 25 insertions(+), 57 deletions(-)
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h index
5451b061ee..efb8013912 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
+
+#define MEM_LOADn(SIZE, VA) ({ \
+check_noshuf(env, pkt_has_store_s1, slot, VA, SIZE); \
+cpu_ldub_data_ra(env, VA, GETPC()); \
+})
Note that check_noshuf calls HELPER(probe_noshuf_load) and
HELPER(commit_store). Both of those call GETPC() from within. So, you'll
need to pull the contents into separate functions that take ra as an
argument.
Does this pass the test suite? You are only using the SIZE parameter in
check_noshuf, but cpu_ldub_data_ra only reads a single byte.
+#define MEM_LOAD1s(VA) ((int8_t)MEM_LOADn(1, VA)) #define
+MEM_LOAD1u(VA) ((uint8_t)MEM_LOADn(1, VA)) #define MEM_LOAD2s(VA)
+((int16_t)MEM_LOADn(2, VA)) #define MEM_LOAD2u(VA)
+((uint16_t)MEM_LOADn(2, VA)) #define MEM_LOAD4s(VA)
+((int32_t)MEM_LOADn(4, VA)) #define MEM_LOAD4u(VA)
+((uint32_t)MEM_LOADn(4, VA)) #define MEM_LOAD8s(VA)
+((int64_t)MEM_LOADn(8, VA)) #define MEM_LOAD8u(VA)
+((uint64_t)MEM_LOADn(8, VA))
A further cleanup would be to remove these altogether and modify the
definition of fLOAD to use MEM_LOADn.
#define MEM_STORE1(VA, DATA, SLOT) log_store32(env, VA, DATA, 1, SLOT)
#define MEM_STORE2(VA, DATA, SLOT) log_store32(env, VA, DATA, 2, SLOT) diff
--git a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h index
8f3764d15e..845c3d197e 100644
--- a/target/hexagon/op_helper.h
+++ b/target/hexagon/op_helper.h
@@ -19,15 +19,8 @@
#define HEXAGON_OP_HELPER_H
+void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
+ uint32_t slot, target_ulong vaddr, int size);
Does this really need to be non-static?
Thanks,
Taylor
RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators object oriented - gen_helper_protos
> -Original Message-
> From: Brian Cain
> Sent: Monday, December 4, 2023 9:46 PM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: Matheus Bernardino (QUIC) ; Sid
> Manning ; Marco Liebel (QUIC)
> ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators object
> oriented - gen_helper_protos
>
>
>
> > -Original Message-
> > From: Taylor Simpson
> > Sent: Monday, December 4, 2023 7:53 PM
> > To: qemu-devel@nongnu.org
> > Cc: Brian Cain ; Matheus Bernardino (QUIC)
> > ; Sid Manning ;
> Marco
> > Liebel (QUIC) ;
> > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > a...@rev.ng; ltaylorsimp...@gmail.com
> > Subject: [PATCH 3/9] Hexagon (target/hexagon) Make generators object
> > oriented - gen_helper_protos
> >
> > Signed-off-by: Taylor Simpson
> > ---
> > target/hexagon/gen_helper_protos.py | 184
> > target/hexagon/hex_common.py| 15 +--
> > 2 files changed, 55 insertions(+), 144 deletions(-)
> >
> > diff --git a/target/hexagon/gen_helper_protos.py
> > b/target/hexagon/gen_helper_protos.py
> > index 131043795a..9277199e1d 100755
> > --- a/target/hexagon/gen_helper_protos.py
> > +++ b/target/hexagon/gen_helper_protos.py
> > ##
> > ## Generate the DEF_HELPER prototype for an instruction
> > ## For A2_add: Rd32=add(Rs32,Rt32)
> > @@ -65,116 +32,62 @@ def gen_helper_prototype(f, tag, tagregs,
> tagimms):
> > regs = tagregs[tag]
> > imms = tagimms[tag]
> >
> > -numresults = 0
> > +## If there is a scalar result, it is the return type
> > +return_type = ""
>
> Should we use `return_type = None` here?
>
> > numscalarresults = 0
> > -numscalarreadwrite = 0
> > for regtype, regid in regs:
> > -if hex_common.is_written(regid):
> > -numresults += 1
> > -if hex_common.is_scalar_reg(regtype):
> > +reg = hex_common.get_register(tag, regtype, regid)
> > +if reg.is_written() and reg.is_scalar_reg():
> > +return_type = reg.helper_proto_type()
> > numscalarresults += 1
> > -if hex_common.is_readwrite(regid):
> > -if hex_common.is_scalar_reg(regtype):
> > -numscalarreadwrite += 1
> > +if numscalarresults == 0:
> > +return_type = "void"
>
> Should we use `return_type = None` here?
I don't see a point of setting it to None. By the time it gets to the use
below, it will definitely have a value. We could initialize it to void instead
of "" and skip this check.
> > +
> > +## Other stuff the helper might need
> > +if hex_common.need_pkt_has_multi_cof(tag):
> > +declared.append("i32")
> > +if hex_common.need_pkt_need_commit(tag):
> > +declared.append("i32")
> > +if hex_common.need_PC(tag):
> > +declared.append("i32")
> > +if hex_common.need_next_PC(tag):
> > +declared.append("i32")
> > +if hex_common.need_slot(tag):
> > +declared.append("i32")
> > +if hex_common.need_part1(tag):
> > +declared.append("i32")
>
> What do you think of this instead? The delta below is on top of this patch.
>
> --- a/target/hexagon/gen_helper_protos.py
> +++ b/target/hexagon/gen_helper_protos.py
> @@ -73,18 +73,9 @@ def gen_helper_prototype(f, tag, tagregs, tagimms):
> declared.append("s32")
>
> ## Other stuff the helper might need
> -if hex_common.need_pkt_has_multi_cof(tag):
> -declared.append("i32")
> -if hex_common.need_pkt_need_commit(tag):
> -declared.append("i32")
> -if hex_common.need_PC(tag):
> -declared.append("i32")
> -if hex_common.need_next_PC(tag):
> -declared.append("i32")
> -if hex_common.need_slot(tag):
> -declared.append("i32")
> -if hex_common.need_part1(tag):
> -declared.append("i32")
> +for stuff in hex_common.other_stuff:
> +if stuff(tag):
> +declared.append('i32')
>
> arguments = ", ".join(declared)
> f.write(f"DEF_HELPER_{len(declared) - 1}({tag}, {arguments})\n") diff
> --git
> a/target/hexagon/gen_tcg_funcs.py b/target/hexagon/gen_tcg_funcs.py
> index 8c2bc03c10..cb02d91886 100755
> --- a/target/hexagon/gen_tcg_funcs.py
> +++ b/target/hexagon/gen_tcg_funcs.py
> @@ -109,18 +109,9 @@ def gen_tcg_func(f, tag, regs, imms):
>
> declared.append(f"tcg_constant_tl({hex_common.imm_name(immlett)})")
>
> ## Other stuff the helper might need
> -if hex_common.need_pkt_has_multi_cof(tag):
> -declared.append("tcg_constant_tl(ctx->pkt->pkt_has_multi_cof)")
> -if hex_common.need_pkt_need_commit(tag):
> -declared.append("tcg_constant_tl(ctx->need_commit)")
> -if hex_common.need_PC(tag):
> -declared.append("tcg_constant_tl(ctx->pkt->pc)")
> -if hex_common.need_next_PC(tag):
> -declared.append("tcg_constant_tl(ctx->nex
RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators object oriented - gen_helper_protos
> -Original Message-
> From: ltaylorsimp...@gmail.com
> Sent: Tuesday, December 5, 2023 11:08 AM
> To: 'Brian Cain' ; qemu-devel@nongnu.org
> Cc: 'Matheus Bernardino (QUIC)' ; 'Sid
> Manning' ; 'Marco Liebel (QUIC)'
> ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators object
> oriented - gen_helper_protos
>
>
>
> > -Original Message-
> > From: Brian Cain
> > Sent: Monday, December 4, 2023 9:46 PM
> > To: Taylor Simpson ; qemu-
> de...@nongnu.org
> > Cc: Matheus Bernardino (QUIC) ; Sid
> Manning
> > ; Marco Liebel (QUIC)
> ;
> > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > a...@rev.ng
> > Subject: RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators
> > object oriented - gen_helper_protos
> >
> >
> >
> > > -Original Message-
> > > From: Taylor Simpson
> > > Sent: Monday, December 4, 2023 7:53 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: Brian Cain ; Matheus Bernardino (QUIC)
> > > ; Sid Manning ;
> > Marco
> > > Liebel (QUIC) ;
> > > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > > a...@rev.ng; ltaylorsimp...@gmail.com
> > > Subject: [PATCH 3/9] Hexagon (target/hexagon) Make generators object
> > > oriented - gen_helper_protos
> > >
> > > Signed-off-by: Taylor Simpson
> > > ---
> > > target/hexagon/gen_helper_protos.py | 184
> > > target/hexagon/hex_common.py| 15 +--
> > > 2 files changed, 55 insertions(+), 144 deletions(-)
> > >
> > > diff --git a/target/hexagon/gen_helper_protos.py
> > > b/target/hexagon/gen_helper_protos.py
> > > index 131043795a..9277199e1d 100755
> > > --- a/target/hexagon/gen_helper_protos.py
> > > +++ b/target/hexagon/gen_helper_protos.py
> > > ##
> > > ## Generate the DEF_HELPER prototype for an instruction
> > > ## For A2_add: Rd32=add(Rs32,Rt32)
> > > @@ -65,116 +32,62 @@ def gen_helper_prototype(f, tag, tagregs,
> > tagimms):
> > > regs = tagregs[tag]
> > > imms = tagimms[tag]
> > >
> > > -numresults = 0
> > > +## If there is a scalar result, it is the return type
> > > +return_type = ""
> >
> > Should we use `return_type = None` here?
> >
> > > numscalarresults = 0
> > > -numscalarreadwrite = 0
> > > for regtype, regid in regs:
> > > -if hex_common.is_written(regid):
> > > -numresults += 1
> > > -if hex_common.is_scalar_reg(regtype):
> > > +reg = hex_common.get_register(tag, regtype, regid)
> > > +if reg.is_written() and reg.is_scalar_reg():
> > > +return_type = reg.helper_proto_type()
> > > numscalarresults += 1
> > > -if hex_common.is_readwrite(regid):
> > > -if hex_common.is_scalar_reg(regtype):
> > > -numscalarreadwrite += 1
> > > +if numscalarresults == 0:
> > > +return_type = "void"
> >
> > Should we use `return_type = None` here?
>
> I don't see a point of setting it to None. By the time it gets to the use
> below,
> it will definitely have a value. We could initialize it to void instead of
> "" and
> skip this check.
>
>
> > > +
> > > +## Other stuff the helper might need
> > > +if hex_common.need_pkt_has_multi_cof(tag):
> > > +declared.append("i32")
> > > +if hex_common.need_pkt_need_commit(tag):
> > > +declared.append("i32")
> > > +if hex_common.need_PC(tag):
> > > +declared.append("i32")
> > > +if hex_common.need_next_PC(tag):
> > > +declared.append("i32")
> > > +if hex_common.need_slot(tag):
> > > +declared.append("i32")
> > > +if hex_common.need_part1(tag):
> > > +declared.append("i32")
> >
> > What do you think of this instead? The delta below is on top of this patch.
> >
> > --- a/target/hexagon/gen_helper_protos.py
> > +++ b/target/hexagon/gen_helper_protos.py
> > @@ -73,18 +73,9 @@ def gen_helper_prototype(f, tag, tagregs, tagimms):
> > declared.append("s32")
> >
> > ## Other stuff the helper might need
> > -if hex_common.need_pkt_has_multi_cof(tag):
> > -declared.append("i32")
> > -if hex_common.need_pkt_need_commit(tag):
> > -declared.append("i32")
> > -if hex_common.need_PC(tag):
> > -declared.append("i32")
> > -if hex_common.need_next_PC(tag):
> > -declared.append("i32")
> > -if hex_common.need_slot(tag):
> > -declared.append("i32")
> > -if hex_common.need_part1(tag):
> > -declared.append("i32")
> > +for stuff in hex_common.other_stuff:
> > +if stuff(tag):
> > +declared.append('i32')
> >
> > arguments = ", ".join(declared)
> > f.write(f"DEF_HELPER_{len(declared) - 1}({tag}, {arguments})\n")
> > diff --git a/target/hexagon/gen_tcg_funcs.py
> > b/target/hexagon/gen_tcg_funcs.py index 8c2bc03c10..cb02d91886 100755
> > --- a/target/hexagon/gen_tcg_funcs.py
> > +++ b/target/hexagon/ge
RE: [RFC PATCH] Hexagon (target/hexagon) Make generators object oriented
> -Original Message-
> From: Brian Cain
> Sent: Wednesday, November 15, 2023 1:51 PM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: Matheus Bernardino (QUIC) ; Sid
> Manning ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: RE: [RFC PATCH] Hexagon (target/hexagon) Make generators object
> oriented
>
>
>
> > -Original Message-
> > From: Taylor Simpson
> > Sent: Thursday, November 9, 2023 3:26 PM
> > To: qemu-devel@nongnu.org
> > Cc: Brian Cain ; Matheus Bernardino (QUIC)
> > ; Sid Manning ;
> > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> a...@rev.ng;
> > ltaylorsimp...@gmail.com
> > Subject: [RFC PATCH] Hexagon (target/hexagon) Make generators object
> > oriented
> >
> > RFC - This patch handles gen_tcg_funcs.py. I'd like to get comments
> > on the general approach before working on the other Python scripts.
> >
> > The generators are generally a bunch of Python if-then-else
> > statements based on the regtype and regid. Encapsulate regtype/regid
> > into a class hierarchy. Clients lookup the register and invoke
> > methods.
> >
> > This has several advantages for making the code easier to read,
> > understand, and maintain
> > - The class name makes it more clear what the operand does
> > - All the methods for a given type of operand are together
> > - Don't need as many calls to hex_common.bad_register
> > - We can remove the functions in hex_common that use regtype/regid
> > (e.g., is_read)
> >
> > Signed-off-by: Taylor Simpson
> > ---
> > diff --git a/target/hexagon/hex_common.py
> b/target/hexagon/hex_common.py
> > index 0da65d6dd6..13ee55b6b2 100755
> > --- a/target/hexagon/hex_common.py
> > +++ b/target/hexagon/hex_common.py
> > +class ModifierSource(Source):
> > +def genptr_decl(self, f, tag, regno):
> > +f.write(f"const int {self.regN} = insn->regno[{regno}];\n")
> > +f.write(f"TCGv {self.regV} = hex_gpr[{self.regN} +
> HEX_REG_M0];\n")
> > +def idef_arg(self, declared):
> > +declared.append(self.regV)
> > +declared.append(self.regN)
> > +
>
> IMO it's easier to reason about a function if it doesn't modify its inputs and
> instead it returns the transformed input. If idef_arg instead returned a new
> list or returned an iterable for the caller to catenate, it would be clearer.
We should figure out a better way to handle the special case of modifier
registers. For every other register type,
Idef_arg simply returns self.regV. For circular addressing, we also need the
value of the corresponding CS register. Currently,
we solve this by passing the register number so that idef-parser can get the
value (i.e., hex_gpr[HEX_REG_CS0 + self.regN]).
We could have idef-parser skip the circular addressing instructions (it already
skips the bit-reverse instructions). That seems
like a big hammer though. Any other thoughts?
> > +class PredReadWrite(ReadWrite):
> > +def genptr_decl(self, f, tag, regno):
> > +f.write(f"const int {self.regN} = insn->regno[{regno}];\n")
> > +f.write(f"TCGv {self.regV} = tcg_temp_new();\n")
> > +f.write(f"tcg_gen_mov_tl({self.regV},
> > hex_pred[{self.regN}]);\n")
>
> Instead of successive calls to f.write(), each passing their own string with a
> newline, use triple quotes:
>
> f.write(f"""const int {self.regN} = insn->regno[{regno}];
> TCGv {self.regV} = tcg_temp_new();
> tcg_gen_mov_tl({self.regV}, hex_pred[{self.regN}]);\n""")
>
> If necessary/appropriate, you can also use textwrap.dedent() to make the
> leading whitespace look appropriate:
> https://docs.python.org/3/library/textwrap.html#textwrap.dedent
>
> import textwrap
> ...
> f.write(textwrap.dedent(f"""const int {self.regN} = insn->regno[{regno}];
> TCGv {self.regV} = tcg_temp_new();
> tcg_gen_mov_tl({self.regV}, hex_pred[{self.regN}]);\n"""))
>
The indenting is for the readability of the output. We could dedent everything
and run the result through the indent utility
as idef-parser does. Not sure it's worth it though.
> > +def init_registers():
> > +registers["Rd"] = GprDest("R", "d")
> > +registers["Re"] = GprDest("R", "e")
> > +registers["Rs"] = GprSource("R", "s")
> > +registers["Rt"] = GprSource("R", "t")
> > +registers["Ru"] = GprSource("R", "u")
> > +registers["Rv"] = GprSource("R", "v")
> > +registers["Rx"] = GprReadWrite("R", "x")
> > +registers["Ry"] = GprReadWrite("R", "y")
> > +registers["Cd"] = ControlDest("C", "d")
> > +registers["Cs"] = ControlSource("C", "s")
> > +registers["Mu"] = ModifierSource("M", "u")
> > +registers["Pd"] = PredDest("P", "d")
> > +registers["Pe"] = PredDest("P", "e")
> > +registers["Ps"] = PredSource("P", "s")
> > +registers["Pt"] = PredSource("P", "t")
> > +registers["Pu"] = PredSource("P", "u")
> > +registers["Pv"] = PredSource("P", "v")
> > +registers["Px"] = PredReadWrite("P", "x")
>
RE: [RFC PATCH] Hexagon (target/hexagon) Make generators object oriented
> -Original Message-
> From: Brian Cain
> Sent: Thursday, November 16, 2023 10:25 AM
> To: ltaylorsimp...@gmail.com; qemu-devel@nongnu.org
> Cc: Matheus Bernardino (QUIC) ; Sid
> Manning ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: RE: [RFC PATCH] Hexagon (target/hexagon) Make generators object
> oriented
>
>
>
> > -Original Message-
> > From: ltaylorsimp...@gmail.com
> > Sent: Wednesday, November 15, 2023 4:03 PM
> > To: Brian Cain ; qemu-devel@nongnu.org
> > Cc: Matheus Bernardino (QUIC) ; Sid
> Manning
> > ; richard.hender...@linaro.org;
> > phi...@linaro.org; a...@rev.ng; a...@rev.ng
> > Subject: RE: [RFC PATCH] Hexagon (target/hexagon) Make generators
> > object oriented
> >
> > > -Original Message-
> > > From: Brian Cain
> > > Sent: Wednesday, November 15, 2023 1:51 PM
> > > To: Taylor Simpson ; qemu-
> de...@nongnu.org
> > > Cc: Matheus Bernardino (QUIC) ; Sid
> > > Manning ; richard.hender...@linaro.org;
> > > phi...@linaro.org; a...@rev.ng; a...@rev.ng
> > > Subject: RE: [RFC PATCH] Hexagon (target/hexagon) Make generators
> > > object oriented
> > >
> > >
> > >
> > > > -Original Message-
> > > > From: Taylor Simpson
> > > > Sent: Thursday, November 9, 2023 3:26 PM
> > > > To: qemu-devel@nongnu.org
> > > > Cc: Brian Cain ; Matheus Bernardino (QUIC)
> > > > ; Sid Manning ;
> > > > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > > a...@rev.ng;
> > > > ltaylorsimp...@gmail.com
> > > > Subject: [RFC PATCH] Hexagon (target/hexagon) Make generators
> > > > object oriented
> > > >
> > > > RFC - This patch handles gen_tcg_funcs.py. I'd like to get
> > > > comments on the general approach before working on the other
> Python scripts.
> > > >
> > > > The generators are generally a bunch of Python if-then-else
> > > > statements based on the regtype and regid. Encapsulate
> > > > regtype/regid into a class hierarchy. Clients lookup the register
> > > > and invoke methods.
> > > >
> > > > This has several advantages for making the code easier to read,
> > > > understand, and maintain
> > > > - The class name makes it more clear what the operand does
> > > > - All the methods for a given type of operand are together
> > > > - Don't need as many calls to hex_common.bad_register
> > > > - We can remove the functions in hex_common that use regtype/regid
> > > > (e.g., is_read)
> > > >
> > > > Signed-off-by: Taylor Simpson
> > > > ---
> > > > diff --git a/target/hexagon/hex_common.py
> > > b/target/hexagon/hex_common.py
> > > > index 0da65d6dd6..13ee55b6b2 100755
> > > > --- a/target/hexagon/hex_common.py
> > > > +++ b/target/hexagon/hex_common.py
> > > > +class PredReadWrite(ReadWrite):
> > > > +def genptr_decl(self, f, tag, regno):
> > > > +f.write(f"const int {self.regN} = insn->regno[{regno}];\n")
> > > > +f.write(f"TCGv {self.regV} = tcg_temp_new();\n")
> > > > +f.write(f"tcg_gen_mov_tl({self.regV},
> hex_pred[{self.regN}]);\n")
> > >
> > > Instead of successive calls to f.write(), each passing their own
> > > string with a newline, use triple quotes:
> > >
> > > f.write(f"""const int {self.regN} = insn->regno[{regno}];
> > > TCGv {self.regV} = tcg_temp_new();
> > > tcg_gen_mov_tl({self.regV}, hex_pred[{self.regN}]);\n""")
> > >
> > > If necessary/appropriate, you can also use textwrap.dedent() to make
> > > the leading whitespace look appropriate:
> > > https://docs.python.org/3/library/textwrap.html#textwrap.dedent
> > >
> > > import textwrap
> > > ...
> > > f.write(textwrap.dedent(f"""const int {self.regN} = insn-
> >regno[{regno}];
> > > TCGv {self.regV} = tcg_temp_new();
> > > tcg_gen_mov_tl({self.regV}, hex_pred[{self.regN}]);\n"""))
> > >
> >
> > The indenting is for the readability of the output. We could dedent
> > everything and run the result through the indent utility as
> > idef-parser does. Not sure it's worth it though.
>
> Regardless of textwrap.dedent(), I think the output is most readable with
> triple-quotes. It's more readable than it is with multiple f.write("this
> line\n")
> invocations.
>
> The intent of calling textwrap.dedent is to allow you to not out-dent the text
> in the python program. Since the indentation of the other lines next to the
> string literal are significant to the program, it might be odd/confusing to
> see
> the python program have out-dented text lines.
>
> Consider the readability of the python program:
>
> if foo:
> f.write(textwrap.dedent(f"""\
> const int {self.regN} = insn->regno[{regno}];
> TCGv {self.regV} = tcg_temp_new();
> tcg_gen_mov_tl({self.regV}, hex_pred[{self.regN}]);
> """))
> if bar:
> f.write(textwrap.dedent(f"""\
> int x = {bar.reg};
> """)
> vs
>
> if foo:
> f.write(f"""\
> const int {self.regN} = insn->regno[{regno}]; TCGv {self.regV} =
> tcg_temp_new(); tcg_gen_mov_tl({self.reg
RE: [PATCH 0/3] Hexagon (target/hexagon) Enable more short-circuit packets
Looks like a problem with git send-email ☹ I will troubleshoot ... > -Original Message- > From: Anton Johansson > Sent: Thursday, November 2, 2023 6:44 PM > To: Taylor Simpson > Cc: qemu-devel@nongnu.org > Subject: Re: [PATCH 0/3] Hexagon (target/hexagon) Enable more short- > circuit packets > > Hi, Taylor!:) Always nice to see your name pop up here. The patches seem to > have been sent as attachments for whatever reason. > > / Anton
RE: [PATCH] Hexagon: fix F2_conv_* instructions for negative zero
> -Original Message- > From: Matheus Tavares Bernardino > Sent: Sunday, July 28, 2024 11:16 AM > To: qemu-devel@nongnu.org > Cc: ltaylorsimp...@gmail.com; bc...@quicinc.com; sidn...@quicinc.com; > a...@rev.ng; a...@rev.ng > Subject: [PATCH] Hexagon: fix F2_conv_* instructions for negative zero > > The implementation for these instructions handles -0 as an invalid float point > value, whereas the Hexagon hardware considers it the same as +0 (which is > valid). Let's fix that and add a regression test. > > Signed-off-by: Matheus Tavares Bernardino > --- > target/hexagon/op_helper.c | 16 > tests/tcg/hexagon/usr.c| 10 ++ > 2 files changed, 18 insertions(+), 8 deletions(-) You should update the copyright year to 2024 in the files you changed. Otherwise Reviewed-by: Taylor Simpson
RE: [PATCH] target/hexagon: define a v66 CPU
> -Original Message- > From: Brian Cain > Sent: Tuesday, July 30, 2024 7:10 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; quic_mathb...@quicinc.com; > sidn...@quicinc.com; quic_mlie...@quicinc.com; > ltaylorsimp...@gmail.com > Subject: [PATCH] target/hexagon: define a v66 CPU > > For now, v66 behavior is the same as other CPUs. > > Signed-off-by: Brian Cain > --- > target/hexagon/cpu-qom.h | 1 + > target/hexagon/cpu.c | 2 ++ > 2 files changed, 3 insertions(+) Reviewed-by: Taylor Simpson
RE: [PATCH] target/hexagon: switch to dc set_props() list
> -Original Message- > From: Brian Cain > Sent: Tuesday, July 30, 2024 7:13 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; quic_mathb...@quicinc.com; > sidn...@quicinc.com; quic_mlie...@quicinc.com; > ltaylorsimp...@gmail.com > Subject: [PATCH] target/hexagon: switch to dc set_props() list > > Define a hexagon_cpu_properties list to match the idiom used by other > targets. > > Signed-off-by: Brian Cain > --- > target/hexagon/cpu.c | 18 -- > 1 file changed, 8 insertions(+), 10 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH] Hexagon (target/hexagon) Remove HEX_DEBUG/HEX_DEBUG_LOG
> -Original Message- > From: Brian Cain > Sent: Monday, November 4, 2024 3:52 PM > To: Taylor Simpson ; qemu-devel@nongnu.org > Cc: bc...@quicinc.com; quic_mathb...@quicinc.com; > sidn...@quicinc.com; quic_mlie...@quicinc.com; > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng; a...@rev.ng > Subject: Re: [PATCH] Hexagon (target/hexagon) Remove > HEX_DEBUG/HEX_DEBUG_LOG > > > On 11/4/2024 11:49 AM, Taylor Simpson wrote: > > All Hexagon debugging is now done with QEMU mechanisms (e.g., -d > > in_asm) or with a connected debugger (lldb). > > > > Signed-off-by: Taylor Simpson > > --- > > target/hexagon/cpu.h | 8 +-- > > target/hexagon/helper.h| 5 +- > > target/hexagon/internal.h | 13 + > > target/hexagon/translate.h | 2 - > > target/hexagon/genptr.c| 9 +-- > > target/hexagon/op_helper.c | 112 - > > target/hexagon/translate.c | 66 -- > > target/hexagon/README | 9 --- > > 8 files changed, 4 insertions(+), 220 deletions(-) > > > > diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h index > > 764f3c38cc..bb7d83b53e 100644 > > --- a/target/hexagon/cpu.h > > +++ b/target/hexagon/cpu.h > > @@ -1,5 +1,5 @@ > > /* > > - * Copyright(c) 2019-2023 Qualcomm Innovation Center, Inc. All Rights > Reserved. > > + * Copyright(c) 2019-2024 Qualcomm Innovation Center, Inc. All Rights > Reserved. > >* > > You should not modify the QuIC copyright dates. But you can add your own > copyright to these files for this change, if you like. I'll undo the QuIC copyright changes, but I hardly think removing code warrants adding my own copyright. Thanks, Taylor
RE: [RFC PATCH v1 36/43] target/hexagon: Add temporary vector storage
> -Original Message-
> From: Brian Cain
> Sent: Tuesday, December 3, 2024 1:28 PM
> To: Anton Johansson ; Richard Henderson
>
> Cc: qemu-devel@nongnu.org; a...@rev.ng; ltaylorsimp...@gmail.com;
> bc...@quicinc.com; phi...@linaro.org; alex.ben...@linaro.org
> Subject: Re: [RFC PATCH v1 36/43] target/hexagon: Add temporary vector
> storage
>
>
> On 12/3/2024 12:56 PM, Anton Johansson via wrote:
> > On 22/11/24, Richard Henderson wrote:
> >> On 11/20/24 19:49, Anton Johansson wrote:
> >>> Temporary vectors in helper-to-tcg generated code are allocated from
> >>> an array of bytes in CPUArchState, specified with --temp-vector-block.
> >>>
> >>> This commits adds such a block of memory to CPUArchState, if
> >>> CONFIG_HELPER_TO_TCG is set.
> >>>
> >>> Signed-off-by: Anton Johansson
> >>> ---
> >>>target/hexagon/cpu.h | 4
> >>>1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h index
> >>> 7be4b5769e..fa6ac83e01 100644
> >>> --- a/target/hexagon/cpu.h
> >>> +++ b/target/hexagon/cpu.h
> >>> @@ -97,6 +97,10 @@ typedef struct CPUArchState {
> >>>MMVector future_VRegs[VECTOR_TEMPS_MAX]
> QEMU_ALIGNED(16);
> >>>MMVector tmp_VRegs[VECTOR_TEMPS_MAX] QEMU_ALIGNED(16);
> >>> +#ifdef CONFIG_HELPER_TO_TCG
> >>> +uint8_t tmp_vmem[4096] QEMU_ALIGNED(16); #endif
> >>> +
> >>>MMQReg QRegs[NUM_QREGS] QEMU_ALIGNED(16);
> >>>MMQReg future_QRegs[NUM_QREGS] QEMU_ALIGNED(16);
> >> Wow. Do you really require 4k in temp storage?
> > No, 4k is overkill used during testing. But consider that Hexagon
> > uses
> > 128- and 256-byte vectors in some cases so if the emitted code uses
> > say
> > 5 temporaries in its computation we end up at 1280 bytes as an upper
> > bound.
>
> Per-packet there should be a maximum of one temporary. But per-TB it's
> unbound. Could we/should we have some guidance to put the brakes on
> translation early if we encounter ~N temp references?
>
> But maybe that's not needed since the temp space can be reused within a TB
> among packets.
You should only need enough temporaries for one instruction. There are already
temporaries (future_VRegs, tmp_VRegs, future_QRegs) in CPUHexagonState to
handle the needs within a packet. There shouldn't be any temps needed between
the packets in a TB.
The number of temps needed for a given instruction is determined by the
compiler - version, level of optimization. So, you can determine this by
compiling all the instructions (i.e., build qemu). I'd recommend having a few
extra to future proof against changes to LLVM.
Taylor
RE: [PATCH 33/39] target/hexagon: initialize sys/guest reg TCGvs
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:29 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 33/39] target/hexagon: initialize sys/guest reg TCGvs
>
> From: Brian Cain
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/translate.c | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c index
> ff881d1060..248ed60f29 100644
> --- a/target/hexagon/translate.c
> +++ b/target/hexagon/translate.c
> @@ -1295,6 +1295,26 @@ void hexagon_translate_init(void)
>
> opcode_init();
>
> +#ifndef CONFIG_USER_ONLY
> +for (i = 0; i < NUM_GREGS; i++) {
> +hex_greg[i] = tcg_global_mem_new(tcg_env,
> +offsetof(CPUHexagonState, greg[i]),
> +hexagon_gregnames[i]);
> +}
> +hex_g_sreg_ptr = tcg_global_mem_new_ptr(tcg_env,
> +offsetof(CPUHexagonState, g_sreg), "hex_g_sreg_ptr");
> +for (i = 0; i < NUM_SREGS; i++) {
> +if (i < HEX_SREG_GLB_START) {
> +hex_t_sreg[i] = tcg_global_mem_new(tcg_env,
> +offsetof(CPUHexagonState, t_sreg[i]),
> +hexagon_sregnames[i]);
> +} else {
> +hex_g_sreg[i] = tcg_global_mem_new(hex_g_sreg_ptr,
> +i * sizeof(target_ulong),
> +hexagon_sregnames[i]);
I assume this will be changed with the redo of global resource handling
> +}
> +}
> +#endif
> for (i = 0; i < TOTAL_PER_THREAD_REGS; i++) {
> hex_gpr[i] = tcg_global_mem_new(tcg_env,
> offsetof(CPUHexagonState, gpr[i]),
> --
> 2.34.1
RE: [PATCH 34/39] target/hexagon: Add TLB, k0 {un,}lock
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:29 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 34/39] target/hexagon: Add TLB, k0 {un,}lock
>
> From: Brian Cain
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/sys_macros.h | 8 +--
> target/hexagon/op_helper.c | 104
>
> 2 files changed, 108 insertions(+), 4 deletions(-)
>
> diff --git a/target/hexagon/sys_macros.h b/target/hexagon/sys_macros.h
> index 3c4c3c7aa5..e5dc1ce0ab 100644
> --- a/target/hexagon/sys_macros.h
> +++ b/target/hexagon/sys_macros.h
> @@ -143,11 +143,11 @@
> #define fDCINVIDX(REG)
> #define fDCINVA(REG) do { REG = REG; } while (0) /* Nothing to do in qemu
> */
>
> -#define fSET_TLB_LOCK() g_assert_not_reached()
> -#define fCLEAR_TLB_LOCK() g_assert_not_reached()
> +#define fSET_TLB_LOCK() hex_tlb_lock(env);
> +#define fCLEAR_TLB_LOCK() hex_tlb_unlock(env);
Move these to the patch that implements TLB lock/unlock.
>
> -#define fSET_K0_LOCK()g_assert_not_reached()
> -#define fCLEAR_K0_LOCK() g_assert_not_reached()
> +#define fSET_K0_LOCK()hex_k0_lock(env);
> +#define fCLEAR_K0_LOCK() hex_k0_unlock(env);
>
> #define fTLB_IDXMASK(INDEX) \
> ((INDEX) & (fPOW2_ROUNDUP(fCAST4u(env_archcpu(env)->num_tlbs)) -
> 1)) diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
> index 702c3dd3c6..f3b14fbf58 100644
> --- a/target/hexagon/op_helper.c
> +++ b/target/hexagon/op_helper.c
> @@ -1184,6 +1184,110 @@ void HELPER(modify_ssr)(CPUHexagonState
> *env, uint32_t new, uint32_t old)
> BQL_LOCK_GUARD();
> hexagon_modify_ssr(env, new, old);
> }
> +
> +static void hex_k0_lock(CPUHexagonState *env) {
> +BQL_LOCK_GUARD();
> +g_assert((env->k0_lock_count == 0) || (env->k0_lock_count == 1));
> +
> +uint32_t syscfg = arch_get_system_reg(env, HEX_SREG_SYSCFG);
> +if (GET_SYSCFG_FIELD(SYSCFG_K0LOCK, syscfg)) {
> +if (env->k0_lock_state == HEX_LOCK_QUEUED) {
> +env->next_PC += 4;
> +env->k0_lock_count++;
> +env->k0_lock_state = HEX_LOCK_OWNER;
> +SET_SYSCFG_FIELD(env, SYSCFG_K0LOCK, 1);
> +return;
> +}
> +if (env->k0_lock_state == HEX_LOCK_OWNER) {
> +qemu_log_mask(LOG_GUEST_ERROR,
> + "Double k0lock at PC: 0x%x, thread may hang\n",
> + env->next_PC);
> +env->next_PC += 4;
> +CPUState *cs = env_cpu(env);
QEMU coding standards prefer to put declarations at the beginning of the code
block (just after the open curly brace).
> +cpu_interrupt(cs, CPU_INTERRUPT_HALT);
> +return;
> +}
> +env->k0_lock_state = HEX_LOCK_WAITING;
> +CPUState *cs = env_cpu(env);
Ditto
> +cpu_interrupt(cs, CPU_INTERRUPT_HALT);
> +} else {
> +env->next_PC += 4;
> +env->k0_lock_count++;
> +env->k0_lock_state = HEX_LOCK_OWNER;
> +SET_SYSCFG_FIELD(env, SYSCFG_K0LOCK, 1);
> +}
> +
> +}
> +
> +static void hex_k0_unlock(CPUHexagonState *env) {
> +BQL_LOCK_GUARD();
> +g_assert((env->k0_lock_count == 0) || (env->k0_lock_count == 1));
> +
> +/* Nothing to do if the k0 isn't locked by this thread */
> +uint32_t syscfg = arch_get_system_reg(env, HEX_SREG_SYSCFG);
> +if ((GET_SYSCFG_FIELD(SYSCFG_K0LOCK, syscfg) == 0) ||
> +(env->k0_lock_state != HEX_LOCK_OWNER)) {
> +qemu_log_mask(LOG_GUEST_ERROR,
> + "thread %d attempted to unlock k0 without having the "
> + "lock, k0_lock state = %d, syscfg:k0 = %d\n",
> + env->threadId, env->k0_lock_state,
> + GET_SYSCFG_FIELD(SYSCFG_K0LOCK, syscfg));
> +g_assert(env->k0_lock_state != HEX_LOCK_WAITING);
> +return;
> +}
> +
> +env->k0_lock_count--;
> +env->k0_lock_state = HEX_LOCK_UNLOCKED;
> +SET_SYSCFG_FIELD(env, SYSCFG_K0LOCK, 0);
> +
> +/* Look for a thread to unlock */
> +unsigned int this_threadId = env->threadId;
> +CPUHexagonState *unlock_thread = NULL;
> +CPUState *cs;
Ditto
Otherwise
Reviewed-by: Taylor Simpson
RE: [PATCH 35/39] target/hexagon: Define gen_precise_exception()
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:29 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 35/39] target/hexagon: Define gen_precise_exception() The definition of gen_precise_exception is not in this patch. There is a use but no definition. > > From: Brian Cain > > Add PC to raise_exception helper > > Replace the fGEN_TCG_J2_trap0 macro override with the fTRAP()-generated > system helper instead. > > Signed-off-by: Brian Cain
RE: [PATCH 02/39] target/hexagon: Implement {c,}swi helpers
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:28 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 02/39] target/hexagon: Implement {c,}swi helpers
>
> From: Brian Cain
>
> {c,}swi are the "software interrupt"/"Cancel pending interrupts" instructions.
>
> Signed-off-by: Brian Cain
Reviewed-by: Taylor Simpson
RE: [PATCH 21/39] target/hexagon: Implement hexagon_resume_threads()
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:28 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 21/39] target/hexagon: Implement > hexagon_resume_threads() > > From: Brian Cain > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH 18/39] target/hexagon: Implement exec_interrupt, set_irq
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:28 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 18/39] target/hexagon: Implement exec_interrupt, set_irq > > From: Brian Cain > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH 23/39] target/hexagon: Add sysemu_ops, cpu_get_phys_page_debug()
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:28 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 23/39] target/hexagon: Add sysemu_ops, > cpu_get_phys_page_debug() > > From: Brian Cain > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH 3/8] hw/hexagon: Add v68, sa8775-cdsp0 defs
> -Original Message- > From: Brian Cain > Sent: Saturday, March 1, 2025 11:21 AM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 3/8] hw/hexagon: Add v68, sa8775-cdsp0 defs > > From: Brian Cain > > Signed-off-by: Brian Cain Acked-by: Taylor Simpson
RE: [PATCH 5/8] hw/hexagon: Modify "Standalone" symbols
> -Original Message- > From: Brian Cain > Sent: Saturday, March 1, 2025 11:21 AM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 5/8] hw/hexagon: Modify "Standalone" symbols > > From: Brian Cain > > These symbols are used by Hexagon Standalone OS to indicate whether the > program should halt and wait for interrupts at startup. For QEMU, we want > these programs to just continue crt0 startup through to the user program's > main(). > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH 4/8] hw/hexagon: Add support for cfgbase
> -Original Message- > From: Brian Cain > Sent: Saturday, March 1, 2025 11:21 AM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com > Subject: [PATCH 4/8] hw/hexagon: Add support for cfgbase > > From: Sid Manning > > Signed-off-by: Sid Manning Reviewed-by: Taylor Simpson
RE: [PATCH 2/2] target/hexagon: Drop `ident` postprocess step
> -Original Message-
> From: Anton Johansson
> Sent: Wednesday, March 12, 2025 2:46 PM
> To: qemu-devel@nongnu.org
> Cc: a...@rev.ng; ltaylorsimp...@gmail.com; brian.c...@oss.qualcomm.com;
> phi...@linaro.org
> Subject: [PATCH 2/2] target/hexagon: Drop `ident` postprocess step
>
> The indent command is not available on a default mac osx setup with xcode
> cli tools installed. While it does make idef-parser generated code nicer
to
> debug, it's not crucial and can be dropped.
>
> Signed-off-by: Anton Johansson
> ---
> target/hexagon/meson.build | 21 ++---
> 1 file changed, 2 insertions(+), 19 deletions(-)
>
> diff --git a/target/hexagon/meson.build b/target/hexagon/meson.build
> index abcf00ca1f..246dc7b241 100644
> --- a/target/hexagon/meson.build
> +++ b/target/hexagon/meson.build
> @@ -323,30 +323,13 @@ if idef_parser_enabled and 'hexagon-linux-user' in
> target_dirs
> command: [idef_parser, '@INPUT@', '@OUTPUT0@', '@OUTPUT1@',
> '@OUTPUT2@']
> )
>
> -indent = find_program('indent', required: false)
> -if indent.found()
> -idef_generated_tcg_c = custom_target(
> -'indent',
> -input: idef_generated_tcg[0],
> -output: 'idef-generated-emitter.indented.c',
> -command: [indent, '-linux', '@INPUT@', '-o', '@OUTPUT@']
> -)
> -else
> -idef_generated_tcg_c = custom_target(
> -'copy',
> -input: idef_generated_tcg[0],
> -output: 'idef-generated-emitter.indented.c',
> -command: ['cp', '@INPUT@', '@OUTPUT@']
> -)
> -endif
> -
I prefer to have the indented version present.
Is the above check/fallback not sufficient on MacOS? It works on a Linux
system where indent is not present.
Thanks,
Taylor
RE: [PATCH 1/8] hw/intc: Add l2vic interrupt controller
> -Original Message-
> From: Brian Cain
> Sent: Saturday, March 1, 2025 11:21 AM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Damien Hedde ; Paolo
> Bonzini
> Subject: [PATCH 1/8] hw/intc: Add l2vic interrupt controller
>
> From: Sid Manning
>
> Co-authored-by: Matheus Tavares Bernardino
>
> Co-authored-by: Damien Hedde
> Signed-off-by: Brian Cain
> ---
> MAINTAINERS| 2 +
> docs/devel/hexagon-l2vic.rst | 59 +
> docs/devel/index-internals.rst | 1 +
> include/hw/intc/l2vic.h| 37 +++
> hw/intc/l2vic.c| 417 +
> hw/intc/Kconfig| 3 +
> hw/intc/meson.build| 2 +
> hw/intc/trace-events | 4 +
> 8 files changed, 525 insertions(+)
> create mode 100644 docs/devel/hexagon-l2vic.rst create mode 100644
> include/hw/intc/l2vic.h create mode 100644 hw/intc/l2vic.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 804c07bcd5..a842f7fe1b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -232,6 +232,7 @@ Hexagon TCG CPUs
> M: Brian Cain
> S: Supported
> F: target/hexagon/
> +F: hw/intc/l2vic.[ch]
Consider naming all the files outside target/hexagon as hex_* or hexagon_*
That will make it clear they belong to hexagon and you can use an easy wild
card in the MAINTAINERS file.
Ditto for the docs files.
> X: target/hexagon/idef-parser/
> X: target/hexagon/gen_idef_parser_funcs.py
> F: linux-user/hexagon/
> diff --git a/include/hw/intc/l2vic.h b/include/hw/intc/l2vic.h new file mode
> 100644 index 00..ed8ccf33b1
> --- /dev/null
> +++ b/include/hw/intc/l2vic.h
> @@ -0,0 +1,37 @@
> +/*
> + * QEMU L2VIC Interrupt Controller
> + *
> + * Copyright(c) 2020-2025 Qualcomm Innovation Center, Inc. All Rights
> Reserved.
> + * SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#define L2VIC_VID_GRP_0 0x0 /* Read */
> +#define L2VIC_VID_GRP_1 0x4 /* Read */
> +#define L2VIC_VID_GRP_2 0x8 /* Read */
> +#define L2VIC_VID_GRP_3 0xC /* Read */
> +#define L2VIC_VID_0 0x10 /* Read SOFTWARE DEFINED */ #define
> +L2VIC_VID_1 0x14 /* Read SOFTWARE DEFINED NOT YET USED */ #define
> +L2VIC_INT_ENABLEn 0x100 /* Read/Write */ #define
> +L2VIC_INT_ENABLE_CLEARn 0x180 /* Write */ #define
> L2VIC_INT_ENABLE_SETn
> +0x200 /* Write */ #define L2VIC_INT_TYPEn 0x280 /* Read/Write */
> +#define L2VIC_INT_STATUSn 0x380 /* Read */ #define L2VIC_INT_CLEARn
> +0x400 /* Write */ #define L2VIC_SOFT_INTn 0x480 /* Write */ #define
> +L2VIC_INT_PENDINGn 0x500 /* Read */ #define L2VIC_INT_GRPn_0 0x600
> /*
> +Read/Write */ #define L2VIC_INT_GRPn_1 0x680 /* Read/Write */ #define
> +L2VIC_INT_GRPn_2 0x700 /* Read/Write */ #define L2VIC_INT_GRPn_3
> 0x780
> +/* Read/Write */
> +
> +#define L2VIC_INTERRUPT_MAX 1024
> +#define L2VIC_CIAD_INSTRUCTION -1
> +/*
> + * Note about l2vic groups:
> + * Each interrupt to L2VIC can be configured to associate with one of
> + * four groups.
> + * Group 0 interrupts go to IRQ2 via VID 0 (SSR: 0xC2, the default)
> + * Group 1 interrupts go to IRQ3 via VID 1 (SSR: 0xC3)
> + * Group 2 interrupts go to IRQ4 via VID 2 (SSR: 0xC4)
> + * Group 3 interrupts go to IRQ5 via VID 3 (SSR: 0xC5) */
> diff --git a/hw/intc/l2vic.c b/hw/intc/l2vic.c new file mode 100644 index
> 00..9df6575214
> --- /dev/null
> +++ b/hw/intc/l2vic.c
> @@ -0,0 +1,417 @@
> +/*
> + * QEMU L2VIC Interrupt Controller
> + *
> + * Arm PrimeCell PL190 Vector Interrupt Controller was used as a reference.
> + * Copyright(c) 2020-2025 Qualcomm Innovation Center, Inc. All Rights
> Reserved.
> + * SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include "qemu/osdep.h"
> +#include "hw/irq.h"
> +#include "hw/sysbus.h"
> +#include "migration/vmstate.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "hw/intc/l2vic.h"
> +#include "trace.h"
> +
> +#define L2VICA(s, n) (s[(n) >> 2])
> +
> +#define TYPE_L2VIC "l2vic"
> +#define L2VIC(obj) OBJECT_CHECK(L2VICState, (obj), TYPE_L2VIC)
> +
> +#define SLICE_MAX (L2VIC_INTERRUPT_MAX / 32)
> +
> +typedef struct L2VICState {
> +SysBusDevice parent_obj;
> +
> +QemuMutex active;
> +MemoryRegion iomem;
> +MemoryRegion fast_iomem;
> +uint32_t level;
> +/*
> + * offset 0:vid group 0 etc, 10 bits in each group
> + * are used:
> + */
> +uint32_t vid_group[4];
> +uint32_t vid0;
> +/* Clear Status of Active Edge interrupt, not used: */
> +uint32_t int_clear[SLICE_MAX] QEMU_ALIGNED(16);
> +/* Enable interrupt source */
> +uint32_t int_enable[SLICE_MAX] QEMU_ALIGNED(16);
> +/* Clear (set to 0) corresponding bit in int_enable */
> +uint32_t int_enable_clear;
> +/* Set (to 1) corresponding bit in int_enable */
> +uint32_t int_enabl
RE: [PATCH 16/39] target/hexagon: Implement hex_tlb_lookup_by_asid()
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:28 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 16/39] target/hexagon: Implement > hex_tlb_lookup_by_asid() > > From: Brian Cain > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH 31/39] target/hexagon: Add implicit sysreg writes
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:29 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 31/39] target/hexagon: Add implicit sysreg writes > > From: Brian Cain > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH 17/39] target/hexagon: Implement software interrupt
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:28 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain ; Mike Lambert
>
> Subject: [PATCH 17/39] target/hexagon: Implement software interrupt
>
> From: Brian Cain
>
> Co-authored-by: Mike Lambert
> Signed-off-by: Brian Cain
> ---
> target/hexagon/cpu.h | 1 -
> target/hexagon/hexswi.h| 17 +++
> target/hexagon/cpu.c | 2 +
> target/hexagon/hexswi.c| 258
> +
> target/hexagon/op_helper.c | 1 +
> 5 files changed, 278 insertions(+), 1 deletion(-) create mode 100644
> target/hexagon/hexswi.h create mode 100644 target/hexagon/hexswi.c
>
> diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h index
> dabee310c5..045581d7be 100644
> --- a/target/hexagon/cpu.h
> +++ b/target/hexagon/cpu.h
> @@ -256,5 +256,4 @@ typedef HexagonCPU ArchCPU; void
> hexagon_translate_init(void); void hexagon_translate_code(CPUState *cs,
> TranslationBlock *tb,
> int *max_insns, vaddr pc, void *host_pc);
> -
Gratuitous change
> #endif /* HEXAGON_CPU_H */
> diff --git a/target/hexagon/hexswi.h b/target/hexagon/hexswi.h new file
> mode 100644 index 00..5d232cb06c
> --- /dev/null
> +++ b/target/hexagon/hexswi.h
> @@ -0,0 +1,17 @@
> +/*
> + * Copyright(c) 2025 Qualcomm Innovation Center, Inc. All Rights Reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef HEXSWI_H
> +#define HEXSWI_H
> +
> +
> +#include "cpu.h"
> +
> +void hexagon_cpu_do_interrupt(CPUState *cpu); void
> +register_trap_exception(CPUHexagonState *env, int type, int imm,
> + target_ulong PC);
> +
> +#endif /* HEXSWI_H */
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index
> 89a051b41d..843be8221f 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -33,6 +33,8 @@
> #ifndef CONFIG_USER_ONLY
> #include "sys_macros.h"
> #include "qemu/main-loop.h"
> +#include "hex_interrupts.h"
> +#include "hexswi.h"
Move these added include to a different patch where the contents are needed.
> #endif
>
> static void hexagon_v66_cpu_init(Object *obj) { } diff --git
> a/target/hexagon/hexswi.c b/target/hexagon/hexswi.c new file mode
> 100644 index 00..5fcf9b2be9
> --- /dev/null
> +++ b/target/hexagon/hexswi.c
> @@ -0,0 +1,258 @@
> +/*
> + * Copyright(c) 2019-2025 Qualcomm Innovation Center, Inc. All Rights
> Reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#ifdef CONFIG_USER_ONLY
This file is only included in the system-mode build, so we don't need these
guards. Several in this file.
> +#include "exec/helper-proto.h"
> +#include "qemu.h"
> +#endif
> +#include "exec/cpu_ldst.h"
> +#include "exec/exec-all.h"
> +#include "qemu/log.h"
> +#include "qemu/main-loop.h"
> +#include "arch.h"
> +#include "internal.h"
> +#include "macros.h"
> +#include "sys_macros.h"
> +#include "tcg/tcg-op.h"
> +#ifndef CONFIG_USER_ONLY
> +#include "hex_mmu.h"
> +#include "hexswi.h"
> +#endif
> +
> +#ifndef CONFIG_USER_ONLY
> +
> +
> +static void set_addresses(CPUHexagonState *env, target_ulong pc_offset,
> + target_ulong exception_index)
> +
> +{
> +arch_set_system_reg(env, HEX_SREG_ELR,
> +arch_get_thread_reg(env, HEX_REG_PC) + pc_offset);
> +arch_set_thread_reg(env, HEX_REG_PC,
> +arch_get_system_reg(env, HEX_SREG_EVB) |
> +(exception_index << 2)); }
> +
> +static const char *event_name[] = {
> +[HEX_EVENT_RESET] = "HEX_EVENT_RESET",
> +[HEX_EVENT_IMPRECISE] = "HEX_EVENT_IMPRECISE",
> +[HEX_EVENT_TLB_MISS_X] = "HEX_EVENT_TLB_MISS_X",
> +[HEX_EVENT_TLB_MISS_RW] = "HEX_EVENT_TLB_MISS_RW",
> +[HEX_EVENT_TRAP0] = "HEX_EVENT_TRAP0",
> +[HEX_EVENT_TRAP1] = "HEX_EVENT_TRAP1",
> +[HEX_EVENT_FPTRAP] = "HEX_EVENT_FPTRAP",
> +[HEX_EVENT_DEBUG] = "HEX_EVENT_DEBUG",
> +[HEX_EVENT_INT0] = "HEX_EVENT_INT0",
> +[HEX_EVENT_INT1] = "HEX_EVENT_INT1",
> +[HEX_EVENT_INT2] = "HEX_EVENT_INT2",
> +[HEX_EVENT_INT3] = "HEX_EVENT_INT3",
> +[HEX_EVENT_INT4] = "HEX_EVENT_INT4",
> +[HEX_EVENT_INT5] = "HEX_EVENT_INT5",
> +[HEX_EVENT_INT6] = "HEX_EVENT_INT6",
> +[HEX_EVENT_INT7] = "HEX_EVENT_INT7",
> +[HEX_EVENT_INT8] = "HEX_EVENT_INT8",
> +[HEX_EVENT_INT9] = "HEX_EVENT_INT9",
> +[HEX_EVENT_INTA] = "HEX_EVENT_INTA",
> +[HEX_EVENT_INTB] = "HEX_EVENT_INTB",
> +[HEX_EVENT_INTC] = "HEX_EVENT_INTC",
> +[HEX_EVENT_INTD] = "HEX_EVENT_INTD",
> +[HEX_EVENT_INTE] = "HEX_EVENT_INTE",
> +[HEX_EVENT_INTF] = "HEX_EVENT_INTF"
> +};
RE: [PATCH 05/39] target/hexagon: Implement modify SSR
> -Original Message-
> From: Brian Cain
> Sent: Tuesday, March 18, 2025 6:47 PM
> To: ltaylorsimp...@gmail.com; 'Sid Manning' ;
> qemu-devel@nongnu.org
> Cc: richard.hender...@linaro.org; phi...@linaro.org; 'Matheus Bernardino
> (QUIC)' ; a...@rev.ng; a...@rev.ng; 'Marco
> Liebel (QUIC)' ; alex.ben...@linaro.org; 'Mark
> Burton (QUIC)' ; 'Brian Cain'
>
> Subject: Re: [PATCH 05/39] target/hexagon: Implement modify SSR
>
>
> On 3/18/2025 2:14 PM, ltaylorsimp...@gmail.com wrote:
> >
> >> -Original Message-
> >> From: Sid Manning
> >> Sent: Tuesday, March 18, 2025 1:34 PM
> >> To: ltaylorsimp...@gmail.com; 'Brian Cain'
> >> ; qemu-devel@nongnu.org
> >> Cc: richard.hender...@linaro.org; phi...@linaro.org; Matheus
> >> Bernardino
> >> (QUIC) ; a...@rev.ng; a...@rev.ng; Marco
> >> Liebel (QUIC) ; alex.ben...@linaro.org;
> >> Mark Burton (QUIC) ; Brian Cain
> >>
> >> Subject: RE: [PATCH 05/39] target/hexagon: Implement modify SSR
> >>
> >>
> >>
> >>> -Original Message-
> >>> From: ltaylorsimp...@gmail.com
> >>> Sent: Monday, March 17, 2025 12:37 PM
> >>> To: 'Brian Cain' ; qemu-
> >> de...@nongnu.org
> >>> Cc: richard.hender...@linaro.org; phi...@linaro.org; Matheus
> >>> Bernardino
> >>> (QUIC) ; a...@rev.ng; a...@rev.ng;
> Marco
> >>> Liebel (QUIC) ; alex.ben...@linaro.org;
> >>> Mark Burton (QUIC) ; Sid Manning
> >>> ; Brian Cain
> >>> Subject: RE: [PATCH 05/39] target/hexagon: Implement modify SSR
> >>>
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:28 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng;
> >>> a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> >>> sidn...@quicinc.com;
> Brian Cain
> Subject: [PATCH 05/39] target/hexagon: Implement modify SSR
>
> From: Brian Cain
>
> The per-vCPU System Status Register controls many modal behaviors
> of the system architecture. When the SSR is updated, we trigger
> the necessary effects for interrupts, privilege/MMU, and HVX
> context
> >>> mapping.
> Signed-off-by: Brian Cain
> ---
> >>> What does SSR_XE indicate?
> >> [Sid Manning]
> >> If SSR:XE isn't set this thread doesn't have the coproc enabled so
> >> discard additional checking. Any coproc insn issued when ssr:xe is 0
> >> will trigger a, "Illegal execution of coprocessor instruction." error.
> >
> >
> +if (old_XA != new_XA) {
> +int old_unit = parse_context_idx(env, old_XA);
> +int new_unit = parse_context_idx(env, new_XA);
> >>> Check that old_unit != new_unit. Per the table above, different XA
> >>> values can point to the same unit. For example, if
> >>> cpu->hvx_contexts is 2, the
> >> XA=0
> >>> and XA=2 both point to context 0.
> >>>
> +
> +/* Ownership exchange */
> +memcpy(VRegs[old_unit], env->VRegs, sizeof(env->VRegs));
> +memcpy(QRegs[old_unit], env->QRegs, sizeof(env->QRegs));
> +memcpy(env->VRegs, VRegs[new_unit], sizeof(env->VRegs));
> +memcpy(env->QRegs, QRegs[new_unit], sizeof(env->QRegs));
> >>> What does the hardware do? Does it clear the context, or is that
> >>> the OS'es job?
> >> Nothing would keep a single htid from taking hvx unit 0, doing some hvx-
> ops
> >> , swapping to hvx unit 1 doing some more hvx-ops and so on. We are
> doing
> >> this because each thread has a private copy of the hvx register
> >> state. Since HVX units and threads are independent if one thread
> >> changes its ownership from HVX context 0 to HVX context 1 we have to
> >> do this copy. Instead of memcpy should create a new object that
> >> represents the HVX units available and change env->VRegs/QRegs to
> point to the one currently owned.
> >>
> >> Refactoring this will be an improvement.
> >>
> >>
> >>> If the hardware leaves the context alone, the above should be
> >>> 1) Copy env->{VQ}Regs to old_unit
> >>> 2) Copy new_unit to env->{VQ}Regs
> >>>
> >>> Should you check SSR_EX before doing these copies?
> >>>
> >>> Do you need to do anything when SSR_EX changes?
> >> I think you mean SSR:XE before doing the copies. I think we have to
> >> do the copy here regardless of ssr:xe since a thread could swap
> >> ownership, update ssr:xa, without explicitly having ssr:xe set.
> >> Maybe the OS disables SSR:XE while changing hvx unit ownership?
> > Correct. I meant SSR:XE.
> >
> > Some refactoring is in order but need to understand the HW behavior more
> specifically.
> > - What will the HW do if more than one thread claims ownership of the
> same HVX context?
> > - What happens if a thread claims ownership without setting SSR:XE and
> then sets SSR:XE later?
> > - What about this example?
> > 1) Thread 0 claims context 1 and sets SSR:XE
> > 2)
RE: [PATCH 32/39] target/hexagon: Define system, guest reg names
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:29 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 32/39] target/hexagon: Define system, guest reg names
>
> From: Brian Cain
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/internal.h | 2 ++
> target/hexagon/cpu.c | 29 +
> 2 files changed, 31 insertions(+)
>
> diff --git a/target/hexagon/internal.h b/target/hexagon/internal.h index
> 120cfde7b9..fd2397b9ef 100644
> --- a/target/hexagon/internal.h
> +++ b/target/hexagon/internal.h
> @@ -34,6 +34,8 @@ void hexagon_debug_qreg(CPUHexagonState *env, int
> regnum); void hexagon_debug(CPUHexagonState *env);
>
> extern const char * const hexagon_regnames[TOTAL_PER_THREAD_REGS];
> +extern const char * const hexagon_sregnames[]; extern const char *
> +const hexagon_gregnames[];
Guard these with #ifndef CONFIG_USER_ONLY
>
> void G_NORETURN do_raise_exception(CPUHexagonState *env,
> uint32_t exception,
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index
> c7c470b099..3c4776232e 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -85,6 +85,35 @@ const char * const
> hexagon_regnames[TOTAL_PER_THREAD_REGS] = {
>"c24", "c25", "c26", "c27", "c28", "c29", "c30", "c31", };
>
> +#ifndef CONFIG_USER_ONLY
> +const char * const hexagon_sregnames[] = {
> +"sgp0", "sgp1", "stid", "elr","badva0",
> +"badva1", "ssr","ccr","htid", "badva",
> +"imask", "gevb", "vwctrl", "s13","s14",
> +"s15","evb","modectl","syscfg", "segment",
> +"ipendad","vid","vid1", "bestwait", "s24",
> +"schedcfg", "s26","cfgbase","diag", "rev",
> +"pcyclelo", "pcyclehi", "isdbst", "isdbcfg0", "isdbcfg1",
> +"livelock", "brkptpc0", "brkptcfg0", "brkptpc1", "brkptcfg1",
> +"isdbmbxin", "isdbmbxout", "isdben", "isdbgpr","pmucnt4",
> +"pmucnt5","pmucnt6","pmucnt7","pmucnt0","pmucnt1",
> +"pmucnt2","pmucnt3","pmuevtcfg", "pmustid0", "pmuevtcfg1",
> +"pmustid1", "timerlo","timerhi","pmucfg", "rgdr2",
> +"rgdr", "turkey", "duck", "chicken",
The last 5 names look strange and don't match what's in hex_regs.h.
Otherwise
Reviewed-by: Taylor Simpson
RE: [PATCH 12/39] target/hexagon: Add implementation of cycle counters
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:28 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 12/39] target/hexagon: Add implementation of cycle
> counters
>
> From: Brian Cain
>
> Co-authored-by: Sid Manning
> Signed-off-by: Brian Cain
> ---
> target/hexagon/cpu.h| 25 ++---
> target/hexagon/translate.h | 2 ++
> target/hexagon/cpu_helper.c | 12 +---
> target/hexagon/translate.c | 27 +++
> 4 files changed, 60 insertions(+), 6 deletions(-)
>
> diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h index
> 4b9c9873dc..7e2ea838c5 100644
> --- a/target/hexagon/cpu.h
> +++ b/target/hexagon/cpu.h
> @@ -27,11 +27,15 @@
>
> #include "cpu-qom.h"
> #include "exec/cpu-defs.h"
> +#include "exec/cpu-common.h"
> #include "hex_regs.h"
> #include "mmvec/mmvec.h"
> #include "hw/registerfields.h"
>
> +#ifndef CONFIG_USER_ONLY
> +#include "reg_fields.h"
> typedef struct CPUHexagonTLBContext CPUHexagonTLBContext;
> +#endif
Why is reg_fields.h guarded by #ifndef CONFIG_USER_ONLY?
Also, why wasn't the CPUHexagonTLBContext guarded when it was first inserted?
>
> #define NUM_PREGS 4
> #define TOTAL_PER_THREAD_REGS 64
> @@ -188,6 +192,7 @@ struct ArchCPU {
>
> FIELD(TB_FLAGS, IS_TIGHT_LOOP, 0, 1)
> FIELD(TB_FLAGS, MMU_INDEX, 1, 3)
> +FIELD(TB_FLAGS, PCYCLE_ENABLED, 4, 1)
>
> G_NORETURN void hexagon_raise_exception_err(CPUHexagonState *env,
> uint32_t exception, @@ -201,6
> +206,11 @@ void
> hexagon_cpu_soft_reset(CPUHexagonState *env); #endif
>
> #include "exec/cpu-all.h"
> +
> +#ifndef CONFIG_USER_ONLY
> +#include "cpu_helper.h"
> +#endif
> +
> static inline void cpu_get_tb_cpu_state(CPUHexagonState *env, vaddr *pc,
> uint64_t *cs_base, uint32_t *flags)
> { @@ -210,16
> +220,27 @@ static inline void cpu_get_tb_cpu_state(CPUHexagonState
> *env, vaddr *pc,
> if (*pc == env->gpr[HEX_REG_SA0]) {
> hex_flags = FIELD_DP32(hex_flags, TB_FLAGS, IS_TIGHT_LOOP, 1);
> }
> -*flags = hex_flags;
> if (*pc & PCALIGN_MASK) {
> hexagon_raise_exception_err(env, HEX_CAUSE_PC_NOT_ALIGNED, 0);
> }
> #ifndef CONFIG_USER_ONLY
> +target_ulong syscfg = arch_get_system_reg(env, HEX_SREG_SYSCFG);
> +
> +bool pcycle_enabled = extract32(syscfg,
> +reg_field_info[SYSCFG_PCYCLEEN].offset,
> +
> + reg_field_info[SYSCFG_PCYCLEEN].width);
> +
> hex_flags = FIELD_DP32(hex_flags, TB_FLAGS, MMU_INDEX,
> cpu_mmu_index(env_cpu(env), false));
> +
> +if (pcycle_enabled) {
> +hex_flags = FIELD_DP32(hex_flags, TB_FLAGS, PCYCLE_ENABLED, 1);
> +}
> #else
> +hex_flags = FIELD_DP32(hex_flags, TB_FLAGS, PCYCLE_ENABLED, true);
Are pcycles exposed in linux-user mode? If not, make this flag system-mode
only.
> hex_flags = FIELD_DP32(hex_flags, TB_FLAGS, MMU_INDEX,
> MMU_USER_IDX); #endif
> +*flags = hex_flags;
> }
>
> typedef HexagonCPU ArchCPU;
> @@ -228,6 +249,4 @@ void hexagon_translate_init(void); void
> hexagon_translate_code(CPUState *cs, TranslationBlock *tb,
> int *max_insns, vaddr pc, void *host_pc);
>
> -#include "exec/cpu-all.h"
> -
> #endif /* HEXAGON_CPU_H */
> diff --git a/target/hexagon/translate.h b/target/hexagon/translate.h index
> 0eaa3db03e..9bc4b3ce8b 100644
> --- a/target/hexagon/translate.h
> +++ b/target/hexagon/translate.h
> @@ -83,6 +83,8 @@ typedef struct DisasContext {
> TCGv new_pred_value[NUM_PREGS];
> TCGv branch_taken;
> TCGv dczero_addr;
> +bool pcycle_enabled;
Guard with #ifndef CONFIG_USER_ONLY
> +uint32_t num_cycles;
> } DisasContext;
>
> bool is_gather_store_insn(DisasContext *ctx); diff --git
> a/target/hexagon/cpu_helper.c b/target/hexagon/cpu_helper.c index
> 0b0802bfb9..1d9b9f8bef 100644
> --- a/target/hexagon/cpu_helper.c
> +++ b/target/hexagon/cpu_helper.c
> @@ -48,17 +48,23 @@ uint32_t arch_get_system_reg(CPUHexagonState
> *env, uint32_t reg)
>
> uint64_t hexagon_get_sys_pcycle_count(CPUHexagonState *env) {
> -g_assert_not_reached();
Do we need a lock here?
> +uint64_t cycles = 0;
> +CPUState *cs;
> +CPU_FOREACH(cs) {
> +CPUHexagonState *env_ = cpu_env(cs);
> +cycles += env_->t_cycle_count;
> +}
> +return *(env->g_pcycle_base) + cycles;
> }
>
> uint32_t hexagon_get_sys_pcycle_count_high(CPUHexagonState *env) {
> -g_assert_not_reached();
> +return hexagon_get_sys_pcycle_count(env) >> 32;
> }
>
> uint32_t hexagon_get_sys_pcycle_count_low(CPUHexagonState *env) {
RE: [PATCH 13/39] target/hexagon: Implement modify_syscfg()
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:28 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 13/39] target/hexagon: Implement modify_syscfg()
>
> From: Brian Cain
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/op_helper.c | 51
> +-
> 1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
> index 03bed11f6e..42805d0f1d 100644
> --- a/target/hexagon/op_helper.c
> +++ b/target/hexagon/op_helper.c
> @@ -1522,7 +1522,56 @@ static bool
> handle_pmu_sreg_write(CPUHexagonState *env, uint32_t reg,
>
> static void modify_syscfg(CPUHexagonState *env, uint32_t val) {
> -g_assert_not_reached();
> +g_assert(bql_locked());
> +
> +uint32_t old;
> +uint32_t syscfg_read_only_mask = 0x80001c00;
> +uint32_t syscfg = arch_get_system_reg(env, HEX_SREG_SYSCFG);
> +
> +/* clear read-only bits if they are set in the new value. */
> +val &= ~syscfg_read_only_mask;
> +/* if read-only are currently set in syscfg keep them set. */
> +val |= (syscfg & syscfg_read_only_mask);
> +
> +uint32_t tmp = val;
> +old = arch_get_system_reg(env, HEX_SREG_SYSCFG);
This is the same as syscfg declared above
> +arch_set_system_reg(env, HEX_SREG_SYSCFG, tmp);
Why is tmp needed? Just use val here.
> +
> +/* Check for change in MMU enable */
> +target_ulong old_mmu_enable = GET_SYSCFG_FIELD(SYSCFG_MMUEN,
> old);
> +uint8_t old_en = GET_SYSCFG_FIELD(SYSCFG_PCYCLEEN, old);
> +uint8_t old_gie = GET_SYSCFG_FIELD(SYSCFG_GIE, old);
> +target_ulong new_mmu_enable =
> +GET_SYSCFG_FIELD(SYSCFG_MMUEN, val);
Move these declarations to the beginning of the function.
> +if (new_mmu_enable && !old_mmu_enable) {
> +hex_mmu_on(env);
> +} else if (!new_mmu_enable && old_mmu_enable) {
> +hex_mmu_off(env);
> +}
> +
> +/* Changing pcycle enable from 0 to 1 resets the counters */
> +uint8_t new_en = GET_SYSCFG_FIELD(SYSCFG_PCYCLEEN, val);
> +CPUState *cs;
Move the declarations to the beginning of the function
> +if (old_en == 0 && new_en == 1) {
You could put declaration of cs here if you prefer
> +CPU_FOREACH(cs) {
> +CPUHexagonState *_env = cpu_env(cs);
> +_env->t_cycle_count = 0;
I'm not a fan of _env as a variable name. Just do
cpu_env(cs)->t_cycle_count = 0
> +}
> +}
> +
> +/* See if global interrupts are turned on */
> +uint8_t new_gie = GET_SYSCFG_FIELD(SYSCFG_GIE, val);
Move the declaration to the beginning
> +if (!old_gie && new_gie) {
> +qemu_log_mask(CPU_LOG_INT, "%s: global interrupts enabled\n",
> __func__);
> +hex_interrupt_update(env);
> +}
> +
> +if (qemu_loglevel_mask(LOG_UNIMP)) {
> +int new_v2x = GET_SYSCFG_FIELD(SYSCFG_V2X, val);
> +if (!new_v2x) {
> +qemu_log("HVX: 64 byte vector length is unsupported\n");
> +}
> +}
> }
>
> static uint32_t hexagon_find_last_irq(CPUHexagonState *env, uint32_t vid)
> --
> 2.34.1
RE: [PATCH 30/39] target/hexagon: Add next_PC, {s,g}reg writes
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:29 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 30/39] target/hexagon: Add next_PC, {s,g}reg writes
>
> From: Brian Cain
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/cpu.h | 2 +-
> target/hexagon/translate.h | 2 +
> target/hexagon/genptr.c| 7 +-
> target/hexagon/translate.c | 142 -
>
> 4 files changed, 132 insertions(+), 21 deletions(-)
>
> diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h index
> 4667a1f748..73c3bb34b0 100644
> --- a/target/hexagon/cpu.h
> +++ b/target/hexagon/cpu.h
> @@ -142,9 +142,9 @@ typedef struct CPUArchState {
> hex_lock_state_t k0_lock_state;
> target_ulong tlb_lock_count;
> target_ulong k0_lock_count;
> -target_ulong next_PC;
> CPUHexagonTLBContext *hex_tlb;
> #endif
> +target_ulong next_PC;
You are moving this from system-mode only to unconditional. There must be an
earlier patch in this series that put it in system-mode. Find that and remove
it, so there is only a single addition.
Also, does this need to be part of the global state? The answer is no if it
doesn't live across packets. If it is only used within the context of a single
packet, you can just create a temporary TCGv in DisasContext. There are
several examples already.
> target_ulong new_value_usr;
>
> MemLog mem_log_stores[STORES_MAX];
> diff --git a/target/hexagon/translate.h b/target/hexagon/translate.h index
> c9533fee1f..ad1a2f4045 100644
> --- a/target/hexagon/translate.h
> +++ b/target/hexagon/translate.h
> @@ -85,6 +85,7 @@ typedef struct DisasContext {
> TCGv dczero_addr;
> bool pcycle_enabled;
> bool pkt_ends_tb;
> +bool need_next_pc;
> uint32_t num_cycles;
> } DisasContext;
>
> @@ -306,6 +307,7 @@ static inline void ctx_log_qreg_read(DisasContext
> *ctx, }
>
> extern TCGv hex_gpr[TOTAL_PER_THREAD_REGS];
> +extern TCGv hex_next_PC;
> extern TCGv hex_pred[NUM_PREGS];
> extern TCGv hex_slot_cancelled;
> extern TCGv hex_new_value_usr;
> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
> 5554c9515c..afc7e5f3a5 100644
> --- a/target/hexagon/genptr.c
> +++ b/target/hexagon/genptr.c
> @@ -634,14 +634,15 @@ static void gen_write_new_pc_addr(DisasContext
> *ctx, TCGv addr,
> tcg_gen_brcondi_tl(cond, pred, 0, pred_false);
> }
>
> +TCGv PC_wr = ctx->need_next_pc ? hex_next_PC :
> hex_gpr[HEX_REG_PC];
> if (ctx->pkt->pkt_has_multi_cof) {
> /* If there are multiple branches in a packet, ignore the second one
> */
> -tcg_gen_movcond_tl(TCG_COND_NE, hex_gpr[HEX_REG_PC],
> +tcg_gen_movcond_tl(TCG_COND_NE, PC_wr,
> ctx->branch_taken, tcg_constant_tl(0),
> - hex_gpr[HEX_REG_PC], addr);
> + PC_wr, addr);
> tcg_gen_movi_tl(ctx->branch_taken, 1);
> } else {
> -tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], addr);
> +tcg_gen_mov_tl(PC_wr, addr);
> }
>
> if (cond != TCG_COND_ALWAYS) {
> diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c index
> 475726388a..d4b22acb72 100644
> --- a/target/hexagon/translate.c
> +++ b/target/hexagon/translate.c
> @@ -49,6 +49,7 @@ static const AnalyzeInsn
> opcode_analyze[XX_LAST_OPCODE] = { TCGv
> hex_gpr[TOTAL_PER_THREAD_REGS]; TCGv hex_pred[NUM_PREGS]; TCGv
> hex_slot_cancelled;
> +TCGv hex_next_PC;
> TCGv hex_new_value_usr;
> TCGv hex_store_addr[STORES_MAX];
> TCGv hex_store_width[STORES_MAX];
> @@ -61,12 +62,14 @@ TCGv_i64 hex_cycle_count; TCGv
> hex_vstore_addr[VSTORES_MAX]; TCGv hex_vstore_size[VSTORES_MAX];
> TCGv hex_vstore_pending[VSTORES_MAX];
> +static bool need_next_PC(DisasContext *ctx);
You don't need this. The function definition is before any reference to it.
>
> #ifndef CONFIG_USER_ONLY
> TCGv hex_greg[NUM_GREGS];
> TCGv hex_t_sreg[NUM_SREGS];
> TCGv_ptr hex_g_sreg_ptr;
> TCGv hex_g_sreg[NUM_SREGS];
> +TCGv hex_cause_code;
This doesn't belong in this patch.
> #endif
>
> static const char * const hexagon_prednames[] = { @@ -184,6 +187,9 @@
> static void gen_end_tb(DisasContext *ctx)
>
> gen_exec_counters(ctx);
>
> +if (ctx->need_next_pc) {
> +tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], hex_next_PC);
> +}
> if (ctx->branch_cond != TCG_COND_NEVER) {
> if (ctx->branch_cond != TCG_COND_ALWAYS) {
> TCGLabel *skip = gen_new_label(); @@ -371,18 +377,24 @@ static
> bool pkt_ends_tb(Packet *pkt) static bool need_next_PC(DisasContext
> *ctx) {
> Packet *pkt = ctx->pkt;
> -
> -/* Check for condition
RE: [PATCH 38/39] target/hexagon: Add guest reg reading functionality
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:29 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com
> Subject: [PATCH 38/39] target/hexagon: Add guest reg reading functionality
>
> From: Matheus Tavares Bernardino
>
> Signed-off-by: Matheus Tavares Bernardino
> ---
> target/hexagon/cpu.c | 19 ++-
> target/hexagon/op_helper.c | 19 +--
> 2 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index
> 3c4776232e..80f5e23794 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -739,7 +739,24 @@ static void hexagon_cpu_class_init(ObjectClass *c,
> void *data) #ifndef CONFIG_USER_ONLY uint32_t
> hexagon_greg_read(CPUHexagonState *env, uint32_t reg) {
> -g_assert_not_reached();
> +target_ulong ssr = arch_get_system_reg(env, HEX_SREG_SSR);
> +int ssr_ce = GET_SSR_FIELD(SSR_CE, ssr);
Consider moving this check into hexagon_get_sys_pcycle_count*
Otherwise
Reviewed-by: Taylor Simpson
RE: [PATCH 05/39] target/hexagon: Implement modify SSR
> -Original Message-
> From: Sid Manning
> Sent: Tuesday, March 18, 2025 1:34 PM
> To: ltaylorsimp...@gmail.com; 'Brian Cain'
> ; qemu-devel@nongnu.org
> Cc: richard.hender...@linaro.org; phi...@linaro.org; Matheus Bernardino
> (QUIC) ; a...@rev.ng; a...@rev.ng; Marco
> Liebel (QUIC) ; alex.ben...@linaro.org; Mark
> Burton (QUIC) ; Brian Cain
>
> Subject: RE: [PATCH 05/39] target/hexagon: Implement modify SSR
>
>
>
> > -Original Message-
> > From: ltaylorsimp...@gmail.com
> > Sent: Monday, March 17, 2025 12:37 PM
> > To: 'Brian Cain' ; qemu-
> de...@nongnu.org
> > Cc: richard.hender...@linaro.org; phi...@linaro.org; Matheus
> > Bernardino
> > (QUIC) ; a...@rev.ng; a...@rev.ng; Marco
> > Liebel (QUIC) ; alex.ben...@linaro.org; Mark
> > Burton (QUIC) ; Sid Manning
> > ; Brian Cain
> > Subject: RE: [PATCH 05/39] target/hexagon: Implement modify SSR
> >
> > > -Original Message-
> > > From: Brian Cain
> > > Sent: Friday, February 28, 2025 11:28 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> > > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng;
> > a...@rev.ng;
> > > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> > > alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> > sidn...@quicinc.com;
> > > Brian Cain
> > > Subject: [PATCH 05/39] target/hexagon: Implement modify SSR
> > >
> > > From: Brian Cain
> > >
> > > The per-vCPU System Status Register controls many modal behaviors of
> > > the system architecture. When the SSR is updated, we trigger the
> > > necessary effects for interrupts, privilege/MMU, and HVX context
> > mapping.
> > >
> > > Signed-off-by: Brian Cain
> > > ---
> >
> > What does SSR_XE indicate?
> [Sid Manning]
> If SSR:XE isn't set this thread doesn't have the coproc enabled so discard
> additional checking. Any coproc insn issued when ssr:xe is 0 will trigger a,
> "Illegal execution of coprocessor instruction." error.
> > > +if (old_XA != new_XA) {
> > > +int old_unit = parse_context_idx(env, old_XA);
> > > +int new_unit = parse_context_idx(env, new_XA);
> >
> > Check that old_unit != new_unit. Per the table above, different XA values
> > can point to the same unit. For example, if cpu->hvx_contexts is 2, the
> XA=0
> > and XA=2 both point to context 0.
> >
> > > +
> > > +/* Ownership exchange */
> > > +memcpy(VRegs[old_unit], env->VRegs, sizeof(env->VRegs));
> > > +memcpy(QRegs[old_unit], env->QRegs, sizeof(env->QRegs));
> > > +memcpy(env->VRegs, VRegs[new_unit], sizeof(env->VRegs));
> > > +memcpy(env->QRegs, QRegs[new_unit], sizeof(env->QRegs));
> >
> > What does the hardware do? Does it clear the context, or is that the OS'es
> > job?
> Nothing would keep a single htid from taking hvx unit 0, doing some hvx-ops
> , swapping to hvx unit 1 doing some more hvx-ops and so on. We are doing
> this because each thread has a private copy of the hvx register state. Since
> HVX units and threads are independent if one thread changes its ownership
> from HVX context 0 to HVX context 1 we have to do this copy. Instead of
> memcpy should create a new object that represents the HVX units available
> and change env->VRegs/QRegs to point to the one currently owned.
>
> Refactoring this will be an improvement.
>
>
> >
> > If the hardware leaves the context alone, the above should be
> > 1) Copy env->{VQ}Regs to old_unit
> > 2) Copy new_unit to env->{VQ}Regs
> >
> > Should you check SSR_EX before doing these copies?
> >
> > Do you need to do anything when SSR_EX changes?
>
> I think you mean SSR:XE before doing the copies. I think we have to do the
> copy here regardless of ssr:xe since a thread could swap ownership, update
> ssr:xa, without explicitly having ssr:xe set. Maybe the OS disables SSR:XE
> while changing hvx unit ownership?
Correct. I meant SSR:XE.
Some refactoring is in order but need to understand the HW behavior more
specifically.
- What will the HW do if more than one thread claims ownership of the same HVX
context?
- What happens if a thread claims ownership without setting SSR:XE and then
sets SSR:XE later?
- What about this example?
1) Thread 0 claims context 1 and sets SSR:XE
2) Thread 0 does some HVX computation
3) Thread 0 is done with HVX for now so clears SSR:XE
4) Thread 1 claims context 1 and sets SSR:XE, does some work, then clears
SSR:XE
5) Thread 0 wants to do more HVX, so it sets SSR:XE (still pointing to HVX
context 1)
We should keep the copies for the contexts and local copies inside
CPUHexagonState. This makes TCG generation easier as well as having common
code between system mode and linux-user mode.
Also, since check_overcommitted_hvx only prints a log message, add an early
exit if LOG_GUEST_ERROR is off.
Thanks,
Taylor
RE: [PATCH 2/8] hw/hexagon: Add machine configs for sysemu
> -Original Message-
> From: Brian Cain
> Sent: Saturday, March 1, 2025 11:21 AM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain ; Mike Lambert
> ; Paolo Bonzini ; Eduardo
> Habkost ; Marcel Apfelbaum
> ; Yanan Wang
> ; Zhao Liu ; Eric Blake
> ; Markus Armbruster
> Subject: [PATCH 2/8] hw/hexagon: Add machine configs for sysemu
>
> From: Brian Cain
>
> Co-authored-by: Mike Lambert
> Co-authored-by: Sid Manning
> Signed-off-by: Brian Cain
> diff --git a/hw/meson.build b/hw/meson.build index
> b827c82c5d..91969d6fec 100644
> --- a/hw/meson.build
> +++ b/hw/meson.build
> @@ -64,3 +64,4 @@ subdir('sparc')
> subdir('sparc64')
> subdir('tricore')
> subdir('xtensa')
> +subdir('hexagon')
Keep this list in alphabetical order, move hexagon between avr and hppa.
Otherwise
Reviewed-by: Taylor Simpson
RE: [PATCH 37/39] target/hexagon: Add support for loadw_phys
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:29 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 37/39] target/hexagon: Add support for loadw_phys > > From: Brian Cain > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH 04/39] target/hexagon: Implement start/stop helpers
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:28 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 04/39] target/hexagon: Implement start/stop helpers
>
> From: Brian Cain
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/cpu.h| 3 ++
> target/hexagon/cpu_bits.h | 1 +
> target/hexagon/cpu_helper.h | 3 ++
> target/hexagon/cpu.c| 14 +-
> target/hexagon/cpu_helper.c | 94
> +
> target/hexagon/op_helper.c | 4 +-
> 6 files changed, 116 insertions(+), 3 deletions(-)
>
> diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h index
> 894219fd20..1549c4f1f0 100644
> --- a/target/hexagon/cpu.h
> +++ b/target/hexagon/cpu.h
> @@ -41,6 +41,7 @@ typedef struct CPUHexagonTLBContext
> CPUHexagonTLBContext; #define REG_WRITES_MAX 32
> #define PRED_WRITES_MAX 5 /* 4 insns + endloop */
> #define VSTORES_MAX 2
> +#define VECTOR_UNIT_MAX 8
Not related to start/stop and not used in this patch.
>
> #ifndef CONFIG_USER_ONLY
> #define CPU_INTERRUPT_SWI CPU_INTERRUPT_TGT_INT_0
> @@ -178,6 +179,7 @@ struct ArchCPU {
> #ifndef CONFIG_USER_ONLY
> uint32_t num_tlbs;
> uint32_t l2vic_base_addr;
> +uint32_t hvx_contexts;
Not related to start/stop.
> #endif
> };
>
> @@ -194,6 +196,7 @@ G_NORETURN void
> hexagon_raise_exception_err(CPUHexagonState *env, uint32_t
> hexagon_greg_read(CPUHexagonState *env, uint32_t reg); uint32_t
> hexagon_sreg_read(CPUHexagonState *env, uint32_t reg); void
> hexagon_gdb_sreg_write(CPUHexagonState *env, uint32_t reg, uint32_t
> val);
> +void hexagon_cpu_soft_reset(CPUHexagonState *env);
> #endif
>
> #include "exec/cpu-all.h"
> diff --git a/target/hexagon/cpu_bits.h b/target/hexagon/cpu_bits.h index
> b559a7ba88..610094a759 100644
> --- a/target/hexagon/cpu_bits.h
> +++ b/target/hexagon/cpu_bits.h
> @@ -52,6 +52,7 @@ enum hex_event {
>
> enum hex_cause {
> HEX_CAUSE_NONE = -1,
> +HEX_CAUSE_RESET = 0x000,
> HEX_CAUSE_TRAP0 = 0x172,
> HEX_CAUSE_FETCH_NO_UPAGE = 0x012,
> HEX_CAUSE_INVALID_PACKET = 0x015,
> diff --git a/target/hexagon/cpu_helper.h b/target/hexagon/cpu_helper.h
> index 6f0c6697ad..95a0cc0788 100644
> --- a/target/hexagon/cpu_helper.h
> +++ b/target/hexagon/cpu_helper.h
> @@ -17,6 +17,9 @@ void
> hexagon_set_sys_pcycle_count_high(CPUHexagonState *env, uint32_t);
> void hexagon_modify_ssr(CPUHexagonState *env, uint32_t new, uint32_t
> old); int get_exe_mode(CPUHexagonState *env); void
> clear_wait_mode(CPUHexagonState *env);
> +void hexagon_ssr_set_cause(CPUHexagonState *env, uint32_t cause); void
> +hexagon_start_threads(CPUHexagonState *env, uint32_t mask); void
> +hexagon_stop_thread(CPUHexagonState *env);
>
> static inline void arch_set_thread_reg(CPUHexagonState *env, uint32_t reg,
> uint32_t val) diff --git
> a/target/hexagon/cpu.c
> b/target/hexagon/cpu.c index cb56b929cf..84a96a194b 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -32,6 +32,7 @@
>
> #ifndef CONFIG_USER_ONLY
> #include "sys_macros.h"
> +#include "qemu/main-loop.h"
> #endif
>
> static void hexagon_v66_cpu_init(Object *obj) { } @@ -61,6 +62,7 @@ static
> const Property hexagon_cpu_properties[] = {
> DEFINE_PROP_UINT32("jtlb-entries", HexagonCPU, num_tlbs,
> MAX_TLB_ENTRIES),
> DEFINE_PROP_UINT32("l2vic-base-addr", HexagonCPU, l2vic_base_addr,
> 0xULL),
> +DEFINE_PROP_UINT32("hvx-contexts", HexagonCPU, hvx_contexts, 0),
Not related to start/stop.
> #endif
> DEFINE_PROP_BOOL("lldb-compat", HexagonCPU, lldb_compat, false),
RE: [PATCH 05/39] target/hexagon: Implement modify SSR
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:28 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 05/39] target/hexagon: Implement modify SSR
>
> From: Brian Cain
>
> The per-vCPU System Status Register controls many modal behaviors of the
> system architecture. When the SSR is updated, we trigger the necessary
> effects for interrupts, privilege/MMU, and HVX context mapping.
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/cpu_helper.c | 100
> +++-
> 1 file changed, 99 insertions(+), 1 deletion(-)
>
> diff --git a/target/hexagon/cpu_helper.c b/target/hexagon/cpu_helper.c
> index e151c6335a..3e2364a7b0 100644
> --- a/target/hexagon/cpu_helper.c
> +++ b/target/hexagon/cpu_helper.c
> @@ -14,6 +14,8 @@
> #else
> #include "hw/boards.h"
> #include "hw/hexagon/hexagon.h"
> +#include "hex_interrupts.h"
> +#include "hex_mmu.h"
> #endif
> #include "exec/exec-all.h"
> #include "exec/cpu_ldst.h"
> @@ -69,9 +71,105 @@ void
> hexagon_set_sys_pcycle_count(CPUHexagonState *env, uint64_t cycles)
> g_assert_not_reached();
> }
>
> +static MMVector VRegs[VECTOR_UNIT_MAX][NUM_VREGS];
> +static MMQReg QRegs[VECTOR_UNIT_MAX][NUM_QREGS];
This won't scale for a system with multiple Hexagon instances. See discussion
on how to handle shared resources.
> +
> +/*
> + *EXT_CONTEXTS
> + * SSR.XA 2 4 6 8
> + * 000 HVX Context 0 HVX Context 0 HVX Context 0 HVX Context 0
> + * 001 HVX Context 1 HVX Context 1 HVX Context 1 HVX Context 1
> + * 010 HVX Context 0 HVX Context 2 HVX Context 2 HVX Context 2
> + * 011 HVX Context 1 HVX Context 3 HVX Context 3 HVX Context 3
> + * 100 HVX Context 0 HVX Context 0 HVX Context 4 HVX Context 4
> + * 101 HVX Context 1 HVX Context 1 HVX Context 5 HVX Context 5
> + * 110 HVX Context 0 HVX Context 2 HVX Context 2 HVX Context 6
> + * 111 HVX Context 1 HVX Context 3 HVX Context 3 HVX Context 7
> + */
This is different from what the HVX PRM says. It only specifies what XA values
4, 5, 6, 7 mean.
Here is what it says:
The HVX scalar core can contain any number of hardware threads greater or equal
to the number
of vector contexts. The scalar hardware thread is assignable to a vector
context through per-
thread SSR:XA programming, as follows:
- SSR:XA = 4: HVX instructions use vector context 0.
- SSR:XA = 5: HVX instructions use vector context 1, if it is available.
- SSR:XA = 6: HVX instructions use vector context 2, if it is available.
- SSR:XA = 7: HVX instructions use vector context 3, if it is available.
> +static int parse_context_idx(CPUHexagonState *env, uint8_t XA) {
> +int ret;
> +HexagonCPU *cpu = env_archcpu(env);
You should assert that cpu->hvx_contexts is in { 2, 4, 6, 8 }. This will
future proof against changes to the hardware as well as protect against bad
command-line settings.
> +if (cpu->hvx_contexts == 6 && XA >= 6) {
> +ret = XA - 6 + 2;
> +} else {
> +ret = XA % cpu->hvx_contexts;
> +}
> +g_assert(ret >= 0 && ret < VECTOR_UNIT_MAX);
> +return ret;
> +}
> +
> +static void check_overcommitted_hvx(CPUHexagonState *env, uint32_t
> ssr)
> +{
> +if (!GET_FIELD(SSR_XE, ssr)) {
> +return;
> +}
What does SSR_XE indicate?
> +
> +uint8_t XA = GET_SSR_FIELD(SSR_XA, ssr);
> +
> +CPUState *cs;
> +CPU_FOREACH(cs) {
> +CPUHexagonState *env_ = cpu_env(cs);
This underscore is confusing. Use a full name such as thread_env.
> +if (env_ == env) {
> +continue;
> +}
> +/* Check if another thread has the XE bit set and same XA */
> +uint32_t ssr_ = arch_get_system_reg(env_, HEX_SREG_SSR);
Ditto
> +if (GET_SSR_FIELD(SSR_XE2, ssr_) && GET_FIELD(SSR_XA, ssr_) == XA) {
The comment says check the XE bit but the code checks XE2. Also, note the XE
check on the current thread above.
> +qemu_log_mask(LOG_GUEST_ERROR,
> +"setting SSR.XA '%d' on thread %d but thread"
> +" %d has same extension active\n", XA, env->threadId,
> +env_->threadId);
> +}
> +}
> +}
> +
> void hexagon_modify_ssr(CPUHexagonState *env, uint32_t new, uint32_t
> old) {
> -g_assert_not_reached();
> +g_assert(bql_locked());
> +
> +bool old_EX = GET_SSR_FIELD(SSR_EX, old);
> +bool old_UM = GET_SSR_FIELD(SSR_UM, old);
> +bool old_GM = GET_SSR_FIELD(SSR_GM, old);
> +bool old_IE = GET_SSR_FIELD(SSR_IE, old);
> +uint8_t old_XA = GET_SSR_FIELD(SSR_XA, old);
> +bool new_EX = GET_SSR_FIE
RE: [PATCH 06/39] target/hexagon: Implement {g,s}etimask helpers
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:28 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 06/39] target/hexagon: Implement {g,s}etimask helpers
>
> From: Brian Cain
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/op_helper.c | 31 +--
> 1 file changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
> index 9f79b1a20c..83088cfaa3 100644
> --- a/target/hexagon/op_helper.c
> +++ b/target/hexagon/op_helper.c
> @@ -1468,12 +1468,39 @@ void HELPER(resume)(CPUHexagonState *env,
> uint32_t mask)
>
> uint32_t HELPER(getimask)(CPUHexagonState *env, uint32_t tid) {
> -g_assert_not_reached();
> +CPUState *cs;
> +CPU_FOREACH(cs) {
> +HexagonCPU *found_cpu = HEXAGON_CPU(cs);
> +CPUHexagonState *found_env = &found_cpu->env;
> +if (found_env->threadId == tid) {
> +target_ulong imask = arch_get_system_reg(found_env,
> HEX_SREG_IMASK);
> +qemu_log_mask(CPU_LOG_INT, "%s: tid %d imask = 0x%x\n",
> + __func__, env->threadId,
> + (unsigned)GET_FIELD(IMASK_MASK, imask));
> +return GET_FIELD(IMASK_MASK, imask);
> +}
> +}
> +return 0;
> }
>
> void HELPER(setimask)(CPUHexagonState *env, uint32_t pred, uint32_t
> imask) {
The name pred sounds like a predicate register. Use tid instead.
> -g_assert_not_reached();
> +CPUState *cs;
> +
> +BQL_LOCK_GUARD();
> +CPU_FOREACH(cs) {
> +HexagonCPU *found_cpu = HEXAGON_CPU(cs);
> +CPUHexagonState *found_env = &found_cpu->env;
> +
> +if (pred == found_env->threadId) {
> +SET_SYSTEM_FIELD(found_env, HEX_SREG_IMASK, IMASK_MASK,
> imask);
> +qemu_log_mask(CPU_LOG_INT, "%s: tid %d imask 0x%x\n",
> + __func__, found_env->threadId, imask);
> +hex_interrupt_update(env);
Shouldn't this be found_env?
> +return;
> +}
> +}
> +hex_interrupt_update(env);
Do you need to update if the thread wasn't found?
> }
>
> static bool handle_pmu_sreg_write(CPUHexagonState *env, uint32_t reg,
> --
> 2.34.1
RE: [PATCH 03/39] target/hexagon: Implement iassign{r,w} helpers
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:28 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 03/39] target/hexagon: Implement iassign{r,w} helpers
>
> From: Brian Cain
>
> iassign{r,w} are the "Interrupt to thread assignment {read,write}"
> instructions.
>
> Signed-off-by: Brian Cain
Reviewed-by: Taylor Simpson
RE: [PATCH 01/39] target/hexagon: Implement ciad helper
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:28 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 01/39] target/hexagon: Implement ciad helper
>
> From: Brian Cain
>
> ciad is the clear interrupt auto disable instruction.
>
> This instruction is defined in the Qualcomm Hexagon V71 Programmer's
> Reference Manual - https://docs.qualcomm.com/bundle/publicresource/80-
> N2040-51_REV_AB_Hexagon_V71_ProgrammerS_Reference_Manual.pdf
> See §11.9.2 SYSTEM MONITOR.
Put this reference in somewhere easier to find. See prior discussion on this.
If it's only in the commit comment, it will be lost quickly.
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/op_helper.c | 39 -
> -
> 1 file changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
> index fd9caafefc..b28a18adf6 100644
> --- a/target/hexagon/op_helper.c
> +++ b/target/hexagon/op_helper.c
> @@ -34,6 +34,11 @@
> #include "op_helper.h"
> #include "cpu_helper.h"
> #include "translate.h"
> +#ifndef CONFIG_USER_ONLY
> +#include "hex_mmu.h"
> +#include "hw/intc/l2vic.h"
> +#include "hex_interrupts.h"
> +#endif
>
> #define SF_BIAS127
> #define SF_MANTBITS23
> @@ -1338,9 +1343,36 @@ void HELPER(vwhist128qm)(CPUHexagonState
> *env, int32_t uiV) }
>
> #ifndef CONFIG_USER_ONLY
> +static void hexagon_set_vid(CPUHexagonState *env, uint32_t offset, int
> +val) {
> +g_assert((offset == L2VIC_VID_0) || (offset == L2VIC_VID_1));
> +CPUState *cs = env_cpu(env);
> +HexagonCPU *cpu = HEXAGON_CPU(cs);
> +const hwaddr pend_mem = cpu->l2vic_base_addr + offset;
> +cpu_physical_memory_write(pend_mem, &val, sizeof(val)); }
Careful here - an int is different sizes on 32-bit and 64-bit hosts. Change
the type to int32_t or int64_t.
> +
> +static void hexagon_clear_last_irq(CPUHexagonState *env, uint32_t
> +offset) {
> +/*
> + * currently only l2vic is the only attached it uses vid0, remove
> + * the assert below if anther is added
What assert?
> + */
> +hexagon_set_vid(env, offset, L2VIC_CIAD_INSTRUCTION); }
> +
> void HELPER(ciad)(CPUHexagonState *env, uint32_t mask) {
> -g_assert_not_reached();
> +uint32_t ipendad;
> +uint32_t iad;
> +
> +BQL_LOCK_GUARD();
> +ipendad = READ_SREG(HEX_SREG_IPENDAD);
> +iad = fGET_FIELD(ipendad, IPENDAD_IAD);
> +fSET_FIELD(ipendad, IPENDAD_IAD, iad & ~(mask));
> +arch_set_system_reg(env, HEX_SREG_IPENDAD, ipendad);
> +hexagon_clear_last_irq(env, L2VIC_VID_0);
> +hex_interrupt_update(env);
> }
>
> void HELPER(siad)(CPUHexagonState *env, uint32_t mask) @@ -1416,11
> +1448,6 @@ static void modify_syscfg(CPUHexagonState *env, uint32_t val)
> g_assert_not_reached();
> }
>
> -static void hexagon_set_vid(CPUHexagonState *env, uint32_t offset, int
> val) -{
> -g_assert_not_reached();
> -}
> -
> static uint32_t hexagon_find_last_irq(CPUHexagonState *env, uint32_t vid)
> {
> g_assert_not_reached();
> --
> 2.34.1
RE: [PATCH 07/39] target/hexagon: Implement wait helper
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:28 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 07/39] target/hexagon: Implement wait helper
>
> From: Brian Cain
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/cpu_helper.h | 1 +
> target/hexagon/cpu_helper.c | 40
> +
> target/hexagon/op_helper.c | 6 +-
> 3 files changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/target/hexagon/cpu_helper.h b/target/hexagon/cpu_helper.h
> index 95a0cc0788..e8d89d8526 100644
> --- a/target/hexagon/cpu_helper.h
> +++ b/target/hexagon/cpu_helper.h
> @@ -20,6 +20,7 @@ void clear_wait_mode(CPUHexagonState *env); void
> hexagon_ssr_set_cause(CPUHexagonState *env, uint32_t cause); void
> hexagon_start_threads(CPUHexagonState *env, uint32_t mask); void
> hexagon_stop_thread(CPUHexagonState *env);
> +void hexagon_wait_thread(CPUHexagonState *env, target_ulong PC);
>
> static inline void arch_set_thread_reg(CPUHexagonState *env, uint32_t reg,
> uint32_t val) diff --git
> a/target/hexagon/cpu_helper.c
> b/target/hexagon/cpu_helper.c index 3e2364a7b0..e64568b9fc 100644
> --- a/target/hexagon/cpu_helper.c
> +++ b/target/hexagon/cpu_helper.c
> @@ -71,6 +71,46 @@ void
> hexagon_set_sys_pcycle_count(CPUHexagonState *env, uint64_t cycles)
> g_assert_not_reached();
> }
>
> +static void set_wait_mode(CPUHexagonState *env) {
> +g_assert(bql_locked());
> +
> +const uint32_t modectl = arch_get_system_reg(env,
> HEX_SREG_MODECTL);
> +uint32_t thread_wait_mask = GET_FIELD(MODECTL_W, modectl);
> +thread_wait_mask |= 0x1 << env->threadId;
> +SET_SYSTEM_FIELD(env, HEX_SREG_MODECTL, MODECTL_W,
> +thread_wait_mask); }
> +
> +void hexagon_wait_thread(CPUHexagonState *env, target_ulong PC) {
> +g_assert(bql_locked());
> +
> +if (qemu_loglevel_mask(LOG_GUEST_ERROR) &&
> +(env->k0_lock_state != HEX_LOCK_UNLOCKED ||
> + env->tlb_lock_state != HEX_LOCK_UNLOCKED)) {
> +qemu_log("WARNING: executing wait() with acquired lock"
> + "may lead to deadlock\n");
> +}
> +g_assert(get_exe_mode(env) != HEX_EXE_MODE_WAIT);
> +
> +CPUState *cs = env_cpu(env);
> +/*
> + * The addtion of cpu_has_work is borrowed from arm's wfi helper
> + * and is critical for our stability
> + */
> +if ((cs->exception_index != HEX_EVENT_NONE) ||
> +(cpu_has_work(cs))) {
> +qemu_log_mask(CPU_LOG_INT,
> +"%s: thread %d skipping WAIT mode, have some work\n",
> +__func__, env->threadId);
> +return;
> +}
> +set_wait_mode(env);
> +env->wait_next_pc = PC + 4;
> +
> +cpu_interrupt(cs, CPU_INTERRUPT_HALT); }
> +
Why not put this in op_helper.c?
Otherwise
Reviewed-by: Taylor Simpson
RE: [PATCH 19/39] target/hexagon: Implement hexagon_tlb_fill()
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:28 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 19/39] target/hexagon: Implement hexagon_tlb_fill() > > From: Brian Cain > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH 20/39] target/hexagon: Implement siad inst
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:28 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 20/39] target/hexagon: Implement siad inst > > From: Brian Cain > > siad is the 'Set interrupt auto disable' instruction. > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH 24/39] target/hexagon: Add exec-start-addr prop
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:29 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 24/39] target/hexagon: Add exec-start-addr prop
>
> From: Brian Cain
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/cpu.h | 1 +
> target/hexagon/cpu.c | 7 ++-
> 2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h index
> baa48ec051..4667a1f748 100644
> --- a/target/hexagon/cpu.h
> +++ b/target/hexagon/cpu.h
> @@ -194,6 +194,7 @@ struct ArchCPU {
> uint32_t num_tlbs;
> uint32_t l2vic_base_addr;
> uint32_t hvx_contexts;
> +uint32_t boot_addr;
> #endif
> };
>
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index
> 9f4cfd03c4..7afdcbf9d0 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -66,6 +66,7 @@ static const Property hexagon_cpu_properties[] = {
> DEFINE_PROP_UINT32("l2vic-base-addr", HexagonCPU, l2vic_base_addr,
> 0xULL),
> DEFINE_PROP_UINT32("hvx-contexts", HexagonCPU, hvx_contexts, 0),
> +DEFINE_PROP_UINT32("exec-start-addr", HexagonCPU, boot_addr,
> + 0xULL),
Don't put a ULL value for a UINT32 property.
Ditto for l2vic-base-addr above (must've missed it when it came in).
> #endif
> DEFINE_PROP_BOOL("lldb-compat", HexagonCPU, lldb_compat, false),
> DEFINE_PROP_UNSIGNED("lldb-stack-adjust", HexagonCPU,
> lldb_stack_adjust, 0, @@ -362,8 +363,6 @@ static void
> hexagon_cpu_reset_hold(Object *obj, ResetType type)
> mmu_reset(env);
> arch_set_system_reg(env, HEX_SREG_HTID, cs->cpu_index);
> hexagon_cpu_soft_reset(env);
> -memset(env->t_sreg, 0, sizeof(target_ulong) * NUM_SREGS);
> -memset(env->greg, 0, sizeof(target_ulong) * NUM_GREGS);
Why are you removing these here?
> env->threadId = cs->cpu_index;
> env->tlb_lock_state = HEX_LOCK_UNLOCKED;
> env->k0_lock_state = HEX_LOCK_UNLOCKED; @@ -372,6 +371,7 @@ static
> void hexagon_cpu_reset_hold(Object *obj, ResetType type)
> env->next_PC = 0;
> env->wait_next_pc = 0;
> env->cause_code = -1;
> +arch_set_thread_reg(env, HEX_REG_PC, cpu->boot_addr);
> #endif
> }
>
> @@ -414,9 +414,6 @@ static void hexagon_cpu_realize(DeviceState *dev,
> Error **errp) #ifndef CONFIG_USER_ONLY
> CPUHexagonState *env = cpu_env(cs);
> hex_mmu_realize(env);
> -#endif
> -cpu_reset(cs);
> -#ifndef CONFIG_USER_ONLY
Why are you removing these here?
> if (cs->cpu_index == 0) {
> env->g_sreg = g_new0(target_ulong, NUM_SREGS);
> env->g_pcycle_base = g_malloc0(sizeof(*env->g_pcycle_base));
> --
> 2.34.1
RE: [PATCH 25/39] target/hexagon: Add hexagon_cpu_mmu_index()
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:29 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 25/39] target/hexagon: Add hexagon_cpu_mmu_index() > > From: Brian Cain > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH 26/39] target/hexagon: Decode trap1, rte as COF
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:29 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 26/39] target/hexagon: Decode trap1, rte as COF > > From: Brian Cain > > Also: handle rte instructions at the end of the packet. > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH 28/39] target/hexagon: Implement modify_ssr, resched, pending_interrupt
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:29 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 28/39] target/hexagon: Implement modify_ssr, resched, > pending_interrupt > > From: Brian Cain > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH 29/39] target/hexagon: Add pkt_ends_tb to translation
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:29 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 29/39] target/hexagon: Add pkt_ends_tb to translation
>
> From: Brian Cain
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/translate.h | 1 +
> target/hexagon/translate.c | 99
> +-
> 2 files changed, 99 insertions(+), 1 deletion(-)
>
> diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c index
> 060df6e5eb..475726388a 100644
> --- a/target/hexagon/translate.c
> +++ b/target/hexagon/translate.c
> @@ -259,6 +259,18 @@ static bool check_for_attrib(Packet *pkt, int attrib)
> return false;
> }
>
> +#ifndef CONFIG_USER_ONLY
> +static bool check_for_opcode(Packet *pkt, uint16_t opcode) {
> +for (int i = 0; i < pkt->num_insns; i++) {
> +if (pkt->insn[i].opcode == opcode) {
> +return true;
> +}
> +}
> +return false;
> +}
> +#endif
> +
> static bool need_slot_cancelled(Packet *pkt) {
> /* We only need slot_cancelled for conditional store instructions */ @@ -
> 272,6 +284,90 @@ static bool need_slot_cancelled(Packet *pkt)
> return false;
> }
>
> +#ifndef CONFIG_USER_ONLY
> +static bool sreg_write_to_global(int reg_num) {
> +return reg_num == HEX_SREG_SSR ||
> + reg_num == HEX_SREG_STID ||
> + reg_num == HEX_SREG_IMASK ||
The name of this function is misleading since SSR, STID, and IMASK are not
global.
A better name would be sreg_write_ends_tb
> + reg_num == HEX_SREG_IPENDAD ||
> + reg_num == HEX_SREG_BESTWAIT ||
> + reg_num == HEX_SREG_SCHEDCFG; }
> +
> +static bool has_sreg_write_to_global(Packet const *pkt) {
Ditto
Otherwise
Reviewed-by: Taylor Simpson
RE: [PATCH 09/39] target/hexagon: Implement arch_get_system_reg()
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:28 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 09/39] target/hexagon: Implement arch_get_system_reg() > > From: Brian Cain > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH 15/39] target/hexagon: Implement hex_tlb_entry_get_perm()
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:28 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 15/39] target/hexagon: Implement > hex_tlb_entry_get_perm() > > From: Brian Cain > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH 14/39] target/hexagon: Add system event, cause codes
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:28 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 14/39] target/hexagon: Add system event, cause codes > > From: Brian Cain > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH 08/39] target/hexagon: Implement get_exe_mode()
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:28 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 08/39] target/hexagon: Implement get_exe_mode()
>
> From: Brian Cain
>
> Signed-off-by: Brian Cain
> diff --git a/target/hexagon/cpu_helper.c b/target/hexagon/cpu_helper.c
> index e64568b9fc..e0dd120cd4 100644
> --- a/target/hexagon/cpu_helper.c
> +++ b/target/hexagon/cpu_helper.c
> @@ -237,6 +237,30 @@ void hexagon_ssr_set_cause(CPUHexagonState
> *env, uint32_t cause)
>
> int get_exe_mode(CPUHexagonState *env)
> {
> +g_assert(bql_locked());
> +
> +target_ulong modectl = arch_get_system_reg(env,
> HEX_SREG_MODECTL);
> +uint32_t thread_enabled_mask = GET_FIELD(MODECTL_E, modectl);
> +bool E_bit = thread_enabled_mask & (0x1 << env->threadId);
> +uint32_t thread_wait_mask = GET_FIELD(MODECTL_W, modectl);
> +bool W_bit = thread_wait_mask & (0x1 << env->threadId);
> +target_ulong isdbst = arch_get_system_reg(env, HEX_SREG_ISDBST);
> +uint32_t debugmode = GET_FIELD(ISDBST_DEBUGMODE, isdbst);
> +bool D_bit = debugmode & (0x1 << env->threadId);
> +
> +/* Figure 4-2 */
Figure 4-2 in which document?
Otherwise
Reviewed-by: Taylor Simpson
RE: [PATCH 10/39] target/hexagon: Implement arch_{s, g}et_{thread, system}_reg()
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:28 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 10/39] target/hexagon: Implement
> arch_{s,g}et_{thread,system}_reg()
>
> From: Brian Cain
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/cpu_helper.h | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/target/hexagon/cpu_helper.h b/target/hexagon/cpu_helper.h
> index e8d89d8526..1cdf4f8dd0 100644
> --- a/target/hexagon/cpu_helper.h
> +++ b/target/hexagon/cpu_helper.h
> @@ -26,20 +26,27 @@ static inline void
> arch_set_thread_reg(CPUHexagonState *env, uint32_t reg,
> uint32_t val) {
> g_assert(reg < TOTAL_PER_THREAD_REGS);
> -g_assert_not_reached();
> +env->gpr[reg] = val;
Gotta be careful here. Not all registers can be assigned directly. Look at
gen_write_ctrl_reg in genptr.c.
> }
>
> static inline uint32_t arch_get_thread_reg(CPUHexagonState *env, uint32_t
> reg) {
> g_assert(reg < TOTAL_PER_THREAD_REGS);
> -g_assert_not_reached();
> +return env->gpr[reg];
Ditto - look at gen_read_ctrl_reg in genptr.c
> }
>
> +#ifndef CONFIG_USER_ONLY
> static inline void arch_set_system_reg(CPUHexagonState *env, uint32_t
> reg,
> uint32_t val) {
> -g_assert_not_reached();
> +g_assert(reg < NUM_SREGS);
> +if (reg < HEX_SREG_GLB_START) {
> +env->t_sreg[reg] = val;
> +} else {
> +env->g_sreg[reg] = val;
> +}
Be careful here too. Look at gen_log_sreg_write.
> }
> +#endif
>
> uint32_t arch_get_system_reg(CPUHexagonState *env, uint32_t reg);
Honestly, consider getting rid of these. All the uses are a specific register
number, so there's no benefit of things like g_assert(reg <
TOTAL_PER_THREAD_REGS) or reg < HEX_SREG_GLB_START. Also, keeping them runs
the risk of not having all the proper checks inside.
RE: [PATCH 11/39] target/hexagon: Add representation to count cycles
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:28 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 11/39] target/hexagon: Add representation to count cycles
>
> From: Brian Cain
>
> The PCYCLE register can be enabled to indicate accumulated clock cycles.
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/cpu.h | 3 ++-
> target/hexagon/cpu.c | 3 +++
> target/hexagon/machine.c | 25 -
> 3 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h index
> 1549c4f1f0..4b9c9873dc 100644
> --- a/target/hexagon/cpu.h
> +++ b/target/hexagon/cpu.h
> @@ -113,7 +113,8 @@ typedef struct CPUArchState {
> target_ulong stack_start;
>
> uint8_t slot_cancelled;
> -
> +uint64_t t_cycle_count;
> +uint64_t *g_pcycle_base;
> #ifndef CONFIG_USER_ONLY
> /* Some system registers are per thread and some are global. */
> target_ulong t_sreg[NUM_SREGS];
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index
> 84a96a194b..89a051b41d 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -335,6 +335,7 @@ static void hexagon_cpu_reset_hold(Object *obj,
> ResetType type)
>
> if (cs->cpu_index == 0) {
> arch_set_system_reg(env, HEX_SREG_MODECTL, 0x1);
> +*(env->g_pcycle_base) = 0;
See discussion on shared resources.
> }
> mmu_reset(env);
> arch_set_system_reg(env, HEX_SREG_HTID, cs->cpu_index); @@ -396,10
> +397,12 @@ static void hexagon_cpu_realize(DeviceState *dev, Error
> **errp) #ifndef CONFIG_USER_ONLY
> if (cs->cpu_index == 0) {
> env->g_sreg = g_new0(target_ulong, NUM_SREGS);
> +env->g_pcycle_base = g_malloc0(sizeof(*env->g_pcycle_base));
Shared resource ...
> } else {
> CPUState *cpu0 = qemu_get_cpu(0);
> CPUHexagonState *env0 = cpu_env(cpu0);
> env->g_sreg = env0->g_sreg;
> +env->g_pcycle_base = env0->g_pcycle_base;
Shared resource ...
RE: [PATCH 27/39] target/hexagon: Implement hexagon_find_last_irq()
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:29 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 27/39] target/hexagon: Implement hexagon_find_last_irq() > > From: Brian Cain > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
RE: [PATCH v3 5/5] target/hexagon: Remove unreachable
> -Original Message-
> From: Matheus Tavares Bernardino
>
> Sent: Monday, April 14, 2025 12:10 PM
> To: ltaylorsimp...@gmail.com
> Cc: brian.c...@oss.qualcomm.com; qemu-devel@nongnu.org;
> richard.hender...@linaro.org; phi...@linaro.org;
> matheus.bernard...@oss.qualcomm.com; a...@rev.ng; a...@rev.ng;
> marco.lie...@oss.qualcomm.com; alex.ben...@linaro.org;
> quic_mbur...@quicinc.com; sidn...@quicinc.com
> Subject: RE: [PATCH v3 5/5] target/hexagon: Remove unreachable
>
> On Mon, 14 Apr 2025 11:19:38 -0600 wrote:
> >
> > > -Original Message-
> > > From: Brian Cain
> > > Sent: Monday, April 7, 2025 1:27 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> > > phi...@linaro.org; matheus.bernard...@oss.qualcomm.com;
> a...@rev.ng;
> > > a...@rev.ng; marco.lie...@oss.qualcomm.com;
> > > ltaylorsimp...@gmail.com; alex.ben...@linaro.org;
> > > quic_mbur...@quicinc.com; sidn...@quicinc.com
> > > Subject: [PATCH v3 5/5] target/hexagon: Remove unreachable
> > >
> > > We should raise an exception in the event that we encounter a packet
> > > that can't be correctly decoded, not fault.
> > >
> > > Signed-off-by: Brian Cain
> > > ---
> > > target/hexagon/decode.c | 6 +-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c index
> > > b5ece60450..1db7f1950f 100644
> > > --- a/target/hexagon/decode.c
> > > +++ b/target/hexagon/decode.c
> > > @@ -489,7 +489,6 @@ decode_insns(DisasContext *ctx, Insn *insn,
> > > uint32_t
> > > encoding)
> > > insn->iclass = iclass_bits(encoding);
> > > return 1;
> > > }
> > > -g_assert_not_reached();
> > > } else {
> > > uint32_t iclass = get_duplex_iclass(encoding);
> > > unsigned int slot0_subinsn = get_slot0_subinsn(encoding);
> > > @@ -512,6
> > > +511,11 @@ decode_insns(DisasContext *ctx, Insn *insn, uint32_t
> > > +encoding)
> > > }
> > > g_assert_not_reached();
> >
> > Why leave this one rather than raising an exception?
>
> Good point. I think this one should be removed as well. We have removed it
> downstream already.
>
> > > }
> > > +/*
> > > + * invalid/unrecognized opcode; return 1 and let gen_insn() raise
an
> > > + * exception when it sees this empty insn.
> > > + */
> > > +return 1;
> >
> > You should set insn->generate to NULL if you want to guarantee that
> > gen_insn will raise an exception.
>
> The caller already memset's it to 0 before passing `insn` down.
>
> > Do you have a test case for this?
>
> We do have a softmmu test for this downstream. Maybe we can adjust it for
> user-mode and upstream it with this patch.
Take a look at tests/tcg/hexagon/invalid-slots.c to see how to do this in
linux-user mode. You'll also need to modify Makefile.target in that
directory.
HTH,
Taylor
RE: [PATCH v3 5/5] target/hexagon: Remove unreachable
> -Original Message-
> From: Brian Cain
> Sent: Wednesday, April 16, 2025 4:22 PM
> To: ltaylorsimp...@gmail.com; 'Matheus Tavares Bernardino'
>
> Cc: qemu-devel@nongnu.org; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng;
> marco.lie...@oss.qualcomm.com; alex.ben...@linaro.org;
> quic_mbur...@quicinc.com; sidn...@quicinc.com
> Subject: Re: [PATCH v3 5/5] target/hexagon: Remove unreachable
>
>
> On 4/14/2025 3:59 PM, ltaylorsimp...@gmail.com wrote:
> >
> >> -Original Message-
> >> From: Matheus Tavares Bernardino
> >>
> >> Sent: Monday, April 14, 2025 12:10 PM
> >> To: ltaylorsimp...@gmail.com
> >> Cc: brian.c...@oss.qualcomm.com; qemu-devel@nongnu.org;
> >> richard.hender...@linaro.org; phi...@linaro.org;
> >> matheus.bernard...@oss.qualcomm.com; a...@rev.ng; a...@rev.ng;
> >> marco.lie...@oss.qualcomm.com; alex.ben...@linaro.org;
> >> quic_mbur...@quicinc.com; sidn...@quicinc.com
> >> Subject: RE: [PATCH v3 5/5] target/hexagon: Remove unreachable
> >>
> >> On Mon, 14 Apr 2025 11:19:38 -0600 wrote:
> -Original Message-
> From: Brian Cain
> Sent: Monday, April 7, 2025 1:27 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; matheus.bernard...@oss.qualcomm.com;
> >> a...@rev.ng;
> a...@rev.ng; marco.lie...@oss.qualcomm.com;
> ltaylorsimp...@gmail.com; alex.ben...@linaro.org;
> quic_mbur...@quicinc.com; sidn...@quicinc.com
> Subject: [PATCH v3 5/5] target/hexagon: Remove unreachable
>
> We should raise an exception in the event that we encounter a
> packet that can't be correctly decoded, not fault.
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/decode.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c
> index b5ece60450..1db7f1950f 100644
> --- a/target/hexagon/decode.c
> +++ b/target/hexagon/decode.c
> @@ -489,7 +489,6 @@ decode_insns(DisasContext *ctx, Insn *insn,
> uint32_t
> encoding)
> insn->iclass = iclass_bits(encoding);
> return 1;
> }
> -g_assert_not_reached();
> } else {
> uint32_t iclass = get_duplex_iclass(encoding);
> unsigned int slot0_subinsn = get_slot0_subinsn(encoding);
> @@ -512,6
> +511,11 @@ decode_insns(DisasContext *ctx, Insn *insn, uint32_t
> +encoding)
> }
> g_assert_not_reached();
> >>> Why leave this one rather than raising an exception?
> >> Good point. I think this one should be removed as well. We have
> >> removed it downstream already.
>
>
> Taylor, is it satisfactory to include that update in a subsequent patch
> series? Or should this one replace the second unreachable too?
If you just remove that line, it will fall through to the "return 1" below.
> >>
> }
> +/*
> + * invalid/unrecognized opcode; return 1 and let gen_insn() raise
> > an
> + * exception when it sees this empty insn.
> + */
> +return 1;
> >>> You should set insn->generate to NULL if you want to guarantee that
> >>> gen_insn will raise an exception.
> >> The caller already memset's it to 0 before passing `insn` down.
> >>
> >>> Do you have a test case for this?
> >> We do have a softmmu test for this downstream. Maybe we can adjust it
> for
> >> user-mode and upstream it with this patch.
> > Take a look at tests/tcg/hexagon/invalid-slots.c to see how to do this in
> > linux-user mode. You'll also need to modify Makefile.target in that
> > directory.
>
>
> Matheus provided a linux-user test offline. I'll include it in an
> updated patch.
>
>
> >
> > HTH,
> > Taylor
> >
> >
RE: [PATCH v3 2/5] target/hexagon: Fix badva reference, delete CAUSE
> -Original Message- > From: Brian Cain > Sent: Monday, April 7, 2025 1:27 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; matheus.bernard...@oss.qualcomm.com; a...@rev.ng; > a...@rev.ng; marco.lie...@oss.qualcomm.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com > Subject: [PATCH v3 2/5] target/hexagon: Fix badva reference, delete CAUSE > > The BADVA reg is referred to with the wrong identifier. The CAUSE reg field > of SSR is not yet modeled. > > Signed-off-by: Brian Cain > --- > target/hexagon/cpu.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index > 766b678651..62f1fe15b8 100644 > --- a/target/hexagon/cpu.c > +++ b/target/hexagon/cpu.c > @@ -216,8 +216,7 @@ static void hexagon_dump(CPUHexagonState *env, > FILE *f, int flags) > qemu_fprintf(f, " cs0 = 0x\n"); > qemu_fprintf(f, " cs1 = 0x\n"); #else > -print_reg(f, env, HEX_REG_CAUSE); > -print_reg(f, env, HEX_REG_BADVA); > +print_reg(f, env, HEX_SREG_BADVA); Since BADVA is a proxy for BADVA0/BADVA1, consider naming it HEX_SREG_BADVA_ALIASED to help avoid the problems we've seen with HEX_REG_P3_0_ALIASED. Taylor
RE: [PATCH v3 1/5] target/hexagon: handle .new values
> -Original Message- > From: Brian Cain > Sent: Monday, April 7, 2025 1:27 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; matheus.bernard...@oss.qualcomm.com; a...@rev.ng; > a...@rev.ng; marco.lie...@oss.qualcomm.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com > Subject: [PATCH v3 1/5] target/hexagon: handle .new values > > Signed-off-by: Brian Cain > --- > target/hexagon/hex_common.py | 22 +- > 1 file changed, 17 insertions(+), 5 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH v3 3/5] target/hexagon: Add missing A_CALL attr, hintjumpr to multi_cof
> -Original Message- > From: Brian Cain > Sent: Monday, April 7, 2025 1:27 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; matheus.bernard...@oss.qualcomm.com; a...@rev.ng; > a...@rev.ng; marco.lie...@oss.qualcomm.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com > Subject: [PATCH v3 3/5] target/hexagon: Add missing A_CALL attr, hintjumpr > to multi_cof > > Signed-off-by: Brian Cain > --- > target/hexagon/hex_common.py | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/target/hexagon/hex_common.py > b/target/hexagon/hex_common.py index 6803908718..a2dcb0aa2e 100755 > --- a/target/hexagon/hex_common.py > +++ b/target/hexagon/hex_common.py > @@ -247,8 +247,11 @@ def need_next_PC(tag): > > > def need_pkt_has_multi_cof(tag): > -return "A_COF" in attribdict[tag] > - > +return ( > +"A_JUMP" in attribdict[tag] > +or "A_CALL" in attribdict[tag] > +or "J2_rte" == tag > +) and tag != "J2_hintjumpr" It would be better to make this decision with instruction attributes only rather than a mix of attributes and specific tags. If needed, add another add_qemu_macro_attrib call to hex_common.calculate_attribs. Having said that, the correct tag for hintjumpr is J*4*_hintjumpr. Taylor
RE: [PATCH v3 4/5] target/hexagon: s/pkt_has_store/pkt_has_scalar_store
> -Original Message- > From: Brian Cain > Sent: Monday, April 7, 2025 1:27 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; matheus.bernard...@oss.qualcomm.com; a...@rev.ng; > a...@rev.ng; marco.lie...@oss.qualcomm.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com > Subject: [PATCH v3 4/5] target/hexagon: > s/pkt_has_store/pkt_has_scalar_store > > To remove any confusion with HVX or other potential store instructions, we'll > qualify this context var with "scalar". > > Signed-off-by: Brian Cain > --- > target/hexagon/idef-parser/README.rst | 2 +- > target/hexagon/insn.h | 4 ++-- > target/hexagon/macros.h | 8 > target/hexagon/decode.c | 4 ++-- > target/hexagon/genptr.c | 3 ++- > target/hexagon/idef-parser/parser-helpers.c | 4 ++-- > target/hexagon/op_helper.c | 4 ++-- > target/hexagon/translate.c | 9 + > target/hexagon/gen_helper_funcs.py | 2 +- > 9 files changed, 21 insertions(+), 19 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH v3 5/5] target/hexagon: Remove unreachable
> -Original Message-
> From: Brian Cain
> Sent: Monday, April 7, 2025 1:27 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; matheus.bernard...@oss.qualcomm.com; a...@rev.ng;
> a...@rev.ng; marco.lie...@oss.qualcomm.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com
> Subject: [PATCH v3 5/5] target/hexagon: Remove unreachable
>
> We should raise an exception in the event that we encounter a packet that
> can't be correctly decoded, not fault.
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/decode.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c index
> b5ece60450..1db7f1950f 100644
> --- a/target/hexagon/decode.c
> +++ b/target/hexagon/decode.c
> @@ -489,7 +489,6 @@ decode_insns(DisasContext *ctx, Insn *insn, uint32_t
> encoding)
> insn->iclass = iclass_bits(encoding);
> return 1;
> }
> -g_assert_not_reached();
> } else {
> uint32_t iclass = get_duplex_iclass(encoding);
> unsigned int slot0_subinsn = get_slot0_subinsn(encoding); @@ -512,6
> +511,11 @@ decode_insns(DisasContext *ctx, Insn *insn, uint32_t encoding)
> }
> g_assert_not_reached();
Why leave this one rather than raising an exception?
> }
> +/*
> + * invalid/unrecognized opcode; return 1 and let gen_insn() raise an
> + * exception when it sees this empty insn.
> + */
> +return 1;
You should set insn->generate to NULL if you want to guarantee that gen_insn
will raise an exception. A better option is to return a special value that
indicates "invalid" and have decode_packet return 0 which will cause
decode_and_translate_packet to generate the exception before generating the
code for any other instructions in the packet.
Do you have a test case for this?
Taylor
RE: [PATCH] Hexagon (target/hexagon) Remove gen_tcg_func_table.py
> -Original Message-
> From: ltaylorsimp...@gmail.com
> Sent: Wednesday, April 16, 2025 8:45 AM
> To: 'Philippe Mathieu-Daudé' ; qemu-
> de...@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; quic_mathb...@quicinc.com;
> sidn...@quicinc.com; quic_mlie...@quicinc.com;
> richard.hender...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: RE: [PATCH] Hexagon (target/hexagon) Remove
> gen_tcg_func_table.py
>
>
>
> > -Original Message-
> > From: Philippe Mathieu-Daudé
> > Sent: Wednesday, April 16, 2025 12:18 AM
> > To: Taylor Simpson ; qemu-
> de...@nongnu.org
> > Cc: brian.c...@oss.qualcomm.com; quic_mathb...@quicinc.com;
> > sidn...@quicinc.com; quic_mlie...@quicinc.com;
> > richard.hender...@linaro.org; a...@rev.ng; a...@rev.ng
> > Subject: Re: [PATCH] Hexagon (target/hexagon) Remove
> > gen_tcg_func_table.py
> >
> > Hi Taylor,
> >
> > On 16/4/25 01:55, Taylor Simpson wrote:
> > > This can easily be done in C with opcodes_def_generated.h.inc
> > >
> > > Signed-off-by: Taylor Simpson
> > > ---
> > > target/hexagon/genptr.c | 6 ++-
> > > target/hexagon/README| 1 -
> > > target/hexagon/gen_tcg_func_table.py | 66
> > > target/hexagon/meson.build | 10 -
> > > 4 files changed, 5 insertions(+), 78 deletions(-)
> > > delete mode 100755 target/hexagon/gen_tcg_func_table.py
> > >
> > > diff --git a/target/hexagon/gen_tcg_func_table.py
> > > b/target/hexagon/gen_tcg_func_table.py
> > > deleted file mode 100755
> > > index 299a39b1aa..00
> > > --- a/target/hexagon/gen_tcg_func_table.py
> > > -f.write("const SemanticInsn opcode_genptr[XX_LAST_OPCODE] =
> > {\n")
> > > -for tag in hex_common.tags:
> > > -## Skip the priv instructions
> > > -if "A_PRIV" in hex_common.attribdict[tag]:
> > > -continue
> > > -## Skip the guest instructions
> > > -if "A_GUEST" in hex_common.attribdict[tag]:
> > > -continue
> > > -## Skip the diag instructions
> > > -if tag == "Y6_diag":
> > > -continue
> > > -if tag == "Y6_diag0":
> > > -continue
> > > -if tag == "Y6_diag1":
> > > -continue
> >
> > What about all these skipped tags? IIUC gen_opcodes_def.py doesn't
> > skip them. If it isn't necessary to skip, please mention it in the
> > commit description for clarity.
> >
> > Regards,
> >
> > Phil.
I looked into this. There aren't any instructions currently in the code that
would be skipped by this logic. Perhaps this logic was needed at one point in
time but is no longer needed because those instructions were removed at some
point.
I recall that Brian consolidated the logic to decide which instructions to skip
into a single function in hex_common.py as part of the system mode patch
series. So, I'll go ahead and add a comment to the commit description as you
suggest.
Thanks,
Taylor
RE: [PATCH] Hexagon (target/hexagon) Remove gen_tcg_func_table.py
> -Original Message-
> From: Philippe Mathieu-Daudé
> Sent: Wednesday, April 16, 2025 12:18 AM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; quic_mathb...@quicinc.com;
> sidn...@quicinc.com; quic_mlie...@quicinc.com;
> richard.hender...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: Re: [PATCH] Hexagon (target/hexagon) Remove
> gen_tcg_func_table.py
>
> Hi Taylor,
>
> On 16/4/25 01:55, Taylor Simpson wrote:
> > This can easily be done in C with opcodes_def_generated.h.inc
> >
> > Signed-off-by: Taylor Simpson
> > ---
> > target/hexagon/genptr.c | 6 ++-
> > target/hexagon/README| 1 -
> > target/hexagon/gen_tcg_func_table.py | 66
> > target/hexagon/meson.build | 10 -
> > 4 files changed, 5 insertions(+), 78 deletions(-)
> > delete mode 100755 target/hexagon/gen_tcg_func_table.py
> >
> > diff --git a/target/hexagon/gen_tcg_func_table.py
> > b/target/hexagon/gen_tcg_func_table.py
> > deleted file mode 100755
> > index 299a39b1aa..00
> > --- a/target/hexagon/gen_tcg_func_table.py
> > -f.write("const SemanticInsn opcode_genptr[XX_LAST_OPCODE] =
> {\n")
> > -for tag in hex_common.tags:
> > -## Skip the priv instructions
> > -if "A_PRIV" in hex_common.attribdict[tag]:
> > -continue
> > -## Skip the guest instructions
> > -if "A_GUEST" in hex_common.attribdict[tag]:
> > -continue
> > -## Skip the diag instructions
> > -if tag == "Y6_diag":
> > -continue
> > -if tag == "Y6_diag0":
> > -continue
> > -if tag == "Y6_diag1":
> > -continue
>
> What about all these skipped tags? IIUC gen_opcodes_def.py doesn't skip
> them. If it isn't necessary to skip, please mention it in the commit
> description
> for clarity.
>
> Regards,
>
> Phil.
RE: [PATCH 03/38] target/hexagon: Add System/Guest register definitions
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 10:26 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 03/38] target/hexagon: Add System/Guest register
> definitions
>
> From: Brian Cain
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/gen_analyze_funcs.py | 21 +++-
> target/hexagon/hex_common.py| 163
>
> 2 files changed, 181 insertions(+), 3 deletions(-)
> diff --git a/target/hexagon/hex_common.py
> b/target/hexagon/hex_common.py index 758e5fd12d..db50defeb6 100755
> --- a/target/hexagon/hex_common.py
> +++ b/target/hexagon/hex_common.py
> @@ -33,6 +33,41 @@
> overrides = {} # tags with helper overrides idef_parser_enabled = {} #
> tags
> enabled for idef-parser
>
> +
> +def is_sysemu_tag(tag):
> +return "A_PRIV" in attribdict[tag] or "A_GUEST" in attribdict[tag]
> +
> +
> +def tag_ignore(tag):
> +tag_skips = (
> +"Y6_diag",
> +"Y6_diag0",
> +"Y6_diag1",
> +)
> +attr_skips = (
> +"A_FAKEINSN",
> +"A_MAPPING",
Add A_CONDMAPPING to this list.
> +)
> +return tag in tag_skips or \
> +any(attr in attribdict[tag] for attr in attr_skips)
RE: [PATCH 03/38] target/hexagon: Add System/Guest register definitions
> -Original Message-
> From: Brian Cain
> Sent: Wednesday, April 16, 2025 1:43 PM
> To: ltaylorsimp...@gmail.com; qemu-devel@nongnu.org
> Cc: richard.hender...@linaro.org; phi...@linaro.org;
> quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; alex.ben...@linaro.org;
> quic_mbur...@quicinc.com; sidn...@quicinc.com; 'Brian Cain'
>
> Subject: Re: [PATCH 03/38] target/hexagon: Add System/Guest register
> definitions
>
>
> On 4/16/2025 12:54 PM, ltaylorsimp...@gmail.com wrote:
> >
> >> -Original Message-
> >> From: Brian Cain
> >> Sent: Friday, February 28, 2025 10:26 PM
> >> To: qemu-devel@nongnu.org
> >> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> >> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng;
> >> a...@rev.ng; quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> >> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> >> sidn...@quicinc.com; Brian Cain
> >> Subject: [PATCH 03/38] target/hexagon: Add System/Guest register
> >> definitions
> >>
> >> From: Brian Cain
> >>
> >> Signed-off-by: Brian Cain
> >> ---
> >> target/hexagon/gen_analyze_funcs.py | 21 +++-
> >> target/hexagon/hex_common.py| 163
> >>
> >> 2 files changed, 181 insertions(+), 3 deletions(-) diff --git
> >> a/target/hexagon/hex_common.py b/target/hexagon/hex_common.py
> index
> >> 758e5fd12d..db50defeb6 100755
> >> --- a/target/hexagon/hex_common.py
> >> +++ b/target/hexagon/hex_common.py
> >> @@ -33,6 +33,41 @@
> >> overrides = {} # tags with helper overrides idef_parser_enabled =
> >> {} # tags enabled for idef-parser
> >>
> >> +
> >> +def is_sysemu_tag(tag):
> >> +return "A_PRIV" in attribdict[tag] or "A_GUEST" in
> >> +attribdict[tag]
> >> +
> >> +
> >> +def tag_ignore(tag):
> >> +tag_skips = (
> >> +"Y6_diag",
> >> +"Y6_diag0",
> >> +"Y6_diag1",
> >> +)
> >> +attr_skips = (
> >> +"A_FAKEINSN",
> >> +"A_MAPPING",
> > Add A_CONDMAPPING to this list.
>
>
> Will do.
Great. Also, make sure tag_ignore is used by all the generator scripts. Then,
these won't show up any of the generated files (e.g.,
opcodes_def_generated.h.inc).
RE: [PATCH v3 3/5] target/hexagon: Add missing A_CALL attr, hintjumpr to multi_cof
> -Original Message-
> From: Brian Cain
> Sent: Tuesday, April 15, 2025 12:22 PM
> To: ltaylorsimp...@gmail.com; qemu-devel@nongnu.org
> Cc: richard.hender...@linaro.org; phi...@linaro.org;
> matheus.bernard...@oss.qualcomm.com; a...@rev.ng; a...@rev.ng;
> marco.lie...@oss.qualcomm.com; alex.ben...@linaro.org;
> quic_mbur...@quicinc.com; sidn...@quicinc.com
> Subject: Re: [PATCH v3 3/5] target/hexagon: Add missing A_CALL attr,
> hintjumpr to multi_cof
>
>
> On 4/14/2025 12:04 PM, ltaylorsimp...@gmail.com wrote:
> >
> >> -Original Message-
> >> From: Brian Cain
> >> Sent: Monday, April 7, 2025 1:27 PM
> >> To: qemu-devel@nongnu.org
> >> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> >> phi...@linaro.org; matheus.bernard...@oss.qualcomm.com;
> a...@rev.ng;
> >> a...@rev.ng; marco.lie...@oss.qualcomm.com;
> ltaylorsimp...@gmail.com;
> >> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com
> >> Subject: [PATCH v3 3/5] target/hexagon: Add missing A_CALL attr,
> >> hintjumpr to multi_cof
> >>
> >> Signed-off-by: Brian Cain
> >> ---
> >> target/hexagon/hex_common.py | 7 +--
> >> 1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/target/hexagon/hex_common.py
> >> b/target/hexagon/hex_common.py index 6803908718..a2dcb0aa2e
> 100755
> >> --- a/target/hexagon/hex_common.py
> >> +++ b/target/hexagon/hex_common.py
> >> @@ -247,8 +247,11 @@ def need_next_PC(tag):
> >>
> >>
> >> def need_pkt_has_multi_cof(tag):
> >> -return "A_COF" in attribdict[tag]
> >> -
> >> +return (
> >> +"A_JUMP" in attribdict[tag]
> >> +or "A_CALL" in attribdict[tag]
> >> +or "J2_rte" == tag
> >> +) and tag != "J2_hintjumpr"
> > It would be better to make this decision with instruction attributes only
> rather than a mix of attributes and specific tags. If needed, add another
> add_qemu_macro_attrib call to hex_common.calculate_attribs.
> >
> > Having said that, the correct tag for hintjumpr is J*4*_hintjumpr.
>
>
> Good catch, thanks for finding it. And I suppose we can change it to
> `"A_HINTJR" not in attribdict[tag]` instead.
>
>
> So, now more like this:
>
> add_qemu_macro_attrib('fREAD_SP', 'A_IMPLICIT_READS_SP')
> +add_qemu_macro_attrib('fCLEAR_RTE_EX', 'A_RTE')
>
> # Recurse down macros, find attributes from sub-macros
> macroValues = list(macros.values())
> @@ -291,8 +292,8 @@ def need_pkt_has_multi_cof(tag):
> return (
> "A_JUMP" in attribdict[tag]
> or "A_CALL" in attribdict[tag]
> -or "J2_rte" == tag
> -) and tag != "J2_hintjumpr"
> +or "A_RTE" in attribdict[tag]
> +) and "A_HINTJR" not in attribdict[tag]
Let's take a step back here. The goal is to eliminate the pkt_has_multi_cof
parameter from helpers that don't need it, right?
So, the first step is to change a check for A_COF to a check for A_JUMP or
A_CALL. Here are the opcodes where this distinction matters
J2_endloop* These have fGEN_TCG overrides so there is
no helper function.
J2_pauseDitto
J2_rte Ditto
J4_hintjumpr This is a nop in QEMU, and there is an
idef-parser emit_J4_hintjumpr function that doesn't generate any TCG.
When idef-parser is off, you
get a helper call, but you could easily override this with an empty fGEN_TCG.
J2_trap[01]These have helper functions, so we want
to return false because the helpers don't need this argument.
So the bottom line is you can add an A_TRAP attribute attached to the fTRAP
macro and then this function can be
return "A_COF" in attribdict[tag] and "A_TRAP" not in attribdict[tag]
HTH,
Taylor
RE: [PATCH 15/38] target/hexagon: Add handlers for guest/sysreg r/w
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:26 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 15/38] target/hexagon: Add handlers for guest/sysreg r/w
>
> From: Brian Cain
>
> This commit provides handlers to generate TCG for guest and system register
> reads and writes. They will be leveraged by a future commit.
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/genptr.c | 159
>
> 1 file changed, 159 insertions(+)
>
> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
> 2c5e15cfcf..488d0b4b97 100644
> --- a/target/hexagon/genptr.c
> +++ b/target/hexagon/genptr.c
> +G_GNUC_UNUSED
> +static void gen_read_greg(TCGv dst, int reg_num) {
> +gen_helper_greg_read(dst, tcg_env, tcg_constant_tl(reg_num)); }
> +
> +G_GNUC_UNUSED
> +static void gen_read_greg_pair(TCGv_i64 dst, int reg_num) {
> +gen_helper_greg_read_pair(dst, tcg_env, tcg_constant_tl(reg_num));
> +} #endif
> +
> +
This will work, but G'regs 0:3 could be read more efficiently by reading from
TCGv hex_greg rather than calling the helper.
Otherwise
Reviewed-by: Taylor Simpson
RE: [PATCH 12/38] target/hexagon: Add imported macro, attr defs for sysemu
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:26 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 12/38] target/hexagon: Add imported macro, attr defs for > sysemu > > From: Brian Cain > > Signed-off-by: Brian Cain > --- > target/hexagon/attribs_def.h.inc | 414 +++-- > target/hexagon/imported/macros.def | 558 > + > 2 files changed, 942 insertions(+), 30 deletions(-) mode change 100755 => > 100644 target/hexagon/imported/macros.def > > diff --git a/target/hexagon/attribs_def.h.inc > b/target/hexagon/attribs_def.h.inc > index 9e3a05f882..e6523a739b 100644 > --- a/target/hexagon/attribs_def.h.inc > +++ b/target/hexagon/attribs_def.h.inc > @@ -19,20 +19,41 @@ > DEF_ATTRIB(AA_DUMMY, "Dummy Zeroth Attribute", "", "") > > /* Misc */ > +DEF_ATTRIB(FAKEINSN, "Not a real instruction", "", "") > +DEF_ATTRIB(MAPPING, "Not real -- asm mapped", "", "") > +DEF_ATTRIB(CONDMAPPING, "Not real -- mapped based on values", "", "") > DEF_ATTRIB(EXTENSION, "Extension instruction", "", "") > +DEF_ATTRIB(SHARED_EXTENSION, "Shared extension instruction", "", "") > +DEF_ATTRIB(CABAC, > + "Cabac Instruction. Used in conjuction with QDSP6_CABAC_PRESENT", > "", > + "") > +DEF_ATTRIB(EXPERIMENTAL, "This may not work correctly not supported by > RTL.", > + "", "") Personally, I don't think we should be adding all of these. Few are needed, and we run the risk of having attributes that aren’t used in QEMU and therefore aren’t properly implemented in QEMU. Somewhere down the road, an instruction or macro could show up in the imported directory with such an attribute, and it will cause unnecessary headaches. Examples above are CONDMAPPING and EXPERIMENTAL. These should be included in hex_common.tag_ignore. Better to wait until an instruction in a future version of Hexagon shows up that uses an attribute. These will be few, so it will be simpler to examine each new attribute to ensure it is properly implemented in QEMU. > > /* access to implicit registers */ > DEF_ATTRIB(IMPLICIT_WRITES_LR, "Writes the link register", "", "UREG.LR") > +DEF_ATTRIB(IMPLICIT_READS_LR, "Reads the link register", "UREG.LR", "") > +DEF_ATTRIB(IMPLICIT_READS_LC0, "Reads loop count for loop 0", > +"UREG.LC0", "") DEF_ATTRIB(IMPLICIT_READS_LC1, "Reads loop count for > +loop 1", "UREG.LC1", "") DEF_ATTRIB(IMPLICIT_READS_SA0, "Reads start > +address for loop 0", "UREG.SA0", "") DEF_ATTRIB(IMPLICIT_READS_SA1, > +"Reads start address for loop 1", "UREG.SA1", "") > +DEF_ATTRIB(IMPLICIT_WRITES_PC, "Writes the program counter", "", > +"UREG.PC") DEF_ATTRIB(IMPLICIT_READS_PC, "Reads the program > counter", > +"UREG.PC", "") > DEF_ATTRIB(IMPLICIT_WRITES_SP, "Writes the stack pointer", "", > "UREG.SP") > +DEF_ATTRIB(IMPLICIT_READS_SP, "Reads the stack pointer", "UREG.SP", > "") > DEF_ATTRIB(IMPLICIT_WRITES_FP, "Writes the frame pointer", "", > "UREG.FP") > +DEF_ATTRIB(IMPLICIT_READS_FP, "Reads the frame pointer", "UREG.FP", > "") > +DEF_ATTRIB(IMPLICIT_WRITES_GP, "Writes the GP register", "", > "UREG.GP") > +DEF_ATTRIB(IMPLICIT_READS_GP, "Reads the GP register", "UREG.GP", "") > DEF_ATTRIB(IMPLICIT_WRITES_LC0, "Writes loop count for loop 0", "", > "UREG.LC0") DEF_ATTRIB(IMPLICIT_WRITES_LC1, "Writes loop count for > loop 1", "", "UREG.LC1") DEF_ATTRIB(IMPLICIT_WRITES_SA0, "Writes start > addr for loop 0", "", "UREG.SA0") DEF_ATTRIB(IMPLICIT_WRITES_SA1, > "Writes start addr for loop 1", "", "UREG.SA1") > +DEF_ATTRIB(IMPLICIT_WRITES_R00, "Writes Register 0", "", "UREG.R00") The IMPLICIT_READS_* and IMPLICIT_WRITES_* are examples that would need to be handled properly if ever used. Look at IMPLICIT_*_P0 to see how they are used in translate.c::analyze_packet. Imagine a day in the future when an instruction gets imported with IMPLICIT_WRITES_R00 attribute. When that instruction is in a packet with an instruction that reads R0, analyze_packet will not know there is a conflict and decide it's OK to short-circuit the packet semantics. That bug would go unnoticed for a long time and only show up when a large program runs incorrectly on QEMU. Thanks, Taylor
RE: [PATCH 14/38] target/hexagon: Add new macro definitions for sysemu
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:26 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 14/38] target/hexagon: Add new macro definitions for
> sysemu
>
> From: Brian Cain
>
> Also: add nop TCG overrides for break,unpause,fetchbo,dczeroa
dczeroa is modelled by QEMU. It writes zero's to the cache line.
>
> break: this hardware breakpoint instruction is used with the in-silicon
> debugger feature, this is not modeled.
>
> unpause: this instruction is used to resume hardware threads that are stalled
> by pause instructions. pause is modeled as a nop, or in RR mode as an
> EXCP_YIELD. This instruction is safe to ignore.
>
> Since cache/prefetch functions are not modeled, dczero and fetchbo are
> safe to ignore.
dczero is modelled.
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/gen_tcg.h| 9 ++
> target/hexagon/macros.h | 28 -
> target/hexagon/sys_macros.h | 238
>
> target/hexagon/op_helper.c | 1 +
> 4 files changed, 272 insertions(+), 4 deletions(-) create mode 100644
> target/hexagon/sys_macros.h
>
> diff --git a/target/hexagon/gen_tcg.h b/target/hexagon/gen_tcg.h index
> 8a3b801287..71f8a0e2d0 100644
> --- a/target/hexagon/gen_tcg.h
> +++ b/target/hexagon/gen_tcg.h
> @@ -488,6 +488,7 @@
>
> /* dczeroa clears the 32 byte cache line at the address given */ #define
> fGEN_TCG_Y2_dczeroa(SHORTCODE) SHORTCODE
> +#define fGEN_TCG_Y2_dczeroa_nt(SHORTCODE) SHORTCODE
Is there a Y2_dczeroa_nt instruction? If not, remove this.
> ctx->dczero_addr = tcg_temp_new(); \
> tcg_gen_mov_tl(ctx->dczero_addr, (REG)); \
> } while (0)
> +#else
> +#define fDCZEROA(REG) ((void) REG)
This isn't needed because all the instances of fDCZEROA are inside
QEMU_GENERATE.
> #endif
>
> diff --git a/target/hexagon/sys_macros.h b/target/hexagon/sys_macros.h
> new file mode 100644 index 00..3c4c3c7aa5
> --- /dev/null
> +++ b/target/hexagon/sys_macros.h
> +#define READ_SREG(NUM) arch_get_system_reg(env, NUM)
> +#define READ_SGP0()arch_get_system_reg(env, HEX_SREG_SGP0)
> +#define READ_SGP1()arch_get_system_reg(env, HEX_SREG_SGP1)
> +#define READ_SGP10() ((uint64_t)arch_get_system_reg(env,
> HEX_SREG_SGP0) | \
> +((uint64_t)arch_get_system_reg(env, HEX_SREG_SGP1) << 32))
> +
> +#define WRITE_SREG(NUM, VAL) log_sreg_write(env, NUM, VAL, slot)
> +#define WRITE_SGP0(VAL) log_sreg_write(env, HEX_SREG_SGP0,
> VAL, slot)
> +#define WRITE_SGP1(VAL) log_sreg_write(env, HEX_SREG_SGP1,
> VAL, slot)
> +#define WRITE_SGP10(VAL) \
> +do { \
> +log_sreg_write(env, HEX_SREG_SGP0, (VAL) & 0x, slot); \
> +log_sreg_write(env, HEX_SREG_SGP1, (VAL) >> 32, slot); \
> +} while (0)
> +
READ_SREG and WRITE_SREG look like a hangover for the original generator
scripts which have been rewritten. Are they needed?
> +#ifdef QEMU_GENERATE
> +#define GET_SSR_FIELD(RES, FIELD) \
> +GET_FIELD(RES, FIELD, hex_t_sreg[HEX_SREG_SSR]) #else
> +
> +#define GET_SSR_FIELD(FIELD, REGIN) \
> +(uint32_t)GET_FIELD(FIELD, REGIN)
> +#define GET_SYSCFG_FIELD(FIELD, REGIN) \
> +(uint32_t)GET_FIELD(FIELD, REGIN)
> +#define SET_SYSTEM_FIELD(ENV, REG, FIELD, VAL) \
> +do { \
> +uint32_t regval = arch_get_system_reg(ENV, REG); \
> +fINSERT_BITS(regval, reg_field_info[FIELD].width, \
> + reg_field_info[FIELD].offset, (VAL)); \
> +arch_set_system_reg(ENV, REG, regval); \
> +} while (0)
> +#define SET_SSR_FIELD(ENV, FIELD, VAL) \
> +SET_SYSTEM_FIELD(ENV, HEX_SREG_SSR, FIELD, VAL) #define
> +SET_SYSCFG_FIELD(ENV, FIELD, VAL) \
> +SET_SYSTEM_FIELD(ENV, HEX_SREG_SYSCFG, FIELD, VAL)
> +
> +#define CCR_FIELD_SET(ENV, FIELD) \
> +(!!GET_FIELD(FIELD, arch_get_system_reg(ENV, HEX_SREG_CCR)))
> +
> +/*
> + * Direct-to-guest is not implemented yet, continuing would cause
> +unexpected
> + * behavior, so we abort.
> + */
> +#define ASSERT_DIRECT_TO_GUEST_UNSET(ENV, EXCP) \
> +do { \
> +switch (EXCP) { \
> +case HEX_EVENT_TRAP0: \
> +g_assert(!CCR_FIELD_SET(ENV, CCR_GTE)); \
> +break; \
> +case HEX_EVENT_IMPRECISE: \
> +case HEX_EVENT_PRECISE: \
> +case HEX_EVENT_FPTRAP: \
> +g_assert(!CCR_FIELD_SET(ENV, CCR_GEE)); \
> +break; \
> +default: \
> +if ((EXCP) >= HEX_EVENT_INT0) { \
> +g_assert(!CCR_FIELD_SET(ENV, CCR_GIE)); \
> +} \
> +break; \
> +} \
> +} while (0)
> +#endif
> +
> +#define fREAD_ELR() (READ_SREG(HEX_SREG_ELR))
> +
> +#define fLOAD_PHYS(NUM,
RE: [PATCH 22/38] target/hexagon: Add sysemu TCG overrides
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:26 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 22/38] target/hexagon: Add sysemu TCG overrides
>
> From: Brian Cain
>
> Define TCG overrides for setprio(), crswap(,sgp{0,1,1:0}).
>
> Signed-off-by: Brian Cain
> ---
> target/hexagon/cpu_helper.h | 32
> target/hexagon/gen_tcg_sys.h | 41
>
> target/hexagon/helper.h | 1 +
> target/hexagon/cpu_helper.c | 36 +++
> target/hexagon/genptr.c | 4
> target/hexagon/op_helper.c | 7 ++
> target/hexagon/hex_common.py | 2 ++
> target/hexagon/meson.build | 14 ++--
> 8 files changed, 131 insertions(+), 6 deletions(-) create mode 100644
> target/hexagon/cpu_helper.h create mode 100644
> target/hexagon/gen_tcg_sys.h create mode 100644
> target/hexagon/cpu_helper.c
>
> diff --git a/target/hexagon/cpu_helper.h b/target/hexagon/cpu_helper.h
> new file mode 100644 index 00..194bcbf451
> --- /dev/null
> +++ b/target/hexagon/cpu_helper.h
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright(c) 2019-2025 Qualcomm Innovation Center, Inc. All Rights
> Reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef HEXAGON_CPU_HELPER_H
> +#define HEXAGON_CPU_HELPER_H
> +
> +static inline void arch_set_thread_reg(CPUHexagonState *env, uint32_t
> reg,
> + uint32_t val) {
> +g_assert(reg < TOTAL_PER_THREAD_REGS);
> +g_assert_not_reached();
> +}
> +
> +static inline uint32_t arch_get_thread_reg(CPUHexagonState *env,
> +uint32_t reg) {
> +g_assert(reg < TOTAL_PER_THREAD_REGS);
> +g_assert_not_reached();
> +}
> +
> +static inline void arch_set_system_reg(CPUHexagonState *env, uint32_t
> reg,
> + uint32_t val) {
> +g_assert_not_reached();
> +}
> +
> +uint32_t arch_get_system_reg(CPUHexagonState *env, uint32_t reg);
> +
> +#endif
> +
> diff --git a/target/hexagon/gen_tcg_sys.h b/target/hexagon/gen_tcg_sys.h
> new file mode 100644 index 00..362703ab45
> --- /dev/null
> +++ b/target/hexagon/gen_tcg_sys.h
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright(c) 2022-2025 Qualcomm Innovation Center, Inc. All Rights
> Reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef HEXAGON_GEN_TCG_SYS_H
> +#define HEXAGON_GEN_TCG_SYS_H
> +
> +#define fGEN_TCG_Y2_setprio(SHORTCODE) \
> +gen_helper_setprio(tcg_env, PtV, RsV)
> +
> +#define fGEN_TCG_Y2_crswap0(SHORTCODE) \
> +do { \
> +TCGv tmp = tcg_temp_new(); \
> +tcg_gen_mov_tl(tmp, RxV); \
> +tcg_gen_mov_tl(RxV, hex_t_sreg[HEX_SREG_SGP0]); \
> +tcg_gen_mov_tl(ctx->t_sreg_new_value[HEX_SREG_SGP0], tmp); \
> +} while (0)
> +
> +#define fGEN_TCG_Y4_crswap1(SHORTCODE) \
> +do { \
> +TCGv tmp = tcg_temp_new(); \
> +tcg_gen_mov_tl(tmp, RxV); \
> +tcg_gen_mov_tl(RxV, hex_t_sreg[HEX_SREG_SGP1]); \
> +tcg_gen_mov_tl(ctx->t_sreg_new_value[HEX_SREG_SGP1], tmp); \
> +} while (0)
> +
> +#define fGEN_TCG_Y4_crswap10(SHORTCODE) \
> +do { \
> +g_assert_not_reached(); \
> +TCGv_i64 tmp = tcg_temp_new_i64(); \
> +tcg_gen_mov_i64(tmp, RxxV); \
> +tcg_gen_concat_i32_i64(RxxV, \
> + hex_t_sreg[HEX_SREG_SGP0], \
> + hex_t_sreg[HEX_SREG_SGP1]); \
> +tcg_gen_extrl_i64_i32(ctx->t_sreg_new_value[HEX_SREG_SGP0],
> tmp); \
> +tcg_gen_extrh_i64_i32(ctx->t_sreg_new_value[HEX_SREG_SGP1],
> tmp); \
> +} while (0)
> +
> +#endif
> diff --git a/target/hexagon/helper.h b/target/hexagon/helper.h index
> fddbd99a19..146f4f02e4 100644
> --- a/target/hexagon/helper.h
> +++ b/target/hexagon/helper.h
> @@ -115,4 +115,5 @@ DEF_HELPER_2(greg_read, i32, env, i32)
> DEF_HELPER_2(greg_read_pair, i64, env, i32) DEF_HELPER_3(sreg_write,
> void, env, i32, i32) DEF_HELPER_3(sreg_write_pair, void, env, i32, i64)
> +DEF_HELPER_3(setprio, void, env, i32, i32)
> #endif
> diff --git a/target/hexagon/cpu_helper.c b/target/hexagon/cpu_helper.c
> new file mode 100644 index 00..6e4bc85580
> --- /dev/null
> +++ b/target/hexagon/cpu_helper.c
> @@ -0,0 +1,36 @@
> +/*
> + * Copyright(c) 2019-2025 Qualcomm Innovation Center, Inc. All Rights
> Reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "cpu_helper.h"
> +#include "system/cpus.h"
> +#ifdef CONFIG_USER_ONLY
> +#include "qemu.h"
> +#include "exec/helper-proto.h"
> +#else
> +#include "hw/boards.h"
> +#include "hw/hexagon/hexagon.h"
> +#endif
RE: [PATCH 24/38] target/hexagon: Add TCG overrides for int handler insts
> -Original Message-
> From: Brian Cain
> Sent: Friday, February 28, 2025 11:26 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com; Brian Cain
> Subject: [PATCH 24/38] target/hexagon: Add TCG overrides for int handler
> insts
>
> From: Brian Cain
>
> Define TCG overrides for {c,}swi {c,s}iad, iassign{r,w}, {s,g}etimask
> instructions.
>
> Signed-off-by: Brian Cain
Reviewed-by: Taylor Simpson
RE: [PATCH 25/38] target/hexagon: Add TCG overrides for thread ctl
> -Original Message- > From: Brian Cain > Sent: Friday, February 28, 2025 11:26 PM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 25/38] target/hexagon: Add TCG overrides for thread ctl > > From: Brian Cain > > Define TCG overrides for start, stop, wait, resume instructions. > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
