Message ID | 20220909102247.67324-2-kevin.tian@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Tidy up vfio_device life cycle | expand |
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, - 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@nvidia.com> > Co-developed-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Kevin Tian <kevin.tian@intel.com> > Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Reviewed-by: Eric Auger <eric.auger@redhat.com> > --- > 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);
> From: Ethan Zhao <haifeng.zhao@linux.intel.com> > 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@nvidia.com> > > Co-developed-by: Yi Liu <yi.l.liu@intel.com> > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > Signed-off-by: Kevin Tian <kevin.tian@intel.com> > > Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com> > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > > Reviewed-by: Eric Auger <eric.auger@redhat.com> > > --- > > 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"
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);