On 1/7/21 7:32 AM, Xia, Chenbo wrote:
> Hi Maxime,
>
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coque...@redhat.com>
>> Sent: Monday, December 21, 2020 5:14 AM
>> To: dev@dpdk.org; Xia, Chenbo <chenbo....@intel.com>; olivier.m...@6wind.com;
>> amore...@redhat.com; david.march...@redhat.com
>> Cc: Maxime Coquelin <maxime.coque...@redhat.com>
>> Subject: [PATCH 38/40] net/virtio: move Vhost-user specifics to its backend
>>
>> This patch moves all the Vhost-user backend specific
>> logic like Vhost FD, listen FD and interrupt handling
>> to the vhost-user backend implementation.
>>
>> In order to achieve that, new ops are created to update
>> the link status, disconnect and reconnect the server,
>> and fetch the link state interrupt FD.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
>> ---
>> drivers/net/virtio/virtio_user/vhost.h | 4 +
>> drivers/net/virtio/virtio_user/vhost_kernel.c | 18 +-
>> drivers/net/virtio/virtio_user/vhost_user.c | 169 ++++++++++++++---
>> drivers/net/virtio/virtio_user/vhost_vdpa.c | 16 ++
>> .../net/virtio/virtio_user/virtio_user_dev.c | 175 ++++++++++++++---
>> .../net/virtio/virtio_user/virtio_user_dev.h | 9 +-
>> drivers/net/virtio/virtio_user_ethdev.c | 179 +-----------------
>> 7 files changed, 340 insertions(+), 230 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_user/vhost.h
>> b/drivers/net/virtio/virtio_user/vhost.h
>> index ee5598226d..6001d7a164 100644
>> --- a/drivers/net/virtio/virtio_user/vhost.h
>> +++ b/drivers/net/virtio/virtio_user/vhost.h
>> @@ -75,6 +75,10 @@ struct virtio_user_backend_ops {
>> int (*enable_qp)(struct virtio_user_dev *dev, uint16_t pair_idx, int
>> enable);
>> int (*dma_map)(struct virtio_user_dev *dev, void *addr, uint64_t iova,
>> size_t len);
>> int (*dma_unmap)(struct virtio_user_dev *dev, void *addr, uint64_t iova,
>> size_t len);
>> + int (*update_link_state)(struct virtio_user_dev *dev);
>> + int (*server_disconnect)(struct virtio_user_dev *dev);
>> + int (*server_reconnect)(struct virtio_user_dev *dev);
>> + int (*get_intr_fd)(struct virtio_user_dev *dev);
>
> This get_intr_fd is for getting link state interrupt fd. I think his name is
> a little bit
> generic. What do you think? Maybe we could change the name or add comment
> here to clarify here?
>
>> };
>>
>> extern struct virtio_user_backend_ops virtio_ops_user;
>> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c
>> b/drivers/net/virtio/virtio_user/vhost_kernel.c
>> index 023fddcd69..0ba37b23dc 100644
>> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
>> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
>> @@ -459,6 +459,20 @@ vhost_kernel_get_backend_features(uint64_t *features)
>> return 0;
>> }
>>
>
> <snip>
>
>> +static void
>> +virtio_user_dev_reset_queues_packed(struct rte_eth_dev *eth_dev)
>> +{
>> + struct virtio_user_dev *dev = eth_dev->data->dev_private;
>> + struct virtio_hw *hw = &dev->hw;
>> + struct virtnet_rx *rxvq;
>> + struct virtnet_tx *txvq;
>> + uint16_t i;
>> +
>> + /* Add lock to avoid queue contention. */
>> + rte_spinlock_lock(&hw->state_lock);
>> + hw->started = 0;
>> +
>> + /*
>> + * Waitting for datapath to complete before resetting queues.
>
> s/Waitting/Waiting
Fixed
>> + * 1 ms should be enough for the ongoing Tx/Rx function to finish.
>> + */
>> + rte_delay_ms(1);
>> +
>> + /* Vring reset for each Tx queue and Rx queue. */
>> + for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
>> + rxvq = eth_dev->data->rx_queues[i];
>> + virtqueue_rxvq_reset_packed(rxvq->vq);
>> + virtio_dev_rx_queue_setup_finish(eth_dev, i);
>> + }
>> +
>> + for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
>> + txvq = eth_dev->data->tx_queues[i];
>> + virtqueue_txvq_reset_packed(txvq->vq);
>> + }
>> +
>> + hw->started = 1;
>> + rte_spinlock_unlock(&hw->state_lock);
>> +}
>> +
>> +void
>> +virtio_user_dev_delayed_handler(void *param)
>> +{
>> + struct virtio_user_dev *dev = param;
>> + struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id];
>> +
>> + if (rte_intr_disable(eth_dev->intr_handle) < 0) {
>> + PMD_DRV_LOG(ERR, "interrupt disable failed");
>> + return;
>> + }
>> + rte_intr_callback_unregister(eth_dev->intr_handle,
>> + virtio_interrupt_handler, eth_dev);
>> + if (dev->is_server) {
>> + dev->ops->server_disconnect(dev);
>
> Better to check ops existence too?
Indeed, v2 will check its existence before calling.
>> + eth_dev->intr_handle->fd = dev->ops->get_intr_fd(dev);
>> + rte_intr_callback_register(eth_dev->intr_handle,
>> + virtio_interrupt_handler, eth_dev);
>> + if (rte_intr_enable(eth_dev->intr_handle) < 0) {
>> + PMD_DRV_LOG(ERR, "interrupt enable failed");
>> + return;
>> + }
>> + }
>> +}
>> +
>> +int
>> +virtio_user_dev_server_reconnect(struct virtio_user_dev *dev)
>> +{
>> + int ret, old_status;
>> + struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id];
>> + struct virtio_hw *hw = &dev->hw;
>> +
>> + if (!dev->ops->server_reconnect) {
>> + PMD_DRV_LOG(ERR, "(%s) Missing server reconnect callback", dev-
>>> path);
>> + return -1;
>> + }
>
> Forget to call server_reconnect? :)
Indeed! When I said I didn't tried last 5 patches, I wasn't lying! :)
>> +
>> + old_status = virtio_get_status(hw);
>> +
>> + virtio_reset(hw);
>> +
>> + virtio_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
>> +
>> + virtio_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
>> +
>> + if (dev->ops->get_features(dev, &dev->device_features) < 0) {
>> + PMD_INIT_LOG(ERR, "get_features failed: %s",
>> + strerror(errno));
>> + return -1;
>> + }
>> +
>> + dev->device_features |= dev->frontend_features;
>> +
>> + /* umask vhost-user unsupported features */
>
> s/umask/unmask
Fixed.
Thanks,
Maxime
> Thanks,
> Chenbo
>
