-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Main submit #2
Main submit #2
Conversation
drivers/vdpa/virtio/virtio.c
Outdated
* Copyright (c) 2022 NVIDIA Corporation & Affiliates | ||
*/ | ||
|
||
#include <unistd.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kailiangz1
Can you please take the whole code of this patch out of drivers/vdpa directory?
it should be in drivers/virtio or if its library it should be in lib/virtio.
drivers/vdpa/virtio/virtio_rxtx.h
Outdated
* Copyright (c) 2022 NVIDIA Corporation & Affiliates | ||
*/ | ||
|
||
#ifndef _VIRTIO_RXTX_H_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VIRTNET_RX_TX_H
drivers/vdpa/virtio/virtio_rxtx.h
Outdated
uint64_t multicast; | ||
uint64_t broadcast; | ||
/* Size bins in array as RFC 2819, undersized [0], 64 [1], etc */ | ||
uint64_t size_bins[8]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed?
drivers/vdpa/virtio/virtio_rxtx.h
Outdated
|
||
struct virtnet_rx { | ||
/* dummy mbuf, for wraparound when processing RX ring. */ | ||
struct rte_mbuf *fake_mbuf; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are not working on any mbufs here. please remove all mbuf related code.
drivers/vdpa/virtio/virtio_rxtx.h
Outdated
uint64_t mbuf_initializer; /**< value to init mbufs. */ | ||
struct rte_mempool *mpool; /**< mempool for mbuf allocation */ | ||
|
||
uint16_t queue_id; /**< DPDK queue index. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all of this.
drivers/vdpa/virtio/virtio_api.c
Outdated
|
||
nr_vq = hw->max_queue_pairs * 2; | ||
if (virtio_with_feature(hw, VIRTIO_NET_F_CTRL_VQ)) | ||
nr_vq += 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no. this is net specific code. it cannot be generic here.
drivers/vdpa/virtio/virtio_api.c
Outdated
PMD_INIT_LOG(DEBUG,"Guest_features is 0x%"PRIx64,hw->guest_features); | ||
return vpdev; | ||
|
||
error: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
label must start at start of the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
drivers/vdpa/virtio/virtio_api.c
Outdated
@@ -0,0 +1,321 @@ | |||
/* SPDX-License-Identifier: BSD-3-Clause |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name the file as just virtio.c
drop the _api.
drivers/vdpa/virtio/virtio.h
Outdated
bool use_va; | ||
}; | ||
|
||
struct virtio_ops { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this code copied/modified from the existing dpdk code?
if so, can we use it in its original form?
drivers/vdpa/virtio/virtio.h
Outdated
|
||
#include <rte_ether.h> | ||
|
||
/* The feature bitmap for virtio net */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please keep net specific items in the virtio_net.h header file out of here.
2bbb951
to
88266d2
Compare
drivers/common/virtio/meson.build
Outdated
@@ -0,0 +1,9 @@ | |||
# SPDX-License-Identifier: BSD-3-Clause | |||
# Copyright 2019 Mellanox Technologies, Ltd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change to latest copy right.
drivers/common/virtio/virtio.c
Outdated
virtio_dev_alloc(struct rte_pci_device *pci_dev) | ||
{ | ||
struct virtio_pci_dev *vpdev; | ||
struct virtio_hw *hw = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not need to do null initialize.
you are assigning it to valid value on line 28
drivers/common/virtio/virtio.c
Outdated
#include "virtio_logs.h" | ||
|
||
struct virtio_pci_dev * | ||
virtio_dev_alloc(struct rte_pci_device *pci_dev) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the api name should be virtio_pci_dev_alloc.
because it returns virtio_pci_dev pointer.
Please also change other friend functions.
drivers/common/virtio/virtio.c
Outdated
VTPCI_DEV(hw) = pci_dev; | ||
ret = virtio_dev_pci_init(pci_dev, vpdev); | ||
if (ret) { | ||
PMD_INIT_LOG(ERR, "Failed to init virtio PCI device"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to print the error value along with the pci device name so that we exactly know which device and error occured.
otherwise its unknown to device under debug.
Align other similar prints too.
drivers/common/virtio/virtio.c
Outdated
goto error; | ||
} | ||
|
||
/* Reset the device although not necessary at startup */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better comment is,
Reset the device, so when device is used later, reset time is saved.
drivers/common/virtio/virtio.h
Outdated
uint8_t duplex; | ||
uint8_t intr_lsc; | ||
uint16_t max_mtu; | ||
size_t max_rx_pkt_len; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
many net specific parameters to take this out or put in virtio_net_config new structure.
drivers/common/virtio/virtio.h
Outdated
* such a contention. | ||
*/ | ||
rte_spinlock_t state_lock; | ||
struct rte_mbuf **inject_pkts; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no mbufs please.
}; | ||
|
||
#define VIRTIO_OPS(hw) ((hw)->virtio_ops) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra white space.
drivers/common/virtio/virtio_pci.c
Outdated
|
||
const struct virtio_ops virtio_dev_pci_legacy_ops = { | ||
.read_dev_cfg = legacy_dev_config_read, | ||
.write_dev_cfg = legacy_dev_config_write, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra white spaces
drivers/common/virtio/virtio_pci.c
Outdated
io_write64_twopart(uint64_t val, uint32_t *lo, uint32_t *hi) | ||
{ | ||
rte_write32(val & ((1ULL << 32) - 1), lo); | ||
rte_write32(val >> 32, hi); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra white spaces.
drivers/vdpa/virtio/virtio_vdpa.h
Outdated
uint64_t guest_features; | ||
pthread_mutex_t vq_config_lock; | ||
int configured; | ||
uint16_t nr_virtqs; //number of vq vhost enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no c++ comments.
do /* */
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
} | ||
|
||
static int | ||
virtio_vdpa_vfio_intr_enable(struct virtio_vdpa_priv *priv) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No.
This code belongs in the low level virtio pci driver at virtio_pci_dev level.
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
} | ||
|
||
static int | ||
virtio_vdpa_virtq_doorbell_relay_disable(struct virtio_vdpa_priv *priv, int index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we always want to work with direct doorbells.
so please remove this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, mlx5_vdpa still have this code and it can be used because some time vhost msg come and
doorbell map still not run.
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
struct virtio_dev_vring_info vring_info; | ||
uint64_t gpa; | ||
|
||
if(enable==0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
white spaces
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
|
||
DRV_LOG(INFO, "vDPA device %d set vring %d state %d.", vid, vring, state); | ||
priv->vrings[vring]->enable = !!state; | ||
if((priv->configured)&&(state)&&(vring== (rte_vhost_get_vring_num(vid)-1))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something looks hacky here, or I don;t fully understand this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will move to driver_ok after Yajun's patch, this is temporary solution.
6be860d
to
b23554f
Compare
drivers/common/virtio/virtio_ring.h
Outdated
|
||
struct vring { | ||
unsigned int num; | ||
struct vring_desc *desc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please dont add any extra white space to right align.
C does left align the code. remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
drivers/common/virtio/virtio_ring.h
Outdated
}; | ||
|
||
struct vring { | ||
unsigned int num; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have structure nauturally aligned without holds unless it is absolute necessary.
so move this to the end of the struct.
drivers/common/virtio/virtio_ring.h
Outdated
}; | ||
|
||
struct vring_packed { | ||
unsigned int num; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this variable to end of the structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
drivers/common/virtio/virtio_pci.c
Outdated
break; | ||
} | ||
|
||
if (flags & PCI_MSIX_ENABLE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better rewrite it as,
return (flags & PCI_MSIX_ENABLE) ? VIRTIO_MSIX_ENABLED : VIRTIO_MSIX_DISABLED;
you can see such usage in VIRTIO_PCI_CONFIG.
drivers/common/virtio/virtio_pci.c
Outdated
* Copyright (c) 2022 NVIDIA Corporation & Affiliates | ||
*/ | ||
|
||
#include <stdint.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you are moving the code from X file to Y file.
Do not copy, you have to delete the code from some file that from where you are moving.
drivers/common/virtio/virtio_pci.c
Outdated
return 0; | ||
} | ||
|
||
const struct virtio_ops virtio_dev_pci_modern_ops = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be static.
same for the legacy ops too.
|
||
return irq_info.count; | ||
} | ||
#define VFIO_FD_INVALID -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty line needed before this define
} | ||
return 0; | ||
} | ||
int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank line needed
drivers/common/virtio/virtio.c
Outdated
rte_free(irq_set); | ||
if (ret) { | ||
PMD_INIT_LOG(ERR, "Dev %s enabling MSI-X: %s", VP_DEV_NAME(vpdev), strerror(errno)); | ||
return ret; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for this return inside.
always do return ret on line 260.
drivers/common/virtio/virtio.c
Outdated
struct virtio_hw *hw = &vpdev->hw; | ||
|
||
VIRTIO_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET); | ||
/* Flush status write and wait device ready max 3 seconds. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 seconds is too less for us during dpu reboots.
it should be close to 120 secs.
e28ce93
to
f1d099b
Compare
drivers/vdpa/virtio/virtio_vdpa.h
Outdated
@@ -0,0 +1,63 @@ | |||
/* SPDX-License-Identifier: BSD-3-Clause |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since there is single file who is using this, there is no need of a separate header file.
Move them to virtio_vdpa.c
drivers/vdpa/virtio/virtio_vdpa.h
Outdated
uint64_t used; | ||
uint16_t size; | ||
uint16_t last_avail_idx; | ||
uint16_t last_used_idx; /*other is for future use, not use now*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be,
/* It is for future use */
Actually remove it from this patch, if we are not using it.
lets add when you actually use it.
it keeps review and code small.
drivers/vdpa/virtio/virtio_vdpa.h
Outdated
uint16_t last_used_idx; /*other is for future use, not use now*/ | ||
bool enable; | ||
uint8_t notifier_state; | ||
uint16_t index; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move index variable up.
keep things naturally aligned.
so move bool at the end of the struct along with notifier_state.
drivers/vdpa/virtio/virtio_vdpa.h
Outdated
}; | ||
|
||
#define VIRTIO_VDPA_DRIVER_NAME vdpa_virtio | ||
//TO_DO: need to add VHOST_USER_PROTOCOL_F_CONFIG later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no cpp style comment.
/* Comment */
drivers/vdpa/virtio/virtio_vdpa.h
Outdated
*/ | ||
struct virtio_vdpa_priv { | ||
TAILQ_ENTRY(virtio_vdpa_priv) next; | ||
struct rte_pci_device *pdev; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
evaluate why do we need to know about pdev.
It should not be here.
we are working on top of virtio_pci_dev vpdev or rte_vdpa_device.
we should not directly access rte_pci_device at this layer.
Please remove or schedule a call for all the things that I do not understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to access intr_handle at virtio-vdpa.c to configure device level interrupt .
intr_handle is stored in rte_pci_device
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
if (ret) | ||
return ret; | ||
|
||
ret =virtio_pci_dev_interrupts_enable(priv->vpdev, vq.callfd, index+1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be done only one, or its name should be pci_dev_interrupt_enable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interrupt is enabled per queue, so, i just need to change to pci_dev_interrupt_enable?
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
priv->vrings[index]->size = vq.size; | ||
vring_info.size = vq.size; | ||
|
||
DRV_LOG(DEBUG, "Virtq %d nr_entrys:%d",index, vq.size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
white space before index
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
|
||
priv->vrings[index]->notifier_state = VIRTIO_VDPA_NOTIFIER_STATE_DISABLED; | ||
|
||
ret = virtio_pci_dev_interrupts_disable(priv->vpdev, index+1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
white space
index + 1
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
|
||
virtio_pci_dev_queue_del(priv->vpdev, index); | ||
|
||
priv->vrings[index]->notifier_state = VIRTIO_VDPA_NOTIFIER_STATE_DISABLED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be after interrupt disablement call is successful.
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
uint64_t buf; | ||
int nbytes; | ||
|
||
if(!(priv->configured)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
braces before priv is not needed.
it should be,
if (!priv->configured || !virtq->enable)
return;
cut down line 142 and 143.
686ba38
to
5082a4e
Compare
*/ | ||
|
||
#ifdef RTE_EXEC_ENV_LINUX | ||
#include <dirent.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no white space needed at beginning.
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
virtio_vdpa_find_priv_resource_by_vdev(const struct rte_vdpa_device *vdev) | ||
{ | ||
struct virtio_vdpa_priv *priv; | ||
int found = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use bool
bool found = false;
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
DRV_LOG(ERR, "Invalid vDPA device: %s", vdev->device->name); | ||
return -ENODEV; | ||
} | ||
/* TO_DO:for net, should use max_virtqueue_pairs? may be is same */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this todo, it belong to virtio layer.
virtio layer will do max_qps vs other count.
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
|
||
static int | ||
virtio_vdpa_virtq_doorbell_relay_enable(struct virtio_vdpa_priv *priv, | ||
int vq_idx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix alignment.
cannot figure out from this patch if its right or not.
} | ||
|
||
return 0; | ||
error: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty line needed before the label.
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
priv->vdev->device->name, ret); | ||
} | ||
|
||
virtio_pci_dev_reset(priv->vpdev); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be done before doing dma unmap.
otherwise memory is unmapped but device VQ are still working on these vqs.
So reset the device so that VQ traffic stops.
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
|
||
virtio_pci_dev_reset(priv->vpdev); | ||
|
||
/* Disable all queue*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add white space after "queue"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and say "queues".
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
} | ||
|
||
virtio_pci_dev_queues_free(priv->vpdev, nr_vq); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this empty line.
struct virtio_vdpa_priv *priv = NULL; | ||
int found = 0, ret; | ||
|
||
pthread_mutex_lock(&priv_list_lock); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the function you have start the beginning of the file.
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
rte_free(priv); | ||
} | ||
|
||
if (!found) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return found ? 0 : -ENODEV;
The interface of virtio device driver is virtio_api.h All the api function have parameter struct virtio_pci_dev* and it is allocated in function virtio_dev_alloc Signed-off-by: Kailiang Zhou <kailiangz@nvidia.com>
This vdpa driver works on top of virtio device layer use virtio_api.h Only support 1 queue pair for virtio net device. Signed-off-by: Kailiang Zhou <kailiangz@nvidia.com>
In rte_vhost_driver_unregister which is called from vdpa-rpc thread, vsocket should be removed from reconn_list again after remove vsocket from conn_list. Because vhost_user_read_cb which is called in vhost-events thread can add vsocket to reconn_list again. When qemu close domain socket server, vhost_user_read_cb will be called to clean up vhost device. vsocket->path is NULL #0 0x00007f07665834d1 in __strnlen_sse2 () from /lib64/libc.so.6 Mellanox#1 0x00007f076aee79da in vhost_user_add_connection (fd=160, vsocket=0x7f070406d160) at ../lib/vhost/socket.c:226 Mellanox#2 0x00007f076aee7d63 in vhost_user_client_reconnect (arg=<optimized out>) at ../lib/vhost/socket.c:481 Mellanox#3 0x00007f07668cbdd5 in start_thread () from /lib64/libpthread.so.0 Mellanox#4 0x00007f07665f4ead in clone () from /lib64/libc.so.6 RM: 3585558 Signed-off-by: Yajun Wu <yajunw@nvidia.com>
In rte_vhost_driver_unregister which is called from vdpa-rpc thread, vsocket should be removed from reconn_list again after remove vsocket from conn_list. Because vhost_user_read_cb which is called in vhost-events thread can add vsocket to reconn_list again. When qemu close domain socket server, vhost_user_read_cb will be called to clean up vhost device. vsocket->path is NULL #0 0x00007f07665834d1 in __strnlen_sse2 () from /lib64/libc.so.6 #1 0x00007f076aee79da in vhost_user_add_connection (fd=160, vsocket=0x7f070406d160) at ../lib/vhost/socket.c:226 #2 0x00007f076aee7d63 in vhost_user_client_reconnect (arg=<optimized out>) at ../lib/vhost/socket.c:481 #3 0x00007f07668cbdd5 in start_thread () from /lib64/libpthread.so.0 #4 0x00007f07665f4ead in clone () from /lib64/libc.so.6 RM: 3585558 Signed-off-by: Yajun Wu <yajunw@nvidia.com>
[ upstream commit 1c80a40 ] The net/vhost pmd currently provides a -1 vid when disabling interrupt after a virtio port got disconnected. This can be caught when running with ASan. First, start dpdk-l3fwd-power in interrupt mode with a net/vhost port. $ ./build-clang/examples/dpdk-l3fwd-power -l0,1 --in-memory \ -a 0000:00:00.0 \ --vdev net_vhost0,iface=plop.sock,client=1\ -- \ -p 0x1 \ --interrupt-only \ --config '(0,0,1)' \ --parse-ptype 0 Then start testpmd with virtio-user. $ ./build-clang/app/dpdk-testpmd -l0,2 --single-file-segment --in-memory \ -a 0000:00:00.0 \ --vdev net_virtio_user0,path=plop.sock,server=1 \ -- \ -i Finally stop testpmd. ASan then splats in dpdk-l3fwd-power: ================================================================= ==3641005==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000005ed0778 at pc 0x000001270f81 bp 0x7fddbd2eee20 sp 0x7fddbd2eee18 READ of size 8 at 0x000005ed0778 thread T2 #0 0x1270f80 in get_device .../lib/vhost/vhost.h:801:27 Mellanox#1 0x1270f80 in rte_vhost_get_vhost_vring .../lib/vhost/vhost.c:951:8 Mellanox#2 0x3ac95cb in eth_rxq_intr_disable .../drivers/net/vhost/rte_eth_vhost.c:647:8 Mellanox#3 0x170e0bf in rte_eth_dev_rx_intr_disable .../lib/ethdev/rte_ethdev.c:5443:25 Mellanox#4 0xf72ba7 in turn_on_off_intr .../examples/l3fwd-power/main.c:881:4 Mellanox#5 0xf71045 in main_intr_loop .../examples/l3fwd-power/main.c:1061:6 Mellanox#6 0x17f9292 in eal_thread_loop .../lib/eal/common/eal_common_thread.c:210:9 Mellanox#7 0x18373f5 in eal_worker_thread_loop .../lib/eal/linux/eal.c:915:2 Mellanox#8 0x7fddc16ae12c in start_thread (/lib64/libc.so.6+0x8b12c) (BuildId: 81daba31ee66dbd63efdc4252a872949d874d136) Mellanox#9 0x7fddc172fbbf in __GI___clone3 (/lib64/libc.so.6+0x10cbbf) (BuildId: 81daba31ee66dbd63efdc4252a872949d874d136) 0x000005ed0778 is located 8 bytes to the left of global variable 'vhost_devices' defined in '.../lib/vhost/vhost.c:24' (0x5ed0780) of size 8192 0x000005ed0778 is located 20 bytes to the right of global variable 'vhost_config_log_level' defined in '.../lib/vhost/vhost.c:2174' (0x5ed0760) of size 4 SUMMARY: AddressSanitizer: global-buffer-overflow .../lib/vhost/vhost.h:801:27 in get_device Shadow bytes around the buggy address: 0x000080bd2090: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x000080bd20a0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x000080bd20b0: f9 f9 f9 f9 00 f9 f9 f9 00 f9 f9 f9 00 f9 f9 f9 0x000080bd20c0: 00 00 00 00 00 00 00 f9 f9 f9 f9 f9 04 f9 f9 f9 0x000080bd20d0: 00 00 00 00 00 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 =>0x000080bd20e0: 00 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 04 f9 f9[f9] 0x000080bd20f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x000080bd2100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x000080bd2110: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x000080bd2120: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x000080bd2130: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Thread T2 created by T0 here: #0 0xe98996 in __interceptor_pthread_create (.examples/dpdk-l3fwd-power+0xe98996) (BuildId: d0b984a3b0287b9e0f301b73426fa921aeecca3a) Mellanox#1 0x1836767 in eal_worker_thread_create .../lib/eal/linux/eal.c:952:6 Mellanox#2 0x1834b83 in rte_eal_init .../lib/eal/linux/eal.c:1257:9 Mellanox#3 0xf68902 in main .../examples/l3fwd-power/main.c:2496:8 Mellanox#4 0x7fddc164a50f in __libc_start_call_main (/lib64/libc.so.6+0x2750f) (BuildId: 81daba31ee66dbd63efdc4252a872949d874d136) ==3641005==ABORTING More generally, any application passing an incorrect vid would trigger such an OOB access. Fixes: 4796ad6 ("examples/vhost: import userspace vhost application") Signed-off-by: David Marchand <david.marchand@redhat.com> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
add virtio vdpa driver