Re: [PATCH 0/7] Register API leaks fixes

["Edgar E. Iglesias" <edgar.iglesias@amd.com>]

On Wed, Sep 17, 2025 at 01:44:41PM +0200, Luc Michel wrote:
> Based-on: 20250912100059.103997-1-luc.michel@amd.com ([PATCH v5 00/47] AMD Versal Gen 2 support)
> 
> 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


Hi Luc,

Entire series looks good to me!
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>



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