Hi
On Mon, Sep 28, 2020 at 5:49 PM Peter Maydell <peter.mayd...@linaro.org>
wrote:
> On Mon, 28 Sep 2020 at 14:12, Philippe Mathieu-Daudé <phi...@redhat.com>
> wrote:
> >
> > Since commit efc6c070aca ("configure: Add a test for the
> > minimum compiler version") the minimum compiler version
> > required for GCC is 4.8, which has the GCC BZ#36793 bug fixed.
> >
> > We can safely remove the special case introduced in commit
> > a281ebc11a6 ("virtio: add missing mb() on notification").
>
> > -/*
> > - * We use GCC builtin if it's available, as that can use mfence on
> > - * 32-bit as well, e.g. if built with -march=pentium-m. However, on
> > - * i386 the spec is buggy, and the implementation followed it until
> > - * 4.3 (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36793).
> > - */
> > -#if defined(__i386__) || defined(__x86_64__)
> > -#if !QEMU_GNUC_PREREQ(4, 4)
> > -#if defined __x86_64__
> > -#define smp_mb() ({ asm volatile("mfence" ::: "memory"); (void)0; })
> > -#else
> > -#define smp_mb() ({ asm volatile("lock; addl $0,0(%%esp) " :::
> "memory"); (void)0; })
> > -#endif
> > -#endif
> > -#endif
>
> You need to be a bit cautious about removing QEMU_GNUC_PREREQ()
> checks, because clang advertises itself as GCC 4.2 via the
> __GNUC__ and __GNUC_MINOR__ macros that QEMU_GNUC_PREREQ()
> tests, and so unless the code has explicitly handled clang
> via a different ifdef or feature test clang will be using
> the fallback codepath in cases like this. So you also need
> to find out whether all our supported versions of clang
> are OK with the gcc-4.4-and-up version of this code...
> (I think that deleting this set of #ifs means we end up
> with the "just use __sync_synchronize()" version of smp_mb()
> later on in the header?)
>
With clang 3.8 (xenial amd64) __ATOMIC_RELAXED is defined, so the chunk to
remove which is x86-specific anyway, isn't reached.
Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
--
Marc-André Lureau
