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

2017-09-13 Thread Richard Henderson 
On 09/12/2017 02:04 PM, Paolo Bonzini wrote:
> I wonder if QEMU_BUILD_BUG_ON (at least in atomics) should not use a
> static assertion, but rather the 'error ("MESSAGE")' attribute instead.
> This way, if the code is dead it does not cause a build failure.

I think that would be an excellent idea.


r~

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

2017-09-12 Thread Paolo Bonzini 
On 11/09/2017 23:37, Peter Maydell wrote:
> On 11 September 2017 at 21:49, Philippe Mathieu-Daudé  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 
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>> 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...

Probably because this code is guarded by "if (TCG_TARGET_REG_BITS ==
64)", so actually it only ever runs with 64-bit targets.

I wonder if QEMU_BUILD_BUG_ON (at least in atomics) should not use a
static assertion, but rather the 'error ("MESSAGE")' attribute instead.
This way, if the code is dead it does not cause a build failure.

Paolo

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

2017-09-12 Thread Philippe Mathieu-Daudé 

On 09/12/2017 02:01 PM, Richard Henderson wrote:

On 09/11/2017 01:49 PM, Philippe Mathieu-Daudé wrote:

-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;



Queued, thanks.


Thanks Richard for adding the comment requested by Peter!

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

2017-09-12 Thread Richard Henderson 
On 09/11/2017 01:49 PM, Philippe Mathieu-Daudé wrote:
> -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;
> 

Queued, thanks.


r~

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

2017-09-12 Thread Peter Maydell 
On 12 September 2017 at 05:23, Richard Henderson  wrote:
> On 09/11/2017 02:37 PM, Peter Maydell wrote:
>> On 11 September 2017 at 21:49, Philippe Mathieu-Daudé  
>> 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 
>>> Signed-off-by: Philippe Mathieu-Daudé 
>>> ---
>>> 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

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

2017-09-11 Thread Richard Henderson 
On 09/11/2017 02:37 PM, Peter Maydell wrote:
> On 11 September 2017 at 21:49, Philippe Mathieu-Daudé  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 
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>> 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.


r~

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

2017-09-11 Thread Richard Henderson 
On 09/11/2017 01:49 PM, Philippe Mathieu-Daudé 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 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> This fixes Shippable builds, see:
> https://app.shippable.com/github/qemu/qemu/runs/434/10/console

Reviewed-by: Richard Henderson 


r~

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

2017-09-11 Thread Peter Maydell 
On 11 September 2017 at 21:49, Philippe Mathieu-Daudé  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 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> 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...

thanks
-- PMM

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

2017-09-11 Thread Philippe Mathieu-Daudé 
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 
Signed-off-by: Philippe Mathieu-Daudé 
---
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;
-- 
2.14.1