[Intel-gfx] [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 Intel-gfx mailing list