On Wed, Mar 20, 2024 at 12:18:14PM +0800, Jason Wang wrote:
On Mon, Mar 18, 2024 at 4:27 PM Stefano Garzarella <sgarz...@redhat.com> wrote:
On Mon, Mar 18, 2024 at 12:31:59PM +0800, Jason Wang wrote:
>On Fri, Mar 15, 2024 at 4:23 PM Stefano Garzarella <sgarz...@redhat.com> wrote:
>>
>> On Thu, Mar 14, 2024 at 11:17:01AM +0800, Jason Wang wrote:
>> >On Wed, Feb 7, 2024 at 5:27 PM Stefano Garzarella <sgarz...@redhat.com>
wrote:
>> >>
>> >> vhost_vdpa_set_vring_ready() could already fail, but if Linux's
>> >> patch [1] will be merged, it may fail with more chance if
>> >> userspace does not activate virtqueues before DRIVER_OK when
>> >> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated.
>> >
>> >I wonder what happens if we just leave it as is.
>>
>> Are you referring to this patch or the kernel patch?
>
>This patch.
>
>>
>> Here I'm just checking the return value of vhost_vdpa_set_vring_ready().
>> It can return an error also without that kernel patch, so IMHO is
>> better to check the return value here in QEMU.
>>
>> What issue do you see with this patch applied?
>
>For the parent which can enable after driver_ok but not advertise it.
But this patch is not changing anything in that sense, it just controls
the return value of the VHOST_VDPA_SET_VRING_ENABLE ioctl.
Why would QEMU ignore an error if it can't activate vrings?
If we really want to ignore it we should document it both in QEMU, but
also in the kernel, because honestly the way the code is now it
shouldn't fail from what I understand.
That said, even if we ignore it, IMHO we should at least print a warning
in QEMU.
Right.
>
>(To say the truth, I'm not sure if we need to care about this)
I agree on that, but this is related to the patch in the kernel, not
.> this simple patch to fix QEMU error path, right?
Or it's the charge of the Qemu vDPA layer to avoid calling
set_vq_ready() after driver_ok if no
VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK. Or it might be too late.
Yeah, maybe is too late. We already released several versions without
that.
>
>>
>> >
>> >VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We do know enabling could
>> >be
>> >done after driver_ok.
>> >Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We don't know whether
>> >enabling could be done after driver_ok or not.
>>
>> I see your point, indeed I didn't send a v2 of that patch.
>> Maybe we should document that, because it could be interpreted that if
>> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated the enabling
>> should always be done before driver_ok (which is true for example
>> in
>> VDUSE).
>
>I see, so I think we probably need the fix.
>
>>
>> BTW I think we should discuss it in the kernel patch.
>>
>> Thanks,
>> Stefano
>>
>> >
>> >Thanks
>> >
>> >>
>> >> So better check its return value anyway.
>> >>
>> >> [1]
https://lore.kernel.org/virtualization/20240206145154.118044-1-sgarz...@redhat.com/T/#u
>> >>
>> >> Signed-off-by: Stefano Garzarella <sgarz...@redhat.com>
>> >> ---
>> >> Note: This patch conflicts with [2], but the resolution is simple,
>> >> so for now I sent a patch for the current master, but I'll rebase
>> >> this patch if we merge the other one first.
>
>Will go through [2].
Here I meant that the conflict is only in the code touched, because
Kevin's patch remove/move some of the code touched by this patch.
And rightly he checked the return value of the ioctl as I would like to
do in the other places where we call the same ioctl.
So honestly I still don't understand what's wrong with this patch...
Nothing wrong now.
Acked-by: Jason Wang <jasow...@redhat.com>
Thanks for the review,
I'll send a v2 carrying your and Eugenio acks, rebasing on top of
Kevin's patch, so it should be easy to merge.
Stefano
