On Fri, Jun 23, 2023 at 3:06 PM Peter Maydell <peter.mayd...@linaro.org> wrote: > > On Fri, 26 May 2023 at 16:32, Eugenio Pérez <epere...@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 >