> From: Ethan Zhao <haifeng.zhao@xxxxxxxxxxxxxxx> > Sent: Friday, September 9, 2022 4:24 PM > > Hi, Kevin, > > 在 2022/9/9 18:22, Kevin Tian 写道: > > The idea is to let vfio core manage the vfio_device life cycle instead > > of duplicating the logic cross drivers. This is also a preparatory > > step for adding struct device into vfio_device. > > > > New pair of helpers together with a kref in vfio_device: > > > > - vfio_alloc_device() > > - vfio_put_device() > > To be honest, this pair of functions make me confusing to understand their > > behaviour from wording point of view: > > - vfio_alloc_device(), Okay, it allocates the vfio device, no reference > count thing. but, I think it's quite common to have an alloc() helper initialize refcount, e.g. vfio_group_alloc() both initialize its user refcount and also call device_initialize() to gets kref initialized. Similar example in ib_alloc_device(), etc. > - vfio_put_device() > seems it will decrease reference count and then if it is zero, free it. > so they are not of one *pair* about wording. > > How about > > - vfio_alloc_device() / - vfio_free_device() > or > - vfio_get_device() / - vfio_put_device(), perhaps not match their behviour > in following code. > > > > Thanks, > Ethan > > > > > > Drivers can register @init/@release callbacks to manage any private > > state wrapping the vfio_device. > > > > However vfio-ccw doesn't fit this model due to a life cycle mess > > that its private structure mixes both parent and mdev info hence must > > be allocated/freed outside of the life cycle of vfio device. > > > > Per prior discussions this won't be fixed in short term by IBM folks. > > > > Instead of waiting for those modifications introduce another helper > > vfio_init_device() so ccw can call it to initialize a pre-allocated > > vfio_device. > > > > Further implication of the ccw trick is that vfio_device cannot be > > freed uniformly in vfio core. Instead, require *EVERY* driver to > > implement @release and free vfio_device inside. Then ccw can choose > > to delay the free at its own discretion. > > > > Another trick down the road is that kvzalloc() is used to accommodate > > the need of gvt which uses vzalloc() while all others use kzalloc(). > > So drivers should call a helper vfio_free_device() to free the > > vfio_device instead of assuming that kfree() or vfree() is appliable. > > > > Later once the ccw mess is fixed we can remove those tricks and > > fully handle structure alloc/free in vfio core. > > > > Existing vfio_{un}init_group_dev() will be deprecated after all > > existing usages are converted to the new model. > > > > Suggested-by: Jason Gunthorpe <jgg@xxxxxxxxxx> > > Co-developed-by: Yi Liu <yi.l.liu@xxxxxxxxx> > > Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx> > > Signed-off-by: Kevin Tian <kevin.tian@xxxxxxxxx> > > Reviewed-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx> > > Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx> > > Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx> > > --- > > drivers/vfio/vfio_main.c | 92 > ++++++++++++++++++++++++++++++++++++++++ > > include/linux/vfio.h | 25 ++++++++++- > > 2 files changed, 116 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c > > index 27d9186f35d5..adc1b697bb78 100644 > > --- a/drivers/vfio/vfio_main.c > > +++ b/drivers/vfio/vfio_main.c > > @@ -498,6 +498,98 @@ void vfio_uninit_group_dev(struct vfio_device > *device) > > } > > EXPORT_SYMBOL_GPL(vfio_uninit_group_dev); > > > > +/* Release helper called by vfio_put_device() */ > > +void vfio_device_release(struct kref *kref) > > +{ > > + struct vfio_device *device = > > + container_of(kref, struct vfio_device, kref); > > + > > + vfio_uninit_group_dev(device); > > + > > + /* > > + * kvfree() cannot be done here due to a life cycle mess in > > + * vfio-ccw. Before the ccw part is fixed all drivers are > > + * required to support @release and call vfio_free_device() > > + * from there. > > + */ > > + device->ops->release(device); > > +} > > +EXPORT_SYMBOL_GPL(vfio_device_release); > > + > > +/* > > + * Alloc and initialize vfio_device so it can be registered to vfio > > + * core. > > + * > > + * Drivers should use the wrapper vfio_alloc_device() for allocation. > > + * @size is the size of the structure to be allocated, including any > > + * private data used by the driver. > > + * > > + * Driver may provide an @init callback to cover device private data. > > + * > > + * Use vfio_put_device() to release the structure after success return. > > + */ > > +struct vfio_device *_vfio_alloc_device(size_t size, struct device *dev, > > + const struct vfio_device_ops *ops) > > +{ > > + struct vfio_device *device; > > + int ret; > > + > > + if (WARN_ON(size < sizeof(struct vfio_device))) > > + return ERR_PTR(-EINVAL); > > + > > + device = kvzalloc(size, GFP_KERNEL); > > + if (!device) > > + return ERR_PTR(-ENOMEM); > > + > > + ret = vfio_init_device(device, dev, ops); > > + if (ret) > > + goto out_free; > > + return device; > > + > > +out_free: > > + kvfree(device); > > + return ERR_PTR(ret); > > +} > > +EXPORT_SYMBOL_GPL(_vfio_alloc_device); > > + > > +/* > > + * Initialize a vfio_device so it can be registered to vfio core. > > + * > > + * Only vfio-ccw driver should call this interface. > > + */ > > +int vfio_init_device(struct vfio_device *device, struct device *dev, > > + const struct vfio_device_ops *ops) > > +{ > > + int ret; > > + > > + vfio_init_group_dev(device, dev, ops); > > + > > + if (ops->init) { > > + ret = ops->init(device); > > + if (ret) > > + goto out_uninit; > > + } > > + > > + kref_init(&device->kref); > > + return 0; > > + > > +out_uninit: > > + vfio_uninit_group_dev(device); > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(vfio_init_device); > > + > > +/* > > + * The helper called by driver @release callback to free the device > > + * structure. Drivers which don't have private data to clean can > > + * simply use this helper as its @release. > > + */ > > +void vfio_free_device(struct vfio_device *device) > > +{ > > + kvfree(device); > > +} > > +EXPORT_SYMBOL_GPL(vfio_free_device); > > + > > static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev, > > enum vfio_group_type type) > > { > > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > > index 0e2826559091..f67cac700e6f 100644 > > --- a/include/linux/vfio.h > > +++ b/include/linux/vfio.h > > @@ -47,7 +47,8 @@ struct vfio_device { > > struct kvm *kvm; > > > > /* Members below here are private, not for driver use */ > > - refcount_t refcount; > > + struct kref kref; /* object life cycle */ > > + refcount_t refcount; /* user count on registered device*/ > > unsigned int open_count; > > struct completion comp; > > struct list_head group_next; > > @@ -57,6 +58,8 @@ struct vfio_device { > > /** > > * struct vfio_device_ops - VFIO bus driver device callbacks > > * > > + * @init: initialize private fields in device structure > > + * @release: Reclaim private fields in device structure > > * @open_device: Called when the first file descriptor is opened for this > device > > * @close_device: Opposite of open_device > > * @read: Perform read(2) on device file descriptor > > @@ -74,6 +77,8 @@ struct vfio_device { > > */ > > struct vfio_device_ops { > > char *name; > > + int (*init)(struct vfio_device *vdev); > > + void (*release)(struct vfio_device *vdev); > > int (*open_device)(struct vfio_device *vdev); > > void (*close_device)(struct vfio_device *vdev); > > ssize_t (*read)(struct vfio_device *vdev, char __user *buf, > > @@ -161,6 +166,24 @@ static inline int vfio_check_feature(u32 flags, > size_t argsz, u32 supported_ops, > > return 1; > > } > > > > +struct vfio_device *_vfio_alloc_device(size_t size, struct device *dev, > > + const struct vfio_device_ops *ops); > > +#define vfio_alloc_device(dev_struct, member, dev, ops) > \ > > + container_of(_vfio_alloc_device(sizeof(struct dev_struct) + > \ > > + BUILD_BUG_ON_ZERO(offsetof( > \ > > + struct dev_struct, member)), > \ > > + dev, ops), > \ > > + struct dev_struct, member) > > + > > +int vfio_init_device(struct vfio_device *device, struct device *dev, > > + const struct vfio_device_ops *ops); > > +void vfio_free_device(struct vfio_device *device); > > +void vfio_device_release(struct kref *kref); > > +static inline void vfio_put_device(struct vfio_device *device) > > +{ > > + kref_put(&device->kref, vfio_device_release); > > +} > > + > > void vfio_init_group_dev(struct vfio_device *device, struct device *dev, > > const struct vfio_device_ops *ops); > > void vfio_uninit_group_dev(struct vfio_device *device); > > -- > "firm, enduring, strong, and long-lived"