* [Qemu-devel] [PATCH] tcg/ppc: disable atomic write check on ppc32
@ 2017-09-11 20:49 Philippe Mathieu-Daudé
2017-09-11 21:37 ` Peter Maydell
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-09-11 20:49 UTC (permalink / raw)
To: Richard Henderson, Peter Maydell; +Cc: Philippe Mathieu-Daudé, qemu-devel
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 <rth@twiddle.net>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg/ppc: disable atomic write check on ppc32
2017-09-11 20:49 [Qemu-devel] [PATCH] tcg/ppc: disable atomic write check on ppc32 Philippe Mathieu-Daudé
@ 2017-09-11 21:37 ` Peter Maydell
2017-09-12 4:23 ` Richard Henderson
2017-09-12 21:04 ` Paolo Bonzini
2017-09-12 4:17 ` Richard Henderson
2017-09-12 17:01 ` Richard Henderson
2 siblings, 2 replies; 9+ messages in thread
From: Peter Maydell @ 2017-09-11 21:37 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Richard Henderson, QEMU Developers
On 11 September 2017 at 21:49, Philippe Mathieu-Daudé <f4bug@amsat.org> 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 <rth@twiddle.net>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg/ppc: disable atomic write check on ppc32
2017-09-11 20:49 [Qemu-devel] [PATCH] tcg/ppc: disable atomic write check on ppc32 Philippe Mathieu-Daudé
2017-09-11 21:37 ` Peter Maydell
@ 2017-09-12 4:17 ` Richard Henderson
2017-09-12 17:01 ` Richard Henderson
2 siblings, 0 replies; 9+ messages in thread
From: Richard Henderson @ 2017-09-12 4:17 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Peter Maydell; +Cc: qemu-devel
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 <rth@twiddle.net>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> This fixes Shippable builds, see:
> https://app.shippable.com/github/qemu/qemu/runs/434/10/console
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg/ppc: disable atomic write check on ppc32
2017-09-11 21:37 ` Peter Maydell
@ 2017-09-12 4:23 ` Richard Henderson
2017-09-12 9:08 ` Peter Maydell
2017-09-12 21:04 ` Paolo Bonzini
1 sibling, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2017-09-12 4:23 UTC (permalink / raw)
To: Peter Maydell, Philippe Mathieu-Daudé; +Cc: QEMU Developers
On 09/11/2017 02:37 PM, Peter Maydell wrote:
> On 11 September 2017 at 21:49, Philippe Mathieu-Daudé <f4bug@amsat.org> 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 <rth@twiddle.net>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> 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~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg/ppc: disable atomic write check on ppc32
2017-09-12 4:23 ` Richard Henderson
@ 2017-09-12 9:08 ` Peter Maydell
0 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2017-09-12 9:08 UTC (permalink / raw)
To: Richard Henderson; +Cc: Philippe Mathieu-Daudé, QEMU Developers
On 12 September 2017 at 05:23, Richard Henderson <rth@twiddle.net> wrote:
> On 09/11/2017 02:37 PM, Peter Maydell wrote:
>> On 11 September 2017 at 21:49, Philippe Mathieu-Daudé <f4bug@amsat.org> 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 <rth@twiddle.net>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg/ppc: disable atomic write check on ppc32
2017-09-11 20:49 [Qemu-devel] [PATCH] tcg/ppc: disable atomic write check on ppc32 Philippe Mathieu-Daudé
2017-09-11 21:37 ` Peter Maydell
2017-09-12 4:17 ` Richard Henderson
@ 2017-09-12 17:01 ` Richard Henderson
2017-09-12 21:00 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2017-09-12 17:01 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Peter Maydell; +Cc: qemu-devel
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~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg/ppc: disable atomic write check on ppc32
2017-09-12 17:01 ` Richard Henderson
@ 2017-09-12 21:00 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-09-12 21:00 UTC (permalink / raw)
To: Richard Henderson, Peter Maydell; +Cc: qemu-devel
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!
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg/ppc: disable atomic write check on ppc32
2017-09-11 21:37 ` Peter Maydell
2017-09-12 4:23 ` Richard Henderson
@ 2017-09-12 21:04 ` Paolo Bonzini
2017-09-13 16:45 ` Richard Henderson
1 sibling, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2017-09-12 21:04 UTC (permalink / raw)
To: Peter Maydell, Philippe Mathieu-Daudé
Cc: QEMU Developers, Richard Henderson
On 11/09/2017 23:37, Peter Maydell wrote:
> On 11 September 2017 at 21:49, Philippe Mathieu-Daudé <f4bug@amsat.org> 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 <rth@twiddle.net>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg/ppc: disable atomic write check on ppc32
2017-09-12 21:04 ` Paolo Bonzini
@ 2017-09-13 16:45 ` Richard Henderson
0 siblings, 0 replies; 9+ messages in thread
From: Richard Henderson @ 2017-09-13 16:45 UTC (permalink / raw)
To: Paolo Bonzini, Peter Maydell, Philippe Mathieu-Daudé
Cc: QEMU Developers, 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~
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-09-13 16:45 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-11 20:49 [Qemu-devel] [PATCH] tcg/ppc: disable atomic write check on ppc32 Philippe Mathieu-Daudé
2017-09-11 21:37 ` Peter Maydell
2017-09-12 4:23 ` Richard Henderson
2017-09-12 9:08 ` Peter Maydell
2017-09-12 21:04 ` Paolo Bonzini
2017-09-13 16:45 ` Richard Henderson
2017-09-12 4:17 ` Richard Henderson
2017-09-12 17:01 ` Richard Henderson
2017-09-12 21:00 ` Philippe Mathieu-Daudé
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.