Re: [dpdk-dev] [PATCH 9/9] vhost: only use vDPA config workaround if needed

["Wang, Xiao W" <xiao.w.wang@intel.com>]

Hi,

The original issue is with legacy QEMU (e.g.QEMUv2.6, with centos7.2 as guest kernel, without set_vring_status as an indicator).
For a normal boot, the last 2 messages are set_vring_kick and set_vring_call, inside the set_vring_kick handling,
virtio_is_ready() will reture true (because of that special very early set_vring_call message). Then
vdpa dev_config is called, and the fake call fd is used. As a result, the virtio kernel driver in VM will not
receive interrupt.

+1 for introducing SET_STATUS to make things clearer.
IFCVF driver hasn't added support for MQ and .set_vring_state ops, so no need to test,

Just curious about MQ live migration case, on the dst side, will this SET_STATUS msg comes to vhost-user? and When?

BRs,
Xiao

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, June 18, 2020 3:31 PM
> To: Matan Azrad <matan@mellanox.com>; Ye, Xiaolong
> <xiaolong.ye@intel.com>; Shahaf Shuler <shahafs@mellanox.com>;
> amorenoz@redhat.com; Wang, Xiao W <xiao.w.wang@intel.com>; Slava
> Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> Cc: jasowang@redhat.com; lulu@redhat.com
> Subject: Re: [PATCH 9/9] vhost: only use vDPA config workaround if needed
> 
> 
> 
> On 6/18/20 8:39 AM, Matan Azrad wrote:
> > HI Maxime
> >
> > From: Maxime Coquelin:
> >> On 6/17/20 1:04 PM, Matan Azrad wrote:
> >>
> >>>>> Don’t you think that only enabled queues must be fully initialized
> >>>>> when
> >>>> their status is changed from disabled to enabled?
> >>>>> So, you can assume that disabled queues can stay "not fully
> initialized"...
> >>>>
> >>>> That may work but might not be following the Virtio spec as with 1.0
> >>>> we shouldn't process the rings before DRIVER_OK is set (but we cannot
> >>>> be sure we follow it anyway without SET_STATUS support).
> >>>>
> >>>> I propose to cook a patch doing the following:
> >>>> 1. virtio_is_ready() will only ensure the first queue pair is ready
> >>>> (i.e. enabled and configured). Meaning that app's new_device callback
> >>>> and vDPA drivers dev_conf callback will be called with only the first
> >>>> queue pair configured and enabled.
> >>>>
> >>>> 2. Before handling a new vhost-user request, it saves the ready
> >>>> status for every queue pair.
> >>>>
> >>>> 3. Same handling of the requests, except that we won't notify the
> >>>> vdpa driver and the application of vring state changes in the
> >>>> VHOST_USER_SET_VRING_ENABLE handler.
> >>>>
> >>>> 4. Once the Vhost-user request is handled, it compares the new ready
> >>>> status foe every queues with the old one and send queue state event
> >>>> changes accordingly.
> >>>
> >>> Looks very nice to me.
> >>
> >> Cool!
> >>
> >>> More points:
> >>> By this method some queues may be configured by the set_vring_state
> >> operation so the next calls are expected to be called for each queue by the
> >> driver from the set_vring_state callback :
> >>> 1. rte_vhost_enable_guest_notification
> >>> 	This one takes datapath lock so we need to be sure that datapath
> >> lock is not locked on this queue from the same caller thread (maybe to not
> >> takes datapath locks when vdpa is configured at all).
> >>
> >> Good point, I agree we shouldn't need to use the access lock when vdpa is
> >> configured. We may want to document that all the control path is
> assumed to
> >> be single thread though.
> >>
> >>
> >>> 2. rte_vhost_host_notifier_ctrl
> >>> 	This function API is per device and not per queue, maybe we need to
> >> change this function to be per queue (add new for now and deprecate the
> >> old one in 20.11).
> >>
> >> This one is still experimental, so no issue in reworking the API to make it
> per
> >> queue without deprecation notice.
> >>
> >>> 3. Need to be sure that if ready queue configuration is changed after
> >> dev_conf, we should notify it to the driver. (maybe by
> >> set_vring_state(disabl) and set_vring_state(enable)).
> >>
> >> Agree, I'm not sure yet if we should just toggle set_vring_state as you
> >> proposes, or if we should have a new callback for this.
> >
> > Actually, when the queue configuration is changed, there is one moment
> that configuration is not valid (in the write time).
> > So maybe it makes sense to toggle.
> >
> > But there is one more option:
> >
> > It doesn't make sense that after configuration change the QEMU will not
> send VHOST_USER_SET_VRING_ENABLE massage.
> 
> Agree.
> > So maybe we need to call set_vring_state in the next events:
> > 	1. queue becomes ready (enabled and fully configured) -
> set_vring_state(enable).
> > 	2. queue becomes not ready - set_vring_state(disable).
> > 	3. queue stay ready and VHOST_USER_SET_VRING_ENABLE massage
> was handled - set_vring_state(enable).
> >
> > Then we need to document that every set_vring_state call may point on
> configuration changes in the queue even if the state was not changed.
> >
> > What do you think?
> 
> I think it is worth a try.
> 
> Thanks,
> Maxime