Re: [PATCH v3 01/15] vfio: Add helpers for unifying vfio_device life cycle
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
Co-developed-by: Yi Liu
Signed-off-by: Yi Liu
Signed-off-by: Kevin Tian
Reviewed-by: Tony Krowiak
Reviewed-by: Jason Gunthorpe
Reviewed-by: Eric Auger
---
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(>kref);
+ return 0;
+
+out_uninit:
+
Re: [PATCH v3 01/15] vfio: Add helpers for unifying vfio_device life cycle
On Fri, Sep 09, 2022 at 08:42:25AM +, Tian, Kevin wrote: > 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. Right, it is quite a good/common pattern to have an allocation function return a refcount to the caller. I don't know of any naming standard for this however. Jason
RE: [PATCH v3 01/15] vfio: Add helpers for unifying vfio_device life cycle
> From: Ethan Zhao
> 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
> > Co-developed-by: Yi Liu
> > Signed-off-by: Yi Liu
> > Signed-off-by: Kevin Tian
> > Reviewed-by: Tony Krowiak
> > Reviewed-by: Jason Gunthorpe
> > Reviewed-by: Eric Auger
> > ---
> > 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;
> > +
>
[PATCH v3 01/15] vfio: Add helpers for unifying vfio_device life cycle
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()
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
Co-developed-by: Yi Liu
Signed-off-by: Yi Liu
Signed-off-by: Kevin Tian
Reviewed-by: Tony Krowiak
Reviewed-by: Jason Gunthorpe
Reviewed-by: Eric Auger
---
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(>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
