Message-ID: <ZRPiGk9M3aQr99Y5@liuwe-devbox-debian-v2>
Date:   Wed, 27 Sep 2023 08:04:42 +0000
From:   Wei Liu <wei.liu@...nel.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Nuno Das Neves <nunodasneves@...ux.microsoft.com>,
        Wei Liu <wei.liu@...nel.org>, linux-hyperv@...r.kernel.org,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        linux-arm-kernel@...ts.infradead.org, linux-arch@...r.kernel.org,
        patches@...ts.linux.dev, mikelley@...rosoft.com, kys@...rosoft.com,
        haiyangz@...rosoft.com, decui@...rosoft.com,
        apais@...ux.microsoft.com, Tianyu.Lan@...rosoft.com,
        ssengar@...ux.microsoft.com, mukeshrathor@...rosoft.com,
        stanislav.kinsburskiy@...il.com, jinankjain@...ux.microsoft.com,
        vkuznets@...hat.com, tglx@...utronix.de, mingo@...hat.com,
        bp@...en8.de, dave.hansen@...ux.intel.com, hpa@...or.com,
        will@...nel.org, catalin.marinas@....com
Subject: Re: [PATCH v3 15/15] Drivers: hv: Add modules to expose /dev/mshv to
 VMMs running on Hyper-V

Hi Greg

It is past midnight here, so I may not be able to articulate my thoughts
very well. I want to avoid waiting for another day for another round
trip of emails though. We can look at your reply in the morning and
reply again.

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.

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

I hope this is clear. If we're missing something extremely obvious, that
somehow we can get a reference to struct device from the VFS while
opening the file or doing ioctl without stashing it ourselves in our own
code, please let us know.

At this point I feel like either I'm extremely stupid or we're just
talking past each other. If you tell me it is the former and help me
understand how we can achieve what you described, I am more than happy
to learn new things I don't know or understand. :-)

If we have to propagate that reference ourselves, that then leads to
next question whether it will just be more convenient to use the stashed
value in the static variable directly like other drivers do, instead of
stashing and propagating it around, knowing 100% it is the same object.
I don't feel too strongly about this. As long as we are on the same page
about getting a reference to struct device, we can do it either way.

Thanks,
Wei.


> thanks,
> 
> greg k-h
> 

Powered by blists - more mailing lists