| Message ID | 20200928125859.734287-3-philmd@redhat.com |
|---|---|
| State | Accepted |
| Commit | 6a4757fe51a1c5ea31f33d8a83c03387302ac2d7 |
| Headers | show |
| Series | qemu/compiler: Remove unused special case code for GCC < 4.8 | expand |
On Mon, 28 Sep 2020 at 14:12, Philippe Mathieu-Daudé <philmd@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?) thanks -- PMM
Hi On Mon, Sep 28, 2020 at 5:49 PM Peter Maydell <peter.maydell@linaro.org> wrote: > On Mon, 28 Sep 2020 at 14:12, Philippe Mathieu-Daudé <philmd@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.lureau@redhat.com> -- Marc-André Lureau <div dir="ltr"><div dir="ltr">Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 28, 2020 at 5:49 PM Peter Maydell <<a href="mailto:peter.maydell@linaro.org">peter.maydell@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, 28 Sep 2020 at 14:12, Philippe Mathieu-Daudé <<a href="mailto:philmd@redhat.com" target="_blank">philmd@redhat.com</a>> wrote:<br> ><br> > Since commit efc6c070aca ("configure: Add a test for the<br> > minimum compiler version") the minimum compiler version<br> > required for GCC is 4.8, which has the GCC BZ#36793 bug fixed.<br> ><br> > We can safely remove the special case introduced in commit<br> > a281ebc11a6 ("virtio: add missing mb() on notification").<br> <br> > -/*<br> > - * We use GCC builtin if it's available, as that can use mfence on<br> > - * 32-bit as well, e.g. if built with -march=pentium-m. However, on<br> > - * i386 the spec is buggy, and the implementation followed it until<br> > - * 4.3 (<a href="http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36793" rel="noreferrer" target="_blank">http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36793</a>).<br> > - */<br> > -#if defined(__i386__) || defined(__x86_64__)<br> > -#if !QEMU_GNUC_PREREQ(4, 4)<br> > -#if defined __x86_64__<br> > -#define smp_mb() ({ asm volatile("mfence" ::: "memory"); (void)0; })<br> > -#else<br> > -#define smp_mb() ({ asm volatile("lock; addl $0,0(%%esp) " ::: "memory"); (void)0; })<br> > -#endif<br> > -#endif<br> > -#endif<br> <br> You need to be a bit cautious about removing QEMU_GNUC_PREREQ()<br> checks, because clang advertises itself as GCC 4.2 via the<br> __GNUC__ and __GNUC_MINOR__ macros that QEMU_GNUC_PREREQ()<br> tests, and so unless the code has explicitly handled clang<br> via a different ifdef or feature test clang will be using<br> the fallback codepath in cases like this. So you also need<br> to find out whether all our supported versions of clang<br> are OK with the gcc-4.4-and-up version of this code...<br> (I think that deleting this set of #ifs means we end up<br> with the "just use __sync_synchronize()" version of smp_mb()<br> later on in the header?)<br></blockquote><div><br></div><div>With clang 3.8 (xenial amd64) __ATOMIC_RELAXED is defined, so the chunk to remove which is x86-specific anyway, isn't reached.</div><div><br></div><div>Reviewed-by: Marc-André Lureau <<a href="mailto:marcandre.lureau@redhat.com">marcandre.lureau@redhat.com</a>> </div><br clear="all"></div><br>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h index c1d211a3519..8f4b3a80fbd 100644 --- a/include/qemu/atomic.h +++ b/include/qemu/atomic.h @@ -241,23 +241,6 @@ #else /* __ATOMIC_RELAXED */ -/* - * 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 - - #ifdef __alpha__ #define smp_read_barrier_depends() asm volatile("mb":::"memory") #endif
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"). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- include/qemu/atomic.h | 17 ----------------- 1 file changed, 17 deletions(-)