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

[Moshe Shemesh <moshe@nvidia.com>]

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.

> [ ... ]