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

[Srujana Challa <schalla@marvell.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.
Can we use rte_mem_iova2virt() here?

> 
> >
> > -		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;