All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
@ 2016-03-20 19:20 Sean Bruno
  2016-03-20 20:03 ` Peter Maydell
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Sean Bruno @ 2016-03-20 19:20 UTC (permalink / raw)
  To: QEMU Developers

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

aarch64 targets are now failing to build on i386 hosts due to missing
__atomic_load_8() calls since this commit:

https://github.com/qemu/qemu/commit/a0aa44b488b3601415d55041e4619aef5f3a4ba8#diff-c143d686899ae51d7b927d9c682e12fd

I'm unsure if Linux is disabling aarch64 targets for i386 hosts or if
this commit works "just fine" on Linux hosts right now, as it doesn't
work with clang or gcc.

More or less, the code in question ends up looking like this bit of
test code:

#include <stdio.h>
#include <sys/types.h>
#include <machine/atomic.h>

#define atomic_read(ptr)                          \
    ({                                            \
    typeof(*ptr) _val;                            \
     __atomic_load(ptr, &_val, __ATOMIC_RELAXED); \
    _val;                                         \
    })

int main ()
{
	int foo;
	int64_t foo64;

	atomic_read(&foo);
	atomic_read(&foo64);

	return(0);
}


This test code will manifest the same issue as the aarch64 target
building on FreeBSD i386 with the error:

undefined reference to `__atomic_load_8'
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQF8BAEBCgBmBQJW7vgAXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w
ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRCQUFENDYzMkU3MTIxREU4RDIwOTk3REQx
MjAxRUZDQTFFNzI3RTY0AAoJEBIB78oecn5kZsoH+gOHnQEZp967VbHTGFz+loK1
VnwdJC+fxTt/Una2awaztF5mOWcHPQ/yEtZUmsoBOKlynNxHAFEKQHTqA/0H+78g
J1Xyi28cXFZDB4Seuk2tBgpky1rq9xMTHkh3W79Dcn/g1Kpmf4ha06rZYcSITilK
pTOAHCKYTI5X8LW33NibY3avo/p4NSvbm7DTiLGtzR8xiGrtKKTF5UBQ0If3xvnp
VG4aCm9dt9G/KmYvGQAaZqXc3mV5hfVr1cxG++5zprH8CKHBck9uc8pm0o70qUb5
/uhwaQTMiJbRd642/vwUjZuXa4dZCeb3WLHHIz6pNO6HhBBxtXiSFHYTZ4fqdxI=
=t6x6
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
  2016-03-20 19:20 [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets Sean Bruno
@ 2016-03-20 20:03 ` Peter Maydell
  2016-03-21  9:11   ` Alex Bennée
  2016-03-21  9:06 ` Paolo Bonzini
  2016-04-06 16:11 ` Sean Bruno
  2 siblings, 1 reply; 16+ messages in thread
From: Peter Maydell @ 2016-03-20 20:03 UTC (permalink / raw)
  To: Sean Bruno; +Cc: Paolo Bonzini, Alex Bennée, QEMU Developers

On 20 March 2016 at 19:20, Sean Bruno <sbruno@freebsd.org> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> aarch64 targets are now failing to build on i386 hosts due to missing
> __atomic_load_8() calls since this commit:
>
> https://github.com/qemu/qemu/commit/a0aa44b488b3601415d55041e4619aef5f3a4ba8#diff-c143d686899ae51d7b927d9c682e12fd
>
> I'm unsure if Linux is disabling aarch64 targets for i386 hosts or if
> this commit works "just fine" on Linux hosts right now, as it doesn't
> work with clang or gcc.

I think it just works on most Linux 32-bit architectures because
the compiler support can inline a suitable atomic op (there
is one case where it doesn't, which I think is PPC32).

In any case, we mustn't use atomics on types larger
than the host pointer type, because it's not portable enough.
Paolo or Alex, can you have a look at this?

thanks
-- PMM

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
  2016-03-20 19:20 [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets Sean Bruno
  2016-03-20 20:03 ` Peter Maydell
@ 2016-03-21  9:06 ` Paolo Bonzini
  2016-03-21  9:35   ` Peter Maydell
  2016-04-06 16:11 ` Sean Bruno
  2 siblings, 1 reply; 16+ messages in thread
From: Paolo Bonzini @ 2016-03-21  9:06 UTC (permalink / raw)
  To: Sean Bruno, QEMU Developers



On 20/03/2016 20:20, Sean Bruno wrote:
> aarch64 targets are now failing to build on i386 hosts due to missing
> __atomic_load_8() calls since this commit:
> 
> https://github.com/qemu/qemu/commit/a0aa44b488b3601415d55041e4619aef5f3a4ba8#diff-c143d686899ae51d7b927d9c682e12fd
> 
> I'm unsure if Linux is disabling aarch64 targets for i386 hosts or if
> this commit works "just fine" on Linux hosts right now, as it doesn't
> work with clang or gcc.

Where is the relevant load in QEMU code?

x86 can actually do 64-bit atomic loads and stores through the FPU, but
I'm not sure about other 32-bit targets?

Paolo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
  2016-03-20 20:03 ` Peter Maydell
@ 2016-03-21  9:11   ` Alex Bennée
  2016-03-21 14:52     ` Sean Bruno
  0 siblings, 1 reply; 16+ messages in thread
From: Alex Bennée @ 2016-03-21  9:11 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Paolo Bonzini, Sean Bruno, QEMU Developers


Peter Maydell <peter.maydell@linaro.org> writes:

> On 20 March 2016 at 19:20, Sean Bruno <sbruno@freebsd.org> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA512
>>
>> aarch64 targets are now failing to build on i386 hosts due to missing
>> __atomic_load_8() calls since this commit:
>>
>> https://github.com/qemu/qemu/commit/a0aa44b488b3601415d55041e4619aef5f3a4ba8#diff-c143d686899ae51d7b927d9c682e12fd
>>
>> I'm unsure if Linux is disabling aarch64 targets for i386 hosts or if
>> this commit works "just fine" on Linux hosts right now, as it doesn't
>> work with clang or gcc.
>
> I think it just works on most Linux 32-bit architectures because
> the compiler support can inline a suitable atomic op (there
> is one case where it doesn't, which I think is PPC32).
>
> In any case, we mustn't use atomics on types larger
> than the host pointer type, because it's not portable enough.
> Paolo or Alex, can you have a look at this?

I'll get a BSD up and running and check. What is triggering the
__atomic_load_8 though?

>
> thanks
> -- PMM


--
Alex Bennée

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
  2016-03-21  9:06 ` Paolo Bonzini
@ 2016-03-21  9:35   ` Peter Maydell
  2016-03-21 11:23     ` Paolo Bonzini
  0 siblings, 1 reply; 16+ messages in thread
From: Peter Maydell @ 2016-03-21  9:35 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Sean Bruno, QEMU Developers

On 21 March 2016 at 09:06, Paolo Bonzini <pbonzini@redhat.com> wrote:
> x86 can actually do 64-bit atomic loads and stores through the FPU, but
> I'm not sure about other 32-bit targets?

As I say, ppc32 linux is one that can't -- see commit 8a5956ad6392f1155
for last time this came up.

Ideally we should put a compile-time assert in the atomic ops that
the data being operated on isn't larger than sizeof(void*) so that
this is a compile time error on all 32-bit hosts rather than just
a small subset of them.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
  2016-03-21  9:35   ` Peter Maydell
@ 2016-03-21 11:23     ` Paolo Bonzini
  2016-03-21 11:49       ` Alex Bennée
  0 siblings, 1 reply; 16+ messages in thread
From: Paolo Bonzini @ 2016-03-21 11:23 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Sean Bruno, QEMU Developers



On 21/03/2016 10:35, Peter Maydell wrote:
> > x86 can actually do 64-bit atomic loads and stores through the FPU, but
> > I'm not sure about other 32-bit targets?
> 
> As I say, ppc32 linux is one that can't -- see commit 8a5956ad6392f1155
> for last time this came up.

Yes, I remember that.  Loads and stores are different from RMW
operations, though.

> Ideally we should put a compile-time assert in the atomic ops that
> the data being operated on isn't larger than sizeof(void*) so that
> this is a compile time error on all 32-bit hosts rather than just
> a small subset of them.

Yes, I will do it if Alex doesn't beat me to it.

Paolo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
  2016-03-21 11:23     ` Paolo Bonzini
@ 2016-03-21 11:49       ` Alex Bennée
  0 siblings, 0 replies; 16+ messages in thread
From: Alex Bennée @ 2016-03-21 11:49 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Peter Maydell, Sean Bruno, QEMU Developers


Paolo Bonzini <pbonzini@redhat.com> writes:

> On 21/03/2016 10:35, Peter Maydell wrote:
>> > x86 can actually do 64-bit atomic loads and stores through the FPU, but
>> > I'm not sure about other 32-bit targets?
>>
>> As I say, ppc32 linux is one that can't -- see commit 8a5956ad6392f1155
>> for last time this came up.
>
> Yes, I remember that.  Loads and stores are different from RMW
> operations, though.
>
>> Ideally we should put a compile-time assert in the atomic ops that
>> the data being operated on isn't larger than sizeof(void*) so that
>> this is a compile time error on all 32-bit hosts rather than just
>> a small subset of them.
>
> Yes, I will do it if Alex doesn't beat me to it.

I'm working on it now.

 - fix build failures on 32bit hosts (Linux/BSD)
 - add compile-time assert for sizeof <= sizeof (void*)

Anything else?

>
> Paolo


--
Alex Bennée

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
  2016-03-21  9:11   ` Alex Bennée
@ 2016-03-21 14:52     ` Sean Bruno
  2016-03-21 15:36       ` Alex Bennée
  0 siblings, 1 reply; 16+ messages in thread
From: Sean Bruno @ 2016-03-21 14:52 UTC (permalink / raw)
  To: Alex Bennée, Peter Maydell; +Cc: Paolo Bonzini, QEMU Developers

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512



On 03/21/16 02:11, Alex Bennée wrote:
> 
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On 20 March 2016 at 19:20, Sean Bruno <sbruno@freebsd.org>
>> wrote:
>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512
>>> 
>>> aarch64 targets are now failing to build on i386 hosts due to
>>> missing __atomic_load_8() calls since this commit:
>>> 
>>> https://github.com/qemu/qemu/commit/a0aa44b488b3601415d55041e4619aef5f3a4ba8#diff-c143d686899ae51d7b927d9c682e12fd
>>>
>>>
>>> 
I'm unsure if Linux is disabling aarch64 targets for i386 hosts or if
>>> this commit works "just fine" on Linux hosts right now, as it
>>> doesn't work with clang or gcc.
>> 
>> I think it just works on most Linux 32-bit architectures because 
>> the compiler support can inline a suitable atomic op (there is
>> one case where it doesn't, which I think is PPC32).
>> 
>> In any case, we mustn't use atomics on types larger than the host
>> pointer type, because it's not portable enough. Paolo or Alex,
>> can you have a look at this?
> 
> I'll get a BSD up and running and check. What is triggering the 
> __atomic_load_8 though?
> 
>> 

Specifically, its the atomic_read of vm_clock_warp_start in cpus.c:

 114 /***********************************************************/
 115 /* guest cycle counter */
 116
 117 /* Protected by TimersState seqlock */
 118
 119 static bool icount_sleep = true;
 120 static int64_t vm_clock_warp_start = -1;
 121 /* Conversion factor from emulated instructions to virtual clock
ticks.  */
 122 static int icount_time_shift;


....

 339 static void icount_warp_rt(void)
 340 {
 341     /* The icount_warp_timer is rescheduled soon after
vm_clock_warp_start
 342      * changes from -1 to another value, so the race here is okay.
 343      */
 344     if (atomic_read(&vm_clock_warp_start) == -1) {
 345         return;
 346     }
 347

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQF8BAEBCgBmBQJW8Aq6XxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w
ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRCQUFENDYzMkU3MTIxREU4RDIwOTk3REQx
MjAxRUZDQTFFNzI3RTY0AAoJEBIB78oecn5kktIH+QHD8muIYbAPzJcBMIFv9zIR
tv1OhxHgrWgmxnfzxeXwNkG5JkIrFKkJd6HoA4we+qrQu2Fi9ZCxz/cmFryNzXuT
CQPGcEaKQvAVT9623UVtJkvKv1lFzpclX6wQKkAN1GtnoaoXmOntCoPQFR2/cha3
0YeLM/26nlKr6kcWbX5a5hplKnsPA7IY0lohWGp/qziZKuFQ7GWsM6IkyLoXRn+T
T8Qw7NM40/iIvqYd7NfQlDlGa1GZo8mvsRvvF1C6YEkAmU9TkX0LfgGuIpiYmvYb
gHEJ6JcXRQhGQXWkGu3Znw4JzckjfPrg0/SMIsk4kMVNXunJ89Il4xT8wtFXFYw=
=Lehi
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
  2016-03-21 14:52     ` Sean Bruno
@ 2016-03-21 15:36       ` Alex Bennée
  2016-03-21 15:56         ` Paolo Bonzini
  0 siblings, 1 reply; 16+ messages in thread
From: Alex Bennée @ 2016-03-21 15:36 UTC (permalink / raw)
  To: Sean Bruno; +Cc: Peter Maydell, QEMU Developers, Paolo Bonzini


Sean Bruno <sbruno@freebsd.org> writes:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
>
>
> On 03/21/16 02:11, Alex Bennée wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 20 March 2016 at 19:20, Sean Bruno <sbruno@freebsd.org>
>>> wrote:
>>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512
>>>>
>>>> aarch64 targets are now failing to build on i386 hosts due to
>>>> missing __atomic_load_8() calls since this commit:
>>>>
>>>> https://github.com/qemu/qemu/commit/a0aa44b488b3601415d55041e4619aef5f3a4ba8#diff-c143d686899ae51d7b927d9c682e12fd
>>>>
>>>>
>>>>
> I'm unsure if Linux is disabling aarch64 targets for i386 hosts or if
>>>> this commit works "just fine" on Linux hosts right now, as it
>>>> doesn't work with clang or gcc.
>>>
>>> I think it just works on most Linux 32-bit architectures because
>>> the compiler support can inline a suitable atomic op (there is
>>> one case where it doesn't, which I think is PPC32).
>>>
>>> In any case, we mustn't use atomics on types larger than the host
>>> pointer type, because it's not portable enough. Paolo or Alex,
>>> can you have a look at this?
>>
>> I'll get a BSD up and running and check. What is triggering the
>> __atomic_load_8 though?
>>
>>>
>
> Specifically, its the atomic_read of vm_clock_warp_start in cpus.c:
>
>  114 /***********************************************************/
>  115 /* guest cycle counter */
>  116
>  117 /* Protected by TimersState seqlock */
>  118
>  119 static bool icount_sleep = true;
>  120 static int64_t vm_clock_warp_start = -1;
>  121 /* Conversion factor from emulated instructions to virtual clock
> ticks.  */
>  122 static int icount_time_shift;
>
>
> ....
>
>  339 static void icount_warp_rt(void)
>  340 {
>  341     /* The icount_warp_timer is rescheduled soon after
> vm_clock_warp_start
>  342      * changes from -1 to another value, so the race here is okay.
>  343      */
>  344     if (atomic_read(&vm_clock_warp_start) == -1) {
>  345         return;
>  346     }
>  347

Odd, the comments say that vm_clock_warp start is protected by the
seqlock, and in fact every other access to it is a plain access. It
seems to me the code should probably just be:

    seqlock_write_lock(&timers_state.vm_clock_seqlock);
    if (vm_clock_warp_start !== -1 && runstate_is_running()) {
      .. do stuff ..
    }
    vm_clock_warp_start = -1;
    seqlock_write_unlock(&timers_state.vm_clock_seqlock);

    if (we_did_stuff && qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) {
        qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
    }

Paolo, does that seem sane to you?

--
Alex Bennée

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
  2016-03-21 15:36       ` Alex Bennée
@ 2016-03-21 15:56         ` Paolo Bonzini
  2016-03-22 16:10           ` Sean Bruno
  0 siblings, 1 reply; 16+ messages in thread
From: Paolo Bonzini @ 2016-03-21 15:56 UTC (permalink / raw)
  To: Alex Bennée, Sean Bruno; +Cc: Peter Maydell, QEMU Developers



On 21/03/2016 16:36, Alex Bennée wrote:
> >  341     /* The icount_warp_timer is rescheduled soon after vm_clock_warp_start
> >  342      * changes from -1 to another value, so the race here is okay.
> >  343      */
> >  344     if (atomic_read(&vm_clock_warp_start) == -1) {
> >  345         return;
> >  346     }
> >  347
> Odd, the comments say that vm_clock_warp start is protected by the
> seqlock, and in fact every other access to it is a plain access.

Yes, the comment says why this is safe.

The change from -1 to positive is here:

        if (vm_clock_warp_start == -1 || vm_clock_warp_start > clock) {
            vm_clock_warp_start = clock;
        }
        seqlock_write_unlock(&timers_state.vm_clock_seqlock);
        timer_mod_anticipate(icount_warp_timer, clock + deadline);

If we get a race we must be like this:

	icount_warp_rt           qemu_start_warp_timer
	--------------           ---------------------
	read -1
				 write to vm_clock_warp_start
				 unlock
				 timer_mod_anticipate (*)

As soon as you reach (*) the timer is rescheduled and will read a value
other than -1.

> It seems to me the code should probably just be:
> 
>     seqlock_write_lock(&timers_state.vm_clock_seqlock);
>     if (vm_clock_warp_start !== -1 && runstate_is_running()) {
>       .. do stuff ..
>     }
>     vm_clock_warp_start = -1;
>     seqlock_write_unlock(&timers_state.vm_clock_seqlock);
> 
>     if (we_did_stuff && qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) {
>         qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
>     }

Yes, you can make it like that, or even better wrap the read with a
seqlock_read_begin/seqlock_read_retry loop.  The condition will often be
false and it's pointless to lock/unlock the mutex for that.

Paolo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
  2016-03-21 15:56         ` Paolo Bonzini
@ 2016-03-22 16:10           ` Sean Bruno
  2016-03-22 16:22             ` Alex Bennée
  0 siblings, 1 reply; 16+ messages in thread
From: Sean Bruno @ 2016-03-22 16:10 UTC (permalink / raw)
  To: Paolo Bonzini, Alex Bennée; +Cc: Peter Maydell, QEMU Developers

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512



On 03/21/16 08:56, Paolo Bonzini wrote:
> 
> 
> On 21/03/2016 16:36, Alex Bennée wrote:
>>> 341     /* The icount_warp_timer is rescheduled soon after
>>> vm_clock_warp_start 342      * changes from -1 to another
>>> value, so the race here is okay. 343      */ 344     if
>>> (atomic_read(&vm_clock_warp_start) == -1) { 345
>>> return; 346     } 347
>> Odd, the comments say that vm_clock_warp start is protected by
>> the seqlock, and in fact every other access to it is a plain
>> access.
> 
> Yes, the comment says why this is safe.
> 
> The change from -1 to positive is here:
> 
> if (vm_clock_warp_start == -1 || vm_clock_warp_start > clock) { 
> vm_clock_warp_start = clock; } 
> seqlock_write_unlock(&timers_state.vm_clock_seqlock); 
> timer_mod_anticipate(icount_warp_timer, clock + deadline);
> 
> If we get a race we must be like this:
> 
> icount_warp_rt           qemu_start_warp_timer --------------
> --------------------- read -1 write to vm_clock_warp_start unlock 
> timer_mod_anticipate (*)
> 
> As soon as you reach (*) the timer is rescheduled and will read a
> value other than -1.
> 
>> It seems to me the code should probably just be:
>> 
>> seqlock_write_lock(&timers_state.vm_clock_seqlock); if
>> (vm_clock_warp_start !== -1 && runstate_is_running()) { .. do
>> stuff .. } vm_clock_warp_start = -1; 
>> seqlock_write_unlock(&timers_state.vm_clock_seqlock);
>> 
>> if (we_did_stuff && qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) { 
>> qemu_clock_notify(QEMU_CLOCK_VIRTUAL); }
> 
> Yes, you can make it like that, or even better wrap the read with
> a seqlock_read_begin/seqlock_read_retry loop.  The condition will
> often be false and it's pointless to lock/unlock the mutex for
> that.
> 
> Paolo
> 
> 


Alex:

Do you want me to work up a patch for this or are you dealing with it?

sean
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQF8BAEBCgBmBQJW8W6IXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w
ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRCQUFENDYzMkU3MTIxREU4RDIwOTk3REQx
MjAxRUZDQTFFNzI3RTY0AAoJEBIB78oecn5kCPMH/3dbvHDfw5fxY8AX3mBoEsby
10+JEG8u2HojS37h8xx+gxV5ZI9xFUMVQ8niM5EdCrMUz1YeH3oyVYu6LBrlCm9T
vlsG0huOXkNuVP+++nAVGr/bPm08IIUBYi1MNVSOZ02MxLgeWblsF4Z9slX5KrH2
FS49z7vDZYeJF2OWxBF/PHvFomNiqfOnsKelh8cWX6FDIubBSrz2dmHvKUu7Jumw
EKeZlHP2G+QiuqdUl4t0zvrBYo15IGLUGXNZrp0zgEd2usieA80I1Vb3JQCfrFL1
GFZo7j86W1iXxGTwI//rM52bXAskk8HovtgtkwRbKG18SGpZJzhjqwQAFsDyuUU=
=p9Xu
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
  2016-03-22 16:10           ` Sean Bruno
@ 2016-03-22 16:22             ` Alex Bennée
  2016-03-28 17:00               ` Sean Bruno
  2016-03-31 16:57               ` Sean Bruno
  0 siblings, 2 replies; 16+ messages in thread
From: Alex Bennée @ 2016-03-22 16:22 UTC (permalink / raw)
  To: Sean Bruno; +Cc: Paolo Bonzini, QEMU Developers, Peter Maydell


Sean Bruno <sbruno@freebsd.org> writes:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
>
>
> On 03/21/16 08:56, Paolo Bonzini wrote:
>>
>>
>> On 21/03/2016 16:36, Alex Bennée wrote:
>>>> 341     /* The icount_warp_timer is rescheduled soon after
>>>> vm_clock_warp_start 342      * changes from -1 to another
>>>> value, so the race here is okay. 343      */ 344     if
>>>> (atomic_read(&vm_clock_warp_start) == -1) { 345
>>>> return; 346     } 347
>>> Odd, the comments say that vm_clock_warp start is protected by
>>> the seqlock, and in fact every other access to it is a plain
>>> access.
>>
>> Yes, the comment says why this is safe.
>>
>> The change from -1 to positive is here:
>>
>> if (vm_clock_warp_start == -1 || vm_clock_warp_start > clock) {
>> vm_clock_warp_start = clock; }
>> seqlock_write_unlock(&timers_state.vm_clock_seqlock);
>> timer_mod_anticipate(icount_warp_timer, clock + deadline);
>>
>> If we get a race we must be like this:
>>
>> icount_warp_rt           qemu_start_warp_timer --------------
>> --------------------- read -1 write to vm_clock_warp_start unlock
>> timer_mod_anticipate (*)
>>
>> As soon as you reach (*) the timer is rescheduled and will read a
>> value other than -1.
>>
>>> It seems to me the code should probably just be:
>>>
>>> seqlock_write_lock(&timers_state.vm_clock_seqlock); if
>>> (vm_clock_warp_start !== -1 && runstate_is_running()) { .. do
>>> stuff .. } vm_clock_warp_start = -1;
>>> seqlock_write_unlock(&timers_state.vm_clock_seqlock);
>>>
>>> if (we_did_stuff && qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) {
>>> qemu_clock_notify(QEMU_CLOCK_VIRTUAL); }
>>
>> Yes, you can make it like that, or even better wrap the read with
>> a seqlock_read_begin/seqlock_read_retry loop.  The condition will
>> often be false and it's pointless to lock/unlock the mutex for
>> that.
>>
>> Paolo
>>
>>
>
>
> Alex:
>
> Do you want me to work up a patch for this or are you dealing with it?

I wasn't going to look at it for a few days (other things on my queue)
so if you have time to re-write with the seqlock_read_* loop please be
my guest.

>
> sean
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQF8BAEBCgBmBQJW8W6IXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w
> ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRCQUFENDYzMkU3MTIxREU4RDIwOTk3REQx
> MjAxRUZDQTFFNzI3RTY0AAoJEBIB78oecn5kCPMH/3dbvHDfw5fxY8AX3mBoEsby
> 10+JEG8u2HojS37h8xx+gxV5ZI9xFUMVQ8niM5EdCrMUz1YeH3oyVYu6LBrlCm9T
> vlsG0huOXkNuVP+++nAVGr/bPm08IIUBYi1MNVSOZ02MxLgeWblsF4Z9slX5KrH2
> FS49z7vDZYeJF2OWxBF/PHvFomNiqfOnsKelh8cWX6FDIubBSrz2dmHvKUu7Jumw
> EKeZlHP2G+QiuqdUl4t0zvrBYo15IGLUGXNZrp0zgEd2usieA80I1Vb3JQCfrFL1
> GFZo7j86W1iXxGTwI//rM52bXAskk8HovtgtkwRbKG18SGpZJzhjqwQAFsDyuUU=
> =p9Xu
> -----END PGP SIGNATURE-----


--
Alex Bennée

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
  2016-03-22 16:22             ` Alex Bennée
@ 2016-03-28 17:00               ` Sean Bruno
  2016-03-31 16:57               ` Sean Bruno
  1 sibling, 0 replies; 16+ messages in thread
From: Sean Bruno @ 2016-03-28 17:00 UTC (permalink / raw)
  To: Alex Bennée; +Cc: Paolo Bonzini, QEMU Developers, Peter Maydell

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

This seems to at least allow things to compile, qemu-system seems to
be able to boot images on i386, but I may have missed something:

diff --git a/cpus.c b/cpus.c
index 23cf7aa..11f8bab 100644
- --- a/cpus.c
+++ b/cpus.c
@@ -338,15 +338,8 @@ static int64_t qemu_icount_round(int64_t count)

 static void icount_warp_rt(void)
 {
- -    /* The icount_warp_timer is rescheduled soon after
vm_clock_warp_start
- -     * changes from -1 to another value, so the race here is okay.
- -     */
- -    if (atomic_read(&vm_clock_warp_start) == -1) {
- -        return;
- -    }
- -
     seqlock_write_lock(&timers_state.vm_clock_seqlock);
- -    if (runstate_is_running()) {
+    if ((vm_clock_warp_start != -1) && runstate_is_running()) {
         int64_t clock = REPLAY_CLOCK(REPLAY_CLOCK_VIRTUAL_RT,
                                      cpu_get_clock_locked());
         int64_t warp_delta;
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQF8BAEBCgBmBQJW+WNBXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w
ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRCQUFENDYzMkU3MTIxREU4RDIwOTk3REQx
MjAxRUZDQTFFNzI3RTY0AAoJEBIB78oecn5kyMwH/0Gq6gHKhjtwSCInMNJgG0hv
oAZmv0/2p0U1S3Kp84ALRU90z3K7nwKjShw7uBK6On5Z0mT7CpsFrrVLb1FErJAW
VP3IDlTxUUpY2fvMlJjYRHcBmvVUpiKbFQ9eFBBdYK1erT7lVK4/lGtoCaltoqr+
HRY8XBV5m3IXY/MIypUuTq9hv4/KjFs/Jg5pDYbCGTvXgj3xbqRDdNpEKGLtmKHg
UD3MpzD1LTVsTsT6t8ys6CMU3ykeWKwRc1vY33kfOCalK1VVVIWoq2XUmQK8DUGj
U6FjXuPmA7+jY7C9RQL5uzJnz+gLG3JZE0j8LfLeRzA9fxAMe+tg8ZbhFU9vohw=
=PQrC
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
  2016-03-22 16:22             ` Alex Bennée
  2016-03-28 17:00               ` Sean Bruno
@ 2016-03-31 16:57               ` Sean Bruno
  1 sibling, 0 replies; 16+ messages in thread
From: Sean Bruno @ 2016-03-31 16:57 UTC (permalink / raw)
  To: Alex Bennée; +Cc: Paolo Bonzini, QEMU Developers, Peter Maydell

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

This seems to at least allow things to compile, qemu-system seems to
be able to boot images on i386, but I may have missed something:

diff --git a/cpus.c b/cpus.c
index 23cf7aa..11f8bab 100644
- --- a/cpus.c
+++ b/cpus.c
@@ -338,15 +338,8 @@ static int64_t qemu_icount_round(int64_t count)

 static void icount_warp_rt(void)
 {
- -    /* The icount_warp_timer is rescheduled soon after
vm_clock_warp_start
- -     * changes from -1 to another value, so the race here is okay.
- -     */
- -    if (atomic_read(&vm_clock_warp_start) == -1) {
- -        return;
- -    }
- -
     seqlock_write_lock(&timers_state.vm_clock_seqlock);
- -    if (runstate_is_running()) {
+    if ((vm_clock_warp_start != -1) && runstate_is_running()) {
         int64_t clock = REPLAY_CLOCK(REPLAY_CLOCK_VIRTUAL_RT,
                                      cpu_get_clock_locked());
         int64_t warp_delta;
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQF8BAEBCgBmBQJW/VbrXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w
ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRCQUFENDYzMkU3MTIxREU4RDIwOTk3REQx
MjAxRUZDQTFFNzI3RTY0AAoJEBIB78oecn5k4PUH/0PP+LzCDZmHCUN0ZuQ6Z6SK
VWevvfg6d7mgqcobGD94E8V5AZYO9y4aj+WerCPULXq7I2qAthgqlccUaPMN4KKz
+cJiBo02pzOXJmbPLiFetsQj87+nUzUpcJ2yVY0eiFcO2lbNQV20UZdYDuacQ0ds
bjyhq49dC51wsn6ET1FMY6BH8vmdicWe4SMDEgSw32pFvZmIlGKFwuP3w+2dLpYg
H4aajemN2PrHggcsRI8Gxu2G6yPbuWuGB6eaqS/OQ+6NxUmXz2lqOgzMfBAZjlzQ
5kVp8XRcU42koS2hLPmmxyxZ98W54oM8LtrUYdoecTabvJG3cwaApw7OITWZJPM=
=Hpp4
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
  2016-03-20 19:20 [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets Sean Bruno
  2016-03-20 20:03 ` Peter Maydell
  2016-03-21  9:06 ` Paolo Bonzini
@ 2016-04-06 16:11 ` Sean Bruno
  2016-04-06 18:51   ` Alex Bennée
  2 siblings, 1 reply; 16+ messages in thread
From: Sean Bruno @ 2016-04-06 16:11 UTC (permalink / raw)
  To: qemu-devel

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512



On 03/20/16 12:20, Sean Bruno wrote:
> aarch64 targets are now failing to build on i386 hosts due to
> missing __atomic_load_8() calls since this commit:
> 
> https://github.com/qemu/qemu/commit/a0aa44b488b3601415d55041e4619aef5f
3a4ba8#diff-c143d686899ae51d7b927d9c682e12fd
>
>  I'm unsure if Linux is disabling aarch64 targets for i386 hosts or
> if this commit works "just fine" on Linux hosts right now, as it
> doesn't work with clang or gcc.
> 
> More or less, the code in question ends up looking like this bit
> of test code:
> 
> #include <stdio.h> #include <sys/types.h> #include
> <machine/atomic.h>
> 
> #define atomic_read(ptr)                          \ ({
> \ typeof(*ptr) _val;                            \ 
> __atomic_load(ptr, &_val, __ATOMIC_RELAXED); \ _val;
> \ })
> 
> int main () { int foo; int64_t foo64;
> 
> atomic_read(&foo); atomic_read(&foo64);
> 
> return(0); }
> 
> 
> This test code will manifest the same issue as the aarch64 target 
> building on FreeBSD i386 with the error:
> 
> undefined reference to `__atomic_load_8'
> 
> 


This seems to be fixed with the latest commits.  Thanks!

sean
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQF8BAEBCgBmBQJXBTUqXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w
ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRCQUFENDYzMkU3MTIxREU4RDIwOTk3REQx
MjAxRUZDQTFFNzI3RTY0AAoJEBIB78oecn5kAoYH/3Ai7IdpyxyIiRtgYDWJQcg8
GLmbu1NL5Xrh0af5DU933kCqkDlKK8qKYs89DzHMfay8TIyZFkqKF5pYy66JJAQ2
qAB9eGTL567q3QPk9iYkRLju4Y4exmBL1ZSW1fUpPKjjjBlLR7VrHjEA/Ze0zYsM
+MvRJcHSf8tNawr8WfOzVgFPf8pc2K0Ix8/VZCFEdf4FcATHj2nYXTmzQmTQuWSo
tqDWe02TIov0BSBaA4uG6n02F4KWglGBE+bdsuTiwxxAjkcmHLgg28h7Wupkmatj
5zarlSLIhvv7j3KAS/r8aKtQ04ydXybTo0HnPLJ9JV/xz3bAbvvKDYLMZijpm0M=
=OSgq
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets
  2016-04-06 16:11 ` Sean Bruno
@ 2016-04-06 18:51   ` Alex Bennée
  0 siblings, 0 replies; 16+ messages in thread
From: Alex Bennée @ 2016-04-06 18:51 UTC (permalink / raw)
  To: Sean Bruno; +Cc: qemu-devel


Sean Bruno <sbruno@freebsd.org> writes:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
>
>
> On 03/20/16 12:20, Sean Bruno wrote:
>> aarch64 targets are now failing to build on i386 hosts due to
>> missing __atomic_load_8() calls since this commit:
>>
>> https://github.com/qemu/qemu/commit/a0aa44b488b3601415d55041e4619aef5f
> 3a4ba8#diff-c143d686899ae51d7b927d9c682e12fd
>>
>>  I'm unsure if Linux is disabling aarch64 targets for i386 hosts or
>> if this commit works "just fine" on Linux hosts right now, as it
>> doesn't work with clang or gcc.
>>
>> More or less, the code in question ends up looking like this bit
>> of test code:
>>
>> #include <stdio.h> #include <sys/types.h> #include
>> <machine/atomic.h>
>>
>> #define atomic_read(ptr)                          \ ({
>> \ typeof(*ptr) _val;                            \
>> __atomic_load(ptr, &_val, __ATOMIC_RELAXED); \ _val;
>> \ })
>>
>> int main () { int foo; int64_t foo64;
>>
>> atomic_read(&foo); atomic_read(&foo64);
>>
>> return(0); }
>>
>>
>> This test code will manifest the same issue as the aarch64 target
>> building on FreeBSD i386 with the error:
>>
>> undefined reference to `__atomic_load_8'
>>
>>
>
>
> This seems to be fixed with the latest commits.  Thanks!

Sorry I thought I'd CC'd you. The patch needed re-spinning to get picked
up by Peter. Glad it is working for you now ;-)

>
> sean
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQF8BAEBCgBmBQJXBTUqXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w
> ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRCQUFENDYzMkU3MTIxREU4RDIwOTk3REQx
> MjAxRUZDQTFFNzI3RTY0AAoJEBIB78oecn5kAoYH/3Ai7IdpyxyIiRtgYDWJQcg8
> GLmbu1NL5Xrh0af5DU933kCqkDlKK8qKYs89DzHMfay8TIyZFkqKF5pYy66JJAQ2
> qAB9eGTL567q3QPk9iYkRLju4Y4exmBL1ZSW1fUpPKjjjBlLR7VrHjEA/Ze0zYsM
> +MvRJcHSf8tNawr8WfOzVgFPf8pc2K0Ix8/VZCFEdf4FcATHj2nYXTmzQmTQuWSo
> tqDWe02TIov0BSBaA4uG6n02F4KWglGBE+bdsuTiwxxAjkcmHLgg28h7Wupkmatj
> 5zarlSLIhvv7j3KAS/r8aKtQ04ydXybTo0HnPLJ9JV/xz3bAbvvKDYLMZijpm0M=
> =OSgq
> -----END PGP SIGNATURE-----


--
Alex Bennée

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2016-04-06 18:51 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-20 19:20 [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets Sean Bruno
2016-03-20 20:03 ` Peter Maydell
2016-03-21  9:11   ` Alex Bennée
2016-03-21 14:52     ` Sean Bruno
2016-03-21 15:36       ` Alex Bennée
2016-03-21 15:56         ` Paolo Bonzini
2016-03-22 16:10           ` Sean Bruno
2016-03-22 16:22             ` Alex Bennée
2016-03-28 17:00               ` Sean Bruno
2016-03-31 16:57               ` Sean Bruno
2016-03-21  9:06 ` Paolo Bonzini
2016-03-21  9:35   ` Peter Maydell
2016-03-21 11:23     ` Paolo Bonzini
2016-03-21 11:49       ` Alex Bennée
2016-04-06 16:11 ` Sean Bruno
2016-04-06 18:51   ` Alex Bennée

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.