Bug 86997 - non-portable x86/x86_64 inline assembly
Summary: non-portable x86/x86_64 inline assembly
Status: RESOLVED FIXED
Alias: None
Product: Spice
Classification: Unclassified
Component: protocol (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Spice Bug List
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-04 09:18 UTC by Timo Teräs
Modified: 2018-01-18 23:27 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Use gcc builtin rather than asm for memory barriers (883 bytes, patch)
2014-12-04 10:22 UTC, Christophe Fergeau
Details | Splinter Review

Description Timo Teräs 2014-12-04 09:18:39 UTC
Both the protocol part (and the now removed client part) seem to have some x86/x86_64 specific inline assembly in it making the code unportable. After removing the client issue spice compiles, but e.g. qemu with spice enable still fails to compile due to protocol.

For reference the client patch I used is to just remove the debug breakpoint:
http://git.alpinelinux.org/cgit/aports/tree/main/spice/fix-non-x86-build.patch

The inline assembly protocol part is in spice-common/spice-protocol/spice/barrier.h.

Qemu build on ARM fails with:

  CC    hw/display/qxl.o
  CC    hw/display/qxl-logger.o
  CC    hw/display/qxl-render.o
{standard input}: Assembler messages:
{standard input}:1593: Error: bad instruction `lock'
{standard input}:1593: Error: bad instruction `addl $0,0(%rsp)'
{standard input}:1740: Error: bad instruction `lock'
{standard input}:1740: Error: bad instruction `addl $0,0(%rsp)'
{standard input}:3882: Error: bad instruction `lock'
{standard input}:3882: Error: bad instruction `addl $0,0(%rsp)'
  CC    hw/dma/puv3_dma.o
{standard input}:6349: Error: bad instruction `lock'
{standard input}:6349: Error: bad instruction `addl $0,0(%rsp)'
  CC    hw/dma/rc4030.o
{standard input}:9162: Error: bad instruction `lock'
{standard input}:9162: Error: bad instruction `addl $0,0(%rsp)'
{standard input}:10359: Error: bad instruction `lock'
{standard input}:10359: Error: bad instruction `addl $0,0(%rsp)'
{standard input}:10674: Error: bad instruction `lock'
{standard input}:10674: Error: bad instruction `addl $0,0(%rsp)'
/home/buildozer/aports/main/qemu/src/qemu-2.1.2/rules.mak:31: recipe for target 'hw/display/qxl.o' failed
make: *** [hw/display/qxl.o] Error 1
 
The bad instructions come from spice's barrier.h.
Comment 1 Timo Teräs 2014-12-04 09:29:20 UTC
Seems that memory barrier is the only thing needed. Consider using compiler built-ins when possible.

See:
http://en.wikipedia.org/wiki/Memory_ordering#Compiler_support_for_hardware_memory_barriers

So with gcc>=4.4 __sync_synchronize() would be preferred.
Comment 2 Christophe Fergeau 2014-12-04 10:22:16 UTC
Created attachment 110448 [details] [review]
Use gcc builtin rather than asm for memory barriers

I actually have that patch in my local spice-protocol copy which I never really tested...
Comment 3 Marc-Andre Lureau 2014-12-04 15:13:13 UTC
(In reply to Christophe Fergeau from comment #2)
> Created attachment 110448 [details] [review] [review]
> Use gcc builtin rather than asm for memory barriers
> 
> I actually have that patch in my local spice-protocol copy which I never
> really tested...

my understanding is that this is correct, I think we should try that in the coming upstream release. (if not there are a lot of mb defines in kernel arch/*/include/asm/barrier.h)

(there are possibly better version of the ring, apparently adapted from Tim Deegan and Andrew Warfield)
Comment 4 Frediano Ziglio 2018-01-18 23:27:46 UTC
We already use __sync_synchronize.
Probably the bug was not closed.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.