Based-on: 20250926070806.292065-1-luc.michel@amd.com ([PATCH v6 00/47] AMD Versal Gen 2 support)

v2:
  - Rebased, now applies cleanly. The conflict came from f9ce0848942490
    that was merged in the mean time.
    As discussed with Phil, this is still based on Versal 2 series to
    avoid having to rebase the later on this one, as it is basically
    ready to be merged. [Phil]

Hello,

This series addresses the memory leaks caused by the register API. The
first patches fix the API itself while the last ones take care of the
CANFD model.

The leaks in the register API were caused by:
   - the REGISTER device being initialized without being realized nor
     finalized. Those devices were not parented to anything and were
     not using the qdev API. So in the end I chose to drop the REGISTER
     object completely (patch 1).
   - Register API users not calling `register_finalize_block' on the
     RegisterInfoArray returned by `register_init_block'. Instead of
     fixing all the users, I chose to simplify the API by removing the
     need for this call. I turned the RegisterInfoArray struct into a QOM
     object and parented it to the device in `register_init_block'. This
     way it has its own finalize function that gets called when the
     parent device is finalized. This implies a small API change: the
     `register_finalize_block' function is removed (patch 2, 3, 4).

The CANFD model needed special care. It was rolling out its own version
of `register_init_block', including the discrepancies leading to the
memory leaks. This is because the register map of this device is
composed of main registers (from 0x0 to 0xec), followed by several banks
of multiple registers (Tx banks, filter banks, Txe banks, ...). The
register API is not suited for this kind of layout and the resulting
code to handle that is convoluted. However a simple decoding logic is
easily written for this, those registers having basically no side effect
upon access.

Patch 6 introduces this decoding logic for the banked registers, patch 7
removes the register API bits for those, keeping the one for the base
registers.

Note: this series is based on my Versal 2 series. It modifies the CRL
device during the register API refactoring. It can easily be rebased on
master if needed.

Thanks

Luc


Luc Michel (7):
  hw/core/register: remove the REGISTER device type
  hw/core/register: add the REGISTER_ARRAY type
  hw/core/register: remove the calls to `register_finalize_block'
  hw/core/register: remove the `register_finalize_block' function
  hw/net/can/xlnx-versal-canfd: remove unused include directives
  hw/net/can/xlnx-versal-canfd: refactor the banked registers logic
  hw/net/can/xlnx-versal-canfd: remove register API usage for banked
    regs

 include/hw/misc/xlnx-versal-crl.h      |   1 -
 include/hw/misc/xlnx-versal-xramc.h    |   1 -
 include/hw/misc/xlnx-zynqmp-apu-ctrl.h |   1 -
 include/hw/misc/xlnx-zynqmp-crf.h      |   1 -
 include/hw/net/xlnx-versal-canfd.h     |   8 -
 include/hw/nvram/xlnx-bbram.h          |   1 -
 include/hw/register.h                  |  25 +-
 hw/core/register.c                     |  36 +-
 hw/misc/xlnx-versal-crl.c              |  38 +--
 hw/misc/xlnx-versal-trng.c             |   1 -
 hw/misc/xlnx-versal-xramc.c            |  12 +-
 hw/misc/xlnx-zynqmp-apu-ctrl.c         |  12 +-
 hw/misc/xlnx-zynqmp-crf.c              |  12 +-
 hw/net/can/xlnx-versal-canfd.c         | 434 +++++++++----------------
 hw/nvram/xlnx-bbram.c                  |  13 +-
 hw/nvram/xlnx-versal-efuse-ctrl.c      |   1 -
 hw/nvram/xlnx-zynqmp-efuse.c           |   8 -
 17 files changed, 196 insertions(+), 409 deletions(-)

-- 
2.51.0

Hi Luc,

On 2/10/25 09:34, Luc Michel wrote:

> This series addresses the memory leaks caused by the register API. The
> first patches fix the API itself while the last ones take care of the
> CANFD model.


> Luc Michel (7):
>    hw/core/register: remove the REGISTER device type
>    hw/core/register: add the REGISTER_ARRAY type
>    hw/core/register: remove the calls to `register_finalize_block'
>    hw/core/register: remove the `register_finalize_block' function
>    hw/net/can/xlnx-versal-canfd: remove unused include directives
>    hw/net/can/xlnx-versal-canfd: refactor the banked registers logic
>    hw/net/can/xlnx-versal-canfd: remove register API usage for banked
>      regs

I had few issues with your series:

../../hw/net/can/xlnx-versal-canfd.c:1917:30: error: unused variable 
'canfd_regs_ops' [-Werror,-Wunused-const-variable]
  1917 | static const MemoryRegionOps canfd_regs_ops = {
       |                              ^~~~~~~~~~~~~~

../../hw/net/can/xlnx-versal-canfd.c:1871:42: error: use of undeclared 
identifier 'TYPE_REGISTER'
  1871 |         object_initialize(r, sizeof(*r), TYPE_REGISTER);
       |                                          ^

../../hw/net/can/xlnx-versal-canfd.c:1700:48: error: incompatible 
pointer types passing 'hwaddr *' (aka 'unsigned long long *') to 
parameter of type 'size_t *' (aka 'unsigned long *') 
[-Werror,-Wincompatible-pointer-types]
  1700 |     if (!canfd_decode_addr(s, addr, &bank_idx, &reg_offset)) {
       |                                                ^~~~~~~~~~~
../../hw/net/can/xlnx-versal-canfd.c:1651:52: note: passing argument to 
parameter 'offset' here
  1651 |                               size_t *idx, size_t *offset)
       |                                                    ^
../../hw/net/can/xlnx-versal-canfd.c:1722:48: error: incompatible 
pointer types passing 'hwaddr *' (aka 'unsigned long long *') to 
parameter of type 'size_t *' (aka 'unsigned long *') 
[-Werror,-Wincompatible-pointer-types]
  1722 |     if (!canfd_decode_addr(s, addr, &bank_idx, &reg_offset)) {
       |                                                ^~~~~~~~~~~
../../hw/net/can/xlnx-versal-canfd.c:1651:52: note: passing argument to 
parameter 'offset' here
  1651 |                               size_t *idx, size_t *offset)
       |                                                    ^

I fixed them by re-ordering the xlnx-versal-canfd patches first,
having canfd_decode_FOO() taking a 'hwaddr *offset' and using a
temporary __attribute__ ((unused)) for canfd_regs_ops[].

I'm queuing this series as fixed, except if you disagree.

Regards,

Phil.

On 9/10/25 08:23, Philippe Mathieu-Daudé wrote:
> Hi Luc,
> 
> On 2/10/25 09:34, Luc Michel wrote:
> 
>> This series addresses the memory leaks caused by the register API. The
>> first patches fix the API itself while the last ones take care of the
>> CANFD model.
> 
> 
>> Luc Michel (7):
>>    hw/core/register: remove the REGISTER device type
>>    hw/core/register: add the REGISTER_ARRAY type
>>    hw/core/register: remove the calls to `register_finalize_block'
>>    hw/core/register: remove the `register_finalize_block' function
>>    hw/net/can/xlnx-versal-canfd: remove unused include directives
>>    hw/net/can/xlnx-versal-canfd: refactor the banked registers logic
>>    hw/net/can/xlnx-versal-canfd: remove register API usage for banked
>>      regs
> 
> I had few issues with your series:
> 
> ../../hw/net/can/xlnx-versal-canfd.c:1917:30: error: unused variable 
> 'canfd_regs_ops' [-Werror,-Wunused-const-variable]
>   1917 | static const MemoryRegionOps canfd_regs_ops = {
>        |                              ^~~~~~~~~~~~~~
> 
> ../../hw/net/can/xlnx-versal-canfd.c:1871:42: error: use of undeclared 
> identifier 'TYPE_REGISTER'
>   1871 |         object_initialize(r, sizeof(*r), TYPE_REGISTER);
>        |                                          ^
> 
> ../../hw/net/can/xlnx-versal-canfd.c:1700:48: error: incompatible 
> pointer types passing 'hwaddr *' (aka 'unsigned long long *') to 
> parameter of type 'size_t *' (aka 'unsigned long *') [-Werror,- 
> Wincompatible-pointer-types]
>   1700 |     if (!canfd_decode_addr(s, addr, &bank_idx, &reg_offset)) {
>        |                                                ^~~~~~~~~~~
> ../../hw/net/can/xlnx-versal-canfd.c:1651:52: note: passing argument to 
> parameter 'offset' here
>   1651 |                               size_t *idx, size_t *offset)
>        |                                                    ^
> ../../hw/net/can/xlnx-versal-canfd.c:1722:48: error: incompatible 
> pointer types passing 'hwaddr *' (aka 'unsigned long long *') to 
> parameter of type 'size_t *' (aka 'unsigned long *') [-Werror,- 
> Wincompatible-pointer-types]
>   1722 |     if (!canfd_decode_addr(s, addr, &bank_idx, &reg_offset)) {
>        |                                                ^~~~~~~~~~~
> ../../hw/net/can/xlnx-versal-canfd.c:1651:52: note: passing argument to 
> parameter 'offset' here
>   1651 |                               size_t *idx, size_t *offset)
>        |                                                    ^
> 
> I fixed them by re-ordering the xlnx-versal-canfd patches first,
> having canfd_decode_FOO() taking a 'hwaddr *offset' and using a
> temporary __attribute__ ((unused)) for canfd_regs_ops[].
> 
> I'm queuing this series as fixed, except if you disagree.

Bah, qtest/device-introspect-test is failing:

# Testing device 'xlnx-zynqmp-efuse'
Broken pipe
../../tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from 
signal 11 (Segmentation fault: 11)
Abort trap: 6

(lldb) bt
* thread #5, stop reason = EXC_BAD_ACCESS (code=1, address=0x50)
   * frame #0: 0x00000001007dcf18 
qemu-system-aarch64`object_finalize_child_property(obj=<unavailable>, 
name=<unavailable>, opaque=0x00000001561f0c90) at object.c:1814:23
     frame #1: 0x00000001007d9ebc 
qemu-system-aarch64`object_property_del_all(obj=0x0000000138008000) at 
object.c:667:21 [inlined]
     frame #2: 0x00000001007d9e1c 
qemu-system-aarch64`object_finalize(data=0x0000000138008000) at 
object.c:728:5 [inlined]
     frame #3: 0x00000001007d9e14 
qemu-system-aarch64`object_unref(objptr=0x0000000138008000) at 
object.c:1232:9
     frame #4: 0x00000001008af32c 
qemu-system-aarch64`qmp_device_list_properties(typename=<unavailable>, 
errp=0x000000017002ac98) at qom-qmp-cmds.c:237:5
     frame #5: 0x000000010092d7a4 
qemu-system-aarch64`qmp_marshal_device_list_properties(args=0x0000000145809400, 
ret=0x0000000104827a18, errp=0x0000000104827a20) at 
qapi-commands-qdev.c:65:14
     frame #6: 0x0000000100948784 
qemu-system-aarch64`do_qmp_dispatch_bh(opaque=0x00000001048279e8) at 
qmp-dispatch.c:128:5

I'm dropping this series, please have a look.

Stashed changes:

-- >8 --
diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c
index 81615bc52a6..d559fc06804 100644
--- a/hw/net/can/xlnx-versal-canfd.c
+++ b/hw/net/can/xlnx-versal-canfd.c
@@ -1784,3 +1784,3 @@ static bool 
canfd_decode_reg_bank(XlnxVersalCANFDState *s, hwaddr addr,
                                    hwaddr first_reg, hwaddr last_reg,
-                                  size_t num_banks, size_t *idx, size_t 
*offset)
+                                  size_t num_banks, size_t *idx, hwaddr 
*offset)
  {
@@ -1809,3 +1809,3 @@ static bool 
canfd_decode_reg_bank(XlnxVersalCANFDState *s, hwaddr addr,
  static bool canfd_decode_addr(XlnxVersalCANFDState *s, hwaddr addr,
-                              size_t *idx, size_t *offset)
+                              size_t *idx, hwaddr *offset)
  {
@@ -1916,2 +1916,3 @@ static const MemoryRegionOps canfd_ops = {

+__attribute__ ((unused))
  static const MemoryRegionOps canfd_regs_ops = {
---