Re: [PATCH v3 15/15] Drivers: hv: Add modules to expose /dev/mshv to VMMs running on Hyper-V

[Greg KH]

On Wed, Sep 27, 2023 at 08:04:42AM +0000, Wei Liu wrote:
> On Wed, Sep 27, 2023 at 08:01:01AM +0200, Greg KH wrote:
> [...]
> > > > > If we're working with real devices like network cards or graphics cards
> > > > > I would agree -- it is easy to imagine that we have several cards of the
> > > > > same model in the system -- but in real world there won't be two
> > > > > hypervisor instances running on the same hardware.
> > > > > 
> > > > > We can stash the struct device inside some private data fields, but that
> > > > > doesn't change the fact that we're still having one instance of the
> > > > > structure. Is this what you want? Or do you have something else in mind?
> > > > 
> > > > You have a real device, it's how userspace interacts with your
> > > > subsystem.  Please use that, it is dynamically created and handled and
> > > > is the correct representation here.
> > > > 
> > > 
> > > Are you referring to the struct device we get from calling
> > > misc_register?
> > 
> > Yes.
> > 
> 
> We know about this, please see below. And we plan to use this.
> 
> > > How would you suggest we get a reference to that device via e.g. open()
> > > or ioctl() without keeping a global reference to it?
> > 
> > You explicitly have it in your open() and ioctl() call, you never need a
> > global reference for it the kernel gives it to you!
> > 
> 
> This is what I don't follow.
> 
> Nuno and I discussed this today offline. We looked at the code before
> and looked again today (well, yesterday now).
> 
> Here are the two functions:
> 
>     int vfs_open(const struct path *path, struct file *file)
>     long vfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> 
> Or, if we provide an open function in our file_operations struct, we get
> an additional struct inode pointer.
> 
>     int (*open) (struct inode *, struct file *);
> 
> Neither struct file nor struct inode contains a reference to struct device.
> 
> Then in vfs.rst, there is a section about open:
> 
> ``open``
>         called by the VFS when an inode should be opened.  When the VFS
>         opens a file, it creates a new "struct file".  It then calls the
>         open method for the newly allocated file structure.  You might
>         think that the open method really belongs in "struct
>         inode_operations", and you may be right.  I think it's done the
>         way it is because it makes filesystems simpler to implement.
>         The open() method is a good place to initialize the
>         "private_data" member in the file structure if you want to point
>         to a device structure
> 
> So, the driver is supposed to stash a pointer to struct device in
> private_data. That's what I alluded to in my previous reply. The core
> driver framework or the VFS doesn't give us a reference to struct
> device. We have to do it ourselves.

Please read Linux Device Drivers, 3rd edition, chapter 3, for how to do
this properly.  The book is free online.

Also look at the zillion in-kernel example drivers that use the misc
device api, container_of() is your friend...

> We can do that for sure, but the struct device we stash into
> private_data is going to be the one that is returned from misc_register,
> which at the same time is already stashed inside a static variable in
> our driver by our own code (Note that this is a pervasive pattern in the
> kernel).

Again, don't make this static, there's no requirement to do so.

But even if you do, sure, use it this way, you have a device.  But I
would strongly discourage you from having a static variable, there is no
need to do this at all, and no one else should do so either.

thanks,

greg k-h