Re: [Qemu-devel] [FreeBSD] Host build i386 failing to build aarch64 targets

[Alex Bennée]

Sean Bruno <address@hidden> writes:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
>
>
> On 03/21/16 02:11, Alex Bennée wrote:
>>
>> Peter Maydell <address@hidden> writes:
>>
>>> On 20 March 2016 at 19:20, Sean Bruno <address@hidden>
>>> 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