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

Message ID 20240229132919.2186118-2-schalla@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series net/virtio: support IOVA as PA mode for vDPA backend |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Srujana Challa Feb. 29, 2024, 1:29 p.m. UTC
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(-)
  

Comments

Maxime Coquelin June 28, 2024, 8:07 a.m. UTC | #1
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;
  
Srujana Challa July 2, 2024, 11:09 a.m. UTC | #2
> 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;
  
Maxime Coquelin July 2, 2024, 12:41 p.m. UTC | #3
On 7/2/24 13:09, Srujana Challa wrote:
>> 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?

It should be safe to use it here.
Can you send a new revision ASAP, which would use this API and not take
the shortcurt, i.e. keep fetching buffer addres from descriptors?

Thanks,
Maxime

> 
>>
>>>
>>> -		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;
>
  
Maxime Coquelin July 3, 2024, 2:34 p.m. UTC | #4
On 7/3/24 12:03, Srujana Challa wrote:
> This patch series makes Virtio-user works in IOVA as PA mode
> for vDPA backend.
> 
> First patch fixes the issue when having buffer IOVA address in
> control queue descriptors.
> Second and third patches helps to share descriptor IOVA address,
> to the vhost backend. And also disables the use_va flag for VDPA
> backend type.
> 
> v1->v2:
> - Split single patch into three patches.
> v2->v3:
> - Addressed the review comment by using rte_mem_iova2virt() for desc
>    address translation.
> 
> Srujana Challa (3):
>    net/virtio_user: convert cq descriptor IOVA address to Virtual address
>    net/virtio: store desc IOVA address in vring data structure
>    net/virtio_user: support sharing vq descriptor IOVA to the backend
> 
>   drivers/net/virtio/virtio_ring.h              | 12 ++--
>   .../net/virtio/virtio_user/virtio_user_dev.c  | 59 ++++++++++++-------
>   drivers/net/virtio/virtio_user_ethdev.c       | 10 +++-
>   drivers/net/virtio/virtqueue.c                |  4 +-
>   4 files changed, 57 insertions(+), 28 deletions(-)
> 

Applied to next-virtio/for-next-net.

Thanks,
Maxime
  

Patch

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