| Message ID | 0fcd8293642c6324119fbbab44741164bcbd04fb.1673616964.git.quic_mathbern@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Hexagon: update and enforce hintjr slot constraints | expand |
> -----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
> -----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>
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)) {
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(-)