Re: [Qemu-devel] [PATCH] tcg/ppc: disable atomic write check on ppc32

[Peter Maydell]

On 12 September 2017 at 05:23, Richard Henderson <address@hidden> wrote:
> On 09/11/2017 02:37 PM, Peter Maydell wrote:
>> On 11 September 2017 at 21:49, Philippe Mathieu-Daudé <address@hidden> wrote:
>>> this fixes building for ppc64 on ppc32 (changed in 5964fca8a12c):
>>>
>>>   qemu/tcg/ppc/tcg-target.inc.c: In function 'tb_target_set_jmp_target':
>>>   qemu/include/qemu/compiler.h:86:30: error: static assertion failed: "not 
>>> expecting: sizeof(*(uint64_t *)jmp_addr) > ATOMIC_REG_SIZE"
>>>        QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE); \
>>>        ^
>>>   qemu/tcg/ppc/tcg-target.inc.c:1377:9: note: in expansion of macro 
>>> 'atomic_set'
>>>            atomic_set((uint64_t *)jmp_addr, pair);
>>>            ^
>>>
>>> Suggested-by: Richard Henderson <address@hidden>
>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>> ---
>>> This fixes Shippable builds, see:
>>> https://app.shippable.com/github/qemu/qemu/runs/434/10/console
>>>
>>>  tcg/ppc/tcg-target.inc.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
>>> index 21d764c102..0417901289 100644
>>> --- a/tcg/ppc/tcg-target.inc.c
>>> +++ b/tcg/ppc/tcg-target.inc.c
>>> @@ -1374,7 +1374,7 @@ void tb_target_set_jmp_target(uintptr_t tc_ptr, 
>>> uintptr_t jmp_addr,
>>>          pair = (uint64_t)i2 << 32 | i1;
>>>  #endif
>>>
>>> -        atomic_set((uint64_t *)jmp_addr, pair);
>>> +        atomic_set__nocheck((uint64_t *)jmp_addr, pair);
>>>          flush_icache_range(jmp_addr, jmp_addr + 8);
>>>      } else {
>>>          intptr_t diff = addr - jmp_addr;
>>
>> Can you explain why this is the right thing? On the
>> face of it it looks correct to insist that we don't
>> try to do an atomic set of something that's bigger
>> than the host can actually handle...
>
> It is the correct thing because ppc32 is handled earlier in the function; only
> ppc64 can reach here, therefore a 64-bit atomic_set is always available.
>
> However, I wrote the function intending to minimize the ifdefs so that we can
> be sure that it all compiles -- especially the ppc32 bits which I cannot test
> on gcc cfarm machines.  I didn't think about the fact that ppc32 could not
> compile the _Static_assert within the 64-bit atomic_set here in the ppc64 
> section.

Ah, I see. Can we have a comment about why the __nocheck is ok here,
then, please?

thanks
-- PMM