[PATCH v3 0/6] Register API leaks fixes

Luc Michel posted 6 patches 4 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20251017161809.235740-1-luc.michel@amd.com
Maintainers: Alistair Francis <alistair@alistair23.me>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Peter Maydell <peter.maydell@linaro.org>, Francisco Iglesias <francisco.iglesias@amd.com>, Vikram Garhwal <vikram.garhwal@bytedance.com>, Pavel Pisa <pisa@cmp.felk.cvut.cz>, Jason Wang <jasowang@redhat.com>
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                     |  38 +--
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         | 430 +++++++++----------------
hw/nvram/xlnx-bbram.c                  |  13 +-
hw/nvram/xlnx-versal-efuse-ctrl.c      |   1 -
hw/nvram/xlnx-zynqmp-efuse.c           |   8 -
17 files changed, 197 insertions(+), 406 deletions(-)
[PATCH v3 0/6] Register API leaks fixes
Posted by Luc Michel 4 weeks ago
v3:
  - Rebased on master
  - Fixed compilation issues in intermediate patches [Phil]
  - Parent the memory region in the REGISTER_ARRAY object to the
    REGISTER_ARRAY object itself instead of the REGISTER_ARRAY owner.
    This ensure correct finalizing order and fixes the use-after-free
    encountered by Phil [Phil]

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.

Thanks

Luc

Luc Michel (6):
  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: 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                     |  38 +--
 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         | 430 +++++++++----------------
 hw/nvram/xlnx-bbram.c                  |  13 +-
 hw/nvram/xlnx-versal-efuse-ctrl.c      |   1 -
 hw/nvram/xlnx-zynqmp-efuse.c           |   8 -
 17 files changed, 197 insertions(+), 406 deletions(-)

-- 
2.51.0
Re: [PATCH v3 0/6] Register API leaks fixes
Posted by Philippe Mathieu-Daudé 3 weeks, 3 days ago
On 17/10/25 18:17, Luc Michel wrote:
> v3:
>    - Rebased on master
>    - Fixed compilation issues in intermediate patches [Phil]
>    - Parent the memory region in the REGISTER_ARRAY object to the
>      REGISTER_ARRAY object itself instead of the REGISTER_ARRAY owner.
>      This ensure correct finalizing order and fixes the use-after-free
>      encountered by Phil [Phil]


> Luc Michel (6):
>    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: refactor the banked registers logic
>    hw/net/can/xlnx-versal-canfd: remove register API usage for banked
>      regs

Thanks, queued squashing on patch #5 ...:

-- >8 --
--- a/hw/net/can/xlnx-versal-canfd.c
+++ b/hw/net/can/xlnx-versal-canfd.c
@@ -1411,18 +1411,17 @@ static uint64_t canfd_srr_pre_write(RegisterInfo 
*reg, uint64_t val64)
  }

  static void filter_reg_write(XlnxVersalCANFDState *s, hwaddr addr,
-                             size_t bank_idx, uint32_t val)
+                             unsigned bank_idx, uint32_t val)
  {
      size_t reg_idx = addr / sizeof(uint32_t);

      if (!(s->regs[R_ACCEPTANCE_FILTER_CONTROL_REGISTER] &
          (1 << bank_idx))) {
          s->regs[reg_idx] = val;
      } else {
          g_autofree char *path = object_get_canonical_path(OBJECT(s));

          qemu_log_mask(LOG_GUEST_ERROR, "%s: Acceptance filter register 
0x%"
-                      HWADDR_PRIx " changed while filter %zu enabled\n",
+                      HWADDR_PRIx " changed while filter %u enabled\n",
                        path, addr, bank_idx + 1);
      }
  }
@@ -1782,16 +1781,19 @@ static void xlnx_versal_canfd_ptimer_cb(void 
*opaque)

  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, unsigned *idx,
+                                  hwaddr *offset)
  {
      hwaddr base = addr - first_reg;
      hwaddr span = last_reg - first_reg + sizeof(uint32_t);
+    unsigned index = base / span;

-    *idx = base / span;
-
-    if (*idx >= num_banks) {
+    if (index >= num_banks) {
          return false;
      }
+    if (idx) {
+        *idx = index;
+    }

      *offset = base % span;
      *offset += first_reg;
@@ -1807,7 +1809,7 @@ static bool 
canfd_decode_reg_bank(XlnxVersalCANFDState *s, hwaddr addr,
   * @return true is the decoding succeded, false otherwise
   */
  static bool canfd_decode_addr(XlnxVersalCANFDState *s, hwaddr addr,
-                              size_t *idx, hwaddr *offset)
+                              unsigned *idx, hwaddr *offset)
  {
      if (addr <= A_RX_FIFO_WATERMARK_REGISTER) {
          /* from 0x0 to 0xec. Handled by the register API */
@@ -1852,11 +1854,10 @@ static bool 
canfd_decode_addr(XlnxVersalCANFDState *s, hwaddr addr,
  static uint64_t canfd_read(void *opaque, hwaddr addr, unsigned size)
  {
      XlnxVersalCANFDState *s = XILINX_CANFD(opaque);
-    size_t bank_idx;
      hwaddr reg_offset;
      uint64_t ret;

-    if (!canfd_decode_addr(s, addr, &bank_idx, &reg_offset)) {
+    if (!canfd_decode_addr(s, addr, NULL, &reg_offset)) {
          qemu_log_mask(LOG_GUEST_ERROR, TYPE_XILINX_CANFD
                        ": read to unknown register at address 0x%"
                        HWADDR_PRIx "\n", addr);
@@ -1875,7 +1876,7 @@ static void canfd_write(void *opaque, hwaddr addr, 
uint64_t value,
                          unsigned size)
  {
      XlnxVersalCANFDState *s = XILINX_CANFD(opaque);
-    size_t bank_idx;
+    unsigned bank_idx;
      hwaddr reg_offset;

      if (!canfd_decode_addr(s, addr, &bank_idx, &reg_offset)) {
---

... in order to avoid:

hw/net/can/xlnx-versal-canfd.c:1822:59: error: incompatible pointer 
types passing 'hwaddr *' (aka 'unsigned long long *') to parameter of 
type 'size_t *' (aka 'unsigned long *') 
[-Werror,-Wincompatible-pointer-types]
  1822 |                                      s->cfg.tx_fifo, idx, offset);
       |                                                           ^~~~~~
hw/net/can/xlnx-versal-canfd.c:1785:74: note: passing argument to 
parameter 'offset' here
  1785 |                                   size_t num_banks, size_t 
*idx, size_t *offset)
       | 
          ^
hw/net/can/xlnx-versal-canfd.c:1827:47: error: incompatible pointer 
types passing 'hwaddr *' (aka 'unsigned long long *') to parameter of 
type 'size_t *' (aka 'unsigned long *') 
[-Werror,-Wincompatible-pointer-types]
  1827 |                                      32, idx, offset);
       |                                               ^~~~~~
hw/net/can/xlnx-versal-canfd.c:1785:74: note: passing argument to 
parameter 'offset' here
  1785 |                                   size_t num_banks, size_t 
*idx, size_t *offset)
       | 
          ^
hw/net/can/xlnx-versal-canfd.c:1833:47: error: incompatible pointer 
types passing 'hwaddr *' (aka 'unsigned long long *') to parameter of 
type 'size_t *' (aka 'unsigned long *') 
[-Werror,-Wincompatible-pointer-types]
  1833 |                                      32, idx, offset);
       |                                               ^~~~~~
hw/net/can/xlnx-versal-canfd.c:1785:74: note: passing argument to 
parameter 'offset' here
  1785 |                                   size_t num_banks, size_t 
*idx, size_t *offset)
       | 
          ^
hw/net/can/xlnx-versal-canfd.c:1839:60: error: incompatible pointer 
types passing 'hwaddr *' (aka 'unsigned long long *') to parameter of 
type 'size_t *' (aka 'unsigned long *') 
[-Werror,-Wincompatible-pointer-types]
  1839 |                                      s->cfg.rx0_fifo, idx, offset);
       |                                                            ^~~~~~
hw/net/can/xlnx-versal-canfd.c:1785:74: note: passing argument to 
parameter 'offset' here
  1785 |                                   size_t num_banks, size_t 
*idx, size_t *offset)
       | 
          ^
hw/net/can/xlnx-versal-canfd.c:1845:60: error: incompatible pointer 
types passing 'hwaddr *' (aka 'unsigned long long *') to parameter of 
type 'size_t *' (aka 'unsigned long *') 
[-Werror,-Wincompatible-pointer-types]
  1845 |                                      s->cfg.rx1_fifo, idx, offset);
       |                                                            ^~~~~~
hw/net/can/xlnx-versal-canfd.c:1785:74: note: passing argument to 
parameter 'offset' here
  1785 |                                   size_t num_banks, size_t 
*idx, size_t *offset)
       | 
          ^
5 errors generated.