Re: [PATCH] hw/display/bcm2835_fb: Remove DeviceReset() call in DeviceRealize()

[Philippe Mathieu-Daudé]

On 3/23/21 1:27 PM, Peter Maydell wrote:
> On Sat, 13 Mar 2021 at 17:01, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> When QDev objects have their DeviceReset handler set, they
>> shouldn't worry about calling it at realization stage (it
>> is handled by hw/core/qdev.c::device_set_realized).
>>
>> Remove the pointless/confusing bcm2835_fb_reset() call.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/display/bcm2835_fb.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
>> index 2be77bdd3a0..445e8636770 100644
>> --- a/hw/display/bcm2835_fb.c
>> +++ b/hw/display/bcm2835_fb.c
>> @@ -424,8 +424,6 @@ static void bcm2835_fb_realize(DeviceState *dev, Error 
>> **errp)
>>      s->dma_mr = MEMORY_REGION(obj);
>>      address_space_init(&s->dma_as, s->dma_mr, TYPE_BCM2835_FB "-memory");
>>
>> -    bcm2835_fb_reset(dev);
>> -
>>      s->con = graphic_console_init(dev, 0, &vgafb_ops, s);
>>      qemu_console_resize(s->con, s->config.xres, s->config.yres);
>>  }
> 
> With this patch applied, I get a clang-sanitizer-build failure
> in "make check":
> 
> $ QTEST_QEMU_BINARY=./build/arm-clang/qemu-system-arm
> build/arm-clang/tests/qtest/test-hmp
> /arm/hmp/raspi0: ../../hw/display/bcm2835_fb.c:131:13: runtime error:
> store to null pointer of type 'uint32_t' (aka 'unsigned int')
> UndefinedBehaviorSanitizer:DEADLYSIGNAL
> ==23006==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address
> 0x000000000000 (pc 0x5599adaf839b bp 0x000000000000 sp 0x7ffd81ee77a0
> T23006)
> ==23006==The signal is caused by a WRITE memory access.
> ==23006==Hint: address points to the zero page.
>     #0 0x5599adaf839a in draw_line_src16
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../hw/display/bcm2835_fb.c:131:30
>     #1 0x5599add82e8f in framebuffer_update_display
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../hw/display/framebuffer.c:107:13
>     #2 0x5599adaf7844 in fb_update_display
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../hw/display/bcm2835_fb.c:203:5
>     #3 0x5599ad9e7800 in graphic_hw_update
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../ui/console.c:279:9
>     #4 0x5599aea450d3 in aio_bh_poll
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../util/async.c:164:13
>     #5 0x5599ae9e5d73 in aio_poll
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../util/aio-posix.c:659:17
>     #6 0x5599ad873d2c in handle_hmp_command
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../monitor/hmp.c:1117:9
>     #7 0x5599ae368594 in qmp_human_monitor_command
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../monitor/misc.c:135:5
>     #8 0x5599ae996101 in qmp_marshal_human_monitor_command
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/qapi/qapi-commands-misc.c:266:14
>     #9 0x5599ae9de39c in do_qmp_dispatch_bh
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../qapi/qmp-dispatch.c:131:5
>     #10 0x5599aea450d3 in aio_bh_poll
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../util/async.c:164:13
>     #11 0x5599ae9e332b in aio_dispatch
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../util/aio-posix.c:381:5
>     #12 0x5599aea4799a in aio_ctx_dispatch
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../util/async.c:306:5
>     #13 0x7f74a0a35416 in g_main_context_dispatch
> (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c416)
>     #14 0x5599ae9dc8f4 in glib_pollfds_poll
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../util/main-loop.c:231:9
>     #15 0x5599ae9dc8f4 in os_host_main_loop_wait
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../util/main-loop.c:254
>     #16 0x5599ae9dc8f4 in main_loop_wait
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../util/main-loop.c:530
>     #17 0x5599ae42adf6 in qemu_main_loop
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../softmmu/runstate.c:725:9
>     #18 0x5599ad5bbf0a in main
> /home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/../../softmmu/main.c:50:5
>     #19 0x7f749bcf3bf6 in __libc_start_main
> /build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310
>     #20 0x5599ad59c519 in _start
> (/home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/qemu-system-arm+0x1335519)
> 
> UndefinedBehaviorSanitizer can not provide additional info.
> ==23006==ABORTING
> Broken pipe
> Aborted (core dumped)
> 
> The patch is correct in that the device shouldn't be resetting itself
> in realize, but this is presumably masking a bug elsewhere in the device
> that we need to fix first before we can make this change.
> 
> It looks as if what happens is that the GraphicHwOps methods can
> get called before the device is reset. I don't know if that is
> something we can arrange to have not happen -- certainly it's
> a bit confusing to have to deal with the device not having been
> reset yet -- or if implementations just have to deal with it.

Thanks for the report.

I don't understand well how graphic works, but I noticed
bcm2835_fb_reconfigure() calls qemu_console_resize() ->
qemu_create_displaysurface() -> pixman_image_create_bits(),
so then when framebuffer_update_display() is called,
surface_data() is not NULL.

So we can trigger the qemu_create_displaysurface() call by
replacing the open-coded bcm2835_fb_reconfigure() in reset():

-- >8 --
diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index 445e8636770..d7a44771c44 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -396,10 +396,7 @@ static void bcm2835_fb_reset(DeviceState *dev)

     s->pending = false;

-    s->config = s->initial_config;
-
-    s->invalidate = true;
-    s->lock = false;
+    bcm2835_fb_reconfigure(s, &s->initial_config);
 }
---

I'll send a patch.