Re: [PATCH v3 02/30] drivers: hv: dxgkrnl: Driver initialization and loading

Wei Liu
2022-03-01T22:06:43Z

I will skip things that are pointed out by Greg.

On Tue, Mar 01, 2022 at 11:45:49AM -0800, Iouri Tarassov wrote:
> - 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
>

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

> 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 <iourit@linux.microsoft.com>
> ---
[...]
> +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.

[...]
> + * A helper function to read PCI config space.
> + */
> +static int dxg_pci_read_dwords(struct pci_dev *dev, int offset, int size,
> + void *val)
> +{
> + int off = offset;
> + int ret;
> + int i;
> +

I think you should check for alignment here? size has to be a round
number of sizeof(int) otherwise you risk reading cross boundary and
causes unintended side effect?

> + 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 */
> +
[...]
> + pr_debug("Adapter channel: %pUb\n", &guid);
> + pr_debug("Vmbus interface version: %d\n",
> + dxgglobal->vmbus_ver);

No need to wrap the line here.

> + pr_debug("Host vGPU luid: %x-%x\n",
> + vgpu_luid.b, vgpu_luid.a);

Ditto.

> +
> +cleanup:
> +
> + mutex_unlock(&dxgglobal->device_mutex);
> +
> + if (ret)
> + pr_debug("err: %s %d", __func__, ret);
> + return ret;
> +}
> +
[...]
> +static void __exit dxg_drv_exit(void)
> +{
> + dxgglobal_destroy();
> +}
> +
> +module_init(dxg_drv_init);
> +module_exit(dxg_drv_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Microsoft Dxgkrnl virtual GPU Driver");

Should be "virtual compute device driver" here? Please be consistent.

Thanks,
Wei.