| Message ID | 20230807163459.849766-1-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] target/s390x: Use a 16-bit immediate in VREP | expand |
On 07.08.23 18:34, Ilya Leoshkevich wrote: > Unlike most other instructions that contain an immediate element index, > VREP's one is 16-bit, and not 4-bit. The code uses only 8 bits, so > using, e.g., 0x101 does not lead to a specification exception. > > Fix by checking all 16 bits. > > Cc: qemu-stable@nongnu.org Just curious, why stable? Are there valid programs that set invalid element size and they are fixed by this? LGTM Reviewed-by: David Hildenbrand <david@redhat.com> > Fixes: 28d08731b1d8 ("s390x/tcg: Implement VECTOR REPLICATE") > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > target/s390x/tcg/translate_vx.c.inc | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target/s390x/tcg/translate_vx.c.inc b/target/s390x/tcg/translate_vx.c.inc > index f8df121d3d3..a6d840d4065 100644 > --- a/target/s390x/tcg/translate_vx.c.inc > +++ b/target/s390x/tcg/translate_vx.c.inc > @@ -57,7 +57,7 @@ > #define FPF_LONG 3 > #define FPF_EXT 4 > > -static inline bool valid_vec_element(uint8_t enr, MemOp es) > +static inline bool valid_vec_element(uint16_t enr, MemOp es) > { > return !(enr & ~(NUM_VEC_ELEMENTS(es) - 1)); > } > @@ -964,7 +964,7 @@ static DisasJumpType op_vpdi(DisasContext *s, DisasOps *o) > > static DisasJumpType op_vrep(DisasContext *s, DisasOps *o) > { > - const uint8_t enr = get_field(s, i2); > + const uint16_t enr = get_field(s, i2); > const uint8_t es = get_field(s, m4); > > if (es > ES_64 || !valid_vec_element(enr, es)) {
On Mon, 2023-08-07 at 19:00 +0200, David Hildenbrand wrote: > On 07.08.23 18:34, Ilya Leoshkevich wrote: > > Unlike most other instructions that contain an immediate element > > index, > > VREP's one is 16-bit, and not 4-bit. The code uses only 8 bits, so > > using, e.g., 0x101 does not lead to a specification exception. > > > > Fix by checking all 16 bits. > > > > Cc: qemu-stable@nongnu.org > > Just curious, why stable? Are there valid programs that set invalid > element size and they are fixed by this? None that I know of, but I thought this was still nice to have, and at the same time small enough to not cause any trouble. > LGTM > > Reviewed-by: David Hildenbrand <david@redhat.com> [...] >
On 07.08.23 19:02, Ilya Leoshkevich wrote: > On Mon, 2023-08-07 at 19:00 +0200, David Hildenbrand wrote: >> On 07.08.23 18:34, Ilya Leoshkevich wrote: >>> Unlike most other instructions that contain an immediate element >>> index, >>> VREP's one is 16-bit, and not 4-bit. The code uses only 8 bits, so >>> using, e.g., 0x101 does not lead to a specification exception. >>> >>> Fix by checking all 16 bits. >>> >>> Cc: qemu-stable@nongnu.org >> >> Just curious, why stable? Are there valid programs that set invalid >> element size and they are fixed by this? > > None that I know of, but I thought this was still nice to have, and at > the same time small enough to not cause any trouble. Yes, I was just curious. From my recollection, we didn't backport all specification exception checks.
> On 07.08.23 19:02, Ilya Leoshkevich wrote: ..>> None that I know of, but I thought this was still nice to have, and at >> the same time small enough to not cause any trouble. Ilya, do you have an idea why your messages don't reach qemu-stable@? I see e.g. David's replies to yuor messages in qemu-stable@ - this way I know you sent something, and copy that message from qemu-devel@. For example, this thread is shown in the archive with your messages unavailable: https://mail.gnu.org/archive/html/qemu-stable/2023-08/threads.html#00118
On Mon, 2023-08-07 at 20:27 +0300, Michael Tokarev wrote: > > On 07.08.23 19:02, Ilya Leoshkevich wrote: > ..>> None that I know of, but I thought this was still nice to have, > and at > > > the same time small enough to not cause any trouble. > > Ilya, do you have an idea why your messages don't reach qemu-stable@? > > I see e.g. David's replies to yuor messages in qemu-stable@ - this > way > I know you sent something, and copy that message from qemu-devel@. > > For example, this thread is shown in the archive with your messages > unavailable: > > https://mail.gnu.org/archive/html/qemu-stable/2023-08/threads.html#00118 No, unfortunately I don't know. Everything looks normal on my end, qemu-stable@ is properly Cc:ed and I don't see any typos. I've sent a question about this to the mailing list owner.
diff --git a/target/s390x/tcg/translate_vx.c.inc b/target/s390x/tcg/translate_vx.c.inc index f8df121d3d3..a6d840d4065 100644 --- a/target/s390x/tcg/translate_vx.c.inc +++ b/target/s390x/tcg/translate_vx.c.inc @@ -57,7 +57,7 @@ #define FPF_LONG 3 #define FPF_EXT 4 -static inline bool valid_vec_element(uint8_t enr, MemOp es) +static inline bool valid_vec_element(uint16_t enr, MemOp es) { return !(enr & ~(NUM_VEC_ELEMENTS(es) - 1)); } @@ -964,7 +964,7 @@ static DisasJumpType op_vpdi(DisasContext *s, DisasOps *o) static DisasJumpType op_vrep(DisasContext *s, DisasOps *o) { - const uint8_t enr = get_field(s, i2); + const uint16_t enr = get_field(s, i2); const uint8_t es = get_field(s, m4); if (es > ES_64 || !valid_vec_element(enr, es)) {
Unlike most other instructions that contain an immediate element index, VREP's one is 16-bit, and not 4-bit. The code uses only 8 bits, so using, e.g., 0x101 does not lead to a specification exception. Fix by checking all 16 bits. Cc: qemu-stable@nongnu.org Fixes: 28d08731b1d8 ("s390x/tcg: Implement VECTOR REPLICATE") Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- target/s390x/tcg/translate_vx.c.inc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)