Re: [PATCH net-next 3/3] net/mlx5: Add VHCA_ID page management mode support

[Moshe Shemesh]

On 5/3/2026 4:45 AM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net/mlx5: Add VHCA_ID page management mode support
>
> This change adds a VHCA_ID-based page management mode that uses vhca_id
> instead of function_id as the effective function identifier when the
> firmware advertises icm_mng_function_id_mode. Boot pages keep FUNC_ID
> semantics for backward compatibility, and tracked pages are migrated to
> the vhca_id key after set_hca_cap().
>
> A few questions below on the per-type counter accounting, the boot-page
> migration helper, and locking on the FW-driven page path.
>
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > index e0eafcf0c52a..d3eaefc5c0e0 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > @@ -852,6 +852,48 @@ bool mlx5_esw_vport_vhca_id(struct mlx5_eswitch *esw, u16 vportn, u16 *vhca_id)
> >        return true;
> >   }
> >
> > +u16 mlx5_esw_vhca_id_to_func_type(struct mlx5_core_dev *dev, u16 vhca_id)
> > +{
> > +     struct mlx5_eswitch *esw = dev->priv.eswitch;
> > +     struct mlx5_vport *vport;
> > +     unsigned long i;
> > +     u16 type;
> > +
> > +     if (vhca_id == MLX5_CAP_GEN(dev, vhca_id))
> > +             return MLX5_SELF;
> > +
> > +     if (!esw)
> > +             return MLX5_FUNC_TYPE_NONE;
> > +
> > +     mutex_lock(&esw->state_lock);
>
> This function is reached from give_pages()/reclaim_pages()/
> release_all_pages() via func_vhca_id_to_type(), which in turn runs
> from the pg_wq work handler triggered by firmware page-request EQE
> events. Does acquiring esw->state_lock on that path introduce a new
> lock dependency?
>
> Several eswitch paths (for example mlx5_esw_vport_enable(),
> mlx5_esw_vport_disable(), mlx5_eswitch_set_vport_mac()) hold
> state_lock while synchronously issuing firmware commands. Before this
> patch, the page path held no eswitch locks.
>
> Would it be safer to resolve the func_type outside of state_lock, for
> example by caching the vhca_id-to-type mapping separately, or by
> attaching the resolved type to the fw_page at give time so reclaim
> does not need to look it up again?

I don't think there is a real issue with the lock, but for sure better 
without, I will try caching vhca_id to type mapping.

> > +     mlx5_esw_for_each_vport(esw, i, vport) {
> > +             if (vport->vhca_id != vhca_id)
> > +                     continue;
> > +
> > +             if (vport->vport == MLX5_VPORT_HOST_PF) {
> > +                     type = MLX5_HOST_PF;
> > +                     goto unlock;
> > +             }
> > +
> > +             if (xa_get_mark(&esw->vports, i, MLX5_ESW_VPT_SF)) {
> > +                     type = MLX5_SF;
> > +                     goto unlock;
> > +             }
> > +
> > +             if (xa_get_mark(&esw->vports, i, MLX5_ESW_VPT_VF)) {
> > +                     type = MLX5_VF;
> > +                     goto unlock;
> > +             }
> > +
> > +             type = MLX5_EC_VF;
> > +             goto unlock;
>
> Can this fall-through misattribute counters?
>
> If a matching vport is not HOST_PF and has neither the VPT_SF nor the
> VPT_VF mark, it is unconditionally classified as MLX5_EC_VF. Would an
> UPLINK, ECPF, manager, or adjacent-style vport whose vhca_id happens
> to match end up charged to the EC_VF bucket here?

The fallthrough is correct in practice  - uplink is the physical port, 
not a separate HCA function, FW doesn't request pages for it. ECPF is 
the esw manager, if requested here can be only as self, will catch the 
MLX5_SELF condition. Adjacent VFs are caught correctly by the VF mark.

> > +     }
> > +     type = MLX5_FUNC_TYPE_NONE;
> > +unlock:
> > +     mutex_unlock(&esw->state_lock);
> > +     return type;
> > +}
> > +
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > index a242053f3a58..52cf341ad6b3 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > @@ -597,6 +597,9 @@ static int handle_hca_cap(struct mlx5_core_dev *dev, void *set_ctx)
> >        if (MLX5_CAP_GEN_MAX(dev, release_all_pages))
> >                MLX5_SET(cmd_hca_cap, set_hca_cap, release_all_pages, 1);
> >
> > +     if (MLX5_CAP_GEN_MAX(dev, icm_mng_function_id_mode))
> > +             MLX5_SET(cmd_hca_cap, set_hca_cap, icm_mng_function_id_mode, 1);
> > +
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> > index 77ffa31cc505..7ebe88aa3b3e 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
>
> [ ... ]
>
> > @@ -89,12 +105,21 @@ static u16 func_id_to_type(struct mlx5_core_dev *dev, u16 func_id, bool ec_funct
> >        return MLX5_SF;
> >   }
> >
> > +static u16 func_vhca_id_to_type(struct mlx5_core_dev *dev, u16 func_vhca_id,
> > +                             bool ec_function)
> > +{
> > +     if (mlx5_page_mgt_mode_is_vhca_id(dev))
> > +             return mlx5_esw_vhca_id_to_func_type(dev, func_vhca_id);
> > +
> > +     return func_id_to_type(dev, func_vhca_id, ec_function);
> > +}
>
> Can the give/reclaim accounting become asymmetric in VHCA_ID mode?
>
> The per-type counters are only updated when the resolved type is not
> MLX5_FUNC_TYPE_NONE:
>
>          func_type = func_vhca_id_to_type(dev, func_vhca_id, ec_function);
>          if (func_type != MLX5_FUNC_TYPE_NONE)
>                  dev->priv.page_counters[func_type] += npages;
>
> Since mlx5_esw_vhca_id_to_func_type() walks the eswitch vport table
> dynamically on every call, can the give and the corresponding reclaim
> resolve to different types?
>
> For example, if a give runs before vport->vhca_id is populated in
> mlx5_esw_vport_caps_get(), the lookup returns MLX5_FUNC_TYPE_NONE and
> the counter is not incremented. Later, when the vport is fully
> populated, reclaim resolves to a real type and decrements the counter
> below the amount ever added, which on a u32 drives it to a very large
> value.
>
> Similarly, if a vport is removed before reclaim, the increment at
> give time is recorded but the decrement at reclaim is skipped, so the
> counter leaks upward.
>
> Would caching the func_type on the fw_page at give time and reusing
> it on reclaim make the accounting symmetric by construction?

Following first comment, I will try caching.

> [ ... ]
>
> > @@ -658,30 +708,101 @@ static int req_pages_handler(struct notifier_block *nb,
> >         * req->npages (and not min ()).
> >         */
> >        req->npages = max_t(s32, npages, MAX_RECLAIM_NPAGES);
> > -     req->ec_function = ec_function;
> > +     if (!mlx5_page_mgt_mode_is_vhca_id(dev))
> > +             req->ec_function = ec_function;
> >        req->release_all = release_all;
> >        INIT_WORK(&req->work, pages_work_handler);
> >        queue_work(dev->priv.pg_wq, &req->work);
> >        return NOTIFY_OK;
> >   }
> >
> > +/*
> > + * After set_hca_cap(), the second satisfy_startup_pages(dev, 0) may see
> > + * VHCA_ID mode. If page_root_xa already has the PF entry from the first
> > + * (boot) call under FUNC_ID keys 0 or (ec_function << 16), migrate that
> > + * entry to the device vhca_id key so lookups use VHCA_ID semantics.
> > + */
> > +static int mlx5_pagealloc_migrate_pf_to_vhca_id(struct mlx5_core_dev *dev)
> > +{
> > +     u32 vhca_id_key, old_key;
> > +     struct rb_root *root;
> > +     struct fw_page *fwp;
> > +     struct rb_node *p;
> > +     bool ec_function;
> > +     int err;
> > +
> > +     if (xa_empty(&dev->priv.page_root_xa))
> > +             return 0;
> > +
> > +     vhca_id_key = MLX5_CAP_GEN(dev, vhca_id);
> > +     ec_function = mlx5_core_is_ecpf(dev);
> > +
> > +     old_key = ec_function ? (1U << 16) : 0;
> > +     root = xa_load(&dev->priv.page_root_xa, old_key);
> > +     if (!root)
> > +             return 0;
>
> Does this assume the boot-path func_vhca_id was always 0?
>
> The boot call to mlx5_cmd_query_pages() reads func_vhca_id directly
> from the firmware output, and give_pages() then uses that value to
> compute the key. The migration here instead hardcodes old_key as
> ec_function ? (1U << 16) : 0.
>
> If firmware returned a non-zero boot function_id, xa_load(old_key)
> returns NULL, the function silently returns 0, the caller flips the
> mode to VHCA_ID, and the original rb_root is orphaned in page_root_xa
> under the old key. Subsequent free_fwp()/find_fw_page() paths would
> then hit WARN_ON_ONCE(!root) and leak the DMA mappings and pages.
>
> Would it be more robust to look up the actual key used at boot
> (derived from the stored func_vhca_id), and to treat the "xa not
> empty but old_key absent" case as an invariant violation rather than
> silently succeeding?

During boot (mlx5_satisfy_startup_pages(dev, 1)), the mode is FUNC_ID. 
The mlx5_cmd_query_pages() call sets embedded_cpu_function in the 
request but leaves function_id as 0 (the PF queries its own pages). 
Firmware returns function_id in the output, which for the PF's own pages 
is always 0.
The "silently returns 0" when xa_load returns NULL is also fine, it 
means no boot pages were allocated.

> > +
> > +     if (old_key == vhca_id_key)
> > +             return 0;
> > +
> > +     err = xa_insert(&dev->priv.page_root_xa, vhca_id_key, root, GFP_KERNEL);
> > +     if (err) {
> > +             mlx5_core_warn(dev,
> > +                            "failed to migrate page root key 0x%x to vhca_id 0x%x\n",
> > +                            old_key, vhca_id_key);
> > +             return err;
> > +     }
> > +
> > +     xa_erase(&dev->priv.page_root_xa, old_key);
> > +
> > +     for (p = rb_first(root); p; p = rb_next(p)) {
> > +             fwp = rb_entry(p, struct fw_page, rb_node);
> > +             fwp->function = vhca_id_key;
> > +     }
>
> Is the ordering here safe against any concurrent free_fwp()?
>
> Between xa_erase(old_key) and the loop that updates fwp->function,
> every fw_page still carries the old key while page_root_xa no longer
> resolves it. If a free_fwp() were to run in that window:
>
>          root = xa_load(&dev->priv.page_root_xa, fwp->function);
>          if (WARN_ON_ONCE(!root))
>                  return;
>
> it would return early, skipping dma_unmap_page(), __free_page(), and
> kfree(fwp), leaking the DMA mapping and the backing page.
>
> No concurrent free path is structurally reachable today because this
> runs before the EQ notifier is registered in mlx5_pagealloc_start(),
> but would it be cleaner to update the fwp->function values first,
> then swap the xarray entries (or store the new value at a single key)
> so the two views cannot disagree?

Reordering is safer, I will reorder.

> > +
> > +     return 0;
> > +}
> > +
> >   int mlx5_satisfy_startup_pages(struct mlx5_core_dev *dev, int boot)
> >   {
> > -     u16 func_id;
> > +     bool ec_function = false;
> > +     u16 func_vhca_id;
> >        s32 npages;
> >        int err;
> >
> > -     err = mlx5_cmd_query_pages(dev, &func_id, &npages, boot);
> > +     /* When boot flag is set, the icm_mng_function_id_mode capability is
> > +      * not yet set (only set after set_hca_cap()), so use FUNC_ID mode
> > +      * for backward compatibility. When boot is false, set mode from
> > +      * cap (set_hca_cap has run successfully).
> > +      */
> > +     if (boot) {
> > +             mlx5_page_mgt_mode_set(dev, MLX5_PAGE_MGT_MODE_FUNC_ID);
> > +     } else {
> > +             if (MLX5_CAP_GEN_MAX(dev, icm_mng_function_id_mode) ==
> > +                 MLX5_ID_MODE_FUNCTION_VHCA_ID) {
>
> The comment just above says "set mode from cap (set_hca_cap has run
> successfully)", which reads as "use the current/negotiated cap value",
> but the check uses MLX5_CAP_GEN_MAX rather than MLX5_CAP_GEN. The
> sibling code in drivers/net/ethernet/mellanox/mlx5/core/debugfs.c uses
> MLX5_CAP_GEN(dev, icm_mng_function_id_mode) for the same semantic
> check.
>
> Could the comment and the _MAX usage be made consistent? If anyone
> later adds a conditional around the MLX5_SET() in handle_hca_cap()
> (for example a module parameter), the driver would start flipping to
> VHCA_ID mode based on _MAX without actually having enabled the feature
> in firmware.

OK, I will rewrite the comment.

> > +                     err = mlx5_pagealloc_migrate_pf_to_vhca_id(dev);
> > +                     if (err)
> > +                             return err;
> > +                     mlx5_page_mgt_mode_set(dev, MLX5_PAGE_MGT_MODE_VHCA_ID);
> > +             }
> > +     }
>
> Once page_mgt_mode is flipped to MLX5_PAGE_MGT_MODE_VHCA_ID, it stays
> set in dev->priv and is only reset on the next boot=1 call. Between
> teardown and the next boot=1 path (for example during health
> recovery, PCI reset, or any async flow that runs page work before
> mlx5_satisfy_startup_pages(dev, 1) re-runs), would a FW page-request
> EQE observe a stale mode?
>
> The req_pages EQ notifier is registered via mlx5_pagealloc_start()
> and unregistered via mlx5_pagealloc_stop(), so this is not reachable
> today, but would it be worth resetting the mode explicitly on the
> teardown side rather than relying on the next reinit?

No. Once flipped to MLX5_PAGE_MGT_MODE_VHCA_ID, both driver and firmware 
keep using it till reopen function (next boot=1 call).
> [ ... ]
>
> > @@ -751,6 +874,9 @@ int mlx5_reclaim_startup_pages(struct mlx5_core_dev *dev)
> >        WARN(dev->priv.fw_pages,
> >             "FW pages counter is %d after reclaiming all pages\n",
> >             dev->priv.fw_pages);
> > +     if (mlx5_page_mgt_mode_is_vhca_id(dev) && !dev->priv.eswitch)
> > +             return 0;
> > +
>
> Does this guard address the asymmetric-counter case raised above?
>
> It only skips the per-type WARNs when the eswitch is entirely absent.
> In the common case where the eswitch is present but a vport's
> vhca_id/marks change between give and reclaim, the counters can still
> drift and these WARNs would still fire on normal teardown paths.

Following first comment, will try caching.

> [ ... ]