Re: [PULL 09/17] hw/display: re-arrange memory region tracking

["Alex Bennée" <alex.bennee@linaro.org>]

Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:

> On 2025/06/06 1:26, Alex Bennée wrote:
>> QOM objects can be embedded in other QOM objects and managed as part
>> of their lifetime but this isn't the case for
>> virtio_gpu_virgl_hostmem_region. However before we can split it out we
>> need some other way of associating the wider data structure with the
>> memory region.
>> Fortunately MemoryRegion has an opaque pointer. This is passed down
>> to
>> MemoryRegionOps for device type regions but is unused in the
>> memory_region_init_ram_ptr() case. Use the opaque to carry the
>> reference and allow the final MemoryRegion object to be reaped when
>> its reference count is cleared.
>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-ID: <20250603110204.838117-10-alex.bennee@linaro.org>
>
> I have told you that you should address all comments before sending a
> series again a few times[1][2], but you haven't done that.

I've given reasons. Thanks for your review but you don't get to veto.

> I pointed out it has no effect (fixing or improving something) other
> than adding a memory allocation, but you didn't make a reply to prove
> otherwise.

I explained the commit cover what it is doing.

>
> I also pointed out it leaks memory and you asked for a test case[4],
> but you made this pull request without giving me 24 hours to reply to
> it.

You keep bringing up theoretical issues. We have passing test cases now
and we have plenty of time to address any bugs we might discover. But
holding onto these patches is slowing down other work getting in and I
don't deem it a risk to merge as is.

>
> The situation of "[PULL 03/17] tests/tcg: make aarch64 boot.S handle
> different starting modes" is also similar. I added a comment about
> symbol naming and you gave a reasoning, but I didn't get time to
> review it either[5]. Besides, I also had a suggestion to make the code
> shorter for the past version, but it is also dismissed.
>
> I also pointed out "[PULL 11/17] ui/gtk-gl-area: Remove extra draw
> call in refresh" has an undressed comment[2][7].
>
> I would like to see improvements in how comments are addressed before
> a series is resent.

No - I'm sorry you don't get to veto a pull request because it doesn't
meet your particular standards.

I'm happy with the other review and level of testing of the patches to
put it in a pull request. I held off the other well tested patch in the
series out of an abundance of caution but will keep it in the
virtio-gpu/next tree and re-post once I've done my next sweep for my
maintainer trees.

<snip>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro