| Message ID | 20190829080000.20806-3-maxime.coquelin@redhat.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Maxime Coquelin |
| Headers | show |
| Series | Introduce Virtio vDPA driver | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/Intel-compilation | fail | Compilation issues |
On 08/29, Maxime Coquelin wrote: >There might not have any VHOST_USER_SET_VRING_CALL requests >sent once virtio device is ready. When it happens, the vDPA >device's dev_conf() callback may never be called. > >Fixes: 9f9014512822 ("vhost: configure vDPA device after set vring call message") >Cc: stable@dpdk.org >Cc: xiaolong.ye@intel.com > >Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >--- > lib/librte_vhost/vhost_user.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > >diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c >index 0b72648a5..b1ea80c52 100644 >--- a/lib/librte_vhost/vhost_user.c >+++ b/lib/librte_vhost/vhost_user.c >@@ -2112,8 +2112,7 @@ vhost_user_msg_handler(int vid, int fd) > did = dev->vdpa_dev_id; > vdpa_dev = rte_vdpa_get_device(did); > if (vdpa_dev && virtio_is_ready(dev) && >- !(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) && >- msg.request.master == VHOST_USER_SET_VRING_CALL) { >+ !(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) { > if (vdpa_dev->ops->dev_conf) > vdpa_dev->ops->dev_conf(dev->vid); > dev->flags |= VIRTIO_DEV_VDPA_CONFIGURED; >-- >2.21.0 > Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>
Hi, > -----Original Message----- > From: Ye, Xiaolong > Sent: Monday, September 2, 2019 4:35 PM > To: Maxime Coquelin <maxime.coquelin@redhat.com> > Cc: Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W > <xiao.w.wang@intel.com>; dev@dpdk.org; jfreimann@redhat.com; > stable@dpdk.org > Subject: Re: [PATCH 02/15] vhost: configure vDPA as soon as the device is > ready > > On 08/29, Maxime Coquelin wrote: > >There might not have any VHOST_USER_SET_VRING_CALL requests > >sent once virtio device is ready. When it happens, the vDPA > >device's dev_conf() callback may never be called. > > > >Fixes: 9f9014512822 ("vhost: configure vDPA device after set vring call > message") > >Cc: stable@dpdk.org > >Cc: xiaolong.ye@intel.com > > > >Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > >--- > > lib/librte_vhost/vhost_user.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > >diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c > >index 0b72648a5..b1ea80c52 100644 > >--- a/lib/librte_vhost/vhost_user.c > >+++ b/lib/librte_vhost/vhost_user.c > >@@ -2112,8 +2112,7 @@ vhost_user_msg_handler(int vid, int fd) > > did = dev->vdpa_dev_id; > > vdpa_dev = rte_vdpa_get_device(did); > > if (vdpa_dev && virtio_is_ready(dev) && > >- !(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) && > >- msg.request.master == > VHOST_USER_SET_VRING_CALL) { > >+ !(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) { In the early beginning of vhost user messages, there seems to be a VHOST_USER_SET_VRING_CALL with invalid call fd, not sure if QEMU has any update on this point. If the virtio_is_ready() is based on that invalid call fd, then vdpa_dev_conf() cannot setup interrupt properly. I think that's why in our previous implementation, we wait for the real call fd and then call dev_conf(). BRs, Xiao
On 9/2/19 11:02 AM, Wang, Xiao W wrote: > Hi, > >> -----Original Message----- >> From: Ye, Xiaolong >> Sent: Monday, September 2, 2019 4:35 PM >> To: Maxime Coquelin <maxime.coquelin@redhat.com> >> Cc: Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong >> <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W >> <xiao.w.wang@intel.com>; dev@dpdk.org; jfreimann@redhat.com; >> stable@dpdk.org >> Subject: Re: [PATCH 02/15] vhost: configure vDPA as soon as the device is >> ready >> >> On 08/29, Maxime Coquelin wrote: >>> There might not have any VHOST_USER_SET_VRING_CALL requests >>> sent once virtio device is ready. When it happens, the vDPA >>> device's dev_conf() callback may never be called. >>> >>> Fixes: 9f9014512822 ("vhost: configure vDPA device after set vring call >> message") >>> Cc: stable@dpdk.org >>> Cc: xiaolong.ye@intel.com >>> >>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >>> --- >>> lib/librte_vhost/vhost_user.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c >>> index 0b72648a5..b1ea80c52 100644 >>> --- a/lib/librte_vhost/vhost_user.c >>> +++ b/lib/librte_vhost/vhost_user.c >>> @@ -2112,8 +2112,7 @@ vhost_user_msg_handler(int vid, int fd) >>> did = dev->vdpa_dev_id; >>> vdpa_dev = rte_vdpa_get_device(did); >>> if (vdpa_dev && virtio_is_ready(dev) && >>> - !(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) && >>> - msg.request.master == >> VHOST_USER_SET_VRING_CALL) { >>> + !(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) { > > In the early beginning of vhost user messages, there seems to be a VHOST_USER_SET_VRING_CALL with invalid call fd, > not sure if QEMU has any update on this point. > If the virtio_is_ready() is based on that invalid call fd, then vdpa_dev_conf() cannot setup interrupt properly. > I think that's why in our previous implementation, we wait for the real call fd and then call dev_conf(). I think that when we receive the first SET_VRING_CALL request from Qemu, virtio_is_ready() should be false because the rings addresses won't be received yet. Did it fixed a real issue, or was it based on code/logs review? Thanks for the explanations, Maxime > BRs, > Xiao >
Hi > -----Original Message----- > From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com] > Sent: Tuesday, September 3, 2019 3:35 PM > To: Wang, Xiao W <xiao.w.wang@intel.com>; Ye, Xiaolong > <xiaolong.ye@intel.com> > Cc: Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong > <zhihong.wang@intel.com>; amorenoz@redhat.com; dev@dpdk.org; > jfreimann@redhat.com; stable@dpdk.org > Subject: Re: [PATCH 02/15] vhost: configure vDPA as soon as the device is > ready > > > > On 9/2/19 11:02 AM, Wang, Xiao W wrote: > > Hi, > > > >> -----Original Message----- > >> From: Ye, Xiaolong > >> Sent: Monday, September 2, 2019 4:35 PM > >> To: Maxime Coquelin <maxime.coquelin@redhat.com> > >> Cc: Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong > >> <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W > >> <xiao.w.wang@intel.com>; dev@dpdk.org; jfreimann@redhat.com; > >> stable@dpdk.org > >> Subject: Re: [PATCH 02/15] vhost: configure vDPA as soon as the device is > >> ready > >> > >> On 08/29, Maxime Coquelin wrote: > >>> There might not have any VHOST_USER_SET_VRING_CALL requests > >>> sent once virtio device is ready. When it happens, the vDPA > >>> device's dev_conf() callback may never be called. > >>> > >>> Fixes: 9f9014512822 ("vhost: configure vDPA device after set vring call > >> message") > >>> Cc: stable@dpdk.org > >>> Cc: xiaolong.ye@intel.com > >>> > >>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > >>> --- > >>> lib/librte_vhost/vhost_user.c | 3 +-- > >>> 1 file changed, 1 insertion(+), 2 deletions(-) > >>> > >>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c > >>> index 0b72648a5..b1ea80c52 100644 > >>> --- a/lib/librte_vhost/vhost_user.c > >>> +++ b/lib/librte_vhost/vhost_user.c > >>> @@ -2112,8 +2112,7 @@ vhost_user_msg_handler(int vid, int fd) > >>> did = dev->vdpa_dev_id; > >>> vdpa_dev = rte_vdpa_get_device(did); > >>> if (vdpa_dev && virtio_is_ready(dev) && > >>> - !(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) && > >>> - msg.request.master == > >> VHOST_USER_SET_VRING_CALL) { > >>> + !(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) { > > > > In the early beginning of vhost user messages, there seems to be a > VHOST_USER_SET_VRING_CALL with invalid call fd, > > not sure if QEMU has any update on this point. > > If the virtio_is_ready() is based on that invalid call fd, then vdpa_dev_conf() > cannot setup interrupt properly. > > I think that's why in our previous implementation, we wait for the real call fd > and then call dev_conf(). > > I think that when we receive the first SET_VRING_CALL request from Qemu, > virtio_is_ready() should be false because the rings addresses won't be > received yet. In the last phase of vhost communication, there're usually a sequence of messages "SET_VRING_KICK" and "SET_VRING_CALL". With this patch, at the arrival of "SET_VRING_KICK", virtio_is_ready() will return true, and the invalid call fd will get involved into dev_con(). I'm not sure if the invalid callfd implementation still exist in latest QEMU, if yes, We have to handle this case. > > Did it fixed a real issue, or was it based on code/logs review? In an old implementation with QEMU 2.6, I met the issue. To verify it, you need to Setup a VM (maybe nested) and verify if virtio can get an interrupt. BRs, Xiao > > Thanks for the explanations, > Maxime > > > BRs, > > Xiao > >
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index 0b72648a5..b1ea80c52 100644 --- a/lib/librte_vhost/vhost_user.c +++ b/lib/librte_vhost/vhost_user.c @@ -2112,8 +2112,7 @@ vhost_user_msg_handler(int vid, int fd) did = dev->vdpa_dev_id; vdpa_dev = rte_vdpa_get_device(did); if (vdpa_dev && virtio_is_ready(dev) && - !(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) && - msg.request.master == VHOST_USER_SET_VRING_CALL) { + !(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) { if (vdpa_dev->ops->dev_conf) vdpa_dev->ops->dev_conf(dev->vid); dev->flags |= VIRTIO_DEV_VDPA_CONFIGURED;
There might not have any VHOST_USER_SET_VRING_CALL requests sent once virtio device is ready. When it happens, the vDPA device's dev_conf() callback may never be called. Fixes: 9f9014512822 ("vhost: configure vDPA device after set vring call message") Cc: stable@dpdk.org Cc: xiaolong.ye@intel.com Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- lib/librte_vhost/vhost_user.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)