Re: [kvm-unit-tests PATCH v2] x86: Do not assign values to unaligned pointer to 128 bits

From: Jacob Xu <jacobhxu@google.com>
To: Jim Mattson <jmattson@google.com>
Cc: Sean Christopherson <seanjc@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	kvm list <kvm@vger.kernel.org>
Subject: Re: [kvm-unit-tests PATCH v2] x86: Do not assign values to unaligned pointer to 128 bits
Date: Mon, 10 May 2021 18:47:49 -0700	[thread overview]
Message-ID: <CAJ5mJ6h4413=3go6kRkB0VcCdz2o6sgq_p91ZbcqWv-FZBd4hw@mail.gmail.com> (raw)
In-Reply-To: <CALMp9eQoscqr9p5ayzYkKXHNMcQthntJr_BJ+egEdriEQUqSTw@mail.gmail.com>

> The compiler does not
> have to discard what it can infer about the alignment just because you
> cast 'mem' to a type with weaker alignment constraints.
>
> Why does 'mem' need to have type 'sse_union *'? Why can't it just be
> declared as 'uint8_t *'?

Huh, I see. I'll just delete sse_union then and use uint32_t instead.

> Ewwww.  That's likely because emulator.c does:
>  #define memset __builtin_memset
> As evidenced by this issue, using the compiler's memset() in kvm-unit-tests seems
> inherently dangerous since the tests are often doing intentionally stupid things.

I'll make a separate patch to remove this from emulator.c

On Thu, May 6, 2021 at 1:11 PM Jim Mattson <jmattson@google.com> wrote:
>
> On Thu, May 6, 2021 at 12:14 PM Jacob Xu <jacobhxu@google.com> wrote:
> >
> > > memset() takes a void *, which it casts to an char, i.e. it works on one byte at
> > a time.
> > Huh, TIL. Based on this I'd thought that I don't need a cast at all,
> > but doing so actually results in a movaps instruction.
> > I've changed the cast back to (uint8_t *).
>
> I'm pretty sure you're just getting lucky. If 'mem' is not 16-byte
> aligned, the behavior of the code is undefined. The compiler does not
> have to discard what it can infer about the alignment just because you
> cast 'mem' to a type with weaker alignment constraints.
>
> Why does 'mem' need to have type 'sse_union *'? Why can't it just be
> declared as 'uint8_t *'? Just add a "memory" clobbers to the inline
> asm statements that use 'mem' as an SSE operand.
>
> Of course, passing it as an argument to sseeq() also implies 16-byte
> alignment. Perhaps sseeq should take uint32_t pointers as arguments
> rather than sse_union pointers. I'm not convinced that the sse_union
> buys us anything other than trouble.