Re: [PATCH vfio 10/13] vfio/mlx5: Introduce SW headers for migration states

[Jason Gunthorpe <jgg@nvidia.com>]

On Sun, Nov 06, 2022 at 07:46:27PM +0200, Yishai Hadas wrote:
> From: Shay Drory <shayd@nvidia.com>
> 
> As mentioned in the previous patches, mlx5 is transferring multiple
> states when the PRE_COPY protocol is used. This states mechanism
> requires the target VM to know the states' size in order to execute
> multiple loads.
> Therefore, add SW header, with the needed information, for each saved
> state the source VM is transferring to the target VM.
> 
> This patch implements the source VM handling of the headers, following
> patch will implement the target VM handling of the headers.
> 
> Signed-off-by: Shay Drory <shayd@nvidia.com>
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> ---
>  drivers/vfio/pci/mlx5/cmd.h  |  7 +++++
>  drivers/vfio/pci/mlx5/main.c | 50 +++++++++++++++++++++++++++++++++---
>  2 files changed, 54 insertions(+), 3 deletions(-)

This seems very awkwardly done. If you are going to string together
sg_tables, then just do that consistently. Put the header into its own
small sg_table as well and have a simple queue of sg_table/starting
offset/length chunks to let read figure out on its own. When you start
new stuff you stick it to the back of the queue. eg when you get the
incremental stick on two sg_tables to the queue. when you get to the
final stick on the final sg_table to the back of the queue. Whatever
has or hasn't happened just works out naturally.

There are too many special cases trying to figure out which chunk is
the right chunk.

>  #define MIGF_TOTAL_DATA(migf) \
> -	(migf->table_start_pos + migf->image_length + migf->final_length)
> +	(migf->table_start_pos + migf->image_length + migf->final_length + \
> +	 migf->sw_headers_bytes_sent)

And make all these macros static inline functions in all the patches -
they don't even have proper argument bracketing..

> +static void mlx5vf_send_sw_header(struct mlx5_vf_migration_file *migf,
> +				  loff_t *pos, char __user **buf, size_t *len,
> +				  ssize_t *done)
> +{
> +	struct mlx5_vf_migration_header header = {};
> +	size_t header_size = sizeof(header);
> +	void *header_buf = &header;
> +	size_t size_to_transfer;
> +
> +	if (*pos >= mlx5vf_final_table_start_pos(migf))
> +		header.image_size = migf->final_length;
> +	else
> +		header.image_size = migf->image_length;
> +
> +	size_to_transfer = header_size -
> +			   (migf->sw_headers_bytes_sent % header_size);
> +	size_to_transfer = min_t(size_t, size_to_transfer, *len);
> +	header_buf += header_size - size_to_transfer;
> +	if (copy_to_user(*buf, header_buf, size_to_transfer)) {
> +		*done = -EFAULT;

A function that has errors should return a value, not something like
this..

Jason