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.coque...@redhat.com> > Sent: Thursday, June 18, 2020 3:31 PM > To: Matan Azrad <ma...@mellanox.com>; Ye, Xiaolong > <xiaolong...@intel.com>; Shahaf Shuler <shah...@mellanox.com>; > amore...@redhat.com; Wang, Xiao W <xiao.w.w...@intel.com>; Slava > Ovsiienko <viachesl...@mellanox.com>; dev@dpdk.org > Cc: jasow...@redhat.com; l...@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
