On 11.08.26 04:32, Connor Kite wrote:
On Mon, Aug 3, 2026 at 6:52 AM Hanna Czenczek <[email protected]> wrote:
+ struct vhost_user *u = dev->opaque;
+ int svq_idx = file->index - dev->vq_index;
+ if (u->user->memory_isolation) {
+ VhostShadowVirtqueue *svq = g_ptr_array_index(u->shadow_vqs,
+ svq_idx);
Bounds checking via `vhost_user_get_vq_index()` would be nice. (Same below.)
Adding the bounds checking outside of the memory-isolation only code path, since
file->index wasn't being checked previously.
+ if (svq->hdev_kick.initialized == false) {
+ int r = event_notifier_init(&svq->hdev_kick, 0);
+ if (r) {
+ error_report("Failed to create kick event notifier");
+ return r;
+ }
+ }
+
+ file->fd = event_notifier_get_fd(&svq->hdev_kick);
What if `file->fd` was -1, i.e. `VHOST_FILE_UNBIND`? Should we put a
real FD here then or just continue with -1?
Good point! I think we need to propagate the -1 to the backend device, and
run event_notifier_cleanup on the existing svq->hdev_kick.
...
+
+ if (svq->hdev_call.initialized == false) {
+ int r = event_notifier_init(&svq->hdev_call, 0);
Do we need to check if the call event FD is already active?
Do you mean, do we need to call event_notifier_test_and_clear to see if it's
been set and potentially run the call handler at this stage?
Yes, that is what I’m wondering. `vhost_svq_set_svq_kick_fd()` has this
comment:
> event_notifier_set_handler already checks for guest's notifications if
> they arrive at the new file descriptor in the switch, so there is no
> need to explicitly check for them.
So it sounds like there is some mechanism that will ensure that when the
kick FD is set on a shadow virtqueue, it is checked for activity
(notification), and if it is active, this is… handled? Which I suppose
means the installed handler will be called.
But it’s entirely possible I misunderstand something because the same
code in `vhost_svq_set_svq_kick_fd()` also just calls
`event_notifier_set(svq_kick)`, so… kinda always emulates a kick, I assume?
In any case, there seems to be worry about pending checking kicks from
the guest in this switch, and so I wonder if the same needs to be done
for the call FD.
Hanna
If so, this might be where some of the intricacies of when things happen in
vhost-user vs in vhost would require some extra care, unless isolation mode
implementation moved up into vhost.
...
+ /*Modified from vhost-vdpa*/
+ u->shadow_vqs = g_ptr_array_new_full(dev->nvqs, vhost_svq_free);
+ for (int i = 0; i < dev->nvqs; i++) {
+ VhostShadowVirtqueue *svq;
+ svq = vhost_svq_new(NULL, NULL);
Patch 15 adds clean-up for this; I would squash it into here, as far as
possible. Ideally, a patch series can be stopped at any step and not
break anything, so if an allocation is added, the accompanying freeing
should come with it.
Hanna
In hindsight I shouldn't have considered cleanup a separate stage in the
development. Too easy to introduce bugs that way. I'll move cleanup up
and comment in the Patch 15 thread.
