[RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev
Liu Yi L
yi.l.liu at intel.com
Fri Jan 14 12:28:46 UTC 2022
Hi Eric,
Hope you are back from new year holiday.:-) Have you got chane to consider
this patch?
Regards,
Yi Liu
On 2021/11/30 19:32, Liu, Yi L wrote:
>> From: Eric Farman <farman at linux.ibm.com>
>> Sent: Tuesday, November 30, 2021 1:18 AM
>>
>> On Wed, 2021-11-24 at 12:25 +0000, Liu, Yi L wrote:
>>>> From: Jason Gunthorpe <jgg at nvidia.com>
>>>> Sent: Fri, 1 Oct 2021 14:52:51 -0300
>>>>
>>>> The css_driver's main purpose is to create/destroy the mdev and
>>>> relay the
>>>> shutdown, irq, sch_event, and chp_event css_driver ops to the
>>>> single
>>>> created vfio_device, if it exists.
>>>>
>>>> Reframe the boundary where the css_driver domain switches to the
>>>> vfio
>>>> domain by using rcu to read and refcount the vfio_device out of the
>>>> sch's
>>>> drvdata. The mdev probe/remove will manage the drvdata of the
>>>> parent.
>>>>
>>>> The vfio core code refcounting thus guarantees that when a
>>>> css_driver
>>>> callback is running the vfio_device is registered, simplifying the
>>>> understanding of the whole lifecycle.
>>>>
>>>> Finally the vfio_ccw_private is allocated/freed during probe/remove
>>>> of the
>>>> mdev like any other vfio_device struct.
>>>
>>> Hi Eric,
>>>
>>> how about the status of this patch?
>>
>> Hi YiLiu,
>>
>> To be honest I never got this far in the series, as the middle of the
>> series got into some more involved rework than I had the bandwidth to
>> consider. Not sure I'll be able to do anything with it before the year
>> end holiday, but I've noted your interest in getting this in line with
>> the rest of vfio_device so I don't drop it too far down the list.
>
> thanks. look forward to your further thoughts on it. e.g. the rework
> things and gaps in this cleanup.
>
> Regards,
> Yi Liu
>
>> Thanks,
>> Eric
>>
>>> I found it is a good clean up to make
>>> vfio ccw behave same with other vfio_device users. Also, I'd like to
>>> do a clean up to consolidate the vfio_device allocation which needs the
>>> vfio ccw private allocation happen in the mdev probe. So it would be nice
>>> to build the cleanup based on this patch.
>>>
>>> Regards,
>>> Yi Liu
>>>
>>>> Signed-off-by: Jason Gunthorpe <jgg at nvidia.com>
>>>> ---
>>>> drivers/s390/cio/vfio_ccw_drv.c | 67 ++++++++++++++-----------
>>>> ----
>>>> drivers/s390/cio/vfio_ccw_ops.c | 40 +++++++----------
>>>> drivers/s390/cio/vfio_ccw_private.h | 23 +++++++++-
>>>> 3 files changed, 69 insertions(+), 61 deletions(-)
>>>>
>>>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c
>>>> b/drivers/s390/cio/vfio_ccw_drv.c
>>>> index 18ad047811d111..c5582fc9c46c9e 100644
>>>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>>>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>>>> @@ -86,13 +86,19 @@ static void vfio_ccw_crw_todo(struct
>>>> work_struct *work)
>>>> */
>>>> static void vfio_ccw_sch_irq(struct subchannel *sch)
>>>> {
>>>> - struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>>>> + struct vfio_ccw_private *private = vfio_ccw_get_priv(sch);
>>>> +
>>>> + /* IRQ should not be delivered after the mdev is destroyed */
>>>> + if (WARN_ON(!private))
>>>> + return;
>>>>
>>>> inc_irq_stat(IRQIO_CIO);
>>>> vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
>>>> + vfio_device_put(&private->vdev);
>>>> }
>>>>
>>>> -static struct vfio_ccw_private *vfio_ccw_alloc_private(struct
>>>> subchannel *sch)
>>>> +struct vfio_ccw_private *vfio_ccw_alloc_private(struct mdev_device
>>>> *mdev,
>>>> + struct subchannel *sch)
>>>> {
>>>> struct vfio_ccw_private *private;
>>>>
>>>> @@ -100,6 +106,8 @@ static struct vfio_ccw_private
>>>> *vfio_ccw_alloc_private(struct subchannel *sch)
>>>> if (!private)
>>>> return ERR_PTR(-ENOMEM);
>>>>
>>>> + vfio_init_group_dev(&private->vdev, &mdev->dev,
>>>> + &vfio_ccw_dev_ops);
>>>> private->sch = sch;
>>>> mutex_init(&private->io_mutex);
>>>> private->state = VFIO_CCW_STATE_CLOSED;
>>>> @@ -145,11 +153,12 @@ static struct vfio_ccw_private
>>>> *vfio_ccw_alloc_private(struct subchannel *sch)
>>>> kfree(private->cp.guest_cp);
>>>> out_free_private:
>>>> mutex_destroy(&private->io_mutex);
>>>> + vfio_uninit_group_dev(&private->vdev);
>>>> kfree(private);
>>>> return ERR_PTR(-ENOMEM);
>>>> }
>>>>
>>>> -static void vfio_ccw_free_private(struct vfio_ccw_private
>>>> *private)
>>>> +void vfio_ccw_free_private(struct vfio_ccw_private *private)
>>>> {
>>>> struct vfio_ccw_crw *crw, *temp;
>>>>
>>>> @@ -164,14 +173,14 @@ static void vfio_ccw_free_private(struct
>>>> vfio_ccw_private *private)
>>>> kmem_cache_free(vfio_ccw_io_region, private->io_region);
>>>> kfree(private->cp.guest_cp);
>>>> mutex_destroy(&private->io_mutex);
>>>> - kfree(private);
>>>> + vfio_uninit_group_dev(&private->vdev);
>>>> + kfree_rcu(private, rcu);
>>>> }
>>>>
>>>> static int vfio_ccw_sch_probe(struct subchannel *sch)
>>>> {
>>>> struct pmcw *pmcw = &sch->schib.pmcw;
>>>> - struct vfio_ccw_private *private;
>>>> - int ret = -ENOMEM;
>>>> + int ret;
>>>>
>>>> if (pmcw->qf) {
>>>> dev_warn(&sch->dev, "vfio: ccw: does not support QDIO:
>>>> %s\n",
>>>> @@ -179,15 +188,9 @@ static int vfio_ccw_sch_probe(struct
>>>> subchannel *sch)
>>>> return -ENODEV;
>>>> }
>>>>
>>>> - private = vfio_ccw_alloc_private(sch);
>>>> - if (IS_ERR(private))
>>>> - return PTR_ERR(private);
>>>> -
>>>> - dev_set_drvdata(&sch->dev, private);
>>>> -
>>>> - ret = vfio_ccw_mdev_reg(sch);
>>>> + ret = mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);
>>>> if (ret)
>>>> - goto out_free;
>>>> + return ret;
>>>>
>>>> if (dev_get_uevent_suppress(&sch->dev)) {
>>>> dev_set_uevent_suppress(&sch->dev, 0);
>>>> @@ -198,22 +201,11 @@ static int vfio_ccw_sch_probe(struct
>>>> subchannel *sch)
>>>> sch->schid.cssid, sch->schid.ssid,
>>>> sch->schid.sch_no);
>>>> return 0;
>>>> -
>>>> -out_free:
>>>> - dev_set_drvdata(&sch->dev, NULL);
>>>> - vfio_ccw_free_private(private);
>>>> - return ret;
>>>> }
>>>>
>>>> static void vfio_ccw_sch_remove(struct subchannel *sch)
>>>> {
>>>> - struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>>>> -
>>>> - vfio_ccw_mdev_unreg(sch);
>>>> -
>>>> - dev_set_drvdata(&sch->dev, NULL);
>>>> -
>>>> - vfio_ccw_free_private(private);
>>>> + mdev_unregister_device(&sch->dev);
>>>>
>>>> VFIO_CCW_MSG_EVENT(4, "unbound from
>> subchannel %x.%x.%04x\n",
>>>> sch->schid.cssid, sch->schid.ssid,
>>>> @@ -222,10 +214,14 @@ static void vfio_ccw_sch_remove(struct
>>>> subchannel *sch)
>>>>
>>>> static void vfio_ccw_sch_shutdown(struct subchannel *sch)
>>>> {
>>>> - struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>>>> + struct vfio_ccw_private *private = vfio_ccw_get_priv(sch);
>>>> +
>>>> + if (!private)
>>>> + return;
>>>>
>>>> vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
>>>> vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_BROKEN);
>>>> + vfio_device_put(&private->vdev);
>>>> }
>>>>
>>>> /**
>>>> @@ -240,14 +236,14 @@ static void vfio_ccw_sch_shutdown(struct
>>>> subchannel *sch)
>>>> */
>>>> static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>>>> {
>>>> - struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>>>> + struct vfio_ccw_private *private = vfio_ccw_get_priv(sch);
>>>> unsigned long flags;
>>>> int rc = -EAGAIN;
>>>>
>>>> - spin_lock_irqsave(sch->lock, flags);
>>>> - if (!device_is_registered(&sch->dev))
>>>> - goto out_unlock;
>>>> + if (!private)
>>>> + return -EAGAIN;
>>>>
>>>> + spin_lock_irqsave(sch->lock, flags);
>>>> if (work_pending(&sch->todo_work))
>>>> goto out_unlock;
>>>>
>>>> @@ -260,7 +256,7 @@ static int vfio_ccw_sch_event(struct subchannel
>>>> *sch, int process)
>>>>
>>>> out_unlock:
>>>> spin_unlock_irqrestore(sch->lock, flags);
>>>> -
>>>> + vfio_device_put(&private->vdev);
>>>> return rc;
>>>> }
>>>>
>>>> @@ -294,7 +290,7 @@ static void vfio_ccw_queue_crw(struct
>>>> vfio_ccw_private *private,
>>>> static int vfio_ccw_chp_event(struct subchannel *sch,
>>>> struct chp_link *link, int event)
>>>> {
>>>> - struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>>>> + struct vfio_ccw_private *private = vfio_ccw_get_priv(sch);
>>>> int mask = chp_ssd_get_mask(&sch->ssd_info, link);
>>>> int retry = 255;
>>>>
>>>> @@ -307,8 +303,10 @@ static int vfio_ccw_chp_event(struct
>>>> subchannel *sch,
>>>> sch->schid.ssid, sch->schid.sch_no,
>>>> mask, event);
>>>>
>>>> - if (cio_update_schib(sch))
>>>> + if (cio_update_schib(sch)) {
>>>> + vfio_device_put(&private->vdev);
>>>> return -ENODEV;
>>>> + }
>>>>
>>>> switch (event) {
>>>> case CHP_VARY_OFF:
>>>> @@ -338,6 +336,7 @@ static int vfio_ccw_chp_event(struct subchannel
>>>> *sch,
>>>> break;
>>>> }
>>>>
>>>> + vfio_device_put(&private->vdev);
>>>> return 0;
>>>> }
>>>>
>>>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c
>>>> b/drivers/s390/cio/vfio_ccw_ops.c
>>>> index 68aae25a0a4be0..414b11ea7eebf9 100644
>>>> --- a/drivers/s390/cio/vfio_ccw_ops.c
>>>> +++ b/drivers/s390/cio/vfio_ccw_ops.c
>>>> @@ -17,8 +17,6 @@
>>>>
>>>> #include "vfio_ccw_private.h"
>>>>
>>>> -static const struct vfio_device_ops vfio_ccw_dev_ops;
>>>> -
>>>> static int vfio_ccw_mdev_reset(struct vfio_ccw_private *private)
>>>> {
>>>> /*
>>>> @@ -88,26 +86,27 @@ static struct attribute_group
>>>> *mdev_type_groups[] = {
>>>>
>>>> static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
>>>> {
>>>> - struct vfio_ccw_private *private = dev_get_drvdata(mdev-
>>>>> dev.parent);
>>>> + struct subchannel *sch = to_subchannel(mdev->dev.parent);
>>>> + struct vfio_ccw_private *private;
>>>> int ret;
>>>>
>>>> - memset(&private->vdev, 0, sizeof(private->vdev));
>>>> - vfio_init_group_dev(&private->vdev, &mdev->dev,
>>>> - &vfio_ccw_dev_ops);
>>>> + private = vfio_ccw_alloc_private(mdev, sch);
>>>> + if (IS_ERR(private))
>>>> + return PTR_ERR(private);
>>>>
>>>> VFIO_CCW_MSG_EVENT(2, "mdev %s, sch %x.%x.%04x: create\n",
>>>> - dev_name(private->vdev.dev),
>>>> - private->sch->schid.cssid, private->sch-
>>>>> schid.ssid,
>>>> - private->sch->schid.sch_no);
>>>> + dev_name(private->vdev.dev), sch-
>>>>> schid.cssid,
>>>> + sch->schid.ssid, sch->schid.sch_no);
>>>>
>>>> ret = vfio_register_emulated_iommu_dev(&private->vdev);
>>>> if (ret)
>>>> - goto err_init;
>>>> + goto err_alloc;
>>>> dev_set_drvdata(&mdev->dev, private);
>>>> + dev_set_drvdata(&sch->dev, private);
>>>> return 0;
>>>>
>>>> -err_init:
>>>> - vfio_uninit_group_dev(&private->vdev);
>>>> +err_alloc:
>>>> + vfio_ccw_free_private(private);
>>>> return ret;
>>>> }
>>>>
>>>> @@ -120,8 +119,9 @@ static void vfio_ccw_mdev_remove(struct
>>>> mdev_device *mdev)
>>>> private->sch->schid.cssid, private->sch-
>>>>> schid.ssid,
>>>> private->sch->schid.sch_no);
>>>>
>>>> + dev_set_drvdata(&private->sch->dev, NULL);
>>>> vfio_unregister_group_dev(&private->vdev);
>>>> - vfio_uninit_group_dev(&private->vdev);
>>>> + vfio_ccw_free_private(private);
>>>> }
>>>>
>>>> static int vfio_ccw_mdev_open_device(struct vfio_device *vdev)
>>>> @@ -595,7 +595,7 @@ static unsigned int
>>>> vfio_ccw_get_available(struct mdev_type *mtype)
>>>> return 1;
>>>> }
>>>>
>>>> -static const struct vfio_device_ops vfio_ccw_dev_ops = {
>>>> +const struct vfio_device_ops vfio_ccw_dev_ops = {
>>>> .open_device = vfio_ccw_mdev_open_device,
>>>> .close_device = vfio_ccw_mdev_close_device,
>>>> .read = vfio_ccw_mdev_read,
>>>> @@ -615,19 +615,9 @@ struct mdev_driver vfio_ccw_mdev_driver = {
>>>> .get_available = vfio_ccw_get_available,
>>>> };
>>>>
>>>> -static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
>>>> +const struct mdev_parent_ops vfio_ccw_mdev_ops = {
>>>> .owner = THIS_MODULE,
>>>> .device_driver = &vfio_ccw_mdev_driver,
>>>> .device_api = VFIO_DEVICE_API_CCW_STRING,
>>>> .supported_type_groups = mdev_type_groups,
>>>> };
>>>> -
>>>> -int vfio_ccw_mdev_reg(struct subchannel *sch)
>>>> -{
>>>> - return mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);
>>>> -}
>>>> -
>>>> -void vfio_ccw_mdev_unreg(struct subchannel *sch)
>>>> -{
>>>> - mdev_unregister_device(&sch->dev);
>>>> -}
>>>> diff --git a/drivers/s390/cio/vfio_ccw_private.h
>>>> b/drivers/s390/cio/vfio_ccw_private.h
>>>> index 67ee9c624393b0..852ff94fc107d6 100644
>>>> --- a/drivers/s390/cio/vfio_ccw_private.h
>>>> +++ b/drivers/s390/cio/vfio_ccw_private.h
>>>> @@ -24,6 +24,8 @@
>>>> #include "css.h"
>>>> #include "vfio_ccw_cp.h"
>>>>
>>>> +struct mdev_device;
>>>> +
>>>> #define VFIO_CCW_OFFSET_SHIFT 10
>>>> #define VFIO_CCW_OFFSET_TO_INDEX(off) (off >>
>>>> VFIO_CCW_OFFSET_SHIFT)
>>>> #define VFIO_CCW_INDEX_TO_OFFSET(index) ((u64)(index) <<
>>>> VFIO_CCW_OFFSET_SHIFT)
>>>> @@ -69,6 +71,7 @@ struct vfio_ccw_crw {
>>>> /**
>>>> * struct vfio_ccw_private
>>>> * @vdev: Embedded VFIO device
>>>> + * @rcu: head for kfree_rcu()
>>>> * @sch: pointer to the subchannel
>>>> * @state: internal state of the device
>>>> * @completion: synchronization helper of the I/O completion
>>>> @@ -91,6 +94,7 @@ struct vfio_ccw_crw {
>>>> */
>>>> struct vfio_ccw_private {
>>>> struct vfio_device vdev;
>>>> + struct rcu_head rcu;
>>>> struct subchannel *sch;
>>>> int state;
>>>> struct completion *completion;
>>>> @@ -115,10 +119,25 @@ struct vfio_ccw_private {
>>>> struct work_struct crw_work;
>>>> } __aligned(8);
>>>>
>>>> -extern int vfio_ccw_mdev_reg(struct subchannel *sch);
>>>> -extern void vfio_ccw_mdev_unreg(struct subchannel *sch);
>>>> +struct vfio_ccw_private *vfio_ccw_alloc_private(struct mdev_device
>>>> *mdev,
>>>> + struct subchannel
>>>> *sch);
>>>> +void vfio_ccw_free_private(struct vfio_ccw_private *private);
>>>>
>>>> extern struct mdev_driver vfio_ccw_mdev_driver;
>>>> +extern const struct mdev_parent_ops vfio_ccw_mdev_ops;
>>>> +extern const struct vfio_device_ops vfio_ccw_dev_ops;
>>>> +
>>>> +static inline struct vfio_ccw_private *vfio_ccw_get_priv(struct
>>>> subchannel *sch)
>>>> +{
>>>> + struct vfio_ccw_private *private;
>>>> +
>>>> + rcu_read_lock();
>>>> + private = dev_get_drvdata(&sch->dev);
>>>> + if (private && !vfio_device_try_get(&private->vdev))
>>>> + private = NULL;
>>>> + rcu_read_unlock();
>>>> + return private;
>>>> +}
>>>>
>>>> /*
>>>> * States of the device statemachine.
>>>> --
>>>> 2.33.0
>
--
Regards,
Yi Liu
More information about the dri-devel
mailing list