Driver for Hyper-v virtual compute device

- Create skeleton and add basic functionality for the hyper-v compute device driver (dxgkrnl).

- Register for PCI and VM bus driver notifications and handle initialization of VM bus channels.

- Connect the dxgkrnl module to the drivers/hv/ Makefile and Kconfig

- Create a MAINTAINERS entry

A VM bus channel is a communication interface between the hyper-v guest and the host. The are two type of VM bus channels, used in the driver:
- the global channel
- per virtual compute device channel

A PCI device is created for each virtual compute device, projected by the host. The device vendor is PCI_VENDOR_ID_MICROSOFT and device id is PCI_DEVICE_ID_VIRTUAL_RENDER. dxg_pci_probe_device handles arrival of such devices. The PCI config space of the virtual compute device has luid of the corresponding virtual compute device VM bus channel. This is how the compute device adapter objects are linked to VM bus channels.

VM bus interface version is exchanged by reading/writing the PCI config space of the virtual compute device.

The IO space is used to handle CPU accessible compute device allocations. Hyper-v allocates IO space for the global VM bus channel.

Signed-off-by Iouri Tarassov <[email protected]>

drivers/hv/dxgkrnl/dxgkrnl.h new file +20 -0

+	return 0;
+}
+
+static ssize_t dxgk_write(struct file *f, const char __user *s, size_t len,
+			  loff_t *o)
+{
+	pr_debug("file write\n");
+	return len;
+}
+
+const struct file_operations dxgk_fops = {
+	.owner = THIS_MODULE,
+	.open = dxgk_open,
+	.release = dxgk_release,
+	.write = dxgk_write,
+	.read = dxgk_read,
+};
+
+/*
+ * Interface with the PCI driver

drivers/hv/dxgkrnl/dxgkrnl.h new file +19 -0

+
+	if (uuid_le_cmp(hdev->dev_type, id_table[0].guid) == 0) {
+		/* This is a new virtual GPU channel */
+		guid_to_luid(&hdev->channel->offermsg.offer.if_instance, &luid);
+		pr_debug("vGPU channel: %pUb",
+			    &hdev->channel->offermsg.offer.if_instance);
+		vgpuch = vzalloc(sizeof(struct dxgvgpuchannel));
+		if (vgpuch == NULL) {
+			ret = -ENOMEM;
+			goto error;
+		}
+		vgpuch->adapter_luid = luid;
+		vgpuch->hdev = hdev;
+		list_add_tail(&vgpuch->vgpu_ch_list_entry,
+			      &dxgglobal->vgpu_ch_list_head);
+	} else if (uuid_le_cmp(hdev->dev_type, id_table[1].guid) == 0) {
+		/* This is the global Dxgkgnl channel */
+		pr_debug("Global channel: %pUb",
+			    &hdev->channel->offermsg.offer.if_instance);

drivers/hv/dxgkrnl/dxgkrnl.h new file +36 -0

+	return ret;
+}
+
+static int dxg_remove_vmbus(struct hv_device *hdev)
+{
+	int ret = 0;
+	struct dxgvgpuchannel *vgpu_channel;
+
+	mutex_lock(&dxgglobal->device_mutex);
+
+	if (uuid_le_cmp(hdev->dev_type, id_table[0].guid) == 0) {
+		pr_debug("Remove virtual GPU channel\n");
+		list_for_each_entry(vgpu_channel,
+				    &dxgglobal->vgpu_ch_list_head,
+				    vgpu_ch_list_entry) {
+			if (vgpu_channel->hdev == hdev) {
+				list_del(&vgpu_channel->vgpu_ch_list_entry);
+				vfree(vgpu_channel);
+				break;
+			}
+		}
+	} else if (uuid_le_cmp(hdev->dev_type, id_table[1].guid) == 0) {
+		pr_debug("Remove global channel device\n");
+		dxgglobal_destroy_global_channel();
+	} else {
+		/* Unknown device type */
+		pr_err("remove: unknown device type\n");
+		ret = -EBADE;
+	}
+
+	mutex_unlock(&dxgglobal->device_mutex);
+	if (ret)
+		pr_debug("err: %s %d", __func__, ret);
+	return ret;
+}
+

drivers/hv/dxgkrnl/dxgkrnl.h new file +25 -0

+
+	dxgglobal = vzalloc(sizeof(struct dxgglobal));
+	if (!dxgglobal)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&dxgglobal->plisthead);
+	mutex_init(&dxgglobal->plistmutex);
+	mutex_init(&dxgglobal->device_mutex);
+
+	INIT_LIST_HEAD(&dxgglobal->vgpu_ch_list_head);
+
+	init_rwsem(&dxgglobal->channel_lock);
+
+	pr_debug("dxgglobal_init end\n");
+	return ret;
+}
+
+static void dxgglobal_destroy(void)
+{
+	if (dxgglobal) {
+		if (dxgglobal->vmbus_registered)
+			vmbus_driver_unregister(&dxg_drv);
+
+		dxgglobal_destroy_global_channel();
+

drivers/hv/dxgkrnl/dxgkrnl.h new file +4 -0

+	ret = dxgglobal_create();
+	if (ret) {
+		pr_err("dxgglobal_init failed");
+		return -ENOMEM;

Quoted 23 lines · Iouri Tarassov

- Create skeleton and add basic functionality for the hyper-v compute device driver (dxgkrnl).

- Register for PCI and VM bus driver notifications and handle initialization of VM bus channels.

- Connect the dxgkrnl module to the drivers/hv/ Makefile and Kconfig

- Create a MAINTAINERS entry

A VM bus channel is a communication interface between the hyper-v guest and the host. The are two type of VM bus channels, used in the driver:
- the global channel
- per virtual compute device channel

A PCI device is created for each virtual compute device, projected by the host. The device vendor is PCI_VENDOR_ID_MICROSOFT and device id is PCI_DEVICE_ID_VIRTUAL_RENDER. dxg_pci_probe_device handles arrival of such devices. The PCI config space of the virtual compute device has luid of the corresponding virtual compute device VM bus channel. This is how the compute device adapter objects are linked to VM bus channels.

Quoted 7 lines

VM bus interface version is exchanged by reading/writing the PCI config space of the virtual compute device.

The IO space is used to handle CPU accessible compute device allocations. Hyper-v allocates IO space for the global VM bus channel.

Signed-off-by Iouri Tarassov <[email protected]>

Please work with internal developers to get reviews from them first, before requiring the kernel community to point out basic issues. It will save you a lot of time and stop us from feeling like we are having our time wasted.

Some simple examples below that your coworkers should have caught:

drivers/hv/dxgkrnl/dxgkrnl.h line 4

Quoted 7 lines

drivers/hv/dxgkrnl/dxgkrnl.h new file +4 -0

@@ -0,0 +1,119 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright (c) 2019, Microsoft Corporation.

It is now 2022 :)

drivers/hv/dxgkrnl/dxgkrnl.h line 94

Quoted 1 line

drivers/hv/dxgkrnl/dxgkrnl.h new file +1 -0

+void init_ioctls(void);

That is a horrible global function name you just added to the kernel's namespace for a single driver :(

drivers/hv/dxgkrnl/dxgkrnl.h line 100

Quoted 5 lines

drivers/hv/dxgkrnl/dxgkrnl.h new file +5 -0

+long dxgk_unlocked_ioctl(struct file *f, unsigned int p1, unsigned long p2);
+
+static inline void guid_to_luid(guid_t *guid, struct winluid *luid)
+{
+	*luid = *(struct winluid *)&guid->b[0];

Why is the cast needed? Shouldn't you use real types in your structures?

drivers/hv/dxgkrnl/dxgkrnl.h line 117

Quoted 10 lines

drivers/hv/dxgkrnl/dxgkrnl.h new file +10 -0

+/*
+ * The interface version is used to ensure that the host and the guest use the
+ * same VM bus protocol. It needs to be incremented every time the VM bus
+ * interface changes. DXGK_VMBUS_LAST_COMPATIBLE_INTERFACE_VERSION is
+ * incremented each time the earlier versions of the interface are no longer
+ * compatible with the current version.
+ */
+#define DXGK_VMBUS_INTERFACE_VERSION_OLD		27
+#define DXGK_VMBUS_INTERFACE_VERSION			40
+#define DXGK_VMBUS_LAST_COMPATIBLE_INTERFACE_VERSION	16

Where do these numbers come from, the hypervisor specification?

drivers/hv/dxgkrnl/dxgkrnl.h line 26

Quoted 6 lines

drivers/hv/dxgkrnl/dxgkrnl.h new file +6 -0

+/*
+ * Pointer to the global device data. By design
+ * there is a single vGPU device on the VM bus and a single /dev/dxg device
+ * is created.
+ */
+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

Quoted 76 lines · Greg KH

Quoted 23 lines · Iouri Tarassov

- Create skeleton and add basic functionality for the hyper-v compute device driver (dxgkrnl).

- Register for PCI and VM bus driver notifications and handle initialization of VM bus channels.

- Connect the dxgkrnl module to the drivers/hv/ Makefile and Kconfig

- Create a MAINTAINERS entry

A VM bus channel is a communication interface between the hyper-v guest and the host. The are two type of VM bus channels, used in the driver:
- the global channel
- per virtual compute device channel

A PCI device is created for each virtual compute device, projected by the host. The device vendor is PCI_VENDOR_ID_MICROSOFT and device id is PCI_DEVICE_ID_VIRTUAL_RENDER. dxg_pci_probe_device handles arrival of such devices. The PCI config space of the virtual compute device has luid of the corresponding virtual compute device VM bus channel. This is how the compute device adapter objects are linked to VM bus channels.

Quoted 7 lines

VM bus interface version is exchanged by reading/writing the PCI config space of the virtual compute device.

The IO space is used to handle CPU accessible compute device allocations. Hyper-v allocates IO space for the global VM bus channel.

Signed-off-by Iouri Tarassov <[email protected]>

Please work with internal developers to get reviews from them first, before requiring the kernel community to point out basic issues. It will save you a lot of time and stop us from feeling like we are having our time wasted.

Some simple examples below that your coworkers should have caught:

drivers/hv/dxgkrnl/dxgkrnl.h line 4

Quoted 7 lines

drivers/hv/dxgkrnl/dxgkrnl.h new file +4 -0

@@ -0,0 +1,119 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright (c) 2019, Microsoft Corporation.

It is now 2022 :)

drivers/hv/dxgkrnl/dxgkrnl.h line 94

Quoted 1 line

drivers/hv/dxgkrnl/dxgkrnl.h new file +1 -0

+void init_ioctls(void);

That is a horrible global function name you just added to the kernel's namespace for a single driver :(

drivers/hv/dxgkrnl/dxgkrnl.h line 100

Quoted 5 lines

drivers/hv/dxgkrnl/dxgkrnl.h new file +5 -0

+long dxgk_unlocked_ioctl(struct file *f, unsigned int p1, unsigned long p2);
+
+static inline void guid_to_luid(guid_t *guid, struct winluid *luid)
+{
+	*luid = *(struct winluid *)&guid->b[0];

Why is the cast needed? Shouldn't you use real types in your structures?

drivers/hv/dxgkrnl/dxgkrnl.h line 117

Quoted 10 lines

drivers/hv/dxgkrnl/dxgkrnl.h new file +10 -0

+/*
+ * The interface version is used to ensure that the host and the guest use the
+ * same VM bus protocol. It needs to be incremented every time the VM bus
+ * interface changes. DXGK_VMBUS_LAST_COMPATIBLE_INTERFACE_VERSION is
+ * incremented each time the earlier versions of the interface are no longer
+ * compatible with the current version.
+ */
+#define DXGK_VMBUS_INTERFACE_VERSION_OLD		27
+#define DXGK_VMBUS_INTERFACE_VERSION			40
+#define DXGK_VMBUS_LAST_COMPATIBLE_INTERFACE_VERSION	16

Where do these numbers come from, the hypervisor specification?

Quoted 12 lines

drivers/hv/dxgkrnl/dxgkrnl.h line 26

Quoted 6 lines

drivers/hv/dxgkrnl/dxgkrnl.h new file +6 -0

+/*
+ * Pointer to the global device data. By design
+ * there is a single vGPU device on the VM bus and a single /dev/dxg device
+ * is created.
+ */
+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

This may not be as big an issue as you thought. The device discovery is still done via the normal VMBus probing routine. For all intents and purposes the dxgglobal structure can be broken down into per device fields and a global structure which contains the protocol versioning information -- my understanding is there will always be a global structure to hold information related to the backend, regardless of how many devices there are.

I definitely think splitting is doable, but I also understand why Iouri does not want to do it _now_ given there is no such a model for multiple devices yet, so anything we put into the per-device structure could be incomplete and it requires further changing when such a model arrives later.

Iouri, please correct me if I have the wrong mental model here.

All in all, I hope this is not going to be a deal breaker for the acceptance of this driver.

Thanks,
Wei.

Quoted 93 lines · Wei Liu

Quoted 6 lines · Greg KH

Quoted 5 lines · Iouri Tarassov

- Create skeleton and add basic functionality for the hyper-v compute device driver (dxgkrnl).

Quoted 1 line

Quoted 1 line

- Register for PCI and VM bus driver notifications and

handle initialization of VM bus channels.

Quoted 1 line

Quoted 1 line

- Connect the dxgkrnl module to the drivers/hv/ Makefile and

Kconfig

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

- Create a MAINTAINERS entry
A VM bus channel is a communication interface between the

hyper-v guest

Quoted 4 lines

Quoted 4 lines

and the host. The are two type of VM bus channels, used in the driver:
- the global channel
- per virtual compute device channel

Quoted 1 line

Quoted 1 line

A PCI device is created for each virtual compute device,

projected

Quoted 7 lines

Quoted 7 lines

by the host. The device vendor is PCI_VENDOR_ID_MICROSOFT and device id is PCI_DEVICE_ID_VIRTUAL_RENDER. dxg_pci_probe_device handles arrival of such devices. The PCI config space of the virtual compute device has luid of the corresponding virtual compute device VM bus channel. This is how the compute device adapter objects are linked to VM bus channels.

Quoted 1 line

Quoted 1 line

VM bus interface version is exchanged by reading/writing the PCI

config

Quoted 19 lines

Quoted 5 lines

space of the virtual compute device.

Quoted 1 line

Quoted 1 line

The IO space is used to handle CPU accessible compute device

allocations. Hyper-v allocates IO space for the global VM bus channel.

Quoted 1 line

Quoted 1 line

Signed-off-by Iouri Tarassov <[email protected]>

Please work with internal developers to get reviews from them first,

before requiring the kernel community to point out basic issues. It will save you a lot of time and stop us from feeling like we are having our time wasted.

Quoted 11 lines

Some simple examples below that your coworkers should have caught:

Quoted 1 line

--- /dev/null

+++ b/drivers/hv/dxgkrnl/dxgkrnl.h

drivers/hv/dxgkrnl/dxgkrnl.h new file +4 -0

@@ -0,0 +1,119 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright (c) 2019, Microsoft Corporation.

It is now 2022 :)

Quoted 1 line

+void init_ioctls(void);

That is a horrible global function name you just added to the

kernel's

Quoted 2 lines

namespace for a single driver :(

Quoted 1 line

Quoted 1 line

+long dxgk_unlocked_ioctl(struct file *f, unsigned int p1,

unsigned long p2);

Quoted 24 lines

Quoted 5 lines

drivers/hv/dxgkrnl/dxgkrnl.h new file +4 -0

+
+static inline void guid_to_luid(guid_t *guid, struct winluid *luid)
+{
+	*luid = *(struct winluid *)&guid->b[0];

Why is the cast needed? Shouldn't you use real types in your

structures?

Quoted 18 lines

Quoted 1 line

+/*

+ * The interface version is used to ensure that the host and the guest use the
+ * same VM bus protocol. It needs to be incremented every time the VM bus
+ * interface changes. DXGK_VMBUS_LAST_COMPATIBLE_INTERFACE_VERSION is
+ * incremented each time the earlier versions of the interface are no longer
+ * compatible with the current version.
+ */

drivers/hv/dxgkrnl/dxgkrnl.h new file +3 -0

+#define DXGK_VMBUS_INTERFACE_VERSION_OLD		27
+#define DXGK_VMBUS_INTERFACE_VERSION			40
+#define DXGK_VMBUS_LAST_COMPATIBLE_INTERFACE_VERSION	16

Where do these numbers come from, the hypervisor specification?

Quoted 1 line

+/*

+ * Pointer to the global device data. By design
+ * there is a single vGPU device on the VM bus and a single /dev/dxg device
+ * is created.
+ */
+struct dxgglobal *dxgglobal;
No, make this per-device, NEVER have a single device for your

driver.

Quoted 4 lines

The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

This may not be as big an issue as you thought. The device discovery is still done via the normal VMBus probing routine. For all intents and purposes the dxgglobal structure can be broken down into per device fields and a global structure which contains the protocol versioning information -- my understanding is there will always be a global structure to hold information related to the backend, regardless of how many devices there are.

I definitely think splitting is doable, but I also understand why Iouri does not want to do it _now_ given there is no such a model for multiple devices yet, so anything we put into the per-device structure could be incomplete and it requires further changing when such a model arrives later.

Iouri, please correct me if I have the wrong mental model here.

Quoted 5 lines

All in all, I hope this is not going to be a deal breaker for the acceptance of this driver.

Thanks,
Wei.

I agree with Wei that there always be global driver data.

The driver reflects what the host offers and also it must provide the same interface to user mode as the host driver does. This is because we want the user mode clients to use the same device interface as if they are working on the host directly.

By design a single global VMBus channel is offered by the host and a single /dev/dxg device is created. The /dev/dxg device provides interface to enumerate virtual compute devices via an ioctl.

If we are to change this model, we would need to make changes to user mode clients, which is a big re-design change, affecting many hardware vendors.

Quoted 7 lines

Quoted 1 line

+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

This may not be as big an issue as you thought. The device discovery is still done via the normal VMBus probing routine. For all intents and purposes the dxgglobal structure can be broken down into per device fields and a global structure which contains the protocol versioning information -- my understanding is there will always be a global structure to hold information related to the backend, regardless of how many devices there are.

I definitely think splitting is doable, but I also understand why Iouri does not want to do it _now_ given there is no such a model for multiple devices yet, so anything we put into the per-device structure could be incomplete and it requires further changing when such a model arrives later.

Iouri, please correct me if I have the wrong mental model here.

All in all, I hope this is not going to be a deal breaker for the acceptance of this driver.

Quoted 8 lines · Wei Liu

Quoted 7 lines

Quoted 1 line

+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

Quoted 7 lines

This may not be as big an issue as you thought. The device discovery is still done via the normal VMBus probing routine. For all intents and purposes the dxgglobal structure can be broken down into per device fields and a global structure which contains the protocol versioning information -- my understanding is there will always be a global structure to hold information related to the backend, regardless of how many devices there are.

Then that is wrong and needs to be fixed. Drivers should almost never have any global data, that is not how Linux drivers work. What happens when you get a second device in your system for this? Major rework would have to happen and the code will break. Handle that all now as it takes less work to make this per-device than it does to have a global variable.

Quoted 17 lines · Greg KH

Quoted 8 lines · Wei Liu

Quoted 7 lines

Quoted 1 line

+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

Quoted 7 lines

This may not be as big an issue as you thought. The device discovery is still done via the normal VMBus probing routine. For all intents and purposes the dxgglobal structure can be broken down into per device fields and a global structure which contains the protocol versioning information -- my understanding is there will always be a global structure to hold information related to the backend, regardless of how many devices there are.

Quoted 7 lines

Then that is wrong and needs to be fixed. Drivers should almost never have any global data, that is not how Linux drivers work. What happens when you get a second device in your system for this? Major rework would have to happen and the code will break. Handle that all now as it takes less work to make this per-device than it does to have a global variable.

It is perhaps easier to draw parallel from an existing driver. I feel like we're talking past each other.

Let's look at drivers/iommu/intel/iommu.c. There are a bunch of lists like static LIST_HEAD(dmar_rmrr_units). During the probing phase, new units will be added to the list. I this the current code is following this model. dxgglobal fulfills the role of a list.

Setting aside the question of whether it makes sense to keep a copy of the per-VM state in each device instance, I can see the code be changed to:

    struct mutex device_mutex; /* split out from dxgglobal */
    static LIST_HEAD(dxglist);
    
    /* Rename struct dxgglobal to struct dxgstate */
    struct dxgstate {
       struct list_head dxglist; /* link for dxglist */
       /* ... original fields sans device_mutex */
    }

    /*
     * Provide a bunch of helpers manipulate the list. Called in probe /
     * remove etc.
     */
    struct dxgstate *find_dxgstate(...);
    void remove_dxgstate(...);
    int add_dxgstate(...);

This model is well understood and used in tree. It is just that it doesn't provide much value in doing this now since the list will only contain one element. I hope that you're not saying we cannot even use a per-module pointer to quickly get the data structure we want to use, right?

Are you suggesting Iouri use dev_set_drvdata to stash the dxgstate into the device object? I think that can be done too.

The code can be changed as:

    /* Rename struct dxgglobal to dxgstate and remove unneeded fields */
    struct dxgstate { ... };

    static int dxg_probe_vmbus(...) {

        /* probe successfully */

	struct dxgstate *state = kmalloc(...);
	/* Fill in dxgstate with information from backend */

	/* hdev->dev is the device object from the core driver framework */
	dev_set_drvdata(&hdev->dev, state);
    }

    static int dxg_remove_vmbus(...) {
        /* Normal stuff here ...*/

	struct dxgstate *state = dev_get_drvdata(...);
	dev_set_drvdata(..., NULL);
	kfree(state);
    }

    /* In all other functions */
    void do_things(...) {
        struct dxgstate *state = dev_get_drvdata(...);

	/* Use state in place of where dxgglobal was needed */

    }

Iouri, notice this doesn't change anything regarding how userspace is designed. This is about how kernel organises its data.

I hope what I wrote above can bring our understanding closer.

Thanks,
Wei.

Quoted 96 lines · Wei Liu

Quoted 17 lines · Greg KH

Quoted 8 lines · Wei Liu

Quoted 7 lines

Quoted 1 line

+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

Quoted 7 lines

This may not be as big an issue as you thought. The device discovery is still done via the normal VMBus probing routine. For all intents and purposes the dxgglobal structure can be broken down into per device fields and a global structure which contains the protocol versioning information -- my understanding is there will always be a global structure to hold information related to the backend, regardless of how many devices there are.

Quoted 7 lines

Then that is wrong and needs to be fixed. Drivers should almost never have any global data, that is not how Linux drivers work. What happens when you get a second device in your system for this? Major rework would have to happen and the code will break. Handle that all now as it takes less work to make this per-device than it does to have a global variable.

It is perhaps easier to draw parallel from an existing driver. I feel like we're talking past each other.

Let's look at drivers/iommu/intel/iommu.c. There are a bunch of lists like static LIST_HEAD(dmar_rmrr_units). During the probing phase, new units will be added to the list. I this the current code is following this model. dxgglobal fulfills the role of a list.

Setting aside the question of whether it makes sense to keep a copy of the per-VM state in each device instance, I can see the code be changed to:

    struct mutex device_mutex; /* split out from dxgglobal */
    static LIST_HEAD(dxglist);
    
    /* Rename struct dxgglobal to struct dxgstate */
    struct dxgstate {
       struct list_head dxglist; /* link for dxglist */
       /* ... original fields sans device_mutex */
    }

    /*
     * Provide a bunch of helpers manipulate the list. Called in probe /
     * remove etc.
     */
    struct dxgstate *find_dxgstate(...);
    void remove_dxgstate(...);
    int add_dxgstate(...);

This model is well understood and used in tree. It is just that it doesn't provide much value in doing this now since the list will only contain one element. I hope that you're not saying we cannot even use a per-module pointer to quickly get the data structure we want to use, right?

Are you suggesting Iouri use dev_set_drvdata to stash the dxgstate into the device object? I think that can be done too.

The code can be changed as:

    /* Rename struct dxgglobal to dxgstate and remove unneeded fields */
    struct dxgstate { ... };

    static int dxg_probe_vmbus(...) {

        /* probe successfully */

	struct dxgstate *state = kmalloc(...);
	/* Fill in dxgstate with information from backend */

	/* hdev->dev is the device object from the core driver framework */
	dev_set_drvdata(&hdev->dev, state);
    }

    static int dxg_remove_vmbus(...) {
        /* Normal stuff here ...*/

	struct dxgstate *state = dev_get_drvdata(...);
	dev_set_drvdata(..., NULL);
	kfree(state);
    }

    /* In all other functions */
    void do_things(...) {
        struct dxgstate *state = dev_get_drvdata(...);

	/* Use state in place of where dxgglobal was needed */

    }

Quoted 7 lines

Iouri, notice this doesn't change anything regarding how userspace is designed. This is about how kernel organises its data.

I hope what I wrote above can bring our understanding closer.

Thanks,
Wei.

I can certainly remove dxgglobal and keep the  pointer to the global state in the device object.

This will require passing of the global pointer to all functions, which need to access it.

Maybe my understanding of the Greg's suggestion was not correct. I thought the suggestion was

to have multiple /dev/dxgN devices (one per virtual compute device).

This would change how the user mode
clients enumerate and communicate with compute devices.

Quoted 109 lines · Iouri Tarassov

Quoted 96 lines · Wei Liu

Quoted 17 lines · Greg KH

Quoted 8 lines · Wei Liu

Quoted 7 lines

Quoted 1 line

+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

Quoted 7 lines

This may not be as big an issue as you thought. The device discovery is still done via the normal VMBus probing routine. For all intents and purposes the dxgglobal structure can be broken down into per device fields and a global structure which contains the protocol versioning information -- my understanding is there will always be a global structure to hold information related to the backend, regardless of how many devices there are.

Quoted 7 lines

Then that is wrong and needs to be fixed. Drivers should almost never have any global data, that is not how Linux drivers work. What happens when you get a second device in your system for this? Major rework would have to happen and the code will break. Handle that all now as it takes less work to make this per-device than it does to have a global variable.

It is perhaps easier to draw parallel from an existing driver. I feel like we're talking past each other.

Let's look at drivers/iommu/intel/iommu.c. There are a bunch of lists like static LIST_HEAD(dmar_rmrr_units). During the probing phase, new units will be added to the list. I this the current code is following this model. dxgglobal fulfills the role of a list.

Setting aside the question of whether it makes sense to keep a copy of the per-VM state in each device instance, I can see the code be changed to:

    struct mutex device_mutex; /* split out from dxgglobal */
    static LIST_HEAD(dxglist);
    
    /* Rename struct dxgglobal to struct dxgstate */
    struct dxgstate {
       struct list_head dxglist; /* link for dxglist */
       /* ... original fields sans device_mutex */
    }

    /*
     * Provide a bunch of helpers manipulate the list. Called in probe /
     * remove etc.
     */
    struct dxgstate *find_dxgstate(...);
    void remove_dxgstate(...);
    int add_dxgstate(...);

This model is well understood and used in tree. It is just that it doesn't provide much value in doing this now since the list will only contain one element. I hope that you're not saying we cannot even use a per-module pointer to quickly get the data structure we want to use, right?

Are you suggesting Iouri use dev_set_drvdata to stash the dxgstate into the device object? I think that can be done too.

The code can be changed as:

    /* Rename struct dxgglobal to dxgstate and remove unneeded fields */
    struct dxgstate { ... };

    static int dxg_probe_vmbus(...) {

        /* probe successfully */

	struct dxgstate *state = kmalloc(...);
	/* Fill in dxgstate with information from backend */

	/* hdev->dev is the device object from the core driver framework */
	dev_set_drvdata(&hdev->dev, state);
    }

    static int dxg_remove_vmbus(...) {
        /* Normal stuff here ...*/

	struct dxgstate *state = dev_get_drvdata(...);
	dev_set_drvdata(..., NULL);
	kfree(state);
    }

    /* In all other functions */
    void do_things(...) {
        struct dxgstate *state = dev_get_drvdata(...);

	/* Use state in place of where dxgglobal was needed */

    }

Quoted 7 lines

Iouri, notice this doesn't change anything regarding how userspace is designed. This is about how kernel organises its data.

I hope what I wrote above can bring our understanding closer.

Thanks,
Wei.

I can certainly remove dxgglobal and keep the  pointer to the global state in the device object.

Quoted 8 lines

This will require passing of the global pointer to all functions, which need to access it.

Maybe my understanding of the Greg's suggestion was not correct. I thought the suggestion was

to have multiple /dev/dxgN devices (one per virtual compute device).

You have one device per HV device, as the bus already provides you. That's all you really need, right? Who would be opening the same device node multiple times?

Quoted 2 lines

This would change how the user mode
clients enumerate and communicate with compute devices.

What does userspace have to do here? It should just open the device node that is present when needed. How will there be multiple userspace clients for a single HV device?

Quoted 125 lines · Greg KH

Quoted 109 lines · Iouri Tarassov

Quoted 96 lines · Wei Liu

Quoted 17 lines · Greg KH

Quoted 8 lines · Wei Liu

Quoted 7 lines

Quoted 1 line

+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

Quoted 7 lines

This may not be as big an issue as you thought. The device discovery is still done via the normal VMBus probing routine. For all intents and purposes the dxgglobal structure can be broken down into per device fields and a global structure which contains the protocol versioning information -- my understanding is there will always be a global structure to hold information related to the backend, regardless of how many devices there are.

Quoted 7 lines

Then that is wrong and needs to be fixed. Drivers should almost never have any global data, that is not how Linux drivers work. What happens when you get a second device in your system for this? Major rework would have to happen and the code will break. Handle that all now as it takes less work to make this per-device than it does to have a global variable.

It is perhaps easier to draw parallel from an existing driver. I feel like we're talking past each other.

Let's look at drivers/iommu/intel/iommu.c. There are a bunch of lists like static LIST_HEAD(dmar_rmrr_units). During the probing phase, new units will be added to the list. I this the current code is following this model. dxgglobal fulfills the role of a list.

Setting aside the question of whether it makes sense to keep a copy of the per-VM state in each device instance, I can see the code be changed to:

    struct mutex device_mutex; /* split out from dxgglobal */
    static LIST_HEAD(dxglist);
    
    /* Rename struct dxgglobal to struct dxgstate */
    struct dxgstate {
       struct list_head dxglist; /* link for dxglist */
       /* ... original fields sans device_mutex */
    }

    /*
     * Provide a bunch of helpers manipulate the list. Called in probe /
     * remove etc.
     */
    struct dxgstate *find_dxgstate(...);
    void remove_dxgstate(...);
    int add_dxgstate(...);

This model is well understood and used in tree. It is just that it doesn't provide much value in doing this now since the list will only contain one element. I hope that you're not saying we cannot even use a per-module pointer to quickly get the data structure we want to use, right?

Are you suggesting Iouri use dev_set_drvdata to stash the dxgstate into the device object? I think that can be done too.

The code can be changed as:

    /* Rename struct dxgglobal to dxgstate and remove unneeded fields */
    struct dxgstate { ... };

    static int dxg_probe_vmbus(...) {

        /* probe successfully */

	struct dxgstate *state = kmalloc(...);
	/* Fill in dxgstate with information from backend */

	/* hdev->dev is the device object from the core driver framework */
	dev_set_drvdata(&hdev->dev, state);
    }

    static int dxg_remove_vmbus(...) {
        /* Normal stuff here ...*/

	struct dxgstate *state = dev_get_drvdata(...);
	dev_set_drvdata(..., NULL);
	kfree(state);
    }

    /* In all other functions */
    void do_things(...) {
        struct dxgstate *state = dev_get_drvdata(...);

	/* Use state in place of where dxgglobal was needed */

    }

Quoted 7 lines

Iouri, notice this doesn't change anything regarding how userspace is designed. This is about how kernel organises its data.

I hope what I wrote above can bring our understanding closer.

Thanks,
Wei.

I can certainly remove dxgglobal and keep the  pointer to the global state in the device object.

Quoted 8 lines

This will require passing of the global pointer to all functions, which need to access it.

Maybe my understanding of the Greg's suggestion was not correct. I thought the suggestion was

to have multiple /dev/dxgN devices (one per virtual compute device).

You have one device per HV device, as the bus already provides you. That's all you really need, right? Who would be opening the same device node multiple times?

Quoted 2 lines

This would change how the user mode
clients enumerate and communicate with compute devices.

Quoted 3 lines

What does userspace have to do here? It should just open the device node that is present when needed. How will there be multiple userspace clients for a single HV device?

Dxgkrnl creates a single user mode visible device node /dev/dxg.

It has
nothing to do with a specific hardware compute device on the host. Its purpose is to provide services (IOCTLs) to enumerate and manage virtual compute devices, which represent hardware devices on the host. The VMBus devices are not used directly by user mode clients in the current design.

Virtual compute devices are shared between processes. There could be a Cuda application, Gimp and a Direct3D12 application working at the same time.

This is what I mean by saying that there are multiple user mode clients who use the /dev/dxg driver interface. Each of this applications will open the /dev/dxg device node and enumerate/use virtual compute devices.

If we change the way how the virtual compute devices are visible to user mode, the Cuda runtime, Direct3D runtime would need to be changed.

I think we agreed that I will keep the global driver state in the device object as Wei suggested and remove global variables. There still will be a single /dev/dxg device node. Correct?

Quoted 117 lines · Iouri Tarassov

Quoted 116 lines · Greg KH

Quoted 104 lines · Iouri Tarassov

Quoted 92 lines · Wei Liu

Quoted 14 lines · Greg KH

Quoted 6 lines · Wei Liu

Quoted 6 lines

Quoted 1 line

+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

Quoted 7 lines

This may not be as big an issue as you thought. The device discovery is still done via the normal VMBus probing routine. For all intents and purposes the dxgglobal structure can be broken down into per device fields and a global structure which contains the protocol versioning information -- my understanding is there will always be a global structure to hold information related to the backend, regardless of how many devices there are.

Quoted 7 lines

Then that is wrong and needs to be fixed. Drivers should almost never have any global data, that is not how Linux drivers work. What happens when you get a second device in your system for this? Major rework would have to happen and the code will break. Handle that all now as it takes less work to make this per-device than it does to have a global variable.

It is perhaps easier to draw parallel from an existing driver. I feel like we're talking past each other.

Let's look at drivers/iommu/intel/iommu.c. There are a bunch of lists like static LIST_HEAD(dmar_rmrr_units). During the probing phase, new units will be added to the list. I this the current code is following this model. dxgglobal fulfills the role of a list.

Setting aside the question of whether it makes sense to keep a copy of the per-VM state in each device instance, I can see the code be changed to:

    struct mutex device_mutex; /* split out from dxgglobal */
    static LIST_HEAD(dxglist);
    
    /* Rename struct dxgglobal to struct dxgstate */
    struct dxgstate {
       struct list_head dxglist; /* link for dxglist */
       /* ... original fields sans device_mutex */
    }

    /*
     * Provide a bunch of helpers manipulate the list. Called in probe /
     * remove etc.
     */
    struct dxgstate *find_dxgstate(...);
    void remove_dxgstate(...);
    int add_dxgstate(...);

This model is well understood and used in tree. It is just that it doesn't provide much value in doing this now since the list will only contain one element. I hope that you're not saying we cannot even use a per-module pointer to quickly get the data structure we want to use, right?

Are you suggesting Iouri use dev_set_drvdata to stash the dxgstate into the device object? I think that can be done too.

The code can be changed as:

    /* Rename struct dxgglobal to dxgstate and remove unneeded fields */
    struct dxgstate { ... };

    static int dxg_probe_vmbus(...) {

        /* probe successfully */

	struct dxgstate *state = kmalloc(...);
	/* Fill in dxgstate with information from backend */

	/* hdev->dev is the device object from the core driver framework */
	dev_set_drvdata(&hdev->dev, state);
    }

    static int dxg_remove_vmbus(...) {
        /* Normal stuff here ...*/

	struct dxgstate *state = dev_get_drvdata(...);
	dev_set_drvdata(..., NULL);
	kfree(state);
    }

    /* In all other functions */
    void do_things(...) {
        struct dxgstate *state = dev_get_drvdata(...);

	/* Use state in place of where dxgglobal was needed */

    }

Quoted 7 lines

Iouri, notice this doesn't change anything regarding how userspace is designed. This is about how kernel organises its data.

I hope what I wrote above can bring our understanding closer.

Thanks,
Wei.

I can certainly remove dxgglobal and keep the  pointer to the global state in the device object.

Quoted 8 lines

This will require passing of the global pointer to all functions, which need to access it.

Maybe my understanding of the Greg's suggestion was not correct. I thought the suggestion was

to have multiple /dev/dxgN devices (one per virtual compute device).

You have one device per HV device, as the bus already provides you. That's all you really need, right? Who would be opening the same device node multiple times?

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

This would change how the user mode
clients enumerate and communicate with compute devices.

What does userspace have to do here? It should just open the device node that is present when needed. How will there be multiple userspace clients for a single HV device?

Dxgkrnl creates a single user mode visible device node /dev/dxg.

When you do that, you have a device to put all of your data on. Use that.

Quoted 5 lines

It has
nothing to do with a specific hardware compute device on the host. Its purpose is to provide services (IOCTLs) to enumerate and manage virtual compute devices, which represent hardware devices on the host. The VMBus devices are not used directly by user mode clients in the current design.

That's horrid, why not just export the virtual compute devices properly through individual device nodes instead?

In essence, you are creating a new syscall here to manage and handle devices for just your driver with the use of this ioctl. That's generally a bad idea.

Quoted 3 lines

Virtual compute devices are shared between processes. There could be a Cuda application, Gimp and a Direct3D12 application working at the same time.

Why are all of those applications sharing anything? How are they sharing anything? If you need to share something across processes, use the existing inter-process ways of sharing stuff that we have today (12+ different apis and counting). Don't create yet-another-one just for your tiny little driver here. That's rude to the rest of the kernel.

Quoted 4 lines

This is what I mean by saying that there are multiple user mode clients who use the /dev/dxg driver interface. Each of this applications will open the /dev/dxg device node and enumerate/use virtual compute devices.

That seems like an odd model to follow. How many virtual devices do you support? Is there a limit? Why not just enumerate them all to start with? Or if there are too many, why not do it like the raw device and have a single device node that is used to create the virtual devices you wish to use?

Quoted 2 lines

If we change the way how the virtual compute devices are visible to user mode, the Cuda runtime, Direct3D runtime would need to be changed.

We do not care about existing userspace code at this point in time, you are trying to get the kernel api correct here. Once you have done that, then you can go fix up your userspace code to work properly. Anything that came before today is pointless here, right? :)

Quoted 3 lines

I think we agreed that I will keep the global driver state in the device object as Wei suggested and remove global variables. There still will be a single /dev/dxg device node. Correct?

I do not know, as I can't figure out your goals here at all, sorry.

Please go work with some experienced Linux developers in your company. They should be the ones answering these questions for you, not me :)

thanks,

greg k-h

Quoted 169 lines · Greg KH

Quoted 117 lines · Iouri Tarassov

Quoted 116 lines · Greg KH

Quoted 104 lines · Iouri Tarassov

Quoted 92 lines · Wei Liu

Quoted 14 lines · Greg KH

Quoted 6 lines · Wei Liu

Quoted 6 lines

Quoted 1 line

+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

Quoted 7 lines

This may not be as big an issue as you thought. The device discovery is still done via the normal VMBus probing routine. For all intents and purposes the dxgglobal structure can be broken down into per device fields and a global structure which contains the protocol versioning information -- my understanding is there will always be a global structure to hold information related to the backend, regardless of how many devices there are.

Quoted 7 lines

Then that is wrong and needs to be fixed. Drivers should almost never have any global data, that is not how Linux drivers work. What happens when you get a second device in your system for this? Major rework would have to happen and the code will break. Handle that all now as it takes less work to make this per-device than it does to have a global variable.

It is perhaps easier to draw parallel from an existing driver. I feel like we're talking past each other.

Let's look at drivers/iommu/intel/iommu.c. There are a bunch of lists like static LIST_HEAD(dmar_rmrr_units). During the probing phase, new units will be added to the list. I this the current code is following this model. dxgglobal fulfills the role of a list.

Setting aside the question of whether it makes sense to keep a copy of the per-VM state in each device instance, I can see the code be changed to:

    struct mutex device_mutex; /* split out from dxgglobal */
    static LIST_HEAD(dxglist);
    
    /* Rename struct dxgglobal to struct dxgstate */
    struct dxgstate {
       struct list_head dxglist; /* link for dxglist */
       /* ... original fields sans device_mutex */
    }

    /*
     * Provide a bunch of helpers manipulate the list. Called in probe /
     * remove etc.
     */
    struct dxgstate *find_dxgstate(...);
    void remove_dxgstate(...);
    int add_dxgstate(...);

This model is well understood and used in tree. It is just that it doesn't provide much value in doing this now since the list will only contain one element. I hope that you're not saying we cannot even use a per-module pointer to quickly get the data structure we want to use, right?

Are you suggesting Iouri use dev_set_drvdata to stash the dxgstate into the device object? I think that can be done too.

The code can be changed as:

    /* Rename struct dxgglobal to dxgstate and remove unneeded fields */
    struct dxgstate { ... };

    static int dxg_probe_vmbus(...) {

        /* probe successfully */

	struct dxgstate *state = kmalloc(...);
	/* Fill in dxgstate with information from backend */

	/* hdev->dev is the device object from the core driver framework */
	dev_set_drvdata(&hdev->dev, state);
    }

    static int dxg_remove_vmbus(...) {
        /* Normal stuff here ...*/

	struct dxgstate *state = dev_get_drvdata(...);
	dev_set_drvdata(..., NULL);
	kfree(state);
    }

    /* In all other functions */
    void do_things(...) {
        struct dxgstate *state = dev_get_drvdata(...);

	/* Use state in place of where dxgglobal was needed */

    }

Quoted 7 lines

Iouri, notice this doesn't change anything regarding how userspace is designed. This is about how kernel organises its data.

I hope what I wrote above can bring our understanding closer.

Thanks,
Wei.

I can certainly remove dxgglobal and keep the  pointer to the global state in the device object.

Quoted 8 lines

This will require passing of the global pointer to all functions, which need to access it.

Maybe my understanding of the Greg's suggestion was not correct. I thought the suggestion was

to have multiple /dev/dxgN devices (one per virtual compute device).

You have one device per HV device, as the bus already provides you. That's all you really need, right? Who would be opening the same device node multiple times?

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

This would change how the user mode
clients enumerate and communicate with compute devices.

What does userspace have to do here? It should just open the device node that is present when needed. How will there be multiple userspace clients for a single HV device?

Dxgkrnl creates a single user mode visible device node /dev/dxg.

When you do that, you have a device to put all of your data on. Use that.

Quoted 5 lines

It has
nothing to do with a specific hardware compute device on the host. Its purpose is to provide services (IOCTLs) to enumerate and manage virtual compute devices, which represent hardware devices on the host. The VMBus devices are not used directly by user mode clients in the current design.

That's horrid, why not just export the virtual compute devices properly through individual device nodes instead?

In essence, you are creating a new syscall here to manage and handle devices for just your driver with the use of this ioctl. That's generally a bad idea.

Quoted 3 lines

Virtual compute devices are shared between processes. There could be a Cuda application, Gimp and a Direct3D12 application working at the same time.

Why are all of those applications sharing anything? How are they sharing anything? If you need to share something across processes, use the existing inter-process ways of sharing stuff that we have today (12+ different apis and counting). Don't create yet-another-one just for your tiny little driver here. That's rude to the rest of the kernel.

Quoted 4 lines

This is what I mean by saying that there are multiple user mode clients who use the /dev/dxg driver interface. Each of this applications will open the /dev/dxg device node and enumerate/use virtual compute devices.

That seems like an odd model to follow. How many virtual devices do you support? Is there a limit? Why not just enumerate them all to start with? Or if there are too many, why not do it like the raw device and have a single device node that is used to create the virtual devices you wish to use?

Quoted 2 lines

If we change the way how the virtual compute devices are visible to user mode, the Cuda runtime, Direct3D runtime would need to be changed.

We do not care about existing userspace code at this point in time, you are trying to get the kernel api correct here. Once you have done that, then you can go fix up your userspace code to work properly. Anything that came before today is pointless here, right? :)

Quoted 3 lines

I think we agreed that I will keep the global driver state in the device object as Wei suggested and remove global variables. There still will be a single /dev/dxg device node. Correct?

Quoted 8 lines

I do not know, as I can't figure out your goals here at all, sorry.

Please go work with some experienced Linux developers in your company. They should be the ones answering these questions for you, not me :)

thanks,

greg k-h

Thanks Greg and folks for all the feedback. The userspace API provided by this driver is specifically designed to match the Windows compute device abstraction in order to light up a large eco-system of graphics and compute APIs in our various virtualized Linux environment. This is a pretty critical design point as this is the only way we can scale to such a vast eco-system of APIs and their related tools. This enables APIs and framework which are available natively on Linux, such as CUDA, OpenGL, OpenCL, OpenVINO, OneAPI/L0, TensorFlow, Pytorch, soon Vulkan and more in the future and allows them to share the host GPU or compute device when run under our hypervisor. Most of these APIs sit directly on top of our userspace API and changing that abstraction would have major implications. Our D3D/DML userspace components are only implementation details for a subset of those APIs.

We have millions of developers experiencing and enjoying various Linux distributions through our Windows Subsystem for Linux (WSL) and this gives them a way to get hardware acceleration in a large number of important scenarios. WSL makes it trivial for developers to try out Linux totally risk free on over a billion devices, without having to commit to a native install upfront or understand and manually manage VMs. We think this is a pretty valuable proposition to both the Windows and the Linux community and one which has definitely been quite popular.

That being said, we recognize that our virtualization approach makes some folks in the community uncomfortable and the resulting userspace API exposed by our driver will look a bit different than typical Linux driver. Taking into consideration feedback and reception, we decided to pause our upstream effort for this driver. We will keep our driver open source and available in our downstream kernel tree for folks who wants to leverage it in their private kernel.

Quoted 176 lines · Steve Pronovost

Quoted 167 lines · Greg KH

Quoted 116 lines · Iouri Tarassov

Quoted 115 lines · Greg KH

Quoted 103 lines · Iouri Tarassov

Quoted 92 lines · Wei Liu

Quoted 14 lines · Greg KH

Quoted 6 lines · Wei Liu

Quoted 6 lines

Quoted 1 line

+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

Quoted 7 lines

This may not be as big an issue as you thought. The device discovery is still done via the normal VMBus probing routine. For all intents and purposes the dxgglobal structure can be broken down into per device fields and a global structure which contains the protocol versioning information -- my understanding is there will always be a global structure to hold information related to the backend, regardless of how many devices there are.

Quoted 7 lines

Then that is wrong and needs to be fixed. Drivers should almost never have any global data, that is not how Linux drivers work. What happens when you get a second device in your system for this? Major rework would have to happen and the code will break. Handle that all now as it takes less work to make this per-device than it does to have a global variable.

It is perhaps easier to draw parallel from an existing driver. I feel like we're talking past each other.

Let's look at drivers/iommu/intel/iommu.c. There are a bunch of lists like static LIST_HEAD(dmar_rmrr_units). During the probing phase, new units will be added to the list. I this the current code is following this model. dxgglobal fulfills the role of a list.

Setting aside the question of whether it makes sense to keep a copy of the per-VM state in each device instance, I can see the code be changed to:

    struct mutex device_mutex; /* split out from dxgglobal */
    static LIST_HEAD(dxglist);
    
    /* Rename struct dxgglobal to struct dxgstate */
    struct dxgstate {
       struct list_head dxglist; /* link for dxglist */
       /* ... original fields sans device_mutex */
    }

    /*
     * Provide a bunch of helpers manipulate the list. Called in probe /
     * remove etc.
     */
    struct dxgstate *find_dxgstate(...);
    void remove_dxgstate(...);
    int add_dxgstate(...);

This model is well understood and used in tree. It is just that it doesn't provide much value in doing this now since the list will only contain one element. I hope that you're not saying we cannot even use a per-module pointer to quickly get the data structure we want to use, right?

Are you suggesting Iouri use dev_set_drvdata to stash the dxgstate into the device object? I think that can be done too.

The code can be changed as:

    /* Rename struct dxgglobal to dxgstate and remove unneeded fields */
    struct dxgstate { ... };

    static int dxg_probe_vmbus(...) {

        /* probe successfully */

	struct dxgstate *state = kmalloc(...);
	/* Fill in dxgstate with information from backend */

	/* hdev->dev is the device object from the core driver framework */
	dev_set_drvdata(&hdev->dev, state);
    }

    static int dxg_remove_vmbus(...) {
        /* Normal stuff here ...*/

	struct dxgstate *state = dev_get_drvdata(...);
	dev_set_drvdata(..., NULL);
	kfree(state);
    }

    /* In all other functions */
    void do_things(...) {
        struct dxgstate *state = dev_get_drvdata(...);

	/* Use state in place of where dxgglobal was needed */

    }

Quoted 7 lines

Iouri, notice this doesn't change anything regarding how userspace is designed. This is about how kernel organises its data.

I hope what I wrote above can bring our understanding closer.

Thanks,
Wei.

I can certainly remove dxgglobal and keep the  pointer to the global state in the device object.

Quoted 8 lines

This will require passing of the global pointer to all functions, which need to access it.

Maybe my understanding of the Greg's suggestion was not correct. I thought the suggestion was

to have multiple /dev/dxgN devices (one per virtual compute device).

You have one device per HV device, as the bus already provides you. That's all you really need, right? Who would be opening the same device node multiple times?

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

This would change how the user mode
clients enumerate and communicate with compute devices.

What does userspace have to do here? It should just open the device node that is present when needed. How will there be multiple userspace clients for a single HV device?

Dxgkrnl creates a single user mode visible device node /dev/dxg.

When you do that, you have a device to put all of your data on. Use that.

Quoted 5 lines

It has
nothing to do with a specific hardware compute device on the host. Its purpose is to provide services (IOCTLs) to enumerate and manage virtual compute devices, which represent hardware devices on the host. The VMBus devices are not used directly by user mode clients in the current design.

That's horrid, why not just export the virtual compute devices properly through individual device nodes instead?

In essence, you are creating a new syscall here to manage and handle devices for just your driver with the use of this ioctl. That's generally a bad idea.

Quoted 3 lines

Virtual compute devices are shared between processes. There could be a Cuda application, Gimp and a Direct3D12 application working at the same time.

Why are all of those applications sharing anything? How are they sharing anything? If you need to share something across processes, use the existing inter-process ways of sharing stuff that we have today (12+ different apis and counting). Don't create yet-another-one just for your tiny little driver here. That's rude to the rest of the kernel.

Quoted 4 lines

This is what I mean by saying that there are multiple user mode clients who use the /dev/dxg driver interface. Each of this applications will open the /dev/dxg device node and enumerate/use virtual compute devices.

That seems like an odd model to follow. How many virtual devices do you support? Is there a limit? Why not just enumerate them all to start with? Or if there are too many, why not do it like the raw device and have a single device node that is used to create the virtual devices you wish to use?

Quoted 2 lines

If we change the way how the virtual compute devices are visible to user mode, the Cuda runtime, Direct3D runtime would need to be changed.

We do not care about existing userspace code at this point in time, you are trying to get the kernel api correct here. Once you have done that, then you can go fix up your userspace code to work properly. Anything that came before today is pointless here, right? :)

Quoted 3 lines

I think we agreed that I will keep the global driver state in the device object as Wei suggested and remove global variables. There still will be a single /dev/dxg device node. Correct?

Quoted 8 lines

I do not know, as I can't figure out your goals here at all, sorry.

Please go work with some experienced Linux developers in your company. They should be the ones answering these questions for you, not me :)

thanks,

greg k-h

Quoted 13 lines

Thanks Greg and folks for all the feedback. The userspace API provided by this driver is specifically designed to match the Windows compute device abstraction in order to light up a large eco-system of graphics and compute APIs in our various virtualized Linux environment. This is a pretty critical design point as this is the only way we can scale to such a vast eco-system of APIs and their related tools. This enables APIs and framework which are available natively on Linux, such as CUDA, OpenGL, OpenCL, OpenVINO, OneAPI/L0, TensorFlow, Pytorch, soon Vulkan and more in the future and allows them to share the host GPU or compute device when run under our hypervisor. Most of these APIs sit directly on top of our userspace API and changing that abstraction would have major implications. Our D3D/DML userspace components are only implementation details for a subset of those APIs.

This feels odd to me as I was only discussing the in-kernel apis, and how this driver does not follow them correctly at all (global device state, odd device creation, etc.) That should have nothing to do with the user/kernel api at all. And surely CUDA and the like never have defined a specific user/kernal api that must be followed, right?

Yes, the existing user/kernel api in this driver is a bit odd, but really, we haven't even started to review that. For you all to just cut-and-run at the first sign of a detailed in-kernel code review is strange, why are you giving up now?

Quoted 9 lines

We have millions of developers experiencing and enjoying various Linux distributions through our Windows Subsystem for Linux (WSL) and this gives them a way to get hardware acceleration in a large number of important scenarios. WSL makes it trivial for developers to try out Linux totally risk free on over a billion devices, without having to commit to a native install upfront or understand and manually manage VMs. We think this is a pretty valuable proposition to both the Windows and the Linux community and one which has definitely been quite popular.

This has nothing to do with how this tiny driver uses the in-kernel apis for creating and managing devices. You are discussing userspace applications here, which is a nice marketing discussion about how many developers you have, but means nothing when it comes to how the kernel code works. So why mention it at all?

Quoted 7 lines

That being said, we recognize that our virtualization approach makes some folks in the community uncomfortable and the resulting userspace API exposed by our driver will look a bit different than typical Linux driver. Taking into consideration feedback and reception, we decided to pause our upstream effort for this driver. We will keep our driver open source and available in our downstream kernel tree for folks who wants to leverage it in their private kernel.

The problems I am pointing out here are how your kernel driver is interacting with the kernel driver model. It has almost nothing to do with the user/kernel api at all.

And even if it did concern the user/kernel api, you already have a userspace library that has to talk to this driver, all you need to do is modify that to handle the expected changes that will happen when you actually get experienced kernel developers to review your code and apis.

I don't understand what you all were expecting here. Did you not want to fit into the kernel tree and take advantage of using the proper functions and interfaces that have been created already? That should enable your users to be able to rely on a more stable and secure and widespread driver for everyone to use.

Instead you are saying that you don't appreciate the time and energy we spent on trying to help you improve your code for your users, and wish to just ignore our help?

By doing this, you are telling all community members that they should not waste their time in reviewing your company's code submissions as they are not appreciated or wanted unless they are just a rubber-stamp approval. That's going to ensure that no one ever reviews your contributions again in the future, which is an odd request, don't you think?

Oh well, it's easy to add a filter to just ignore any future code submissions from your email domain. I'll recommend that all other kernel developers and maintainers do the same as we all have changes to review from companies and developers that actually want our feedback.

best of luck!

greg k-h

Quoted 99 lines · Wei Liu

Quoted 17 lines · Greg KH

Quoted 8 lines · Wei Liu

Quoted 7 lines

Quoted 1 line

+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

Quoted 7 lines

This may not be as big an issue as you thought. The device discovery is still done via the normal VMBus probing routine. For all intents and purposes the dxgglobal structure can be broken down into per device fields and a global structure which contains the protocol versioning information -- my understanding is there will always be a global structure to hold information related to the backend, regardless of how many devices there are.

Quoted 7 lines

Then that is wrong and needs to be fixed. Drivers should almost never have any global data, that is not how Linux drivers work. What happens when you get a second device in your system for this? Major rework would have to happen and the code will break. Handle that all now as it takes less work to make this per-device than it does to have a global variable.

It is perhaps easier to draw parallel from an existing driver. I feel like we're talking past each other.

Let's look at drivers/iommu/intel/iommu.c. There are a bunch of lists like static LIST_HEAD(dmar_rmrr_units). During the probing phase, new units will be added to the list. I this the current code is following this model. dxgglobal fulfills the role of a list.

Setting aside the question of whether it makes sense to keep a copy of the per-VM state in each device instance, I can see the code be changed to:

    struct mutex device_mutex; /* split out from dxgglobal */
    static LIST_HEAD(dxglist);
    
    /* Rename struct dxgglobal to struct dxgstate */
    struct dxgstate {
       struct list_head dxglist; /* link for dxglist */
       /* ... original fields sans device_mutex */
    }

    /*
     * Provide a bunch of helpers manipulate the list. Called in probe /
     * remove etc.
     */
    struct dxgstate *find_dxgstate(...);
    void remove_dxgstate(...);
    int add_dxgstate(...);

This model is well understood and used in tree. It is just that it doesn't provide much value in doing this now since the list will only contain one element. I hope that you're not saying we cannot even use a per-module pointer to quickly get the data structure we want to use, right?

Are you suggesting Iouri use dev_set_drvdata to stash the dxgstate into the device object? I think that can be done too.

The code can be changed as:

    /* Rename struct dxgglobal to dxgstate and remove unneeded fields */
    struct dxgstate { ... };

    static int dxg_probe_vmbus(...) {

        /* probe successfully */

	struct dxgstate *state = kmalloc(...);
	/* Fill in dxgstate with information from backend */

	/* hdev->dev is the device object from the core driver framework */
	dev_set_drvdata(&hdev->dev, state);
    }

    static int dxg_remove_vmbus(...) {
        /* Normal stuff here ...*/

	struct dxgstate *state = dev_get_drvdata(...);
	dev_set_drvdata(..., NULL);
	kfree(state);
    }

    /* In all other functions */
    void do_things(...) {
        struct dxgstate *state = dev_get_drvdata(...);

	/* Use state in place of where dxgglobal was needed */

    }

Iouri, notice this doesn't change anything regarding how userspace is designed. This is about how kernel organises its data.

Quoted 4 lines

I hope what I wrote above can bring our understanding closer.

Thanks,
Wei.

I can certainly remove dxgglobal and keep the  pointer to the global state in the device object.

This will require passing of the global pointer to all functions, which need to access it.

Maybe my understanding of the Greg's suggestion was not correct. I thought the suggestion was

to have multiple /dev/dxgN devices (one per virtual compute device). This would change how the user mode

Quoted 8 lines · Wei Liu

Quoted 7 lines

Quoted 1 line

+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

Quoted 7 lines

This may not be as big an issue as you thought. The device discovery is still done via the normal VMBus probing routine. For all intents and purposes the dxgglobal structure can be broken down into per device fields and a global structure which contains the protocol versioning information -- my understanding is there will always be a global structure to hold information related to the backend, regardless of how many devices there are.

Then that is wrong and needs to be fixed. Drivers should almost never have any global data, that is not how Linux drivers work. What happens when you get a second device in your system for this? Major rework would have to happen and the code will break. Handle that all now as it takes less work to make this per-device than it does to have a global variable.

Quoted 10 lines

I definitely think splitting is doable, but I also understand why Iouri does not want to do it _now_ given there is no such a model for multiple devices yet, so anything we put into the per-device structure could be incomplete and it requires further changing when such a model arrives later.

Iouri, please correct me if I have the wrong mental model here.

All in all, I hope this is not going to be a deal breaker for the acceptance of this driver.

For my reviews, yes it will be.

Again, it should be easier to keep things in a per-device state than not as the proper lifetime rules and the like are automatically handled for you. If you have global data, you have to manage that all on your own and it is _MUCH_ harder to review that you got it correct.

Quoted 37 lines · Greg KH

Quoted 8 lines · Wei Liu

Quoted 7 lines

Quoted 1 line

+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

Quoted 7 lines

This may not be as big an issue as you thought. The device discovery is still done via the normal VMBus probing routine. For all intents and purposes the dxgglobal structure can be broken down into per device fields and a global structure which contains the protocol versioning information -- my understanding is there will always be a global structure to hold information related to the backend, regardless of how many devices there are.

Then that is wrong and needs to be fixed. Drivers should almost never have any global data, that is not how Linux drivers work. What happens when you get a second device in your system for this? Major rework would have to happen and the code will break. Handle that all now as it takes less work to make this per-device than it does to have a global variable.

Quoted 10 lines

I definitely think splitting is doable, but I also understand why Iouri does not want to do it _now_ given there is no such a model for multiple devices yet, so anything we put into the per-device structure could be incomplete and it requires further changing when such a model arrives later.

Iouri, please correct me if I have the wrong mental model here.

All in all, I hope this is not going to be a deal breaker for the acceptance of this driver.

For my reviews, yes it will be.

Quoted 4 lines

Again, it should be easier to keep things in a per-device state than not as the proper lifetime rules and the like are automatically handled for you. If you have global data, you have to manage that all on your own and it is _MUCH_ harder to review that you got it correct.

Hi Greg,

I do not really see how the driver be written without the global data. Let's review the design.

Dxgkrnl acts as the aggregator of all virtual compute devices, projected by the host. It needs to do operations, which do not belong to a particular compute device. For example, cross device synchronization and resource sharing.

A PCI device device is created for each virtual compute device. Therefore, there should be a global list of objects and a mutex to synchronize access to the list.

A VMBus channel is offered by the host for each compute device. The list of the VMBus channels should be global.

A global VMBus channel is offered by the host. The channel does not belong to any particular compute device, so it must be global.

IO space is shared by all compute devices, so its parameters should be global.

Dxgkrnl needs to maintain a list of processes, which opened compute device objects. Dxgkrnl maintains private state for each process and when a process opens the /dev/dxg device, Dxgkrnl needs to find if the process state is already created by walking the global process list.

Now, where to keep this global state? It could be kept in the /dev/dxg private device structure. But this structure is not available when, for example, dxg_pci_probe_device() or dxg_probe_vmbus() is called.

Can there be multiple /dev/dxg devices? No. Because the /dev/dxg device represents the driver itself, not a particular compute device.

I am not sure what design model you have in mind when saying there should be no global data. Could you please explain keeping in mind the above requirements?

Quoted 44 lines · Iouri Tarassov

Quoted 41 lines · Greg KH

Quoted 8 lines · Wei Liu

Quoted 7 lines

Quoted 1 line

+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

Quoted 7 lines

This may not be as big an issue as you thought. The device discovery is still done via the normal VMBus probing routine. For all intents and purposes the dxgglobal structure can be broken down into per device fields and a global structure which contains the protocol versioning information -- my understanding is there will always be a global structure to hold information related to the backend, regardless of how many devices there are.

Then that is wrong and needs to be fixed. Drivers should almost never have any global data, that is not how Linux drivers work. What happens when you get a second device in your system for this? Major rework would have to happen and the code will break. Handle that all now as it takes less work to make this per-device than it does to have a global variable.

Quoted 10 lines

I definitely think splitting is doable, but I also understand why Iouri does not want to do it _now_ given there is no such a model for multiple devices yet, so anything we put into the per-device structure could be incomplete and it requires further changing when such a model arrives later.

Iouri, please correct me if I have the wrong mental model here.

All in all, I hope this is not going to be a deal breaker for the acceptance of this driver.

For my reviews, yes it will be.

Again, it should be easier to keep things in a per-device state than not as the proper lifetime rules and the like are automatically handled for you. If you have global data, you have to manage that all on your own and it is _MUCH_ harder to review that you got it correct.

Quoted 3 lines

Hi Greg,

I do not really see how the driver be written without the global data. Let's review the design.

I see it the other way around. It's easier to make it without a static structure, it is more work to keep it as you have done so here. Do it correctly to start with and you will not have any of these issues going forward.

Quoted 1 line

Dxgkrnl acts as the aggregator of all virtual compute devices, projected by the host. It needs to do operations, which do not belong to a particular compute device. For example, cross device synchronization and resource sharing.

Then hang your data off of your device node structure that you created. Why ignore that?

Quoted 3 lines

A PCI device device is created for each virtual compute device. Therefore, there should be a global list of objects and a mutex to synchronize access to the list.

Woah, what? You create a fake PCI device for each virtual device? If so, great, then you are now a PCI bus and create the PCI devices properly so that the PCI core can handle and manage them and then assign them to your driver. You should NEVER have a global list of these devices, as that is what the driver model should be managing. Not you!

Quoted 2 lines

A VMBus channel is offered by the host for each compute device. The list of the VMBus channels should be global.

The vmbus channels are already handled by the driver core. Use those devices that are given to you. You don't need to manage them at all.

Quoted 2 lines

A global VMBus channel is offered by the host. The channel does not belong to any particular compute device, so it must be global.

That channel is attached to your driver, use the device given to your driver by the bus. It's not "global" in any sense of the word.

And what's up with your lack of line wrapping?

Quoted 2 lines

IO space is shared by all compute devices, so its parameters should be global.

Huh? If that's the case then you have bigger problems. Use the aux bus for devices that share io space. That is what it was created for, do not ignore the functionality that Linux already provides you by trying to go around it and writing your own code. Use the frameworks we have already debugged and support. This is why your Linux driver should be at least 1/3 smaller than drivers for other operating systems.

Quoted 5 lines

Dxgkrnl needs to maintain a list of processes, which opened compute device objects. Dxgkrnl maintains private state for each process and when a process opens the /dev/dxg device, Dxgkrnl needs to find if the process state is already created by walking the global process list.

That "list" is handled by the device node structure that was opened. It's not "global" at all. Again, just like any other device node in Linux, this isn't a new thing or anything special at all.

Quoted 4 lines

Now, where to keep this global state? It could be kept in the /dev/dxg private device structure. But this structure is not available when, for example, dxg_pci_probe_device() or dxg_probe_vmbus() is called.

Then your design is wrong. It's as simple as that. Fix it.

Quoted 3 lines

Can there be multiple /dev/dxg devices? No. Because the /dev/dxg device represents the driver itself, not a particular compute device.

Then fix this. Make your compute devices store the needed information when they are created. Again, we have loads of examples in the kernel, this is nothing new.

Quoted 11 lines

Quoted 3 lines

A PCI device device is created for each virtual compute device. Therefore, there should be a global list of objects and a mutex to synchronize access to the list.

Woah, what? You create a fake PCI device for each virtual device? If so, great, then you are now a PCI bus and create the PCI devices properly so that the PCI core can handle and manage them and then assign them to your driver. You should NEVER have a global list of these devices, as that is what the driver model should be managing. Not you!

No, there is no fake PCI device. The device object is still coming from the PCI core driver. There is code to match against PCI vendor ID and device ID, and follow the usual way of managing PCI device.

Iouri understands device specific state should be encapsulated in the private data field in their respective device. And I believe the code can perhaps be rewritten to better conform to Linux kernel's model.

Quoted 11 lines

Quoted 2 lines

IO space is shared by all compute devices, so its parameters should be global.

Huh? If that's the case then you have bigger problems. Use the aux bus for devices that share io space. That is what it was created for, do not ignore the functionality that Linux already provides you by trying to go around it and writing your own code. Use the frameworks we have already debugged and support. This is why your Linux driver should be at least 1/3 smaller than drivers for other operating systems.

To be fair, auxiliary bus was only added in 5.11, while this series was written long before that. Unfortunately one only has so much time to follow Linux kernel development closely. I admit this is the first time I hear about it. :-)

Quoted 4 lines

Then fix this. Make your compute devices store the needed information when they are created. Again, we have loads of examples in the kernel, this is nothing new.

At this point, I think Iouri and I have settled on more encapsulation is needed. Yet there is something I don't know how to square yet. That is, devices (either from vmbus or pci) don't form a clear hierarchy.

If
there isn't a linked list or some sort to organize them it would be difficult to cross-reference.

drivers/hv/dxgkrnl/dxgmodule.c line 33

Quoted 6 lines · Iouri Tarassov

- Create skeleton and add basic functionality for the hyper-v compute device driver (dxgkrnl).

drivers/hv/dxgkrnl/dxgmodule.c new file +3 -0

+
+#undef pr_fmt
+#define pr_fmt(fmt)	"dxgk: " fmt

Use the dev_*() print functions, you are a driver, and you always have a pointer to a struct device. There's no need to ever call pr_*().

thanks,

greg k-h

Quoted 11 lines · Greg KH

drivers/hv/dxgkrnl/dxgmodule.c line 33

Quoted 6 lines · Iouri Tarassov

- Create skeleton and add basic functionality for the hyper-v compute device driver (dxgkrnl).

drivers/hv/dxgkrnl/dxgmodule.c new file +3 -0

+
+#undef pr_fmt
+#define pr_fmt(fmt)	"dxgk: " fmt

Use the dev_*() print functions, you are a driver, and you always have a pointer to a struct device. There's no need to ever call pr_*().

There is no struct device during module initialization until the /dev/dxg device is created.

Is it ok to use pr_* functions in this case?

Should dev_*(NULL,...) be used?

I see other drivers use the pr_* functions in this case (mips.c as an example).

drivers/hv/dxgkrnl/dxgkrnl.h line 100

Quoted 6 lines

drivers/hv/dxgkrnl/dxgkrnl.h new file +5 -0

+long dxgk_unlocked_ioctl(struct file *f, unsigned int p1, unsigned long p2);
+
+static inline void guid_to_luid(guid_t *guid, struct winluid *luid)
+{
+	*luid = *(struct winluid *)&guid->b[0];

Why is the cast needed? Shouldn't you use real types in your structures?

Quoted 4 lines

+/*
+ * The interface version is used to ensure that the host and the guest use the
+ * same VM bus protocol. It needs to be incremented every time the VM bus
+ * interface changes. DXGK_VMBUS_LAST_COMPATIBLE_INTERFACE_VERSION is

drivers/hv/dxgkrnl/dxgkrnl.h line 117

Quoted 6 lines

+ * incremented each time the earlier versions of the interface are no longer
+ * compatible with the current version.
+ */

drivers/hv/dxgkrnl/dxgkrnl.h new file +3 -0

+#define DXGK_VMBUS_INTERFACE_VERSION_OLD		27
+#define DXGK_VMBUS_INTERFACE_VERSION			40
+#define DXGK_VMBUS_LAST_COMPATIBLE_INTERFACE_VERSION	16

Where do these numbers come from, the hypervisor specification?

drivers/hv/dxgkrnl/dxgmodule.c line 26

Quoted 6 lines

drivers/hv/dxgkrnl/dxgmodule.c new file +6 -0

+/*
+ * Pointer to the global device data. By design
+ * there is a single vGPU device on the VM bus and a single /dev/dxg device
+ * is created.
+ */
+struct dxgglobal *dxgglobal;

No, make this per-device, NEVER have a single device for your driver. The Linux driver model makes it harder to do it this way than to do it correctly. Do it correctly please and have no global structures like this.

Quoted 1 line

+#define DXGKRNL_VERSION 0x2216

What does this mean?

Quoted 10 lines

drivers/hv/dxgkrnl/dxgmodule.c new file +10 -0

+
+/*
+ * Interface with the PCI driver
+ */
+
+/*
+ * Part of the PCI config space of the vGPU device is used for vGPU
+ * configuration data. Reading/writing of the PCI config space is forwarded
+ * to the host.
+ */

Quoted 1 line

+/* DXGK_VMBUS_INTERFACE_VERSION (u32) */

What is this?

drivers/hv/dxgkrnl/dxgmodule.c line 115

Quoted 2 lines

drivers/hv/dxgkrnl/dxgmodule.c new file +2 -0

+#define DXGK_VMBUS_VERSION_OFFSET	(DXGK_VMBUS_CHANNEL_ID_OFFSET + \
+					sizeof(guid_t))

offsetof() is your friend, please use it.

Quoted 1 line

+/* Luid of the virtual GPU on the host (struct winluid) */

What is a "luid"?

drivers/hv/dxgkrnl/dxgmodule.c line 508

Quoted 8 lines

drivers/hv/dxgkrnl/dxgmodule.c new file +8 -0

+	ret = vmbus_driver_register(&dxg_drv);
+	if (ret) {
+		pr_err("vmbus_driver_register failed: %d", ret);
+		return ret;
+	}
+	dxgglobal->vmbus_registered = true;
+
+	pr_info("%s  Version: %x", __func__, DXGKRNL_VERSION);

When drivers work, they are quiet.

Also, in-kernel modules do not have a version, they are part of the kernel tree, and that's the "version" they have. You can drop the individual version here, it makes no sense anymore.

Hi Greg,

Thanks a lot for reviewing the patches.

I appreciate the very detailed comments. I my reply I omitted the comments, which I agree with and will address in subsequent patches.

Quoted 9 lines · Greg KH

drivers/hv/dxgkrnl/dxgkrnl.h line 100

Quoted 6 lines

drivers/hv/dxgkrnl/dxgkrnl.h new file +5 -0

+long dxgk_unlocked_ioctl(struct file *f, unsigned int p1, unsigned long p2);
+
+static inline void guid_to_luid(guid_t *guid, struct winluid *luid)
+{
+	*luid = *(struct winluid *)&guid->b[0];

Why is the cast needed? Shouldn't you use real types in your structures?

The VMBus channel ID, which is GUID, is present in the PCI config space of the compute device. The convention is that the lower part of the GUID is LUID (local unique identifier). This function just converts GUID to LUID. I am not sure what is the ask here.

Quoted 4 lines

Quoted 4 lines

+/*
+ * The interface version is used to ensure that the host and the guest use the
+ * same VM bus protocol. It needs to be incremented every time the VM bus
+ * interface changes. DXGK_VMBUS_LAST_COMPATIBLE_INTERFACE_VERSION is

Quoted 8 lines

drivers/hv/dxgkrnl/dxgkrnl.h line 117

Quoted 6 lines

+ * incremented each time the earlier versions of the interface are no longer
+ * compatible with the current version.
+ */

drivers/hv/dxgkrnl/dxgkrnl.h new file +3 -0

+#define DXGK_VMBUS_INTERFACE_VERSION_OLD		27
+#define DXGK_VMBUS_INTERFACE_VERSION			40
+#define DXGK_VMBUS_LAST_COMPATIBLE_INTERFACE_VERSION	16

Where do these numbers come from, the hypervisor specification?

The definitions are for the VMBus interface between the virtual compute device in the guest and the Windows host. The interface version is updated when the VMBus protocol is changed. These are specific to the virtual compute device, not to the hypervisor.

Quoted 3 lines

Quoted 1 line

+#define DXGKRNL_VERSION 0x2216

What does this mean?

This is the driver implementation version. There are many Microsoft shipped Linux kernels, which include the driver. The version allows to quickly determine, which level of functionality or bug fixes is implemented in the current driver.

Quoted 6 lines

Quoted 5 lines

+/*
+ * Part of the PCI config space of the vGPU device is used for vGPU
+ * configuration data. Reading/writing of the PCI config space is forwarded
+ * to the host.
+ */

Quoted 3 lines

Quoted 1 line

+/* DXGK_VMBUS_INTERFACE_VERSION (u32) */

What is this?

This comment explains that the value of DXGK_VMBUS_INTERFACE_VERSION is located at the offset DXGK_VMBUS_CHANNEL_ID_OFFSET in the PCI config space.

Quoted 4 lines

drivers/hv/dxgkrnl/dxgmodule.c line 115

Quoted 2 lines

drivers/hv/dxgkrnl/dxgmodule.c new file +2 -0

+#define DXGK_VMBUS_VERSION_OFFSET	(DXGK_VMBUS_CHANNEL_ID_OFFSET + \
+					sizeof(guid_t))

offsetof() is your friend, please use it.

There is no structure definition for the PCI config space of the device. Therefore, offsets are computed by using data size.

Quoted 3 lines

Quoted 1 line

+/* Luid of the virtual GPU on the host (struct winluid) */

What is a "luid"?

LUID is "Locally Unique Identifier". It is used in Windows and its value is guaranteed to be unique until the computer reboots.

Quoted 11 lines

drivers/hv/dxgkrnl/dxgmodule.c line 508

Quoted 8 lines

drivers/hv/dxgkrnl/dxgmodule.c new file +8 -0

+	ret = vmbus_driver_register(&dxg_drv);
+	if (ret) {
+		pr_err("vmbus_driver_register failed: %d", ret);
+		return ret;
+	}
+	dxgglobal->vmbus_registered = true;
+
+	pr_info("%s  Version: %x", __func__, DXGKRNL_VERSION);

When drivers work, they are quiet.

Quoted 3 lines

Also, in-kernel modules do not have a version, they are part of the kernel tree, and that's the "version" they have. You can drop the individual version here, it makes no sense anymore.

Microsoft develops many Linux kernels when the dxgkrnl driver is included (EFLOW, Android, WSL). The kernels are built at different times and might include a different version of the driver. Printing the version allows to quickly determine what functionality and bug fixes are included in the driver.

I see that other in-tree drivers print some information during init_module (like nfblock.c).

This is done for convenience. So instead of tracking multiple kernel versions, we can track the dxgkrnl driver version. I am ok to remove it if it really hurts something. It is only a single line per driver load.

- Create skeleton and add basic functionality for the hyper-v compute device driver (dxgkrnl).

- Register for PCI and VM bus driver notifications and handle initialization of VM bus channels.

- Connect the dxgkrnl module to the drivers/hv/ Makefile and Kconfig

- Create a MAINTAINERS entry

A VM bus channel is a communication interface between the hyper-v guest and the host. The are two type of VM bus channels, used in the driver:
- the global channel
- per virtual compute device channel

A PCI device is created for each virtual compute device, projected by the host. The device vendor is PCI_VENDOR_ID_MICROSOFT and device id is PCI_DEVICE_ID_VIRTUAL_RENDER. dxg_pci_probe_device handles arrival of such devices. The PCI config space of the virtual compute device has luid of the corresponding virtual compute device VM bus channel. This is how the compute device adapter objects are linked to VM bus channels.

VM bus interface version is exchanged by reading/writing the PCI config space of the virtual compute device.

The IO space is used to handle CPU accessible compute device allocations. Hyper-v allocates IO space for the global VM bus channel.

Signed-off-by Iouri Tarassov <[email protected]>

---

drivers/hv/dxgkrnl/dxgmodule.c new file +32 -0

+	for (i = 0; i < size / sizeof(int); i++) {
+		ret = pci_read_config_dword(dev, off, &((int *)val)[i]);
+		if (ret) {
+			pr_err("Failed to read PCI config: %d", off);
+			return ret;
+		}
+		off += sizeof(int);
+	}
+	return 0;
+}
+
+static int dxg_pci_probe_device(struct pci_dev *dev,
+				const struct pci_device_id *id)
+{
+	int ret;
+	guid_t guid;
+	u32 vmbus_interface_ver = DXGK_VMBUS_INTERFACE_VERSION;
+	struct winluid vgpu_luid = {};
+	struct dxgk_vmbus_guestcaps guest_caps = {.wsl2 = 1};
+
+	mutex_lock(&dxgglobal->device_mutex);
+
+	if (dxgglobal->vmbus_ver == 0)  {
+		/* Report capabilities to the host */
+
+		ret = pci_write_config_dword(dev, DXGK_VMBUS_GUESTCAPS_OFFSET,
+					guest_caps.guest_caps);
+		if (ret)
+			goto cleanup;
+
+		/* Negotiate the VM bus version */
+

drivers/hv/dxgkrnl/dxgmodule.c new file +10 -0

+
+cleanup:
+
+	mutex_unlock(&dxgglobal->device_mutex);
+
+	if (ret)
+		pr_debug("err: %s %d", __func__, ret);
+	return ret;
+}
+

I will skip things that are pointed out by Greg.

Quoted 6 lines · Iouri Tarassov

- Create skeleton and add basic functionality for the hyper-v compute device driver (dxgkrnl).

- Register for PCI and VM bus driver notifications and handle initialization of VM bus channels.

Quoted 9 lines

- Connect the dxgkrnl module to the drivers/hv/ Makefile and Kconfig

- Create a MAINTAINERS entry

A VM bus channel is a communication interface between the hyper-v guest and the host. The are two type of VM bus channels, used in the driver:
- the global channel
- per virtual compute device channel

Same comment regarding the spelling of VMBus and Hyper-V. Please fix other instances in code and comments.

Quoted 16 lines

A PCI device is created for each virtual compute device, projected by the host. The device vendor is PCI_VENDOR_ID_MICROSOFT and device id is PCI_DEVICE_ID_VIRTUAL_RENDER. dxg_pci_probe_device handles arrival of such devices. The PCI config space of the virtual compute device has luid of the corresponding virtual compute device VM bus channel. This is how the compute device adapter objects are linked to VM bus channels.

VM bus interface version is exchanged by reading/writing the PCI config space of the virtual compute device.

The IO space is used to handle CPU accessible compute device allocations. Hyper-v allocates IO space for the global VM bus channel.

Signed-off-by Iouri Tarassov <[email protected]>

---

[...]

drivers/hv/dxgkrnl/dxgkrnl.h line 101

Quoted 4 lines

drivers/hv/dxgkrnl/dxgkrnl.h new file +4 -0

+static inline void guid_to_luid(guid_t *guid, struct winluid *luid)
+{
+	*luid = *(struct winluid *)&guid->b[0];
+}

This should be moved to the header where luid is defined -- presumably this is useful for other things in the future too.

Also, please provide a comment on why this conversion is okay.

drivers/hv/dxgkrnl/dxgvmbus.c new file +4 -0

+
+struct dxgvmbusmsgres {
+/* Points to the allocated buffer */
+	struct dxgvmb_ext_header	*hdr;

(quoted diff) +2 -0

+};
+

drivers/hv/dxgkrnl/dxgvmbus.c new file +13 -0

+		switch (packet->command_type) {
+		case DXGK_VMBCOMMAND_SIGNALGUESTEVENT:
+		case DXGK_VMBCOMMAND_SIGNALGUESTEVENTPASSIVE:
+			break;
+		case DXGK_VMBCOMMAND_SENDWNFNOTIFICATION:
+			break;
+		default:
+			pr_err("unexpected host message %d",
+					packet->command_type);
+		}
+	}
+}
+

drivers/hv/dxgkrnl/dxgvmbus.c new file +7 -0

+
+/* Receive callback for messages from the host */
+void dxgvmbuschannel_receive(void *ctx)
+{
+	struct dxgvmbuschannel *channel = ctx;
+	struct vmpacket_descriptor *desc;
+	u32 packet_length = 0;

drivers/hv/dxgkrnl/dxgvmbus.c new file +6 -0

+			else
+				process_inband_packet(channel, desc);
+		}
+	}
+}
+

drivers/hv/dxgkrnl/dxgvmbus.c new file +12 -0

+int dxgvmb_send_set_iospace_region(u64 start, u64 len,
+	struct vmbus_gpadl *shared_mem_gpadl)
+{
+	int ret;
+	struct dxgkvmb_command_setiospaceregion *command;
+	struct dxgvmbusmsg msg;
+
+	ret = init_message(&msg, NULL, sizeof(*command));
+	if (ret)
+		return ret;
+	command = (void *)msg.msg;
+

drivers/hv/dxgkrnl/dxgvmbus.c new file +13 -0

+	ret = dxgvmb_send_sync_msg_ntstatus(&dxgglobal->channel, msg.hdr,
+					    msg.size);
+	if (ret < 0)
+		pr_err("send_set_iospace_region failed %x", ret);
+
+	dxgglobal_release_channel_lock();
+cleanup:
+	free_message(&msg, NULL);
+	if (ret)
+		pr_debug("err: %s %d", __func__, ret);
+	return ret;
+}
+

drivers/hv/dxgkrnl/misc.h new file +7 -0

+
+
+#define NT_SUCCESS(status)				(status.v >= 0)
+
+#ifndef DEBUG
+
+#define DXGKRNL_ASSERT(exp)

drivers/hv/dxgkrnl/dxgadapter.c new file +9 -0

+int dxgadapter_set_vmbus(struct dxgadapter *adapter, struct hv_device *hdev)
+{
+	int ret;
+
+	guid_to_luid(&hdev->channel->offermsg.offer.if_instance,
+		     &adapter->luid);
+	pr_debug("%s: %x:%x %p %pUb\n",
+		    __func__, adapter->luid.b, adapter->luid.a, hdev->channel,
+		    &hdev->channel->offermsg.offer.if_instance);

drivers/hv/dxgkrnl/dxgadapter.c new file +16 -0

+		pr_err("dxgvmb_send_open_adapter failed: %d\n", ret);
+		goto cleanup;
+	}
+
+	ret = dxgvmb_send_get_internal_adapter_info(adapter);
+	if (ret < 0)
+		pr_err("get_internal_adapter_info failed: %d", ret);
+
+cleanup:
+	if (ret)
+		pr_debug("err: %s %d", __func__, ret);
+	return ret;
+}
+
+void dxgadapter_start(struct dxgadapter *adapter)
+{

+#define LX_IO_MAX 0x45

- Implement opening of the device (/dev/dxg) file object and creation of dxgprocess objects.

static int dxgk_open(struct inode *n, struct file *f) {

drivers/hv/dxgkrnl/dxgmodule.c +8 -1

-	return 0;
+	int ret = 0;
+	struct dxgprocess *process;
+
+	pr_debug("%s %p %d %d",
+		     __func__, f, current->pid, current->tgid);
+
+
+	/* Find/create a dxgprocess structure for this process */

drivers/hv/dxgkrnl/dxgmodule.c +1 -0

+	return ret;
 }

drivers/hv/dxgkrnl/dxgvmbus.c +19 -0

+int dxgvmb_send_create_process(struct dxgprocess *process)
+{
+	int ret;
+	struct dxgkvmb_command_createprocess *command;
+	struct dxgkvmb_command_createprocess_return result = { 0 };
+	struct dxgvmbusmsg msg;
+	char s[WIN_MAX_PATH];
+	int i;
+
+	ret = init_message(&msg, NULL, process, sizeof(*command));
+	if (ret)
+		return ret;
+	command = (void *)msg.msg;
+
+	ret = dxgglobal_acquire_channel_lock();
+	if (ret < 0)
+		goto cleanup;
+
+	command_vm_to_host_init1(&command->hdr, DXGK_VMBCOMMAND_CREATEPROCESS);

drivers/hv/dxgkrnl/dxgadapter.c +3 -0

+}
+
 int dxgadapter_acquire_lock_exclusive(struct dxgadapter *adapter)
 {
 	down_write(&adapter->core_lock);

@@ -170,3 +198,52 @@ void dxgadapter_release_lock_shared(struct dxgadapter *adapter)
 {
 	up_read(&adapter->core_lock);
 }
+