Re: [PATCH v2 1/3] net/virtio_user: avoid cq descriptor buffer address accessing

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 2/29/24 14:29, Srujana Challa wrote:
> This patch makes changes to avoid descriptor buffer address
> accessing while processing shadow control queue.
> So that Virtio-user can work with having IOVA as descriptor
> buffer address.
> 
> Signed-off-by: Srujana Challa <schalla@marvell.com>
> ---
>   .../net/virtio/virtio_user/virtio_user_dev.c  | 68 +++++++++----------
>   1 file changed, 33 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index d395fc1676..bf3da4340f 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -885,11 +885,11 @@ static uint32_t
>   virtio_user_handle_ctrl_msg_split(struct virtio_user_dev *dev, struct vring *vring,
>   			    uint16_t idx_hdr)
>   {
> -	struct virtio_net_ctrl_hdr *hdr;
>   	virtio_net_ctrl_ack status = ~0;
> -	uint16_t i, idx_data, idx_status;
> +	uint16_t i, idx_data;
>   	uint32_t n_descs = 0;
>   	int dlen[CVQ_MAX_DATA_DESCS], nb_dlen = 0;
> +	struct virtio_pmd_ctrl *ctrl;
>   
>   	/* locate desc for header, data, and status */
>   	idx_data = vring->desc[idx_hdr].next;
> @@ -902,34 +902,33 @@ virtio_user_handle_ctrl_msg_split(struct virtio_user_dev *dev, struct vring *vri
>   		n_descs++;
>   	}
>   
> -	/* locate desc for status */
> -	idx_status = i;
>   	n_descs++;
>   
> -	hdr = (void *)(uintptr_t)vring->desc[idx_hdr].addr;
> -	if (hdr->class == VIRTIO_NET_CTRL_MQ &&
> -	    hdr->cmd == VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET) {
> -		uint16_t queues;
> +	/* Access control command via VA from CVQ */
> +	ctrl = (struct virtio_pmd_ctrl *)dev->hw.cvq->hdr_mz->addr;
> +	if (ctrl->hdr.class == VIRTIO_NET_CTRL_MQ &&
> +	    ctrl->hdr.cmd == VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET) {
> +		uint16_t *queues;

This is not future proof, as it just discards the index and assume the
buffer will always be at the same place.
We should find a way to perform the desc address translation.

>   
> -		queues = *(uint16_t *)(uintptr_t)vring->desc[idx_data].addr;
> -		status = virtio_user_handle_mq(dev, queues);
> -	} else if (hdr->class == VIRTIO_NET_CTRL_MQ && hdr->cmd == VIRTIO_NET_CTRL_MQ_RSS_CONFIG) {
> +		queues = (uint16_t *)ctrl->data;
> +		status = virtio_user_handle_mq(dev, *queues);
> +	} else if (ctrl->hdr.class == VIRTIO_NET_CTRL_MQ &&
> +		   ctrl->hdr.cmd == VIRTIO_NET_CTRL_MQ_RSS_CONFIG) {
>   		struct virtio_net_ctrl_rss *rss;
>   
> -		rss = (struct virtio_net_ctrl_rss *)(uintptr_t)vring->desc[idx_data].addr;
> +		rss = (struct virtio_net_ctrl_rss *)ctrl->data;
>   		status = virtio_user_handle_mq(dev, rss->max_tx_vq);
> -	} else if (hdr->class == VIRTIO_NET_CTRL_RX  ||
> -		   hdr->class == VIRTIO_NET_CTRL_MAC ||
> -		   hdr->class == VIRTIO_NET_CTRL_VLAN) {
> +	} else if (ctrl->hdr.class == VIRTIO_NET_CTRL_RX  ||
> +		   ctrl->hdr.class == VIRTIO_NET_CTRL_MAC ||
> +		   ctrl->hdr.class == VIRTIO_NET_CTRL_VLAN) {
>   		status = 0;
>   	}
>   
>   	if (!status && dev->scvq)
> -		status = virtio_send_command(&dev->scvq->cq,
> -				(struct virtio_pmd_ctrl *)hdr, dlen, nb_dlen);
> +		status = virtio_send_command(&dev->scvq->cq, ctrl, dlen, nb_dlen);
>   
>   	/* Update status */
> -	*(virtio_net_ctrl_ack *)(uintptr_t)vring->desc[idx_status].addr = status;
> +	ctrl->status = status;
>   
>   	return n_descs;
>   }
> @@ -948,7 +947,7 @@ virtio_user_handle_ctrl_msg_packed(struct virtio_user_dev *dev,
>   				   struct vring_packed *vring,
>   				   uint16_t idx_hdr)
>   {
> -	struct virtio_net_ctrl_hdr *hdr;
> +	struct virtio_pmd_ctrl *ctrl;
>   	virtio_net_ctrl_ack status = ~0;
>   	uint16_t idx_data, idx_status;
>   	/* initialize to one, header is first */
> @@ -971,32 +970,31 @@ virtio_user_handle_ctrl_msg_packed(struct virtio_user_dev *dev,
>   		n_descs++;
>   	}
>   
> -	hdr = (void *)(uintptr_t)vring->desc[idx_hdr].addr;
> -	if (hdr->class == VIRTIO_NET_CTRL_MQ &&
> -	    hdr->cmd == VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET) {
> -		uint16_t queues;
> +	/* Access control command via VA from CVQ */
> +	ctrl = (struct virtio_pmd_ctrl *)dev->hw.cvq->hdr_mz->addr;
> +	if (ctrl->hdr.class == VIRTIO_NET_CTRL_MQ &&
> +	    ctrl->hdr.cmd == VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET) {
> +		uint16_t *queues;
>   
> -		queues = *(uint16_t *)(uintptr_t)
> -				vring->desc[idx_data].addr;
> -		status = virtio_user_handle_mq(dev, queues);
> -	} else if (hdr->class == VIRTIO_NET_CTRL_MQ && hdr->cmd == VIRTIO_NET_CTRL_MQ_RSS_CONFIG) {
> +		queues = (uint16_t *)ctrl->data;
> +		status = virtio_user_handle_mq(dev, *queues);
> +	} else if (ctrl->hdr.class == VIRTIO_NET_CTRL_MQ &&
> +		   ctrl->hdr.cmd == VIRTIO_NET_CTRL_MQ_RSS_CONFIG) {
>   		struct virtio_net_ctrl_rss *rss;
>   
> -		rss = (struct virtio_net_ctrl_rss *)(uintptr_t)vring->desc[idx_data].addr;
> +		rss = (struct virtio_net_ctrl_rss *)ctrl->data;
>   		status = virtio_user_handle_mq(dev, rss->max_tx_vq);
> -	} else if (hdr->class == VIRTIO_NET_CTRL_RX  ||
> -		   hdr->class == VIRTIO_NET_CTRL_MAC ||
> -		   hdr->class == VIRTIO_NET_CTRL_VLAN) {
> +	} else if (ctrl->hdr.class == VIRTIO_NET_CTRL_RX  ||
> +		   ctrl->hdr.class == VIRTIO_NET_CTRL_MAC ||
> +		   ctrl->hdr.class == VIRTIO_NET_CTRL_VLAN) {
>   		status = 0;
>   	}
>   
>   	if (!status && dev->scvq)
> -		status = virtio_send_command(&dev->scvq->cq,
> -				(struct virtio_pmd_ctrl *)hdr, dlen, nb_dlen);
> +		status = virtio_send_command(&dev->scvq->cq, ctrl, dlen, nb_dlen);
>   
>   	/* Update status */
> -	*(virtio_net_ctrl_ack *)(uintptr_t)
> -		vring->desc[idx_status].addr = status;
> +	ctrl->status = status;
>   
>   	/* Update used descriptor */
>   	vring->desc[idx_hdr].id = vring->desc[idx_status].id;