Re: [PATCH V7 vfio 07/10] vfio/mlx5: Create and destroy page tracker object

[Yishai Hadas <yishaih@nvidia.com>]

On 06/09/2023 11:55, Cédric Le Goater wrote:
> Hello,
>
> On 9/8/22 20:34, Yishai Hadas wrote:
>> Add support for creating and destroying page tracker object.
>>
>> This object is used to control/report the device dirty pages.
>>
>> As part of creating the tracker need to consider the device capabilities
>> for max ranges and adapt/combine ranges accordingly.
>>
>> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
>> ---
>>   drivers/vfio/pci/mlx5/cmd.c | 147 ++++++++++++++++++++++++++++++++++++
>>   drivers/vfio/pci/mlx5/cmd.h |   1 +
>>   2 files changed, 148 insertions(+)
>>
>> diff --git a/drivers/vfio/pci/mlx5/cmd.c b/drivers/vfio/pci/mlx5/cmd.c
>> index 0a362796d567..f1cad96af6ab 100644
>> --- a/drivers/vfio/pci/mlx5/cmd.c
>> +++ b/drivers/vfio/pci/mlx5/cmd.c
>> @@ -410,6 +410,148 @@ int mlx5vf_cmd_load_vhca_state(struct 
>> mlx5vf_pci_core_device *mvdev,
>>       return err;
>>   }
>>   +static void combine_ranges(struct rb_root_cached *root, u32 
>> cur_nodes,
>> +               u32 req_nodes)
>> +{
>> +    struct interval_tree_node *prev, *curr, *comb_start, *comb_end;
>> +    unsigned long min_gap;
>> +    unsigned long curr_gap;
>> +
>> +    /* Special shortcut when a single range is required */
>> +    if (req_nodes == 1) {
>> +        unsigned long last;
>> +
>> +        curr = comb_start = interval_tree_iter_first(root, 0, 
>> ULONG_MAX);
>> +        while (curr) {
>> +            last = curr->last;
>> +            prev = curr;
>> +            curr = interval_tree_iter_next(curr, 0, ULONG_MAX);
>> +            if (prev != comb_start)
>> +                interval_tree_remove(prev, root);
>> +        }
>> +        comb_start->last = last;
>> +        return;
>> +    }
>> +
>> +    /* Combine ranges which have the smallest gap */
>> +    while (cur_nodes > req_nodes) {
>> +        prev = NULL;
>> +        min_gap = ULONG_MAX;
>> +        curr = interval_tree_iter_first(root, 0, ULONG_MAX);
>> +        while (curr) {
>> +            if (prev) {
>> +                curr_gap = curr->start - prev->last;
>> +                if (curr_gap < min_gap) {
>> +                    min_gap = curr_gap;
>> +                    comb_start = prev;
>> +                    comb_end = curr;
>> +                }
>> +            }
>> +            prev = curr;
>> +            curr = interval_tree_iter_next(curr, 0, ULONG_MAX);
>> +        }
>> +        comb_start->last = comb_end->last;
>> +        interval_tree_remove(comb_end, root);
>> +        cur_nodes--;
>> +    }
>> +}
>> +
>> +static int mlx5vf_create_tracker(struct mlx5_core_dev *mdev,
>> +                 struct mlx5vf_pci_core_device *mvdev,
>> +                 struct rb_root_cached *ranges, u32 nnodes)
>> +{
>> +    int max_num_range =
>> +        MLX5_CAP_ADV_VIRTUALIZATION(mdev, pg_track_max_num_range);
>> +    struct mlx5_vhca_page_tracker *tracker = &mvdev->tracker;
>> +    int record_size = MLX5_ST_SZ_BYTES(page_track_range);
>> +    u32 out[MLX5_ST_SZ_DW(general_obj_out_cmd_hdr)] = {};
>> +    struct interval_tree_node *node = NULL;
>> +    u64 total_ranges_len = 0;
>> +    u32 num_ranges = nnodes;
>> +    u8 log_addr_space_size;
>> +    void *range_list_ptr;
>> +    void *obj_context;
>> +    void *cmd_hdr;
>> +    int inlen;
>> +    void *in;
>> +    int err;
>> +    int i;
>> +
>> +    if (num_ranges > max_num_range) {
>> +        combine_ranges(ranges, nnodes, max_num_range);
>> +        num_ranges = max_num_range;
>> +    }
>> +
>> +    inlen = MLX5_ST_SZ_BYTES(create_page_track_obj_in) +
>> +                 record_size * num_ranges;
>> +    in = kzalloc(inlen, GFP_KERNEL);
>> +    if (!in)
>> +        return -ENOMEM;
>> +
>> +    cmd_hdr = MLX5_ADDR_OF(create_page_track_obj_in, in,
>> +                   general_obj_in_cmd_hdr);
>> +    MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, opcode,
>> +         MLX5_CMD_OP_CREATE_GENERAL_OBJECT);
>> +    MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, obj_type,
>> +         MLX5_OBJ_TYPE_PAGE_TRACK);
>> +    obj_context = MLX5_ADDR_OF(create_page_track_obj_in, in, 
>> obj_context);
>> +    MLX5_SET(page_track, obj_context, vhca_id, mvdev->vhca_id);
>> +    MLX5_SET(page_track, obj_context, track_type, 1);
>> +    MLX5_SET(page_track, obj_context, log_page_size,
>> +         ilog2(tracker->host_qp->tracked_page_size));
>> +    MLX5_SET(page_track, obj_context, log_msg_size,
>> +         ilog2(tracker->host_qp->max_msg_size));
>> +    MLX5_SET(page_track, obj_context, reporting_qpn, 
>> tracker->fw_qp->qpn);
>> +    MLX5_SET(page_track, obj_context, num_ranges, num_ranges);
>> +
>> +    range_list_ptr = MLX5_ADDR_OF(page_track, obj_context, 
>> track_range);
>> +    node = interval_tree_iter_first(ranges, 0, ULONG_MAX);
>> +    for (i = 0; i < num_ranges; i++) {
>> +        void *addr_range_i_base = range_list_ptr + record_size * i;
>> +        unsigned long length = node->last - node->start;
>> +
>> +        MLX5_SET64(page_track_range, addr_range_i_base, start_address,
>> +               node->start);
>> +        MLX5_SET64(page_track_range, addr_range_i_base, length, 
>> length);
>> +        total_ranges_len += length;
>> +        node = interval_tree_iter_next(node, 0, ULONG_MAX);
>> +    }
>> +
>> +    WARN_ON(node);
>> +    log_addr_space_size = ilog2(total_ranges_len);
>> +    if (log_addr_space_size <
>> +        (MLX5_CAP_ADV_VIRTUALIZATION(mdev, 
>> pg_track_log_min_addr_space)) ||
>> +        log_addr_space_size >
>> +        (MLX5_CAP_ADV_VIRTUALIZATION(mdev, 
>> pg_track_log_max_addr_space))) {
>> +        err = -EOPNOTSUPP;
>> +        goto out;
>> +    }
>
>
> We are seeing an issue with dirty page tracking when doing migration
> of an OVMF VM guest. The vfio-pci variant driver for the MLX5 VF
> device complains when dirty page tracking is initialized from QEMU :
>
>   qemu-kvm: 0000:b1:00.2: Failed to start DMA logging, err -95 
> (Operation not supported)
>
> The 64-bit computed range is  :
>
>   vfio_device_dirty_tracking_start nr_ranges 2 32:[0x0 - 0x807fffff], 
> 64:[0x100000000 - 0x3838000fffff]
>
> which seems to be too large for the HW. AFAICT, the MLX5 HW has a 42
> bits address space limitation for dirty tracking (min is 12). Is it a
> FW tunable or a strict limitation ?

It's mainly a FW limitation.

Tracking larger address space than 2^42 might take a lot of time in FW 
to allocate the required resources which might end-up in command 
timeout, etc.

>
> We should probably introduce more ranges to overcome the issue.

More ranges can help only if the total address space of the given ranges 
is < 2^42.

So, if there are some areas that don't require tracking (why?), breaking 
into more ranges with smaller total size can help.

Yishai