RE: [RFC 02/20] vfio: Add device class for /dev/vfio/devices

[Liu, Yi L]

Hi Jason,

> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Monday, October 25, 2021 8:53 PM
> 
> On Mon, Oct 25, 2021 at 06:28:09AM +0000, Liu, Yi L wrote:
> >    thanks for the guiding. will also refer to your vfio_group_cdev series.
> >
> >    Need to double confirm here. Not quite following on the kfree. Is
> >    this kfree to free the vfio_device structure? But now the
> >    vfio_device pointer is provided by callers (e.g. vfio-pci). Do
> >    you want to let vfio core allocate the vfio_device struct and
> >    return the pointer to callers?
> 
> There are several common patterns for this problem, two that would be
> suitable:
> 
> - Require each driver to provide a release op inside vfio_device_ops
>   that does the kfree. Have the core provide a struct device release
>   op that calls this one. Keep the kalloc/kfree in the drivers

this way sees to suit the existing vfio registration manner listed
below. right? But device drivers needs to do the kfree in the
newly added release op instead of doing it on their own (e.g.
doing kfree in remove).

vfio_init_group_dev()
vfio_register_group_dev()
vfio_unregister_group_dev()
vfio_uninit_group_dev()

> - Move the kalloc into the core and have the core provide the kfree
>   with an optional release callback for anydriver specific cleanup
> 
>   This requires some macro to make the memory layout work. RDMA has
>   a version of this:
> 
> struct ib_device *_ib_alloc_device(size_t size);
> #define ib_alloc_device(drv_struct, member)                                    \
>         container_of(_ib_alloc_device(sizeof(struct drv_struct) +              \
>                                       BUILD_BUG_ON_ZERO(offsetof(              \
>                                               struct drv_struct, member))),    \
>                      struct drv_struct, member)
> 

thanks for the example. If this way, still requires driver to provide
a release op inside vfio_device_ops. right?

> In part the choice is how many drivers require a release callback
> anyhow, if they all do then the first is easier to understand. If only
> few or none do then the latter is less code in drivers, and never
> exposes the driver to the tricky transition from alloc to refcount
> cleanup.

I'm not quite sure. But per my understanding, since the vfio_device
is expected to be embedded in the device state struct (e.g.
vfio_pci_core_device), I guess most of the drivers will require callback
to do driver specific cleanup. Seems like option #1 may make sense?

Regards,
Yi Liu