| Message ID | 20170911204936.5020-1-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | tcg/ppc: disable atomic write check on ppc32 | expand |
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
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~
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~
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
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~
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!
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
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~
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;
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(-)