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

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

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