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]

Date:   Tue, 8 Feb 2022 08:20:44 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Iouri Tarassov <iourit@...ux.microsoft.com>
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

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.
> 
> I am trying to find compromise between the number of patches and making
> review easy. There are about 70 IOCTLs in the driver interface, so having a
> patch
> for every IOCTL seems excessive.
> 
> I tried to add only definitions for the internal objects, which are used in
> the patch.
> 
> 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?

> 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.

> 3. Is it acceptable to combine logically connected IOCTLs to a single patch?
> For example, IOCTLs for creation/destruction sync object and submission of
> wait/signal operations.

Yes, that makes sense.

Again, what would you want here if you had to review all of this?

I suggest stopping and taking some time and start reviewing code on the
mailing lists first.  Look at how others are doing this for large new
features, and offer up your review to those changes.  That will give you
the experience for how to do it yourself.  To expect to do this all
correct the first time without ever being on the other side of the
process very difficult.

thanks,

greg k-h