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

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

[Philippe Mathieu-Daudé]

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);
 }
-- 
2.26.2

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

[Richard Henderson]

On 3/13/21 11:01 AM, Philippe Mathieu-Daudé 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>
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

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

[Peter Maydell]

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


Applied to target-arm.next, thanks.

-- PMM

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

[Peter Maydell]

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

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.