[PATCH 0/2] Hexagon: update and enforce hintjr slot constraints

[PATCH 0/2] Hexagon: update and enforce hintjr slot constraints

[Matheus Tavares Bernardino]

This small series basically fix an outdated hexagon slot rule for
instruction hintjr and makes the decode machinery more resilient against
any possible outdated slot constraints in the future.

Matheus Tavares Bernardino (2):
  Hexagon (iclass): update J4_hintjumpr slot constraints
  Hexagon (decode): look for pkts with multiple insns at the same slot

 target/hexagon/decode.c           | 30 +++++++++++++++++++++++++++---
 target/hexagon/iclass.c           |  4 +++-
 tests/tcg/hexagon/Makefile.target | 10 ++++++++++
 tests/tcg/hexagon/invalid_slots.c | 29 +++++++++++++++++++++++++++++
 4 files changed, 69 insertions(+), 4 deletions(-)
 create mode 100644 tests/tcg/hexagon/invalid_slots.c

-- 
2.37.2

[PATCH 1/2] Hexagon (iclass): update J4_hintjumpr slot constraints

[Matheus Tavares Bernardino]

The Hexagon PRM says that "The assembler automatically encodes
instructions in the packet in the proper order. In the binary encoding
of a packet, the instructions must be ordered from Slot 3 down to
Slot 0."

Prior to the architecture version v73, the slot constraints from
instruction "hintjr" only allowed it to be executed at slot 2.
With that in mind, consider the packet:

    {
        hintjr(r0)
        nop
        nop
        if (!p0) memd(r1+#0) = r1:0
    }

To satisfy the ordering rule quoted from the PRM, the assembler would,
thus, move one of the nops to the first position, so that it can be
assigned to slot 3 and the subsequent hintjr to slot 2.

However, since v73, hintjr can be executed at either slot 2 or 3. So
there is no need to reorder that packet and the assembler will encode it
as is. When QEMU tries to execute it, however, we end up hitting a
"misaliged store" exception because both the store and the hintjr will
be assigned to store 0, and some functions like `slot_is_predicated()`
expect the decode machinery to assign only one instruction per slot. In
particular, the mentioned function will traverse the packet until it
finds the first instruction at the desired slot which, for slot 0, will
be hintjr. Since hintjr is not predicated, the result is that we try to
execute the store regardless of the predicate. And because the predicate
is false, we had not previously loaded hex_store_addr[0] or
hex_store_width[0]. As a result, the store will decide de width based on
trash memory, causing it to be misaligned.

Update the slot constraints for hintjr so that QEMU can properly handle
such encodings.

Note: to avoid similar-but-not-identical issues in the future, we should
look for multiple instructions at the same slot during decoding time and
throw an invalid packet exception. That will be done in the subsequent
commit.

Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
 target/hexagon/iclass.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/hexagon/iclass.c b/target/hexagon/iclass.c
index 6091286993..081116fc4d 100644
--- a/target/hexagon/iclass.c
+++ b/target/hexagon/iclass.c
@@ -51,8 +51,10 @@ SlotMask find_iclass_slots(Opcode opcode, int itype)
         return SLOTS_0;
     } else if ((opcode == J2_trap0) ||
                (opcode == Y2_isync) ||
-               (opcode == J2_pause) || (opcode == J4_hintjumpr)) {
+               (opcode == J2_pause)) {
         return SLOTS_2;
+    } else if (opcode == J4_hintjumpr) {
+        return SLOTS_23;
     } else if (GET_ATTRIB(opcode, A_CRSLOT23)) {
         return SLOTS_23;
     } else if (GET_ATTRIB(opcode, A_RESTRICT_PREFERSLOT0)) {
-- 
2.37.2

[PATCH 2/2] Hexagon (decode): look for pkts with multiple insns at the same slot

[Matheus Tavares Bernardino]

Each slot in a packet can be assigned to at most one instruction.
Although the assembler generally ought to enforce this rule, we better
be safe than sorry and also do some check to properly throw an "invalid
packet" exception on wrong slot assignments.

This should also make it easier to debug possible future errors caused
by missing updates to `find_iclass_slots()` rules in
target/hexagon/iclass.c.

Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
 target/hexagon/decode.c           | 30 +++++++++++++++++++++++++++---
 tests/tcg/hexagon/Makefile.target | 10 ++++++++++
 tests/tcg/hexagon/invalid_slots.c | 29 +++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 3 deletions(-)
 create mode 100644 tests/tcg/hexagon/invalid_slots.c

diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c
index 041c8de751..65ebf516a5 100644
--- a/target/hexagon/decode.c
+++ b/target/hexagon/decode.c
@@ -1,5 +1,5 @@
 /*
- *  Copyright(c) 2019-2022 Qualcomm Innovation Center, Inc. All Rights Reserved.
+ *  Copyright(c) 2019-2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -797,7 +797,26 @@ static bool decode_parsebits_is_loopend(uint32_t encoding32)
     return bits == 0x2;
 }
 
-static void
+static bool has_valid_slot_assignment(Packet *pkt)
+{
+    int used_slots = 0;
+    for (int i = 0; i < pkt->num_insns; i++) {
+        int slot_mask;
+        Insn *insn = &pkt->insn[i];
+        if (decode_opcode_ends_loop(insn->opcode)) {
+            /* We overload slot 0 for endloop. */
+            continue;
+        }
+        slot_mask = 1 << insn->slot;
+        if (used_slots & slot_mask) {
+            return false;
+        }
+        used_slots |= slot_mask;
+    }
+    return true;
+}
+
+static bool
 decode_set_slot_number(Packet *pkt)
 {
     int slot;
@@ -886,6 +905,8 @@ decode_set_slot_number(Packet *pkt)
         /* Then push it to slot0 */
         pkt->insn[slot1_iidx].slot = 0;
     }
+
+    return has_valid_slot_assignment(pkt);
 }
 
 /*
@@ -962,7 +983,10 @@ int decode_packet(int max_words, const uint32_t *words, Packet *pkt,
     if (!disas_only) {
         decode_remove_extenders(pkt);
     }
-    decode_set_slot_number(pkt);
+    if (!decode_set_slot_number(pkt)) {
+        /* Invalid packet */
+        return 0;
+    }
     decode_fill_newvalue_regno(pkt);
 
     if (pkt->pkt_has_hvx) {
diff --git a/tests/tcg/hexagon/Makefile.target b/tests/tcg/hexagon/Makefile.target
index 18e6a5969e..062aef63c3 100644
--- a/tests/tcg/hexagon/Makefile.target
+++ b/tests/tcg/hexagon/Makefile.target
@@ -45,6 +45,16 @@ HEX_TESTS += fpstuff
 HEX_TESTS += overflow
 HEX_TESTS += signal_context
 HEX_TESTS += reg_mut
+HEX_TESTS += invalid_slots
+
+run-invalid_slots:
+	$(call run-test, $<, $(QEMU) $< 2> $<.stderr, $<); \
+	if [ $$? -ne 1 ] ; then \
+		return 1; \
+	fi
+	$(call quiet-command, \
+		grep -q "exception 0x15" $<.stderr, \
+		"GREP", "exception 0x15");
 
 HEX_TESTS += test_abs
 HEX_TESTS += test_bitcnt
diff --git a/tests/tcg/hexagon/invalid_slots.c b/tests/tcg/hexagon/invalid_slots.c
new file mode 100644
index 0000000000..366ce4f42f
--- /dev/null
+++ b/tests/tcg/hexagon/invalid_slots.c
@@ -0,0 +1,29 @@
+/*
+ *  Copyright(c) 2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+char mem[8] __attribute__((aligned(8)));
+
+int main()
+{
+    asm volatile(
+        "r0 = #mem\n"
+        /* Invalid packet (2 instructions at slot 0): */
+        ".word 0xa1804100\n" /* { memw(r0) = r1;      */
+        ".word 0x28032804\n" /*   r3 = #0; r4 = #0 }  */
+        : : : "r0", "r3", "r4", "memory");
+    return 0;
+}
-- 
2.37.2

RE: [PATCH 1/2] Hexagon (iclass): update J4_hintjumpr slot constraints

[Taylor Simpson]

> -----Original Message-----
> From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Sent: Friday, January 13, 2023 7:39 AM
> To: qemu-devel@nongnu.org
> Cc: Taylor Simpson <tsimpson@quicinc.com>; Brian Cain
> <bcain@quicinc.com>; richard.henderson@linaro.org
> Subject: [PATCH 1/2] Hexagon (iclass): update J4_hintjumpr slot constraints
> 
> The Hexagon PRM says that "The assembler automatically encodes
> instructions in the packet in the proper order. In the binary encoding of a
> packet, the instructions must be ordered from Slot 3 down to Slot 0."
> 
> Prior to the architecture version v73, the slot constraints from instruction
> "hintjr" only allowed it to be executed at slot 2.
> With that in mind, consider the packet:
> 
>     {
>         hintjr(r0)
>         nop
>         nop
>         if (!p0) memd(r1+#0) = r1:0
>     }
> 
> To satisfy the ordering rule quoted from the PRM, the assembler would,
> thus, move one of the nops to the first position, so that it can be assigned to
> slot 3 and the subsequent hintjr to slot 2.
> 
> However, since v73, hintjr can be executed at either slot 2 or 3. So there is no
> need to reorder that packet and the assembler will encode it as is. When
> QEMU tries to execute it, however, we end up hitting a "misaliged store"
> exception because both the store and the hintjr will be assigned to store 0,
> and some functions like `slot_is_predicated()` expect the decode machinery
> to assign only one instruction per slot. In particular, the mentioned function
> will traverse the packet until it finds the first instruction at the desired slot
> which, for slot 0, will be hintjr. Since hintjr is not predicated, the result is that
> we try to execute the store regardless of the predicate. And because the
> predicate is false, we had not previously loaded hex_store_addr[0] or
> hex_store_width[0]. As a result, the store will decide de width based on
> trash memory, causing it to be misaligned.
> 
> Update the slot constraints for hintjr so that QEMU can properly handle such
> encodings.
> 
> Note: to avoid similar-but-not-identical issues in the future, we should look
> for multiple instructions at the same slot during decoding time and throw an
> invalid packet exception. That will be done in the subsequent commit.
> 
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
>  target/hexagon/iclass.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Could you add a check-tcg test case?

Thanks,
Taylor

RE: [PATCH 2/2] Hexagon (decode): look for pkts with multiple insns at the same slot

[Taylor Simpson]

> -----Original Message-----
> From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Sent: Friday, January 13, 2023 7:39 AM
> To: qemu-devel@nongnu.org
> Cc: Taylor Simpson <tsimpson@quicinc.com>; Brian Cain
> <bcain@quicinc.com>; richard.henderson@linaro.org
> Subject: [PATCH 2/2] Hexagon (decode): look for pkts with multiple insns at
> the same slot
> 
> Each slot in a packet can be assigned to at most one instruction.
> Although the assembler generally ought to enforce this rule, we better be
> safe than sorry and also do some check to properly throw an "invalid packet"
> exception on wrong slot assignments.
> 
> This should also make it easier to debug possible future errors caused by
> missing updates to `find_iclass_slots()` rules in target/hexagon/iclass.c.
> 
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
>  target/hexagon/decode.c           | 30 +++++++++++++++++++++++++++---
>  tests/tcg/hexagon/Makefile.target | 10 ++++++++++
> tests/tcg/hexagon/invalid_slots.c | 29 +++++++++++++++++++++++++++++
>  3 files changed, 66 insertions(+), 3 deletions(-)  create mode 100644
> tests/tcg/hexagon/invalid_slots.c

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

RE: [PATCH 1/2] Hexagon (iclass): update J4_hintjumpr slot constraints

[Taylor Simpson]

> -----Original Message-----
> From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Sent: Friday, January 13, 2023 7:39 AM
> To: qemu-devel@nongnu.org
> Cc: Taylor Simpson <tsimpson@quicinc.com>; Brian Cain
> <bcain@quicinc.com>; richard.henderson@linaro.org
> Subject: [PATCH 1/2] Hexagon (iclass): update J4_hintjumpr slot constraints
> 
> The Hexagon PRM says that "The assembler automatically encodes
> instructions in the packet in the proper order. In the binary encoding of a
> packet, the instructions must be ordered from Slot 3 down to Slot 0."
> 
> Prior to the architecture version v73, the slot constraints from instruction
> "hintjr" only allowed it to be executed at slot 2.
> With that in mind, consider the packet:
> 
>     {
>         hintjr(r0)
>         nop
>         nop
>         if (!p0) memd(r1+#0) = r1:0
>     }
> 
> To satisfy the ordering rule quoted from the PRM, the assembler would,
> thus, move one of the nops to the first position, so that it can be assigned to
> slot 3 and the subsequent hintjr to slot 2.
> 
> However, since v73, hintjr can be executed at either slot 2 or 3. So there is no
> need to reorder that packet and the assembler will encode it as is. When
> QEMU tries to execute it, however, we end up hitting a "misaliged store"
> exception because both the store and the hintjr will be assigned to store 0,
> and some functions like `slot_is_predicated()` expect the decode machinery
> to assign only one instruction per slot. In particular, the mentioned function
> will traverse the packet until it finds the first instruction at the desired slot
> which, for slot 0, will be hintjr. Since hintjr is not predicated, the result is that
> we try to execute the store regardless of the predicate. And because the
> predicate is false, we had not previously loaded hex_store_addr[0] or
> hex_store_width[0]. As a result, the store will decide de width based on
> trash memory, causing it to be misaligned.
> 
> Update the slot constraints for hintjr so that QEMU can properly handle such
> encodings.
> 
> Note: to avoid similar-but-not-identical issues in the future, we should look
> for multiple instructions at the same slot during decoding time and throw an
> invalid packet exception. That will be done in the subsequent commit.
> 
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
>  target/hexagon/iclass.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/target/hexagon/iclass.c b/target/hexagon/iclass.c index
> 6091286993..081116fc4d 100644
> --- a/target/hexagon/iclass.c
> +++ b/target/hexagon/iclass.c
> @@ -51,8 +51,10 @@ SlotMask find_iclass_slots(Opcode opcode, int itype)
>          return SLOTS_0;
>      } else if ((opcode == J2_trap0) ||
>                 (opcode == Y2_isync) ||
> -               (opcode == J2_pause) || (opcode == J4_hintjumpr)) {
> +               (opcode == J2_pause)) {
>          return SLOTS_2;
> +    } else if (opcode == J4_hintjumpr) {
> +        return SLOTS_23;
>      } else if (GET_ATTRIB(opcode, A_CRSLOT23)) {
>          return SLOTS_23;
>      } else if (GET_ATTRIB(opcode, A_RESTRICT_PREFERSLOT0)) {

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