QEMU v8.0 is able to switch dynamically between vhost-vdpa passthrough
and SVQ mode as long as the net device does not have CVQ.  The net device
state followed (and migrated) by CVQ requires special care.

A pre-requisite to add CVQ to that framework is to determine if devices with
CVQ are migratable or not at initialization time.  The solution to it is to
always shadow only CVQ, and vq groups and ASID are used for that.

However, current qemu version only checks ASID at device start (as "driver set
DRIVER_OK status bit"), not at device initialization.  A check at
initialization time is required.  Otherwise, the guest would be able to set
and remove migration blockers at will [1].

This series is a requisite for migration of vhost-vdpa net devices with CVQ.
However it already makes sense by its own, as it reduces the number of ioctls
at migration time, decreasing the error paths there.

[1] https://lore.kernel.org/qemu-devel/2616f0cd-f9e8-d183-ea78-db1be4825d9c@redhat.com/
---
v4:
* Only probe one of MQ or !MQ.
* Merge vhost_vdpa_cvq_is_isolated in vhost_vdpa_probe_cvq_isolation
* Call ioctl directly instead of adding functions.

v3:
* Only record cvq_isolated, true if the device have cvq isolated in both !MQ
* and MQ configurations.
* Drop the cache of cvq group, it can be done on top

v2:
* Take out the reset of the device from vhost_vdpa_cvq_is_isolated
  (reported by Lei Yang).
* Expand patch messages by Stefano G. questions.

Eugenio Pérez (2):
  vdpa: return errno in vhost_vdpa_get_vring_group error
  vdpa: move CVQ isolation check to net_init_vhost_vdpa

 net/vhost-vdpa.c | 147 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 112 insertions(+), 35 deletions(-)

-- 
2.31.1

On Fri, 26 May 2023 at 16:32, Eugenio Pérez <eperezma@redhat.com> wrote:
>
> QEMU v8.0 is able to switch dynamically between vhost-vdpa passthrough
> and SVQ mode as long as the net device does not have CVQ.  The net device
> state followed (and migrated) by CVQ requires special care.
>
> A pre-requisite to add CVQ to that framework is to determine if devices with
> CVQ are migratable or not at initialization time.  The solution to it is to
> always shadow only CVQ, and vq groups and ASID are used for that.
>
> However, current qemu version only checks ASID at device start (as "driver set
> DRIVER_OK status bit"), not at device initialization.  A check at
> initialization time is required.  Otherwise, the guest would be able to set
> and remove migration blockers at will [1].
>
> This series is a requisite for migration of vhost-vdpa net devices with CVQ.
> However it already makes sense by its own, as it reduces the number of ioctls
> at migration time, decreasing the error paths there.

Hi -- since you're working on the net_init_vhost_vdpa() code,
would you mind having a look at Coverity CID 1490785 ?
This is about a leak of the vdpa_device_fd. We fixed one
instance of that leak in commit aed5da45daf734ddc54 but
it looks like there's still a different leak:

    for (i = 0; i < queue_pairs; i++) {
        ncs[i] = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
                                     vdpa_device_fd, i, 2, true, opts->x_svq,
                                     iova_range, features);
        if (!ncs[i])
            goto err;
    }

    if (has_cvq) {
        nc = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
                                 vdpa_device_fd, i, 1, false,
                                 opts->x_svq, iova_range, features);
        if (!nc)
            goto err;
    }

    return 0;

In this code, if queue_pairs is non-zero we will use
vdpa_device_fd because we pass it to net_vhost_vdpa_init().
Similarly, if has_cvq is true then we'll also use the fd.
But if queue_pairs is zero and has_cvq is false then we
will not do anything with the fd, and will return 0,
leaking the file descriptor.

Maybe this combination is not supposed to happen, but
I can't see anything in vhost_vdpa_get_max_queue_pairs()
or in this function which guards against it. If it's
an invalid setup we should detect it and return an
error, I think.

thanks
-- PMM

On Fri, Jun 23, 2023 at 3:06 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 26 May 2023 at 16:32, Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > QEMU v8.0 is able to switch dynamically between vhost-vdpa passthrough
> > and SVQ mode as long as the net device does not have CVQ.  The net device
> > state followed (and migrated) by CVQ requires special care.
> >
> > A pre-requisite to add CVQ to that framework is to determine if devices with
> > CVQ are migratable or not at initialization time.  The solution to it is to
> > always shadow only CVQ, and vq groups and ASID are used for that.
> >
> > However, current qemu version only checks ASID at device start (as "driver set
> > DRIVER_OK status bit"), not at device initialization.  A check at
> > initialization time is required.  Otherwise, the guest would be able to set
> > and remove migration blockers at will [1].
> >
> > This series is a requisite for migration of vhost-vdpa net devices with CVQ.
> > However it already makes sense by its own, as it reduces the number of ioctls
> > at migration time, decreasing the error paths there.
>
> Hi -- since you're working on the net_init_vhost_vdpa() code,
> would you mind having a look at Coverity CID 1490785 ?
> This is about a leak of the vdpa_device_fd. We fixed one
> instance of that leak in commit aed5da45daf734ddc54 but
> it looks like there's still a different leak:
>
>     for (i = 0; i < queue_pairs; i++) {
>         ncs[i] = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
>                                      vdpa_device_fd, i, 2, true, opts->x_svq,
>                                      iova_range, features);
>         if (!ncs[i])
>             goto err;
>     }
>
>     if (has_cvq) {
>         nc = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
>                                  vdpa_device_fd, i, 1, false,
>                                  opts->x_svq, iova_range, features);
>         if (!nc)
>             goto err;
>     }
>
>     return 0;
>
> In this code, if queue_pairs is non-zero we will use
> vdpa_device_fd because we pass it to net_vhost_vdpa_init().
> Similarly, if has_cvq is true then we'll also use the fd.
> But if queue_pairs is zero and has_cvq is false then we
> will not do anything with the fd, and will return 0,
> leaking the file descriptor.
>
> Maybe this combination is not supposed to happen, but
> I can't see anything in vhost_vdpa_get_max_queue_pairs()
> or in this function which guards against it. If it's
> an invalid setup we should detect it and return an
> error, I think.
>

Yes, a device that returns 0 max_vq_pairs would be invalid. I will
introduce a guard for that.

Thanks for the heads up!

> thanks
> -- PMM
>

QE did a sanity test on v4 of this series using the vdpa_sim
device,everything is working fine.

Tested-by: Lei Yang <leiyang@redhat.com>

On Fri, May 26, 2023 at 11:31 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> QEMU v8.0 is able to switch dynamically between vhost-vdpa passthrough
> and SVQ mode as long as the net device does not have CVQ.  The net device
> state followed (and migrated) by CVQ requires special care.
>
> A pre-requisite to add CVQ to that framework is to determine if devices with
> CVQ are migratable or not at initialization time.  The solution to it is to
> always shadow only CVQ, and vq groups and ASID are used for that.
>
> However, current qemu version only checks ASID at device start (as "driver set
> DRIVER_OK status bit"), not at device initialization.  A check at
> initialization time is required.  Otherwise, the guest would be able to set
> and remove migration blockers at will [1].
>
> This series is a requisite for migration of vhost-vdpa net devices with CVQ.
> However it already makes sense by its own, as it reduces the number of ioctls
> at migration time, decreasing the error paths there.
>
> [1] https://lore.kernel.org/qemu-devel/2616f0cd-f9e8-d183-ea78-db1be4825d9c@redhat.com/
> ---
> v4:
> * Only probe one of MQ or !MQ.
> * Merge vhost_vdpa_cvq_is_isolated in vhost_vdpa_probe_cvq_isolation
> * Call ioctl directly instead of adding functions.
>
> v3:
> * Only record cvq_isolated, true if the device have cvq isolated in both !MQ
> * and MQ configurations.
> * Drop the cache of cvq group, it can be done on top
>
> v2:
> * Take out the reset of the device from vhost_vdpa_cvq_is_isolated
>   (reported by Lei Yang).
> * Expand patch messages by Stefano G. questions.
>
> Eugenio Pérez (2):
>   vdpa: return errno in vhost_vdpa_get_vring_group error
>   vdpa: move CVQ isolation check to net_init_vhost_vdpa
>
>  net/vhost-vdpa.c | 147 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 112 insertions(+), 35 deletions(-)
>
> --
> 2.31.1
>
>