> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Saturday, September 3, 2022 12:25 AM
>
> On Fri, Sep 02, 2022 at 08:39:34AM -0600, Alex Williamson wrote:
> > On Fri, 2 Sep 2022 03:51:01 +0000
> > "Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote:
> >
> > > > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > > Sent: Friday, September 2, 2022 8:29 AM
> > > >
> > > > On Wed, Aug 31, 2022 at 08:46:30AM +0000, Tian, Kevin wrote:
> > > > > > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > > > > Sent: Wednesday, August 31, 2022 9:02 AM
> > > > > >
> > > > > > To vfio_container_detatch_group(). This function is really a
> container
> > > > > > function.
> > > > > >
> > > > > > Fold the WARN_ON() into it as a precondition assertion.
> > > > > >
> > > > > > A following patch will move the vfio_container functions to their
> own .c
> > > > > > file.
> > > > > >
> > > > > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > > > > ---
> > > > > > drivers/vfio/vfio_main.c | 11 +++++------
> > > > > > 1 file changed, 5 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> > > > > > index bfa6119ba47337..e145c87f208f3a 100644
> > > > > > --- a/drivers/vfio/vfio_main.c
> > > > > > +++ b/drivers/vfio/vfio_main.c
> > > > > > @@ -928,12 +928,13 @@ static const struct file_operations
> vfio_fops = {
> > > > > > /*
> > > > > > * VFIO Group fd, /dev/vfio/$GROUP
> > > > > > */
> > > > > > -static void __vfio_group_unset_container(struct vfio_group *group)
> > > > > > +static void vfio_container_detatch_group(struct vfio_group *group)
> > > > >
> > > > > s/detatch/detach/
> > > >
> > > > Oops
> > > >
> > > > > Given it's a vfio_container function is it better to have a container
> pointer
> > > > > as the first parameter, i.e.:
> > > > >
> > > > > static void vfio_container_detatch_group(struct vfio_container
> *container,
> > > > > struct vfio_group *group)
> > > >
> > > > Ah, I'm not so sure, it seems weird to make the caller do
> > > > group->container then pass the group in as well.
> > > >
> > > > This call assumes the container is the container of the group, it
> > > > doesn't work in situations where that isn't true.
> > >
> > > Yes, this is a valid interpretation on doing this way.
> > >
> > > Other places e.g. iommu_detach_group(domain, group) go the other way
> > > even if internally domain is not used at all. I kind of leave that pattern
> > > in mind thus raised this comment. But not a strong opinion.
> > >
> > > >
> > > > It is kind of weird layering in a way, arguably we should have the
> > > > current group stored in the container if we want things to work that
> > > > way, but that is a big change and not that wortwhile I think.
> > > >
> > > > Patch 7 is pretty much the same, it doesn't work right unless the
> > > > container and device are already matched
> > > >
> > >
> > > If Alex won't have a different preference and with the typo fixed,
> >
> > I don't get it, if a group is detaching itself from a container, why
> > isn't it vfio_group_detach_container(). Given our naming scheme,
> > vfio_container_detach_group() absolutely implies the args Kevin
> > suggests, even if they're redundant. vfio_foo_* functions should
> > operate on a a vfio_foo object.
>
> Look at the overall picture. This series moves struct vfio_container
> into a .c file and then pulls all of the code that relies on it into
> the c file too.
>
> With our current function layout that results in these cut points:
>
> struct vfio_container *vfio_container_from_file(struct file *filep);
> int vfio_container_use(struct vfio_group *group);
> void vfio_container_unuse(struct vfio_group *group);
> int vfio_container_attach_group(struct vfio_container *container,
> struct vfio_group *group);
> void vfio_container_detach_group(struct vfio_group *group);
> void vfio_container_register_device(struct vfio_device *device);
> void vfio_container_unregister_device(struct vfio_device *device);
> long vfio_container_ioctl_check_extension(struct vfio_container *container,
> unsigned long arg);
> int vfio_container_pin_pages(struct vfio_device *device, dma_addr_t iova,
> int npage, int prot, struct page **pages);
> void vfio_container_unpin_pages(struct vfio_device *device, dma_addr_t
> iova,
> int npage);
> int vfio_container_dma_rw(struct vfio_device *device, dma_addr_t iova,
> void *data, size_t len, bool write);
>
> Notice almost none of them use container as a function argument. The
> container is always implied by another object that is linked to the
> container.
>
> Yet these are undeniably all container functions because they are only
> necessary when the container code is actually being used. Every one of
> this functions is bypassed if iommmufd is used. I even have a patch
> that just replaces them all with nops if container and type 1 is
> compiled out.
>
> So, this presents two possiblities for naming. Either we call
> everything in container.c vfio_container because it is primarily
> related to the container.c *system*, or we label each function
> according to OOP via the first argument type (vfio_XX_YY_container
> perhaps) and still place them in container.c.
>
> Keep in mind I have plans to see a group.c and rename vfio_main.c to
> device.c, so having a vfio_group or vfio_device function in
> container.c seems to get confusing.
IMHO I don't see a big difference between two naming options if the
first parameter is kept as group or device object.
>
> In other words, I prefer to think of the group of functions above as
> the exported API of the vfio container system (ie container.c) - not in
> terms of an OOP modeling of a vfio_container object.
>
After reading above IMHO the OOP modeling wins a bit as far as
readability is concerned. Probably just my personal preference
but having most vfio_maintaier_xxx() helpers w/o explicitly providing
a vfio_maintainer object does affect my reading of related code.
At least vfio_XX_YY_container makes me feel better if we want to
avoid duplicating a vfio_maintainer object, e.g.:
struct vfio_container *vfio_container_from_file(struct file *filep);
int vfio_group_use_container(struct vfio_group *group);
void vfio_group_unuse_container(struct vfio_group *group);
int vfio_group_attach_container(struct vfio_group *group,
vfio_container *container);
void vfio_group_detach_container(struct vfio_group *group);
void vfio_device_register_container(struct vfio_device *device);
void vfio_device_unregister_container(struct vfio_device *device);
long vfio_container_ioctl_check_extension(struct vfio_container *container,
unsigned long arg);
int vfio_pin_pages_container(struct vfio_device *device, dma_addr_t iova,
int npage, int prot, struct page **pages);
void vfio_unpin_pages_container(struct vfio_device *device, dma_addr_t
iova,
int npage);
int vfio_dma_rw_container(struct vfio_device *device, dma_addr_t iova,
void *data, size_t len, bool write);
They are all container related. So although the first parameter is not
container we put them in container.c as the last word in the function
name is container.
Thanks
Kevin
