Re: [RFC PATCH] target/i386: avoid copying junk to extended ZMMReg fields

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 11 Apr 2022 at 15:56, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> When change b7711471f5 was made to alias XMMReg to ZMMReg for the
> purposes of easing the handling of AVX512 registers we unwittingly
> broke the SSE helpers which construct a temporary value on the stack
> before copying them out. To avoid this lets encode REG_WIDTH based on
> shift and convert the pointer indirection with an explicit memcpy.
>
> An incomplete sampling of the affected instructions seems to indicate
> the default behaviour for legacy SSE is "the upper bits (MAXVL-1:128)
> of the corresponding YMM register destination are unmodified."
>
> Fixes: b7711471f5 ("target-i386: make xmm_regs 512-bit wide")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/420
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/i386/ops_sse.h | 71 ++++++++++++++++++++++++-------------------
>  1 file changed, 40 insertions(+), 31 deletions(-)
>
> diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
> index 6f1fc174b3..adfb498a71 100644
> --- a/target/i386/ops_sse.h
> +++ b/target/i386/ops_sse.h
> @@ -28,6 +28,7 @@
>  #define L(n) MMX_L(n)
>  #define Q(n) MMX_Q(n)
>  #define SUFFIX _mmx
> +#define REG_WIDTH 8
>  #else
>  #define Reg ZMMReg
>  #define XMM_ONLY(...) __VA_ARGS__
> @@ -36,6 +37,7 @@
>  #define L(n) ZMM_L(n)
>  #define Q(n) ZMM_Q(n)
>  #define SUFFIX _xmm
> +#define REG_WIDTH 16
>  #endif
>
>  void glue(helper_psrlw, SUFFIX)(CPUX86State *env, Reg *d, Reg *s)
> @@ -516,7 +518,7 @@ void glue(helper_pshufw, SUFFIX)(Reg *d, Reg *s, int order)
>      r.W(1) = s->W((order >> 2) & 3);
>      r.W(2) = s->W((order >> 4) & 3);
>      r.W(3) = s->W((order >> 6) & 3);
> -    *d = r;
> +    memcpy(d, &r, REG_WIDTH);
>  }

Looking a bit more closely, this won't work on big-endian
hosts, because there we want to copy across the last 16
bytes of the struct, not the first 16. So I think we need
some more macro magic:

/*
 * Copy the relevant parts of a Reg value around. For the
 * SHIFT == 1 case these helpers operate only on the lower
 * 16 bytes of a 64 byte ZMMReg, so we must copy only those
 * so the guest-visible destination register has the top
 * bytes left untouched. For the SHIFT == 0 case we are
 * working with an MMXReg struct which is the correct size.
 * Note that we can't memcpy() here because that will do
 * the wrong thing on big-endian hosts.
 */
#if SHIFT == 0
#define COPY_REG(DEST, SRC) (DEST) = (SRC)
#else
#define COPY_REG(DEST, SRC) do { \
    (DEST).Q(0) = (SRC).Q(0);    \
    (DEST).Q(1) = (SRC).Q(1);    \
  } while (0)
#endif

and then use COPY_REG(*d, r);

(adjust syntax to taste, not compile tested).

We could probably try to write endian-specific flavours of
memcpy() invocation, but "do two 64-bit word copies" is what
the compiler would hopefully turn the memcpy into anyway :-)

thanks
-- PMM