Re: [Qemu-devel] [PULL 16/16] tcg/i386: Use MOVDQA for TCG_TYPE_V128 load/store

[David Hildenbrand]

On 23.05.19 00:28, Richard Henderson wrote:
> This instruction raises #GP, aka SIGSEGV, if the effective address
> is not aligned to 16-bytes.
> 
> We have assertions in tcg-op-gvec.c that the offset from ENV is
> aligned, for vector types <= V128.  But the offset itself does not
> validate that the final pointer is aligned -- one must also remember
> to use the QEMU_ALIGNED() attribute on the vector member within ENV.
> 
> PowerPC Altivec has vector load/store instructions that silently
> discard the low 4 bits of the address, making alignment mistakes
> difficult to discover.  Aid that by making the most popular host
> visibly signal the error.
> 
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  tcg/i386/tcg-target.inc.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c
> index 6ec5e60448..c0443da4af 100644
> --- a/tcg/i386/tcg-target.inc.c
> +++ b/tcg/i386/tcg-target.inc.c
> @@ -1082,14 +1082,24 @@ static void tcg_out_ld(TCGContext *s, TCGType type, 
> TCGReg ret,
>          }
>          /* FALLTHRU */
>      case TCG_TYPE_V64:
> +        /* There is no instruction that can validate 8-byte alignment.  */
>          tcg_debug_assert(ret >= 16);
>          tcg_out_vex_modrm_offset(s, OPC_MOVQ_VqWq, ret, 0, arg1, arg2);
>          break;
>      case TCG_TYPE_V128:
> +        /*
> +         * The gvec infrastructure is asserts that v128 vector loads
> +         * and stores use a 16-byte aligned offset.  Validate that the
> +         * final pointer is aligned by using an insn that will SIGSEGV.
> +         */
>          tcg_debug_assert(ret >= 16);
> -        tcg_out_vex_modrm_offset(s, OPC_MOVDQU_VxWx, ret, 0, arg1, arg2);
> +        tcg_out_vex_modrm_offset(s, OPC_MOVDQA_VxWx, ret, 0, arg1, arg2);
>          break;
>      case TCG_TYPE_V256:
> +        /*
> +         * The gvec infrastructure only requires 16-byte alignment,
> +         * so here we must use an unaligned load.
> +         */
>          tcg_debug_assert(ret >= 16);
>          tcg_out_vex_modrm_offset(s, OPC_MOVDQU_VxWx | P_VEXL,
>                                   ret, 0, arg1, arg2);
> @@ -1117,14 +1127,24 @@ static void tcg_out_st(TCGContext *s, TCGType type, 
> TCGReg arg,
>          }
>          /* FALLTHRU */
>      case TCG_TYPE_V64:
> +        /* There is no instruction that can validate 8-byte alignment.  */
>          tcg_debug_assert(arg >= 16);
>          tcg_out_vex_modrm_offset(s, OPC_MOVQ_WqVq, arg, 0, arg1, arg2);
>          break;
>      case TCG_TYPE_V128:
> +        /*
> +         * The gvec infrastructure is asserts that v128 vector loads
> +         * and stores use a 16-byte aligned offset.  Validate that the
> +         * final pointer is aligned by using an insn that will SIGSEGV.
> +         */
>          tcg_debug_assert(arg >= 16);
> -        tcg_out_vex_modrm_offset(s, OPC_MOVDQU_WxVx, arg, 0, arg1, arg2);
> +        tcg_out_vex_modrm_offset(s, OPC_MOVDQA_WxVx, arg, 0, arg1, arg2);
>          break;
>      case TCG_TYPE_V256:
> +        /*
> +         * The gvec infrastructure only requires 16-byte alignment,
> +         * so here we must use an unaligned store.
> +         */
>          tcg_debug_assert(arg >= 16);
>          tcg_out_vex_modrm_offset(s, OPC_MOVDQU_WxVx | P_VEXL,
>                                   arg, 0, arg1, arg2);
> 

This is the problematic patch. Haven't looked into the details yet, so I
can't tell what's wrong. Maybe really an alignemnt issue?

-- 

Thanks,

David / dhildenb