Re: [PATCH 1/2] target/s390x: Use a 16-bit immediate in VREP

[David Hildenbrand]

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)) {

--
Cheers,

David / dhildenb