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

[Long Li <longli@microsoft.com>]

> Subject: RE: [PATCH 2/2] Drivers: hv: add XStore Fastpath driver
> 
> 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

I'm making changes in v2 as you suggested.

Long

> 
> > > > >
> > > > > > +		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,
> > > > > > +	},
> > > > > > +};
> > > > > > +