Re: [RFC PATCH-for-5.1? v3 1/2] memory: Allow monkey-patching MemoryRegion access sizes

[Peter Maydell <peter.maydell@linaro.org>]

On Tue, 21 Jul 2020 at 15:23, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 7/21/20 2:49 PM, Peter Maydell wrote:
> > On Tue, 21 Jul 2020 at 13:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> On 7/21/20 2:33 PM, Peter Maydell wrote:
> >>> On Tue, 21 Jul 2020 at 13:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>> Don't you now need to g_memfree() mr->ops somewhere? Otherwise
> >>> you've leaked it if the device which owned this MemoryRegion
> >>> is hot-unplugged, I think.
> >>
> >> I haven't thinking of hot-unplug. I went with the simplest fix
> >> considering we are in freeze, and fixing the bug was more
> >> important that a leak at this point.
> >> I'll have a look at freeing this memory, hoping it is still less
> >> disruptive than a proper architectural change to fix this problem.
> >
> > Instead of g_memdup()ing the ops struct here, you could maybe
> > do it in isa_register_ioport() instead. Then you don't need to
> > worry about leaks because we know all ISA devices are not
> > hotpluggable, and the ugliness is also a bit more constrained
> > to the ISA code. (Coverity probably still thinks it's a leak though.)
>
> I tried that first but got:
>
> memory.c: In function ‘memory_region_initfn’:
> memory.c:1221:13: error: assignment discards ‘const’ qualifier from
> pointer target type [-Werror=discarded-qualifiers]
>  1221 |     mr->ops = &unassigned_mem_ops;
>       |             ^
> memory.c: In function ‘memory_region_init_io’:
> memory.c:1488:13: error: assignment discards ‘const’ qualifier from
> pointer target type [-Werror=discarded-qualifiers]
>  1488 |     mr->ops = ops ? ops : &unassigned_mem_ops;
>       |             ^
> memory.c: In function ‘memory_region_init_ram_device_ptr’:
> memory.c:1625:13: error: assignment discards ‘const’ qualifier from
> pointer target type [-Werror=discarded-qualifiers]
>  1625 |     mr->ops = &ram_device_mem_ops;
>       |             ^
> memory.c: In function ‘memory_region_init_rom_device_nomigrate’:
> memory.c:1667:13: error: assignment discards ‘const’ qualifier from
> pointer target type [-Werror=discarded-qualifiers]
>  1667 |     mr->ops = ops;
>       |             ^

I don't think you should need to change MemoryRegion::ops to
non-const for this approach. Since you allocate the memory for the
new ops struct in isa_register_ioport() you can set it
up there before assigning it to mr->ops -- you're not trying
to modify-in-place the mr->ops that's already there.

thanks
-- PMM