RE: [PATCH 2/2] Drivers: hv: add XStore Fastpath driver

[Michael Kelley <mikelley@microsoft.com>]

From: Long Li <longli@microsoft.com> Sent: Wednesday, June 23, 2021 3:59 PM

[snip]

> > > > > +
> > > > > +static void xs_fastpath_remove_vmbus(struct hv_device *device) {
> > > > > +	struct xs_fastpath_device *dev = hv_get_drvdata(device);
> > > > > +	unsigned long flags;
> > > > > +
> > > > > +	spin_lock_irqsave(&dev->vsp_pending_lock, flags);
> > > > > +	if (!list_empty(&dev->vsp_pending_list)) {
> > > >
> > > > I don't think this can ever happen.  If the device has already been
> > > > removed by xs_fastpath_remove_device(), then we know that the device
> > > > isn't open in any processes, so there can't be any ioctl's in
> > > > progress that would have entries in the vsp_pending_list.
> > >
> > > The VSC is designed to support asynchronous I/O (while not implemented
> > > in this version), so vsp_pending_list is needed if a user-mode decides
> > > to close the file and not waiting for I/O.
> > >

I was thinking more about the handling of asynchronous I/Os.  As I noted
previously, this function is called after we know that the device isn't
open in any processes.  In fact, a process that previously had the device
open might have terminated.  So it seems problematic to have async I/Os
still pending, since they would have passed guest physical addresses to
Hyper-V.  If the process making the original async I/O request has
terminated, presumably any pinned pages have been unpinned, so
who knows how the memory is now being used in the guest.

With that thinking in mind, it seems like waiting for any pending
asynchronous I/Os needs to be done in xs_fastpath_fop_release, not
here.  The waiting would have to be only for asynchronous I/Os
associated with that particular open of the device.  With that
waiting in place, when the close completes we know that no
memory is pinned for associated asynchronous I/Os, and Hyper-V
doesn't have any PFNs that would be problematic if it later
wrote to them.

Michael

> > > >
> > > > > +		spin_unlock_irqrestore(&dev->vsp_pending_lock, flags);
> > > > > +		xs_fastpath_dbg("wait for vsp_pending_list\n");
> > > > > +		wait_event(dev->wait_vsp,
> > > > > +			list_empty(&dev->vsp_pending_list));
> > > > > +	} else
> > > > > +		spin_unlock_irqrestore(&dev->vsp_pending_lock, flags);
> > > > > +
> > > > > +	/* At this point, no VSC/VSP traffic is possible over vmbus */
> > > > > +	hv_set_drvdata(device, NULL);
> > > > > +	vmbus_close(device->channel);
> > > > > +}
> > > > > +
> > > > > +static int xs_fastpath_probe(struct hv_device *device,
> > > > > +			const struct hv_vmbus_device_id *dev_id) {
> > > > > +	int rc;
> > > > > +
> > > > > +	xs_fastpath_dbg("probing device\n");
> > > > > +
> > > > > +	rc = xs_fastpath_connect_to_vsp(device, xs_fastpath_ringbuffer_size);
> > > > > +	if (rc) {
> > > > > +		xs_fastpath_err("error connecting to VSP rc %d\n", rc);
> > > > > +		return rc;
> > > > > +	}
> > > > > +
> > > > > +	// create user-mode client library facing device
> > > > > +	rc = xs_fastpath_create_device(&xs_fastpath_dev);
> > > > > +	if (rc) {
> > > > > +		xs_fastpath_remove_vmbus(device);
> > > > > +		return rc;
> > > > > +	}
> > > > > +
> > > > > +	xs_fastpath_dbg("successfully probed device\n");
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int xs_fastpath_remove(struct hv_device *dev) {
> > > > > +	struct xs_fastpath_device *device = hv_get_drvdata(dev);
> > > > > +
> > > > > +	device->removing = true;
> > > > > +	xs_fastpath_remove_device(device);
> > > > > +	xs_fastpath_remove_vmbus(dev);
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static struct hv_driver xs_fastpath_drv = {
> > > > > +	.name = KBUILD_MODNAME,
> > > > > +	.id_table = id_table,
> > > > > +	.probe = xs_fastpath_probe,
> > > > > +	.remove = xs_fastpath_remove,
> > > > > +	.driver = {
> > > > > +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> > > > > +	},
> > > > > +};
> > > > > +