linux-kernel - Re: [PATCH v2 01/24] drivers: hv: dxgkrnl: Driver initialization and creation of dxgadapter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5493bb21-7c85-9a8a-07f6-983d1d5c425b@linux.microsoft.com>
Date:   Tue, 8 Feb 2022 10:24:26 -0800
From:   Iouri Tarassov <iourit@...ux.microsoft.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     kys@...rosoft.com, haiyangz@...rosoft.com, sthemmin@...rosoft.com,
        wei.liu@...nel.org, linux-hyperv@...r.kernel.org,
        linux-kernel@...r.kernel.org, spronovo@...rosoft.com
Subject: Re: [PATCH v2 01/24] drivers: hv: dxgkrnl: Driver initialization and
 creation of dxgadapter

Hi Greg,

On 2/7/2022 11:20 PM, Greg KH wrote:
> On Mon, Feb 07, 2022 at 10:59:25AM -0800, Iouri Tarassov wrote:
> > 
> > On 2/5/2022 12:25 AM, Greg KH wrote:
> > > On Fri, Feb 04, 2022 at 06:33:59PM -0800, Iouri Tarassov wrote:
> > > > This is the first commit for adding support for a Hyper-V based
> > > > vGPU implementation that exposes the DirectX API to Linux userspace.
> > > >
> > > 
> > > Only add the interfaces for the changes that you need in this commit.
> > > Do not add them all and then use them later, that makes it impossible to
> > > review.
> > > 
> > > > ---
> > > >  MAINTAINERS                     |    7 +
> > > >  drivers/hv/Kconfig              |    2 +
> > > >  drivers/hv/Makefile             |    1 +
> > > >  drivers/hv/dxgkrnl/Kconfig      |   26 +
> > > >  drivers/hv/dxgkrnl/Makefile     |    5 +
> > > >  drivers/hv/dxgkrnl/dxgadapter.c |  172 +++
> > > >  drivers/hv/dxgkrnl/dxgkrnl.h    |  223 ++++
> > > >  drivers/hv/dxgkrnl/dxgmodule.c  |  736 ++++++++++++
> > > >  drivers/hv/dxgkrnl/dxgprocess.c |   17 +
> > > >  drivers/hv/dxgkrnl/dxgvmbus.c   |  578 +++++++++
> > > >  drivers/hv/dxgkrnl/dxgvmbus.h   |  855 ++++++++++++++
> > > >  drivers/hv/dxgkrnl/hmgr.c       |   23 +
> > > >  drivers/hv/dxgkrnl/hmgr.h       |   75 ++
> > > >  drivers/hv/dxgkrnl/ioctl.c      |   24 +
> > > >  drivers/hv/dxgkrnl/misc.c       |   37 +
> > > >  drivers/hv/dxgkrnl/misc.h       |   89 ++
> > > >  include/linux/hyperv.h          |   16 +
> > > >  include/uapi/misc/d3dkmthk.h    | 1945 +++++++++++++++++++++++++++++++
> > > >  18 files changed, 4831 insertions(+)
> > > 
> > > Would you want to review a 4800 line patch all at once?
> > > 
> > > greg k-h
> > 
> > Hi Greg,
> > 
> > Thank you for reviewing. I appreciate your time.
> > 
> > 1. d3dkmthk.h defines the user mode interface structures. This is ported
> > from
> >  the windows header at once. Is it acceptable to add it at it is?
>
> No, again, would you want to be presented with code that is not used at
> all?  How would you want this to look if you had to review this?

Could you recommend a similar in size driver to look how it was first 
submitted?

I looked at the Habanalabs driver submission, which was signed off by you.

The commit 1ea2a20e91a4d0543a933b4df706c2585db7e3ae adds 94 header 
files, without using the definitions.

     habanalabs: add Goya registers header files
     This patch just adds a lot of header files that contain description of
     Goya's registers.
     Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>

How did you review this? I do not see much difference between defining 
an interface to a virtual device and
defining an interface to a hardware device.

d3dkmthk.h defines a binary interface to the compute driver. It cannot 
be changed, because it must
be binary compatible with the Windows display graphics model.
In my opinion the only thing to review here is the usage of the correct 
Linux types and coding style.
I can submit the file in a dedicated patch.

> > 2. dxgvmbus.h defines the VM bus interface between the linux guest and the
> > host.
> > It was ported from the windows version at once. Is it acceptable to add it
> > as it is?
>
> Again, no.

The same here.
dxgvmbus.h defines the binary VM bus interface between the host and guest.
It must be compatible with the existing interface.It cannot be changed.
In my opinion the only thing to review here is the usage of the correct 
Linux types and coding style.
I can submit the file in a dedicated patch.

What are you looking to review in these interface? I am trying to avoid 
unnecessary work, but will do it
if it really helps during review.

Thanks a lot,
Iouri