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

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: <20220302203440.inzt6z6mxhs56wab@liuwe-devbox-debian-v2>
Date:   Wed, 2 Mar 2022 20:34:40 +0000
From:   Wei Liu <wei.liu@...nel.org>
To:     Iouri Tarassov <iourit@...ux.microsoft.com>
Cc:     Wei Liu <wei.liu@...nel.org>, Greg KH <gregkh@...uxfoundation.org>,
        kys@...rosoft.com, haiyangz@...rosoft.com, sthemmin@...rosoft.com,
        linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org,
        spronovo@...rosoft.com, spronovo@...ux.microsoft.com
Subject: Re: [PATCH v3 02/30] drivers: hv: dxgkrnl: Driver initialization and
 loading

On Wed, Mar 02, 2022 at 10:49:15AM -0800, Iouri Tarassov wrote:
> On 3/2/2022 3:53 AM, Wei Liu wrote:
> > On Wed, Mar 02, 2022 at 08:53:15AM +0100, Greg KH wrote:
> > > On Tue, Mar 01, 2022 at 10:23:21PM +0000, Wei Liu wrote:
> > > > > > +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.
> > > 
> > > 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.
> 
> 
> I can certainly remove dxgglobal and keep the  pointer to the global
> state in the device object.
> 

No, no more global pointer needed. You just call dev_drv_setdata in the
place that you assign to the global pointer.

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

And in the place you need the global pointer, call dev_drv_getdata.

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

No. You still have only one /dev/dxg here.

Wei.