[PATCH 0/6] Implement memory_region_new_* functions

BALATON Zoltan posted 6 patches 1 month, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/cover.1766525089.git.balaton@eik.bme.hu
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@kernel.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
There is a newer version of this series
include/system/memory.h | 360 ++++++++++++++++++++++++++++++
system/memory.c         | 484 +++++++++++++++++++++++++---------------
2 files changed, 668 insertions(+), 176 deletions(-)
[PATCH 0/6] Implement memory_region_new_* functions
Posted by BALATON Zoltan 1 month, 2 weeks ago
Our documentation says that memory regions are automatically freed
when the owner dies and the reference counting to do this is also
implemented. However this relies on the QOM free funtion that can only
be set by creating objects with object_new but memory API only
provides constructors that call object_initialize which clears the
free function that prevents QOM to manage the memory region lifetime.
Implement corresponding memory_region_new_* functions that do the same
as the memory_region_init_* functions but create the memory region
with object_new so the lifetime can be automatically managed by QOM as
documented.

BALATON Zoltan (6):
  memory: Add internal memory_region_set_ops helper function
  memory: Factor out common ram region initialization
  memory: Factor out more common ram region initialization
  memory: Shorten memory_region_init_rom_nomigrate
  memory: Add internal memory_region_register_ram function
  memory: Add memory_region_new* functions

 include/system/memory.h | 360 ++++++++++++++++++++++++++++++
 system/memory.c         | 484 +++++++++++++++++++++++++---------------
 2 files changed, 668 insertions(+), 176 deletions(-)

-- 
2.41.3
Re: [PATCH 0/6] Implement memory_region_new_* functions
Posted by Akihiko Odaki 1 month, 2 weeks ago
On 2025/12/24 6:49, BALATON Zoltan wrote:
> Our documentation says that memory regions are automatically freed
> when the owner dies and the reference counting to do this is also
> implemented. However this relies on the QOM free funtion that can only
> be set by creating objects with object_new but memory API only
> provides constructors that call object_initialize which clears the
> free function that prevents QOM to manage the memory region lifetime.
> Implement corresponding memory_region_new_* functions that do the same
> as the memory_region_init_* functions but create the memory region
> with object_new so the lifetime can be automatically managed by QOM as
> documented.

The documentation explains the existing functions so the discrepancy 
between them you see should be fixed by updating them, not adding new ones.

Regards,
Akihiko Odaki
Re: [PATCH 0/6] Implement memory_region_new_* functions
Posted by BALATON Zoltan 1 month, 2 weeks ago
On Wed, 24 Dec 2025, Akihiko Odaki wrote:
> On 2025/12/24 6:49, BALATON Zoltan wrote:
>> Our documentation says that memory regions are automatically freed
>> when the owner dies and the reference counting to do this is also
>> implemented. However this relies on the QOM free funtion that can only
>> be set by creating objects with object_new but memory API only
>> provides constructors that call object_initialize which clears the
>> free function that prevents QOM to manage the memory region lifetime.
>> Implement corresponding memory_region_new_* functions that do the same
>> as the memory_region_init_* functions but create the memory region
>> with object_new so the lifetime can be automatically managed by QOM as
>> documented.
>
> The documentation explains the existing functions so the discrepancy between 
> them you see should be fixed by updating them, not adding new ones.

Do you mean replacing memory_region_init_* with these memory_region_new_* 
functions? The memory_region_init_* is still useful for embedded memory 
regions that are managed by some other way which is also mentioned in the 
documentation as an alternative so I think both of them are useful for 
different cases. If you mean we need to update docs to refer to 
memory_region_new instead of memory_region_init at some places then I 
think you're right, the docs may also need to be updated or clarified.

Regards,
BALATON Zoltan
Re: [PATCH 0/6] Implement memory_region_new_* functions
Posted by Akihiko Odaki 1 month, 1 week ago
On 2025/12/24 22:47, BALATON Zoltan wrote:
> On Wed, 24 Dec 2025, Akihiko Odaki wrote:
>> On 2025/12/24 6:49, BALATON Zoltan wrote:
>>> Our documentation says that memory regions are automatically freed
>>> when the owner dies and the reference counting to do this is also
>>> implemented. However this relies on the QOM free funtion that can only
>>> be set by creating objects with object_new but memory API only
>>> provides constructors that call object_initialize which clears the
>>> free function that prevents QOM to manage the memory region lifetime.
>>> Implement corresponding memory_region_new_* functions that do the same
>>> as the memory_region_init_* functions but create the memory region
>>> with object_new so the lifetime can be automatically managed by QOM as
>>> documented.
>>
>> The documentation explains the existing functions so the discrepancy 
>> between them you see should be fixed by updating them, not adding new 
>> ones.
> 
> Do you mean replacing memory_region_init_* with these 
> memory_region_new_* functions? The memory_region_init_* is still useful 
> for embedded memory regions that are managed by some other way which is 
> also mentioned in the documentation as an alternative so I think both of 
> them are useful for different cases. If you mean we need to update docs 
> to refer to memory_region_new instead of memory_region_init at some 
> places then I think you're right, the docs may also need to be updated 
> or clarified.

I'd like to see a correspondence between the stated problem and the 
solution. If the intention is to solve the mismatched documentation and 
implementation, I think there are only two possible options:

1) Update the documentation and/or the implementation.
2) Delete them.

But this patch series does neither of them. Replacing 
memory_region_init_* with memory_region_new_* does solve as it will do 
2) in the process. An alternative solution that implements 1) or 2) is 
necessary if it's not ideal.

Regards,
Akihiko Odaki
Re: [PATCH 0/6] Implement memory_region_new_* functions
Posted by Peter Xu 1 month, 2 weeks ago
On Wed, Dec 24, 2025 at 02:47:20PM +0100, BALATON Zoltan wrote:
> On Wed, 24 Dec 2025, Akihiko Odaki wrote:
> > On 2025/12/24 6:49, BALATON Zoltan wrote:
> > > Our documentation says that memory regions are automatically freed
> > > when the owner dies and the reference counting to do this is also
> > > implemented. However this relies on the QOM free funtion that can only
> > > be set by creating objects with object_new but memory API only
> > > provides constructors that call object_initialize which clears the
> > > free function that prevents QOM to manage the memory region lifetime.
> > > Implement corresponding memory_region_new_* functions that do the same
> > > as the memory_region_init_* functions but create the memory region
> > > with object_new so the lifetime can be automatically managed by QOM as
> > > documented.
> > 
> > The documentation explains the existing functions so the discrepancy
> > between them you see should be fixed by updating them, not adding new
> > ones.
> 
> Do you mean replacing memory_region_init_* with these memory_region_new_*
> functions? The memory_region_init_* is still useful for embedded memory
> regions that are managed by some other way which is also mentioned in the
> documentation as an alternative so I think both of them are useful for
> different cases. If you mean we need to update docs to refer to
> memory_region_new instead of memory_region_init at some places then I think
> you're right, the docs may also need to be updated or clarified.

To me, it's less convincing to add new APIs without a solid user.

IMHO we can go either (1) leave patch 6 for later, making this series a
cleanup first, or, (2) add users for every new functions introduced, so at
least we know why each of the new functions are introduced and necessary.

Thanks,

-- 
Peter Xu