diff mbox series

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

Message ID f8b829443523568823d062adf8bf6659bc6d4a3f.1683552984.git.quic_mathbern@quicinc.com
State New
Headers show
Series [v2] Hexagon (decode): look for pkts with multiple insns at the same slot | expand

Commit Message

Matheus Tavares Bernardino May 8, 2023, 1:37 p.m. UTC 
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.

Co-authored-by: Taylor Simpson <tsimpson@quicinc.com>
Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
Changes in v2:
- Only call decode_set_slot_number() with !disas_only, fixing the -d
  in_asm case.  

v1: https://lore.kernel.org/qemu-devel/7a90f0925f182e56cf49ec3ec01484739fa2f174.1683226473.git.quic_mathbern@quicinc.com/

 target/hexagon/decode.c           | 30 +++++++++++++++++++++++++++---
 tests/tcg/hexagon/invalid-slots.c | 29 +++++++++++++++++++++++++++++
 tests/tcg/hexagon/Makefile.target | 11 +++++++++++
 3 files changed, 67 insertions(+), 3 deletions(-)
 create mode 100644 tests/tcg/hexagon/invalid-slots.c

Comments

Taylor Simpson May 9, 2023, 4:46 p.m. UTC | #1 
> -----Original Message-----
> From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Sent: Monday, May 8, 2023 8:37 AM
> To: qemu-devel@nongnu.org
> Cc: Taylor Simpson <tsimpson@quicinc.com>
> Subject: [PATCH v2] 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.
> 
> Co-authored-by: Taylor Simpson <tsimpson@quicinc.com>
> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
> Changes in v2:
> - Only call decode_set_slot_number() with !disas_only, fixing the -d
>   in_asm case.
> 
> v1: https://lore.kernel.org/qemu-
> devel/7a90f0925f182e56cf49ec3ec01484739fa2f174.1683226473.git.quic_mat
> hbern@quicinc.com/
> 
>  target/hexagon/decode.c           | 30 +++++++++++++++++++++++++++---
>  tests/tcg/hexagon/invalid-slots.c | 29 +++++++++++++++++++++++++++++
> tests/tcg/hexagon/Makefile.target | 11 +++++++++++
>  3 files changed, 67 insertions(+), 3 deletions(-)  create mode 100644
> tests/tcg/hexagon/invalid-slots.c
> 
> diff --git a/tests/tcg/hexagon/Makefile.target
> b/tests/tcg/hexagon/Makefile.target
> index 7c94db4bc4..0c69216c6c 100644
> --- a/tests/tcg/hexagon/Makefile.target
> +++ b/tests/tcg/hexagon/Makefile.target
> @@ -49,6 +49,17 @@ HEX_TESTS += vector_add_int  HEX_TESTS +=
> scatter_gather  HEX_TESTS += hvx_misc  HEX_TESTS += hvx_histogram
> +HEX_TESTS += invalid-slots
> +
> +run-and-check-exception = $(call run-test,$2,$3 2>$2.stderr; \
> +	test $$? -eq 1 && grep -q "exception $(strip $1)" $2.stderr)
> +
> +run-invalid-slots: invalid-slots
> +	$(call run-and-check-exception, 0x15, $@, $(QEMU) $(QEMU_OPTS)
> $<)
> +
> +run-plugin-invalid-slots-with-%: invalid-slots
> +	$(call run-and-check-exception, 0x15, $@, $(QEMU) $(QEMU_OPTS)
> \
> +	-plugin $(PLUGIN_LIB)/$(call extract-plugin,$@) $(call
> +strip-plugin,$<))

This isn't invoked and is missing some pieces.  I'll remove it from the PR.

Otherwise,
Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
Taylor Simpson May 9, 2023, 5:10 p.m. UTC | #2 
> -----Original Message-----
> From: Taylor Simpson
> Sent: Tuesday, May 9, 2023 11:46 AM
> To: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>; qemu-
> devel@nongnu.org
> Subject: RE: [PATCH v2] Hexagon (decode): look for pkts with multiple insns
> at the same slot
> 
> 
> 
> > -----Original Message-----
> > From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> > Sent: Monday, May 8, 2023 8:37 AM
> > To: qemu-devel@nongnu.org
> > Cc: Taylor Simpson <tsimpson@quicinc.com>
> > Subject: [PATCH v2] 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.
> >
> > Co-authored-by: Taylor Simpson <tsimpson@quicinc.com>
> > Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> > Signed-off-by: Matheus Tavares Bernardino
> <quic_mathbern@quicinc.com>
> > ---
> > Changes in v2:
> > - Only call decode_set_slot_number() with !disas_only, fixing the -d
> >   in_asm case.
> >
> > v1: https://lore.kernel.org/qemu-
> >
> devel/7a90f0925f182e56cf49ec3ec01484739fa2f174.1683226473.git.quic_mat
> > hbern@quicinc.com/
> >
> >  target/hexagon/decode.c           | 30 +++++++++++++++++++++++++++---
> >  tests/tcg/hexagon/invalid-slots.c | 29
> +++++++++++++++++++++++++++++
> > tests/tcg/hexagon/Makefile.target | 11 +++++++++++
> >  3 files changed, 67 insertions(+), 3 deletions(-)  create mode 100644
> > tests/tcg/hexagon/invalid-slots.c
> >
> > diff --git a/tests/tcg/hexagon/Makefile.target
> > b/tests/tcg/hexagon/Makefile.target
> > index 7c94db4bc4..0c69216c6c 100644
> > --- a/tests/tcg/hexagon/Makefile.target
> > +++ b/tests/tcg/hexagon/Makefile.target
> > @@ -49,6 +49,17 @@ HEX_TESTS += vector_add_int  HEX_TESTS +=
> > scatter_gather  HEX_TESTS += hvx_misc  HEX_TESTS += hvx_histogram
> > +HEX_TESTS += invalid-slots
> > +
> > +run-and-check-exception = $(call run-test,$2,$3 2>$2.stderr; \
> > +	test $$? -eq 1 && grep -q "exception $(strip $1)" $2.stderr)
> > +
> > +run-invalid-slots: invalid-slots
> > +	$(call run-and-check-exception, 0x15, $@, $(QEMU) $(QEMU_OPTS)
> > $<)
> > +
> > +run-plugin-invalid-slots-with-%: invalid-slots
> > +	$(call run-and-check-exception, 0x15, $@, $(QEMU) $(QEMU_OPTS)
> > \
> > +	-plugin $(PLUGIN_LIB)/$(call extract-plugin,$@) $(call
> > +strip-plugin,$<))
> 
> This isn't invoked and is missing some pieces.  I'll remove it from the PR.
> 
> Otherwise,
> Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>

Also
Tested-by: Taylor Simpson <tsimpson@quicinc.com>
diff mbox series

Patch

diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c
index 041c8de751..946c55cc71 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);
 }
 
 /*
@@ -961,8 +982,11 @@  int decode_packet(int max_words, const uint32_t *words, Packet *pkt,
     decode_apply_extenders(pkt);
     if (!disas_only) {
         decode_remove_extenders(pkt);
+        if (!decode_set_slot_number(pkt)) {
+            /* Invalid packet */
+            return 0;
+        }
     }
-    decode_set_slot_number(pkt);
     decode_fill_newvalue_regno(pkt);
 
     if (pkt->pkt_has_hvx) {
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;
+}
diff --git a/tests/tcg/hexagon/Makefile.target b/tests/tcg/hexagon/Makefile.target
index 7c94db4bc4..0c69216c6c 100644
--- a/tests/tcg/hexagon/Makefile.target
+++ b/tests/tcg/hexagon/Makefile.target
@@ -49,6 +49,17 @@  HEX_TESTS += vector_add_int
 HEX_TESTS += scatter_gather
 HEX_TESTS += hvx_misc
 HEX_TESTS += hvx_histogram
+HEX_TESTS += invalid-slots
+
+run-and-check-exception = $(call run-test,$2,$3 2>$2.stderr; \
+	test $$? -eq 1 && grep -q "exception $(strip $1)" $2.stderr)
+
+run-invalid-slots: invalid-slots
+	$(call run-and-check-exception, 0x15, $@, $(QEMU) $(QEMU_OPTS) $<)
+
+run-plugin-invalid-slots-with-%: invalid-slots
+	$(call run-and-check-exception, 0x15, $@, $(QEMU) $(QEMU_OPTS) \
+	-plugin $(PLUGIN_LIB)/$(call extract-plugin,$@) $(call strip-plugin,$<))
 
 HEX_TESTS += test_abs
 HEX_TESTS += test_bitcnt