[PATCH 0/4] drivers: hv: Microsoft Virtual GPU Driver

[PATCH 0/4] drivers: hv: Microsoft Virtual GPU Driver

[Sasha Levin]

This is a follow-up on the RFC sent a few months back[1].

Changes since the RFC:

 - Move to drivers/hv/
 - Address comments from Greg KH
   - Misc device initialization
   - Remove typedefs/variable defines
   - Use the kernel's ioctl declarations
 - Clean up random code bugs.


[1] https://lore.kernel.org/lkml/20200519163234.226513-1-sashal@kernel.org/


Sasha Levin (4):
  drivers: hv: dxgkrnl: core code
  drivers: hv: dxgkrnl: hook up dxgkrnl
  drivers: hv: vmbus: hook up dxgkrnl
  drivers: hv: dxgkrnl: create a MAINTAINERS entry

 MAINTAINERS                     |    7 +
 drivers/hv/Kconfig              |    2 +
 drivers/hv/Makefile             |    1 +
 drivers/hv/dxgkrnl/Kconfig      |   10 +
 drivers/hv/dxgkrnl/Makefile     |   12 +
 drivers/hv/dxgkrnl/d3dkmthk.h   | 1636 ++++++++++
 drivers/hv/dxgkrnl/dxgadapter.c | 1406 ++++++++
 drivers/hv/dxgkrnl/dxgkrnl.h    |  927 ++++++
 drivers/hv/dxgkrnl/dxgmodule.c  |  656 ++++
 drivers/hv/dxgkrnl/dxgprocess.c |  357 ++
 drivers/hv/dxgkrnl/dxgvmbus.c   | 3084 ++++++++++++++++++
 drivers/hv/dxgkrnl/dxgvmbus.h   |  873 +++++
 drivers/hv/dxgkrnl/hmgr.c       |  604 ++++
 drivers/hv/dxgkrnl/hmgr.h       |  112 +
 drivers/hv/dxgkrnl/ioctl.c      | 5413 +++++++++++++++++++++++++++++++
 drivers/hv/dxgkrnl/misc.c       |  279 ++
 drivers/hv/dxgkrnl/misc.h       |  309 ++
 include/linux/hyperv.h          |   16 +
 18 files changed, 15704 insertions(+)
 create mode 100644 drivers/hv/dxgkrnl/Kconfig
 create mode 100644 drivers/hv/dxgkrnl/Makefile
 create mode 100644 drivers/hv/dxgkrnl/d3dkmthk.h
 create mode 100644 drivers/hv/dxgkrnl/dxgadapter.c
 create mode 100644 drivers/hv/dxgkrnl/dxgkrnl.h
 create mode 100644 drivers/hv/dxgkrnl/dxgmodule.c
 create mode 100644 drivers/hv/dxgkrnl/dxgprocess.c
 create mode 100644 drivers/hv/dxgkrnl/dxgvmbus.c
 create mode 100644 drivers/hv/dxgkrnl/dxgvmbus.h
 create mode 100644 drivers/hv/dxgkrnl/hmgr.c
 create mode 100644 drivers/hv/dxgkrnl/hmgr.h
 create mode 100644 drivers/hv/dxgkrnl/ioctl.c
 create mode 100644 drivers/hv/dxgkrnl/misc.c
 create mode 100644 drivers/hv/dxgkrnl/misc.h

-- 
2.25.1

[PATCH 2/4] drivers: hv: dxgkrnl: hook up dxgkrnl

[Sasha Levin]

Connect the dxgkrnl module to the drivers/hv/ makefile and Kconfig.

Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/hv/Kconfig  | 2 ++
 drivers/hv/Makefile | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index 79e5356a737a..07d4e7c36e3a 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -26,4 +26,6 @@ config HYPERV_BALLOON
 	help
 	  Select this option to enable Hyper-V Balloon driver.
 
+source "drivers/hv/dxgkrnl/Kconfig"
+
 endmenu
diff --git a/drivers/hv/Makefile b/drivers/hv/Makefile
index 94daf8240c95..2474b70c161d 100644
--- a/drivers/hv/Makefile
+++ b/drivers/hv/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_HYPERV)		+= hv_vmbus.o
 obj-$(CONFIG_HYPERV_UTILS)	+= hv_utils.o
 obj-$(CONFIG_HYPERV_BALLOON)	+= hv_balloon.o
+obj-$(CONFIG_DXGKRNL)		+= dxgkrnl/
 
 CFLAGS_hv_trace.o = -I$(src)
 CFLAGS_hv_balloon.o = -I$(src)
-- 
2.25.1

[PATCH 3/4] drivers: hv: vmbus: hook up dxgkrnl

[Sasha Levin]

Register a new device type with vmbus.

Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 include/linux/hyperv.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 40df3103e890..40fff19ecde3 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1343,6 +1343,22 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size);
 	.guid = GUID_INIT(0xda0a7802, 0xe377, 0x4aac, 0x8e, 0x77, \
 			  0x05, 0x58, 0xeb, 0x10, 0x73, 0xf8)
 
+/*
+ * GPU paravirtualization global DXGK channel
+ * {DDE9CBC0-5060-4436-9448-EA1254A5D177}
+ */
+#define HV_GPUP_DXGK_GLOBAL_GUID \
+	.guid = GUID_INIT(0xdde9cbc0, 0x5060, 0x4436, 0x94, 0x48, \
+			  0xea, 0x12, 0x54, 0xa5, 0xd1, 0x77)
+
+/*
+ * GPU paravirtualization per virtual GPU DXGK channel
+ * {6E382D18-3336-4F4B-ACC4-2B7703D4DF4A}
+ */
+#define HV_GPUP_DXGK_VGPU_GUID \
+	.guid = GUID_INIT(0x6e382d18, 0x3336, 0x4f4b, 0xac, 0xc4, \
+			  0x2b, 0x77, 0x3, 0xd4, 0xdf, 0x4a)
+
 /*
  * Synthetic FC GUID
  * {2f9bcc4a-0069-4af3-b76b-6fd0be528cda}
-- 
2.25.1

[PATCH 4/4] drivers: hv: dxgkrnl: create a MAINTAINERS entry

[Sasha Levin]

Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4e2698cc7e23..1e725a9e6335 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8003,6 +8003,13 @@ F:	Documentation/devicetree/bindings/mtd/ti,am654-hbmc.txt
 F:	drivers/mtd/hyperbus/
 F:	include/linux/mtd/hyperbus.h
 
+Hyper-V vGPU DRIVER
+M:	Sasha Levin <sashal@kernel.org>
+M:	Iouri Tarassov <iourit@microsoft.com>
+L:	linux-hyperv@vger.kernel.org
+S:	Supported
+F:	drivers/hv/dxgkrnl/
+
 HYPERVISOR VIRTUAL CONSOLE DRIVER
 L:	linuxppc-dev@lists.ozlabs.org
 S:	Odd Fixes
-- 
2.25.1

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Greg KH]

On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> Add support for a Hyper-V based vGPU implementation that exposes the
> DirectX API to Linux userspace.
> 
> Signed-off-by: Sasha Levin <sashal@kernel.org>

Better, but what is this mess:

> +#define ISERROR(ret)					(ret < 0)

?

> +#define EINTERNALERROR					EINVAL

And that?

> +
> +#define DXGKRNL_ASSERT(exp)
> +#define UNUSED(x) (void)(x)

Ick, no, please.

> +#undef pr_fmt

In a .h file?

> +#define pr_fmt(fmt)	"dxgk:err: " fmt
> +#define pr_fmt1(fmt)	"dxgk: " fmt
> +#define pr_fmt2(fmt)	"dxgk:    " fmt

Why?

> +
> +#define DXGKDEBUG 1
> +/* #define USEPRINTK 1 */
> +
> +#ifndef DXGKDEBUG
> +#define TRACE_DEBUG(...)
> +#define TRACE_DEFINE(...)
> +#define TRACE_FUNC_ENTER(...)
> +#define TRACE_FUNC_EXIT(...)

No, please do not to custom "trace" printk messages, that is what ftrace
is for, no individual driver should ever need to do that.

Just use the normal dev_*() calls for error messages and the like, do
not try to come up with a custom tracing framework for one tiny
individual driver.  If every driver in kernel did that, we would have a
nightmare...

thanks,

greg k-h

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Greg KH]

On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> Add support for a Hyper-V based vGPU implementation that exposes the
> DirectX API to Linux userspace.

Api questions:

> +struct d3dkmthandle {
> +	union {
> +		struct {
> +			u32 instance	:  6;
> +			u32 index	: 24;
> +			u32 unique	: 2;

What is the endian of this?

> +		};
> +		u32 v;
> +	};
> +};
> +
> +extern const struct d3dkmthandle zerohandle;
> +
> +struct ntstatus {
> +	union {
> +		struct {
> +			int code	: 16;
> +			int facility	: 13;
> +			int customer	: 1;
> +			int severity	: 2;

Same here.

Are these things that cross the user/kernel boundry?

And why int on one and u32 on the other?

> +		};
> +		int v;
> +	};
> +};
> +
> +struct winluid {
> +	uint a;
> +	uint b;

And now uint?  Come on, be consistent please :)

thanks,

greg k-h

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Greg KH]

On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> Add support for a Hyper-V based vGPU implementation that exposes the
> DirectX API to Linux userspace.
> 
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  drivers/hv/dxgkrnl/Kconfig      |   10 +
>  drivers/hv/dxgkrnl/Makefile     |   12 +
>  drivers/hv/dxgkrnl/d3dkmthk.h   | 1636 ++++++++++
>  drivers/hv/dxgkrnl/dxgadapter.c | 1406 ++++++++
>  drivers/hv/dxgkrnl/dxgkrnl.h    |  927 ++++++
>  drivers/hv/dxgkrnl/dxgmodule.c  |  656 ++++
>  drivers/hv/dxgkrnl/dxgprocess.c |  357 ++
>  drivers/hv/dxgkrnl/dxgvmbus.c   | 3084 ++++++++++++++++++
>  drivers/hv/dxgkrnl/dxgvmbus.h   |  873 +++++
>  drivers/hv/dxgkrnl/hmgr.c       |  604 ++++
>  drivers/hv/dxgkrnl/hmgr.h       |  112 +
>  drivers/hv/dxgkrnl/ioctl.c      | 5413 +++++++++++++++++++++++++++++++
>  drivers/hv/dxgkrnl/misc.c       |  279 ++
>  drivers/hv/dxgkrnl/misc.h       |  309 ++
>  14 files changed, 15678 insertions(+)

It's almost impossible to review 15k lines at once, please break this up
into reviewable chunks next time.

> +++ b/drivers/hv/dxgkrnl/Kconfig
> @@ -0,0 +1,10 @@
> +#
> +# dxgkrnl configuration
> +#
> +
> +config DXGKRNL
> +	tristate "Microsoft virtual GPU support"
> +	depends on HYPERV
> +	help
> +	  This driver supports Microsoft virtual GPU.
> +

You need more text here, this isn't a staging driver submission :)

> diff --git a/drivers/hv/dxgkrnl/Makefile b/drivers/hv/dxgkrnl/Makefile
> new file mode 100644
> index 000000000000..11505a153d9d
> --- /dev/null
> +++ b/drivers/hv/dxgkrnl/Makefile
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Makefile for the Linux video drivers.
> +# 5 Aug 1999, James Simmons, <mailto:jsimmons@users.sf.net>
> +# Rewritten to use lists instead of if-statements.

I really doubt these last 3 lines are relevant.

> +
> +# Each configuration option enables a list of files.

We know this.

> +
> +# Uncomment to enable printing debug messages by default
> +#ccflags-y := -DDEBUG

No, don't do this please.

> +
> +obj-$(CONFIG_DXGKRNL)	+= dxgkrnl.o
> +dxgkrnl-y		:= dxgmodule.o hmgr.o misc.o dxgadapter.o ioctl.o dxgvmbus.o dxgprocess.o
> diff --git a/drivers/hv/dxgkrnl/d3dkmthk.h b/drivers/hv/dxgkrnl/d3dkmthk.h
> new file mode 100644
> index 000000000000..90cf5134b361
> --- /dev/null
> +++ b/drivers/hv/dxgkrnl/d3dkmthk.h
> @@ -0,0 +1,1636 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright (c) 2019, Microsoft Corporation.
> + *
> + * Author:
> + *   Iouri Tarassov <iourit@microsoft.com>
> + *
> + * Dxgkrnl Graphics Port Driver user mode interface
> + *
> + */
> +
> +#ifndef _D3DKMTHK_H
> +#define _D3DKMTHK_H
> +
> +#include "misc.h"
> +
> +#define D3DDDI_MAX_WRITTEN_PRIMARIES		16
> +#define D3DDDI_MAX_MPO_PRESENT_DIRTY_RECTS	0xFFF
> +
> +#define D3DKMT_CREATEALLOCATION_MAX		1024
> +#define D3DKMT_ADAPTERS_MAX			64
> +#define D3DDDI_MAX_BROADCAST_CONTEXT		64
> +#define D3DDDI_MAX_OBJECT_WAITED_ON		32
> +#define D3DDDI_MAX_OBJECT_SIGNALED		32
> +
> +struct d3dkmt_adapterinfo {
> +	struct d3dkmthandle		adapter_handle;
> +	struct winluid			adapter_luid;
> +	uint				num_sources;
> +	uint				present_move_regions_preferred;
> +};
> +
> +struct d3dkmt_enumadapters2 {
> +	uint				num_adapters;

Use kernel types please, here and everywhere.  u32?

> +	struct d3dkmt_adapterinfo	*adapters;
> +};
> +
> +struct d3dkmt_closeadapter {
> +	struct d3dkmthandle		adapter_handle;
> +};

A "handle"?  And that has to be one of the most difficult structure
names ever :)

Why not just use the "handle" for the structure as obviously that's all
that is needed here.
> +
> +struct d3dkmt_openadapterfromluid {
> +	struct winluid			adapter_luid;
> +	struct d3dkmthandle		adapter_handle;
> +};
> +
> +struct d3dddi_allocationlist {
> +	struct d3dkmthandle		allocation;
> +	union {
> +		struct {
> +			uint		write_operation		:1;
> +			uint		do_not_retire_instance	:1;
> +			uint		offer_priority		:3;
> +			uint		reserved		:27;

endian issues?

If not, why are these bit fields?

> +struct d3dkmt_destroydevice {
> +	struct d3dkmthandle		device;
> +};

Again, single entity structures?

Are you trying to pass around "handles" and cast them backwards?

If so, great, but then use the real kernel structures for that like
'struct device' if these are actually devices.


> +
> +enum d3dkmt_clienthint {
> +	D3DKMT_CLIENTHINT_UNKNOWN	= 0,
> +	D3DKMT_CLIENTHINT_OPENGL	= 1,
> +	D3DKMT_CLIENTHINT_CDD		= 2,
> +	D3DKMT_CLIENTHINT_DX7		= 7,
> +	D3DKMT_CLIENTHINT_DX8		= 8,
> +	D3DKMT_CLIENTHINT_DX9		= 9,
> +	D3DKMT_CLIENTHINT_DX10		= 10,
> +};
> +
> +struct d3dddi_createcontextflags {
> +	union {
> +		struct {
> +			uint		null_rendering:1;
> +			uint		initial_data:1;
> +			uint		disable_gpu_timeout:1;
> +			uint		synchronization_only:1;
> +			uint		hw_queue_supported:1;
> +			uint		reserved:27;

Endian?

> +		};
> +		uint			value;
> +	};
> +};

<...>


> +static int dxgglobal_init_global_channel(struct hv_device *hdev)
> +{
> +	int ret = 0;
> +
> +	TRACE_DEBUG(1, "%s %x  %x", __func__, hdev->vendor_id, hdev->device_id);
> +	{
> +		TRACE_DEBUG(1, "device type   : %pUb\n", &hdev->dev_type);
> +		TRACE_DEBUG(1, "device channel: %pUb %p primary: %p\n",
> +			    &hdev->channel->offermsg.offer.if_type,
> +			    hdev->channel, hdev->channel->primary_channel);
> +	}
> +
> +	if (dxgglobal->hdev) {
> +		/* This device should appear only once */
> +		pr_err("dxgglobal already initialized\n");
> +		ret = -EBADE;
> +		goto error;
> +	}
> +
> +	dxgglobal->hdev = hdev;
> +
> +	ret = dxgvmbuschannel_init(&dxgglobal->channel, hdev);
> +	if (ret) {
> +		pr_err("dxgvmbuschannel_init failed: %d\n", ret);
> +		goto error;
> +	}
> +
> +	ret = dxgglobal_getiospace(dxgglobal);
> +	if (ret) {
> +		pr_err("getiospace failed: %d\n", ret);
> +		goto error;
> +	}
> +
> +	ret = dxgvmb_send_set_iospace_region(dxgglobal->mmiospace_base,
> +					     dxgglobal->mmiospace_size, 0);
> +	if (ISERROR(ret)) {
> +		pr_err("send_set_iospace_region failed");
> +		goto error;

You forgot to unwind from the things you initialized above :(

> +	}
> +
> +	hv_set_drvdata(hdev, dxgglobal);
> +
> +	dxgglobal->dxgdevice.minor = MISC_DYNAMIC_MINOR;
> +	dxgglobal->dxgdevice.name = "dxg";
> +	dxgglobal->dxgdevice.fops = &dxgk_fops;
> +	dxgglobal->dxgdevice.mode = 0666;
> +	ret = misc_register(&dxgglobal->dxgdevice);
> +	if (ret) {
> +		pr_err("misc_register failed: %d", ret);
> +		goto error;

Again, no cleanups so you leak resources?  Not nice :(


> +	}
> +	dxgglobaldev = dxgglobal->dxgdevice.this_device;
> +	dxgglobal->device_initialized = true;
> +
> +error:
> +	return ret;
> +}
> +
> +static void dxgglobal_destroy_global_channel(void)
> +{
> +	dxglockorder_acquire(DXGLOCK_GLOBAL_CHANNEL);
> +	down_write(&dxgglobal->channel_lock);
> +
> +	TRACE_DEBUG(1, "%s", __func__);

ftrace is your friend :)

Re: [PATCH 4/4] drivers: hv: dxgkrnl: create a MAINTAINERS entry

[Greg KH]

On Fri, Aug 14, 2020 at 08:38:56AM -0400, Sasha Levin wrote:
> Signed-off-by: Sasha Levin <sashal@kernel.org>

I know I don't take patches without changelog text :)

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Wei Liu]

On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
[...]
> +
> +#include "dxgkrnl.h"
> +
> +int dxgadapter_init(struct dxgadapter *adapter, struct hv_device *hdev)
> +{
> +	int ret = 0;
> +	char s[80];
> +
> +	UNUSED(s);

If s is not used, why not just remove it?

Indeed it is not used anywhere in this function. That saves you 80 bytes
on stack.

> +static int dxgk_destroy_hwcontext(struct dxgprocess *process,
> +					      void *__user inargs)
> +{
> +	/* This is obsolete entry point */
> +	return ENOTTY;
> +}
> +

Other places have been using negative numbers for errors. I guess you
want -ENOTTY here too.

Wei.

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Pavel Machek]

On Fri 2020-08-14 08:38:53, Sasha Levin wrote:
> Add support for a Hyper-V based vGPU implementation that exposes the
> DirectX API to Linux userspace.

NAK.

Kernel APIs should be documented. ... in tree and with suitable license.
	
> +int dxgadapter_init(struct dxgadapter *adapter, struct hv_device *hdev)
> +{
> +	int ret = 0;
> +	char s[80];
> +
> +	UNUSED(s);

What is going on here?

> +{
> +	struct dxgprocess_adapter *adapter_info = dxgmem_alloc(process,
> +							       DXGMEM_PROCESS_ADAPTER,
> +							       sizeof
> +							       (*adapter_info));

We normally use kernel functions in kernel code.

> +	dxgmutex_unlock(&device->adapter_info->device_list_mutex);

And you should not have private locking primitives, either.

> +bool dxghwqueue_acquire_reference(struct dxghwqueue *hwqueue)
> +{
> +	return refcount_inc_not_zero(&hwqueue->refcount);
> +}

Midlayers are evil.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Iouri Tarassov]

On 8/14/2020 6:18 AM, Wei Liu wrote:
> On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> [...]
>> +
>> +#include "dxgkrnl.h"
>> +
>> +int dxgadapter_init(struct dxgadapter *adapter, struct hv_device *hdev)
>> +{
>> +	int ret = 0;
>> +	char s[80];
>> +
>> +	UNUSED(s);
> If s is not used, why not just remove it?
>
> Indeed it is not used anywhere in this function. That saves you 80 bytes
> on stack.
>
>> +static int dxgk_destroy_hwcontext(struct dxgprocess *process,
>> +					      void *__user inargs)
>> +{
>> +	/* This is obsolete entry point */
>> +	return ENOTTY;
>> +}
>> +
> Other places have been using negative numbers for errors. I guess you
> want -ENOTTY here too.
>
> Wei.
>
>

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Iouri Tarassov]

Hello

On 8/14/2020 6:18 AM, Wei Liu wrote:
> On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> [...]
> > +
> > +#include "dxgkrnl.h"
> > +
> > +int dxgadapter_init(struct dxgadapter *adapter, struct hv_device *hdev)
> > +{
> > +	int ret = 0;
> > +	char s[80];
> > +
> > +	UNUSED(s);
>
> If s is not used, why not just remove it?
>
> Indeed it is not used anywhere in this function. That saves you 80 bytes
> on stack.
>
> > +static int dxgk_destroy_hwcontext(struct dxgprocess *process,
> > +					      void *__user inargs)
> > +{
> > +	/* This is obsolete entry point */
> > +	return ENOTTY;
> > +}
> > +
>
> Other places have been using negative numbers for errors. I guess you
> want -ENOTTY here too.
>
> Wei.
>
>

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Iouri Tarassov]

Thank you,Wei. These issues will be fixed in the next patchset.

Iouri

On 8/14/2020 6:18 AM, Wei Liu wrote:
> On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> [...]
> > +
> > +#include "dxgkrnl.h"
> > +
> > +int dxgadapter_init(struct dxgadapter *adapter, struct hv_device *hdev)
> > +{
> > +	int ret = 0;
> > +	char s[80];
> > +
> > +	UNUSED(s);
>
> If s is not used, why not just remove it?
>
> Indeed it is not used anywhere in this function. That saves you 80 bytes
> on stack.
>
> > +static int dxgk_destroy_hwcontext(struct dxgprocess *process,
> > +					      void *__user inargs)
> > +{
> > +	/* This is obsolete entry point */
> > +	return ENOTTY;
> > +}
> > +
>
> Other places have been using negative numbers for errors. I guess you
> want -ENOTTY here too.
>
> Wei.
>
>

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Iouri Tarassov]

On 8/14/2020 5:55 AM, Greg KH wrote:
> On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> > Add support for a Hyper-V based vGPU implementation that exposes the
> > DirectX API to Linux userspace.
> > 
> > Signed-off-by: Sasha Levin <sashal@kernel.org>
>
> Better, but what is this mess:
>
> > +#define ISERROR(ret)					(ret < 0)

The VM bus messages return the NTSTATUS error code from the host. 
NTSTATUS is integer and the positive values indicate success.
The success error code needs to be returned by IOCTLs to the DxCore 
applications. The ISERROR() macro is used in places where the NTSTATUS 
success code could be returned from a function. "if (ret)" cannot be 
used. I will add a comment to the macro in the next patch.

>
> ?
>
> > +#define EINTERNALERROR					EINVAL
This macro will be removed in the next patch
> And that?
>
> > +
> > +#define DXGKRNL_ASSERT(exp)
> > +#define UNUSED(x) (void)(x)
The UNUSED macro will be removed.
The DXGKRNL_ASSERT() macro will be changed to generate a memory dump and 
BUG_ON when DXGDEBUG is enabled. It is used to catch internal errors in 
the debug code. There will be no bugcheck in the released driver.
>
> Ick, no, please.
>
> > +#undef pr_fmt
>
> In a .h file?
>
> > +#define pr_fmt(fmt)	"dxgk:err: " fmt
> > +#define pr_fmt1(fmt)	"dxgk: " fmt
> > +#define pr_fmt2(fmt)	"dxgk:    " fmt
This will be fixed in the next patch set.
> Why?
>
> > +
> > +#define DXGKDEBUG 1
> > +/* #define USEPRINTK 1 */
> > +
> > +#ifndef DXGKDEBUG
> > +#define TRACE_DEBUG(...)
> > +#define TRACE_DEFINE(...)
> > +#define TRACE_FUNC_ENTER(...)
> > +#define TRACE_FUNC_EXIT(...)
>
> No, please do not to custom "trace" printk messages, that is what ftrace
> is for, no individual driver should ever need to do that.
>
> Just use the normal dev_*() calls for error messages and the like, do
> not try to come up with a custom tracing framework for one tiny
> individual driver.  If every driver in kernel did that, we would have a
> nightmare...
I understand the concern. This will be fixed later when we learn and 
pick the final tracing technology for the driver. The current 
implementation allows the hardware vendors to quickly identify issues 
without learning about ftrace. They just need to enable the WSL debug 
console and mount debugfs.
> thanks,
>
> greg k-h
>
Thank you for your time and good comments.
Iouri

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Iouri Tarassov]

On 8/14/2020 5:57 AM, Greg KH wrote:
> On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> > Add support for a Hyper-V based vGPU implementation that exposes the
> > DirectX API to Linux userspace.
>
> Api questions:
>
> > +struct d3dkmthandle {
> > +	union {
> > +		struct {
> > +			u32 instance	:  6;
> > +			u32 index	: 24;
> > +			u32 unique	: 2;
>
> What is the endian of this?
>
> > +		};
> > +		u32 v;
> > +	};
> > +};
> > +
> > +extern const struct d3dkmthandle zerohandle;
> > +
The definition is the same as on the Windows side. The driver 
communicates with a Windows host, so I do not expect any issues with 
endiannes. Windows currently runs only on the little endian platforms.
User mode applications see this as an opaque 32 bit value 
(D3DKMTHANDLE). I prefer not to use the u32 definition to avoid mistakes 
when another integer or a 64-bit handle is assigned to the handle or  
the handle is assigned to a 64 or 32 bit integer variable. There are 
many handles in the driver model (shared NT handle, d3dkmthandle, etc). 
Using a specific type allows to avoid assigning one handle to another.
> > +struct ntstatus {
> > +	union {
> > +		struct {
> > +			int code	: 16;
> > +			int facility	: 13;
> > +			int customer	: 1;
> > +			int severity	: 2;
>
> Same here.
>
> Are these things that cross the user/kernel boundry?
>
> And why int on one and u32 on the other?
>
> > +		};
> > +		int v;
> > +	};
> > +};
> > +
"struct ntstatus" follows the definition for NTSTATUS on the Windows 
side. NTSTATUS is an integer where the negative values indicate errors. 
It is success otherwise. NTSTATUS is returned by the VM bus messages 
from host. IOCTLs from the driver return Linux negative error code or 
NTSTATUS positive success codes. DxCore applications expect certain 
positive success codes. DxCore is a shared library, which translates the 
D3DKMT* Windows interface to Linux ioctls. Applications link with DxCore 
to use a paravirtualized GPU.
D3DKMTHANDLE is a 32-bit unsigned value (bitfield), not an integer.
> > +struct winluid {
> > +	uint a;
> > +	uint b;
>
> And now uint?  Come on, be consistent please :)
Sorry about this. This came from the Windows size where we use UINT a 
lot. All uints will be replaced by u32 in the next patch set.
>
> thanks,
>
> greg k-h
>
Thank you
Iouri

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Iouri Tarassov]

On 8/14/2020 6:04 AM, Greg KH wrote:
> On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> > Add support for a Hyper-V based vGPU implementation that exposes the
> > DirectX API to Linux userspace.
> > 
> > Signed-off-by: Sasha Levin <sashal@kernel.org>
> > ---
> >  drivers/hv/dxgkrnl/Kconfig      |   10 +
> >  drivers/hv/dxgkrnl/Makefile     |   12 +
> >  drivers/hv/dxgkrnl/d3dkmthk.h   | 1636 ++++++++++
> >  drivers/hv/dxgkrnl/dxgadapter.c | 1406 ++++++++
> >  drivers/hv/dxgkrnl/dxgkrnl.h    |  927 ++++++
> >  drivers/hv/dxgkrnl/dxgmodule.c  |  656 ++++
> >  drivers/hv/dxgkrnl/dxgprocess.c |  357 ++
> >  drivers/hv/dxgkrnl/dxgvmbus.c   | 3084 ++++++++++++++++++
> >  drivers/hv/dxgkrnl/dxgvmbus.h   |  873 +++++
> >  drivers/hv/dxgkrnl/hmgr.c       |  604 ++++
> >  drivers/hv/dxgkrnl/hmgr.h       |  112 +
> >  drivers/hv/dxgkrnl/ioctl.c      | 5413 +++++++++++++++++++++++++++++++
> >  drivers/hv/dxgkrnl/misc.c       |  279 ++
> >  drivers/hv/dxgkrnl/misc.h       |  309 ++
> >  14 files changed, 15678 insertions(+)
>
> It's almost impossible to review 15k lines at once, please break this up
> into reviewable chunks next time.

Sorry about this, but we had to replace a lot of typedefs, which are not 
allowed by the coding style.
We expect one more big patch, which cannot be split in my opinion. The 
VM vbus message format was changed to include additional header. As the 
result, every function in dxgvmbus.c needs to be changed to handle the 
new header. I do not see how this can be split to multiple patches so 
each patch produces a working driver.

>
> > +++ b/drivers/hv/dxgkrnl/Kconfig
> > @@ -0,0 +1,10 @@
> > +#
> > +# dxgkrnl configuration
> > +#
> > +
> > +config DXGKRNL
> > +	tristate "Microsoft virtual GPU support"
> > +	depends on HYPERV
> > +	help
> > +	  This driver supports Microsoft virtual GPU.
> > +
>
> You need more text here, this isn't a staging driver submission :)
Is the the proposed description good enough?
"This driver handles paravirtualized GPU devices exposed by Microsoft 
Hyper-V when Linux is running inside of a virtual machine hosted 
by Windows."
>
> > diff --git a/drivers/hv/dxgkrnl/Makefile b/drivers/hv/dxgkrnl/Makefile
> > new file mode 100644
> > index 000000000000..11505a153d9d
> > --- /dev/null
> > +++ b/drivers/hv/dxgkrnl/Makefile
> > @@ -0,0 +1,12 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Makefile for the Linux video drivers.
> > +# 5 Aug 1999, James Simmons, <mailto:jsimmons@users.sf.net>
> > +# Rewritten to use lists instead of if-statements.
>
> I really doubt these last 3 lines are relevant.
>
> > +
> > +# Each configuration option enables a list of files.
>
> We know this.
>
> > +
> > +# Uncomment to enable printing debug messages by default
> > +#ccflags-y := -DDEBUG
>
> No, don't do this please.
These lines will be removed.
>
> > +
> > +obj-$(CONFIG_DXGKRNL)	+= dxgkrnl.o
> > +dxgkrnl-y		:= dxgmodule.o hmgr.o misc.o dxgadapter.o ioctl.o dxgvmbus.o dxgprocess.o
> > diff --git a/drivers/hv/dxgkrnl/d3dkmthk.h b/drivers/hv/dxgkrnl/d3dkmthk.h
> > new file mode 100644
> > index 000000000000..90cf5134b361
> > --- /dev/null
> > +++ b/drivers/hv/dxgkrnl/d3dkmthk.h
> > @@ -0,0 +1,1636 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Copyright (c) 2019, Microsoft Corporation.
> > + *
> > + * Author:
> > + *   Iouri Tarassov <iourit@microsoft.com>
> > + *
> > + * Dxgkrnl Graphics Port Driver user mode interface
> > + *
> > + */
> > +
> > +#ifndef _D3DKMTHK_H
> > +#define _D3DKMTHK_H
> > +
> > +#include "misc.h"
> > +
> > +#define D3DDDI_MAX_WRITTEN_PRIMARIES		16
> > +#define D3DDDI_MAX_MPO_PRESENT_DIRTY_RECTS	0xFFF
> > +
> > +#define D3DKMT_CREATEALLOCATION_MAX		1024
> > +#define D3DKMT_ADAPTERS_MAX			64
> > +#define D3DDDI_MAX_BROADCAST_CONTEXT		64
> > +#define D3DDDI_MAX_OBJECT_WAITED_ON		32
> > +#define D3DDDI_MAX_OBJECT_SIGNALED		32
> > +
> > +struct d3dkmt_adapterinfo {
> > +	struct d3dkmthandle		adapter_handle;
> > +	struct winluid			adapter_luid;
> > +	uint				num_sources;
> > +	uint				present_move_regions_preferred;
> > +};
> > +
> > +struct d3dkmt_enumadapters2 {
> > +	uint				num_adapters;
>
> Use kernel types please, here and everywhere.  u32?
The definition will be changed to u32.
>
> > +	struct d3dkmt_adapterinfo	*adapters;
> > +};
> > +
> > +struct d3dkmt_closeadapter {
> > +	struct d3dkmthandle		adapter_handle;
> > +};
>
> A "handle"?  And that has to be one of the most difficult structure
> names ever :)
>
> Why not just use the "handle" for the structure as obviously that's all
> that is needed here.
The structure definition matches the Windows D3DKMT interface. Some 
input structures to the interface functions have only one member. But 
there is possibility that new member could be added in the future. We 
prefer to have matching names between Windows and Linux to avoid confusion.
> > +
> > +struct d3dkmt_openadapterfromluid {
> > +	struct winluid			adapter_luid;
> > +	struct d3dkmthandle		adapter_handle;
> > +};
> > +
> > +struct d3dddi_allocationlist {
> > +	struct d3dkmthandle		allocation;
> > +	union {
> > +		struct {
> > +			uint		write_operation		:1;
> > +			uint		do_not_retire_instance	:1;
> > +			uint		offer_priority		:3;
> > +			uint		reserved		:27;
>
> endian issues?
>
> If not, why are these bit fields?
This matches the definition on the Windows side. Windows only works on 
little endian platforms.
>
> > +struct d3dkmt_destroydevice {
> > +	struct d3dkmthandle		device;
> > +};
>
> Again, single entity structures?
>
> Are you trying to pass around "handles" and cast them backwards?
>
> If so, great, but then use the real kernel structures for that like
> 'struct device' if these are actually devices.
>
Again. The structure matches the definition on the Windows side to avoid 
confusion.
> > +
> > +enum d3dkmt_clienthint {
> > +	D3DKMT_CLIENTHINT_UNKNOWN	= 0,
> > +	D3DKMT_CLIENTHINT_OPENGL	= 1,
> > +	D3DKMT_CLIENTHINT_CDD		= 2,
> > +	D3DKMT_CLIENTHINT_DX7		= 7,
> > +	D3DKMT_CLIENTHINT_DX8		= 8,
> > +	D3DKMT_CLIENTHINT_DX9		= 9,
> > +	D3DKMT_CLIENTHINT_DX10		= 10,
> > +};
> > +
> > +struct d3dddi_createcontextflags {
> > +	union {
> > +		struct {
> > +			uint		null_rendering:1;
> > +			uint		initial_data:1;
> > +			uint		disable_gpu_timeout:1;
> > +			uint		synchronization_only:1;
> > +			uint		hw_queue_supported:1;
> > +			uint		reserved:27;
>
> Endian?
>
> > +		};
> > +		uint			value;
> > +	};
> > +};
>
> <...>
The structure matches definition on the Windows side.
>
> > +static int dxgglobal_init_global_channel(struct hv_device *hdev)
> > +{
> > +	int ret = 0;
> > +
> > +	TRACE_DEBUG(1, "%s %x  %x", __func__, hdev->vendor_id, hdev->device_id);
> > +	{
> > +		TRACE_DEBUG(1, "device type   : %pUb\n", &hdev->dev_type);
> > +		TRACE_DEBUG(1, "device channel: %pUb %p primary: %p\n",
> > +			    &hdev->channel->offermsg.offer.if_type,
> > +			    hdev->channel, hdev->channel->primary_channel);
> > +	}
> > +
> > +	if (dxgglobal->hdev) {
> > +		/* This device should appear only once */
> > +		pr_err("dxgglobal already initialized\n");
> > +		ret = -EBADE;
> > +		goto error;
> > +	}
> > +
> > +	dxgglobal->hdev = hdev;
> > +
> > +	ret = dxgvmbuschannel_init(&dxgglobal->channel, hdev);
> > +	if (ret) {
> > +		pr_err("dxgvmbuschannel_init failed: %d\n", ret);
> > +		goto error;
> > +	}
> > +
> > +	ret = dxgglobal_getiospace(dxgglobal);
> > +	if (ret) {
> > +		pr_err("getiospace failed: %d\n", ret);
> > +		goto error;
> > +	}
> > +
> > +	ret = dxgvmb_send_set_iospace_region(dxgglobal->mmiospace_base,
> > +					     dxgglobal->mmiospace_size, 0);
> > +	if (ISERROR(ret)) {
> > +		pr_err("send_set_iospace_region failed");
> > +		goto error;
>
> You forgot to unwind from the things you initialized above :(
The caller of dxgglobal_init_global_channel() checks the return value 
and calls dxgglobal_destroy_global_channel() in case of an error, which 
does the cleanup. If preferred the call to destroy the channel could be 
moved to the end of this function.
>
> > +	}
> > +
> > +	hv_set_drvdata(hdev, dxgglobal);
> > +
> > +	dxgglobal->dxgdevice.minor = MISC_DYNAMIC_MINOR;
> > +	dxgglobal->dxgdevice.name = "dxg";
> > +	dxgglobal->dxgdevice.fops = &dxgk_fops;
> > +	dxgglobal->dxgdevice.mode = 0666;
> > +	ret = misc_register(&dxgglobal->dxgdevice);
> > +	if (ret) {
> > +		pr_err("misc_register failed: %d", ret);
> > +		goto error;
>
> Again, no cleanups so you leak resources?  Not nice :(
>
>
> > +	}
> > +	dxgglobaldev = dxgglobal->dxgdevice.this_device;
> > +	dxgglobal->device_initialized = true;
> > +
> > +error:
> > +	return ret;
> > +}
> > +
> > +static void dxgglobal_destroy_global_channel(void)
> > +{
> > +	dxglockorder_acquire(DXGLOCK_GLOBAL_CHANNEL);
> > +	down_write(&dxgglobal->channel_lock);
> > +
> > +	TRACE_DEBUG(1, "%s", __func__);
>
> ftrace is your friend :)
I mentioned in other mail that these macros will be removed when we pick 
to final tracing technology for the driver.
>
>
Thank you
Iouri

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Iouri Tarassov]

Hi Pavel,

Thanks for your time reviewing the driver.

On 8/21/2020 6:53 AM, Pavel Machek wrote:
> On Fri 2020-08-14 08:38:53, Sasha Levin wrote:
> > Add support for a Hyper-V based vGPU implementation that exposes the
> > DirectX API to Linux userspace.
>
> NAK.
>
> Kernel APIs should be documented. ... in tree and with suitable license.
We are considering to add required documentation. The design is that the 
driver IOCTLs will not be called directly by user mode drivers or 
applications. They will instead link with libdxcore.so and use the 
D3DKMT* interface to send requests to the driver.  libdxcore will 
translate the request to driver IOCTLs and do some additional work. For 
example, it will translate the path of the user mode driver name to the 
Linux namespace. The D3DKMTinterface is documented on MSDN. Do you 
suggest that the IOCTL interface needs to be documented or the D3DKMT 
interface?
> 	
> > +int dxgadapter_init(struct dxgadapter *adapter, struct hv_device *hdev)
> > +{
> > +	int ret = 0;
> > +	char s[80];
> > +
> > +	UNUSED(s);
>
> What is going on here?
This is a mistake, which will be fixed.
>
> > +{
> > +	struct dxgprocess_adapter *adapter_info = dxgmem_alloc(process,
> > +							       DXGMEM_PROCESS_ADAPTER,
> > +							       sizeof
> > +							       (*adapter_info));
>
> We normally use kernel functions in kernel code.
Using a custom memory allocation function allows us to track memory 
allocations per DXGPROCESS and catch memory leaks when a DXGPROCESS is 
destroyed or when the driver is unloaded. It also allows to easily 
change the memory allocation implementation if needed.
>
> > +	dxgmutex_unlock(&device->adapter_info->device_list_mutex);
>
> And you should not have private locking primitives, either.
This is done to allow the implementation of custom lock order 
verification. We learnt recently about lock dependency checking in 
kernel and will evaluate if it can replace the custom lock order 
verification.
>
> > +bool dxghwqueue_acquire_reference(struct dxghwqueue *hwqueue)
> > +{
> > +	return refcount_inc_not_zero(&hwqueue->refcount);
> > +}
>
> Midlayers are evil.
I strongly agree in general, but think that in our case the layers are 
very small. It allows to quickly find all places where a 
reference/dereference on an object is done and addition of debug tracing 
to catch errors.
>
> Best regards,
> 									Pavel

Thank you
Iouri

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Greg KH]

On Thu, Aug 27, 2020 at 04:29:59PM -0700, Iouri Tarassov wrote:
> On 8/14/2020 5:55 AM, Greg KH wrote:
> > On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> > > Add support for a Hyper-V based vGPU implementation that exposes the
> > > DirectX API to Linux userspace.
> > > > Signed-off-by: Sasha Levin <sashal@kernel.org>
> > 
> > Better, but what is this mess:
> > 
> > > +#define ISERROR(ret)					(ret < 0)
> 
> The VM bus messages return the NTSTATUS error code from the host. NTSTATUS
> is integer and the positive values indicate success.
> The success error code needs to be returned by IOCTLs to the DxCore
> applications. The ISERROR() macro is used in places where the NTSTATUS
> success code could be returned from a function. "if (ret)" cannot be used. I
> will add a comment to the macro in the next patch.

No, please just "open code" this, there is no need for a macro that is
actually more characters than the original test.

> > > +#define DXGKDEBUG 1
> > > +/* #define USEPRINTK 1 */
> > > +
> > > +#ifndef DXGKDEBUG
> > > +#define TRACE_DEBUG(...)
> > > +#define TRACE_DEFINE(...)
> > > +#define TRACE_FUNC_ENTER(...)
> > > +#define TRACE_FUNC_EXIT(...)
> > 
> > No, please do not to custom "trace" printk messages, that is what ftrace
> > is for, no individual driver should ever need to do that.
> > 
> > Just use the normal dev_*() calls for error messages and the like, do
> > not try to come up with a custom tracing framework for one tiny
> > individual driver.  If every driver in kernel did that, we would have a
> > nightmare...
> I understand the concern. This will be fixed later when we learn and pick
> the final tracing technology for the driver.

When is "later"?  We don't want to review something that you do not
think is ready to be merged, do it now so we don't trip over things that
you know you want to fix up.

> The current implementation
> allows the hardware vendors to quickly identify issues without learning
> about ftrace. They just need to enable the WSL debug console and mount
> debugfs.

Hardware vendors who know Linux already know about ftrace, don't make
people have to read up and learn about custom ways to debug
just-your-tiny-individual-driver.  Instead follow the customs and ways
the _WHOLE_ kernel works, that is just polite, don't you think?

thanks,

greg k-h

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Greg KH]

On Thu, Aug 27, 2020 at 05:05:44PM -0700, Iouri Tarassov wrote:
> 
> On 8/14/2020 6:04 AM, Greg KH wrote:
> > On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> > > Add support for a Hyper-V based vGPU implementation that exposes the
> > > DirectX API to Linux userspace.
> > > > Signed-off-by: Sasha Levin <sashal@kernel.org>
> > > ---
> > >  drivers/hv/dxgkrnl/Kconfig      |   10 +
> > >  drivers/hv/dxgkrnl/Makefile     |   12 +
> > >  drivers/hv/dxgkrnl/d3dkmthk.h   | 1636 ++++++++++
> > >  drivers/hv/dxgkrnl/dxgadapter.c | 1406 ++++++++
> > >  drivers/hv/dxgkrnl/dxgkrnl.h    |  927 ++++++
> > >  drivers/hv/dxgkrnl/dxgmodule.c  |  656 ++++
> > >  drivers/hv/dxgkrnl/dxgprocess.c |  357 ++
> > >  drivers/hv/dxgkrnl/dxgvmbus.c   | 3084 ++++++++++++++++++
> > >  drivers/hv/dxgkrnl/dxgvmbus.h   |  873 +++++
> > >  drivers/hv/dxgkrnl/hmgr.c       |  604 ++++
> > >  drivers/hv/dxgkrnl/hmgr.h       |  112 +
> > >  drivers/hv/dxgkrnl/ioctl.c      | 5413 +++++++++++++++++++++++++++++++
> > >  drivers/hv/dxgkrnl/misc.c       |  279 ++
> > >  drivers/hv/dxgkrnl/misc.h       |  309 ++
> > >  14 files changed, 15678 insertions(+)
> > 
> > It's almost impossible to review 15k lines at once, please break this up
> > into reviewable chunks next time.
> 
> Sorry about this, but we had to replace a lot of typedefs, which are not
> allowed by the coding style.

Ok, nice work, but that has nothing to do with how you submit a patch to
us for review.

> We expect one more big patch, which cannot be split in my opinion.

I disagree with that opinion, and so do thousands of other Linux kernel
developers who have done this successfully in the past :)

Remember, it is your job to make this as simple and as easy as possible
for me to review your code, such that it is trivial for me to understand
and accept it.  That takes more work on your side to do this, as we have
thousands of developers, but very few reviewers.  We know we waste
engineering time on this type of thing, but the end result makes for
better reviews and consequentially, better reviews.

So don't ignore this advice, remember, you are wanting me to do
something for you, for free.  Make it easy for me to do so.

> The VM
> vbus message format was changed to include additional header. As the result,
> every function in dxgvmbus.c needs to be changed to handle the new header. I
> do not see how this can be split to multiple patches so each patch produces
> a working driver.

It doesn't have to "work" fully, see many many examples of how to do
this every week submitted to us.  It's not an impossible task at all.

> > > +++ b/drivers/hv/dxgkrnl/Kconfig
> > > @@ -0,0 +1,10 @@
> > > +#
> > > +# dxgkrnl configuration
> > > +#
> > > +
> > > +config DXGKRNL
> > > +	tristate "Microsoft virtual GPU support"
> > > +	depends on HYPERV
> > > +	help
> > > +	  This driver supports Microsoft virtual GPU.
> > > +
> > 
> > You need more text here, this isn't a staging driver submission :)
> Is the the proposed description good enough?

What proposed description?

> "This driver handles paravirtualized GPU devices exposed by Microsoft
> Hyper-V when Linux is running inside of a virtual machine hosted
> by Windows."

That's better, but really, when a tiny serial port driver has more text
than this huge thing, you might want to consider expanding on exactly
what you want people to understand...

> > > +struct d3dkmt_closeadapter {
> > > +	struct d3dkmthandle		adapter_handle;
> > > +};
> > 
> > A "handle"?  And that has to be one of the most difficult structure
> > names ever :)
> > 
> > Why not just use the "handle" for the structure as obviously that's all
> > that is needed here.
> The structure definition matches the Windows D3DKMT interface. Some input
> structures to the interface functions have only one member. But there is
> possibility that new member could be added in the future. We prefer to have
> matching names between Windows and Linux to avoid confusion.

Don't write code because "it might change in the future".  Write code
for what you have today.  If it does change in the future, wonderful, go
and change the code.  You have the full ability to do so then, no need
to hurt all of us today for that potential.

As for "matching names", why does that matter?  Who sees both names at
the same time?

> > > +
> > > +struct d3dkmt_openadapterfromluid {
> > > +	struct winluid			adapter_luid;
> > > +	struct d3dkmthandle		adapter_handle;
> > > +};
> > > +
> > > +struct d3dddi_allocationlist {
> > > +	struct d3dkmthandle		allocation;
> > > +	union {
> > > +		struct {
> > > +			uint		write_operation		:1;
> > > +			uint		do_not_retire_instance	:1;
> > > +			uint		offer_priority		:3;
> > > +			uint		reserved		:27;
> > 
> > endian issues?
> > 
> > If not, why are these bit fields?
> This matches the definition on the Windows side. Windows only works on
> little endian platforms.

But Linux works on both, so you need to properly document/handle this somehow.

> > 
> > > +struct d3dkmt_destroydevice {
> > > +	struct d3dkmthandle		device;
> > > +};
> > 
> > Again, single entity structures?
> > 
> > Are you trying to pass around "handles" and cast them backwards?
> > 
> > If so, great, but then use the real kernel structures for that like
> > 'struct device' if these are actually devices.
> > 
> Again. The structure matches the definition on the Windows side to avoid
> confusion.

Who is confused here?  We accept naming conventions that do not match
the normal Linux style when they are referring to external sources of
the data.  Examples of this are USB device field names, and other
hardware specifications that are public.  You aren't sharing code with a
Windows system, so please follow the Linux coding style rules, as you
want Linux developers to be helping you maintain this code, not
developers who have ever read code from other operating systems.

So please follow the rule of, "unless these fields and structures are
publically defined somewhere, use Linux naming rules", like all of the
rest of us do.

> > > +	ret = dxgglobal_getiospace(dxgglobal);
> > > +	if (ret) {
> > > +		pr_err("getiospace failed: %d\n", ret);
> > > +		goto error;
> > > +	}
> > > +
> > > +	ret = dxgvmb_send_set_iospace_region(dxgglobal->mmiospace_base,
> > > +					     dxgglobal->mmiospace_size, 0);
> > > +	if (ISERROR(ret)) {
> > > +		pr_err("send_set_iospace_region failed");
> > > +		goto error;
> > 
> > You forgot to unwind from the things you initialized above :(
> The caller of dxgglobal_init_global_channel() checks the return value and
> calls dxgglobal_destroy_global_channel() in case of an error, which does the
> cleanup. If preferred the call to destroy the channel could be moved to the
> end of this function.

It is generally a good idea for a function to clean up after itself if
things go wrong as it is almost impossible for a reader of the code, or
automated tools, to determine that these resources are freed up by an
external call later on in the code path.

So yes, please fix this up.

> > > +static void dxgglobal_destroy_global_channel(void)
> > > +{
> > > +	dxglockorder_acquire(DXGLOCK_GLOBAL_CHANNEL);
> > > +	down_write(&dxgglobal->channel_lock);
> > > +
> > > +	TRACE_DEBUG(1, "%s", __func__);
> > 
> > ftrace is your friend :)
> I mentioned in other mail that these macros will be removed when we pick to
> final tracing technology for the driver.

Please pick now, no need to wait :)

thanks,

greg k-h

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Greg KH]

On Thu, Aug 27, 2020 at 04:45:55PM -0700, Iouri Tarassov wrote:
> 
> On 8/14/2020 5:57 AM, Greg KH wrote:
> > On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> > > Add support for a Hyper-V based vGPU implementation that exposes the
> > > DirectX API to Linux userspace.
> > 
> > Api questions:
> > 
> > > +struct d3dkmthandle {
> > > +	union {
> > > +		struct {
> > > +			u32 instance	:  6;
> > > +			u32 index	: 24;
> > > +			u32 unique	: 2;
> > 
> > What is the endian of this?
> > 
> > > +		};
> > > +		u32 v;
> > > +	};
> > > +};
> > > +
> > > +extern const struct d3dkmthandle zerohandle;
> > > +
> The definition is the same as on the Windows side. The driver communicates
> with a Windows host, so I do not expect any issues with endiannes. Windows
> currently runs only on the little endian platforms.

As I mentioned before, you need to document that somewhere (like maybe
preventing your code from being built on big endian systems?)

> User mode applications see this as an opaque 32 bit value (D3DKMTHANDLE). I
> prefer not to use the u32 definition to avoid mistakes when another integer
> or a 64-bit handle is assigned to the handle or  the handle is assigned to a
> 64 or 32 bit integer variable. There are many handles in the driver model
> (shared NT handle, d3dkmthandle, etc). Using a specific type allows to avoid
> assigning one handle to another.

Specific types are great, that is fine.

> > > +struct ntstatus {
> > > +	union {
> > > +		struct {
> > > +			int code	: 16;
> > > +			int facility	: 13;
> > > +			int customer	: 1;
> > > +			int severity	: 2;
> > 
> > Same here.
> > 
> > Are these things that cross the user/kernel boundry?
> > 
> > And why int on one and u32 on the other?
> > 
> > > +		};
> > > +		int v;
> > > +	};
> > > +};
> > > +
> "struct ntstatus" follows the definition for NTSTATUS on the Windows side.
> NTSTATUS is an integer where the negative values indicate errors. It is
> success otherwise. NTSTATUS is returned by the VM bus messages from host.
> IOCTLs from the driver return Linux negative error code or NTSTATUS positive
> success codes. DxCore applications expect certain positive success codes.
> DxCore is a shared library, which translates the D3DKMT* Windows interface
> to Linux ioctls. Applications link with DxCore to use a paravirtualized GPU.
> D3DKMTHANDLE is a 32-bit unsigned value (bitfield), not an integer.

Ok, again, please document this, and as these fields are crossing the
kernel/user boundry, use the correct types (hint, 'int' is never that
type).

> > > +struct winluid {
> > > +	uint a;
> > > +	uint b;
> > 
> > And now uint?  Come on, be consistent please :)
> Sorry about this. This came from the Windows size where we use UINT a lot.
> All uints will be replaced by u32 in the next patch set.

thank you.

greg k-h

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Greg KH]

On Thu, Aug 27, 2020 at 05:25:23PM -0700, Iouri Tarassov wrote:
> > > +{
> > > +	struct dxgprocess_adapter *adapter_info = dxgmem_alloc(process,
> > > +							       DXGMEM_PROCESS_ADAPTER,
> > > +							       sizeof
> > > +							       (*adapter_info));
> > 
> > We normally use kernel functions in kernel code.
> Using a custom memory allocation function allows us to track memory
> allocations per DXGPROCESS and catch memory leaks when a DXGPROCESS is
> destroyed or when the driver is unloaded. It also allows to easily change
> the memory allocation implementation if needed.

There is only one "memory allocation implementation" in the kernel,
please use that and not any wrapper functions.  You wouldn't want to see
1000's of different memory allocation functions, each driver having a
unique one, right?

Remember, your code is becoming part of the larger kernel, so follow the
guidelines and rules of it.  There is nothing different from your code
and a serial port driver when it comes to these expectations.

thanks,

greg k-h

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Greg KH]

On Thu, Aug 27, 2020 at 05:25:23PM -0700, Iouri Tarassov wrote:
> > > +bool dxghwqueue_acquire_reference(struct dxghwqueue *hwqueue)
> > > +{
> > > +	return refcount_inc_not_zero(&hwqueue->refcount);
> > > +}
> > 
> > Midlayers are evil.
> I strongly agree in general, but think that in our case the layers are very
> small. It allows to quickly find all places where a reference/dereference on
> an object is done and addition of debug tracing to catch errors.

Again, no, please remove all layers like this.  They just make it
impossible for others to review and understand the code over time.

Also, in this specific case, it would have allowed me to easily realize
that you are doing this type of call incorrectly and should be using a
different data structure :)

thanks,

greg k-h

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Iouri Tarassov]

Hi Greg,

I appreciate your comments and working to address them.

On 8/27/2020 11:12 PM, Greg KH wrote:
> As for "matching names", why does that matter?  Who sees both names at
> the same time?
>
> > > 
> > > endian issues?
> > > 
> > > If not, why are these bit fields?
> > This matches the definition on the Windows side. Windows only works on
> > little endian platforms.
>
> But Linux works on both, so you need to properly document/handle this somehow.
This driver works only in a Linux container in conjunction with the 
Windows host. The structure definitions are  the same on the host and 
the container. The driver will not be enabled or work on platforms, 
where Windows does not run.
>
> > > 
> > > > +struct d3dkmt_destroydevice {
> > > > +	struct d3dkmthandle		device;
> > > > +};
> > > 
> > > Again, single entity structures?
> > > 
> > > Are you trying to pass around "handles" and cast them backwards?
> > > 
> > > If so, great, but then use the real kernel structures for that like
> > > 'struct device' if these are actually devices.
> > > 
> > Again. The structure matches the definition on the Windows side to avoid
> > confusion.
>
> Who is confused here?  We accept naming conventions that do not match
> the normal Linux style when they are referring to external sources of
> the data.  Examples of this are USB device field names, and other
> hardware specifications that are public.  You aren't sharing code with a
> Windows system, so please follow the Linux coding style rules, as you
> want Linux developers to be helping you maintain this code, not
> developers who have ever read code from other operating systems.
>
> So please follow the rule of, "unless these fields and structures are
> publically defined somewhere, use Linux naming rules", like all of the
> rest of us do.
>
The d3dkmt* structures, like d3dkmt_destroydevice are publicly 
documented on MSDN 
(https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/d3dkmthk/ns-d3dkmthk-_d3dkmt_destroydevice). 
I am using the same definitions in the driver, so it is easy to find the 
corresponding definition and description of the structure. I have no 
problem to change the names, but I think using the same public 
definition will help the driver maintainers.

Thanks

Iouri

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Greg KH]

On Thu, Sep 03, 2020 at 11:55:16AM -0700, Iouri Tarassov wrote:
> Hi Greg,
> 
> I appreciate your comments and working to address them.
> 
> On 8/27/2020 11:12 PM, Greg KH wrote:
> > As for "matching names", why does that matter?  Who sees both names at
> > the same time?
> > 
> > > > > > endian issues?
> > > > > > If not, why are these bit fields?
> > > This matches the definition on the Windows side. Windows only works on
> > > little endian platforms.
> > 
> > But Linux works on both, so you need to properly document/handle this somehow.
> This driver works only in a Linux container in conjunction with the Windows
> host. The structure definitions are  the same on the host and the container.
> The driver will not be enabled or work on platforms, where Windows does not
> run.

That's fine, you can create your structures in a way that works no
matter what endian is in use, in very simple ways.  Don't rely on
bit fields like this in a structure to actually work the way you think
they work (hint, compilers hate them and do horrible things with them
usually...)

So do it that way please, especially for when you are passing things
across the user/kernel boundry.  It's much simpler and easier to do it
right now, than to have to fix it up later.

thanks,

greg k-h

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Iouri Tarassov]

Hi Greg,

On 8/27/2020 11:18 PM, Greg KH wrote:
> On Thu, Aug 27, 2020 at 05:25:23PM -0700, Iouri Tarassov wrote:
> > > > +bool dxghwqueue_acquire_reference(struct dxghwqueue *hwqueue)
> > > > +{
> > > > +	return refcount_inc_not_zero(&hwqueue->refcount);
> > > > +}
> > > 
> > > Midlayers are evil.
> > I strongly agree in general, but think that in our case the layers are very
> > small. It allows to quickly find all places where a reference/dereference on
> > an object is done and addition of debug tracing to catch errors.
>
> Again, no, please remove all layers like this.  They just make it
> impossible for others to review and understand the code over time.
>
> Also, in this specific case, it would have allowed me to easily realize
> that you are doing this type of call incorrectly and should be using a
> different data structure :)

Are you suggesting that the current code is incorrect? Could you comment 
what changes need to be done?

Thanks
Iouri

Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

[Greg KH]

On Thu, Sep 03, 2020 at 02:39:05PM -0700, Iouri Tarassov wrote:
> Hi Greg,
> 
> On 8/27/2020 11:18 PM, Greg KH wrote:
> > On Thu, Aug 27, 2020 at 05:25:23PM -0700, Iouri Tarassov wrote:
> > > > > +bool dxghwqueue_acquire_reference(struct dxghwqueue *hwqueue)
> > > > > +{
> > > > > +	return refcount_inc_not_zero(&hwqueue->refcount);
> > > > > +}
> > > > > > Midlayers are evil.
> > > I strongly agree in general, but think that in our case the layers are very
> > > small. It allows to quickly find all places where a reference/dereference on
> > > an object is done and addition of debug tracing to catch errors.
> > 
> > Again, no, please remove all layers like this.  They just make it
> > impossible for others to review and understand the code over time.
> > 
> > Also, in this specific case, it would have allowed me to easily realize
> > that you are doing this type of call incorrectly and should be using a
> > different data structure :)
> 
> Are you suggesting that the current code is incorrect? Could you comment
> what changes need to be done?

You should be using the built-in reference counting object (a kref) and
not trying to roll your own as you did here.  That way we "know" you got
the logic right and do not have to audit your codebase to prove that
your hand-made one is correct.

thanks,

greg k-h