* [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev @ 2021-11-24 12:25 ` Liu, Yi L 0 siblings, 0 replies; 21+ messages in thread From: Liu, Yi L @ 2021-11-24 12:25 UTC (permalink / raw) To: Jason Gunthorpe, farman Cc: Matthew Rosato, airlied, dri-devel, kwankhede, Vineeth Vijayan, linux-s390, kvm, corbet, Halil Pasic, borntraeger, intel-gfx, Wang, Zhi A, Jason Herne, Eric Farman, Vasily Gorbik, Heiko Carstens, alex.williamson, Harald Freudenberger, Vivi, Rodrigo, akrowiak, cohuck, Peter Oberparleiter > From: Jason Gunthorpe <jgg@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? 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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev @ 2021-11-24 12:25 ` Liu, Yi L 0 siblings, 0 replies; 21+ messages in thread From: Liu, Yi L @ 2021-11-24 12:25 UTC (permalink / raw) To: Jason Gunthorpe, farman Cc: airlied, akrowiak, alex.williamson, borntraeger, cohuck, corbet, Daniel Vetter, dri-devel, Eric Farman, Harald Freudenberger, Vasily Gorbik, Heiko Carstens, Jani Nikula, Jason Herne, Joonas Lahtinen, intel-gfx, Matthew Rosato, Peter Oberparleiter, Halil Pasic, Vivi, Rodrigo, Vineeth Vijayan, Zhenyu Wang, Wang, Zhi A, kwankhede, kvm, linux-s390 > From: Jason Gunthorpe <jgg@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? 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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [Intel-gfx] [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev @ 2021-11-24 12:25 ` Liu, Yi L 0 siblings, 0 replies; 21+ messages in thread From: Liu, Yi L @ 2021-11-24 12:25 UTC (permalink / raw) To: Jason Gunthorpe, farman Cc: Matthew Rosato, airlied, dri-devel, kwankhede, Vineeth Vijayan, linux-s390, kvm, corbet, Halil Pasic, borntraeger, intel-gfx, Jason Herne, Eric Farman, Vasily Gorbik, Heiko Carstens, Harald Freudenberger, akrowiak, cohuck, Peter Oberparleiter > From: Jason Gunthorpe <jgg@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? 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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev 2021-11-24 12:25 ` [Intel-gfx] " Liu, Yi L (?) @ 2021-11-29 17:18 ` Eric Farman -1 siblings, 0 replies; 21+ messages in thread From: Eric Farman @ 2021-11-29 17:18 UTC (permalink / raw) To: Liu, Yi L, Jason Gunthorpe Cc: airlied, akrowiak, alex.williamson, borntraeger, cohuck, corbet, Daniel Vetter, dri-devel, Harald Freudenberger, Vasily Gorbik, Heiko Carstens, Jani Nikula, Jason Herne, Joonas Lahtinen, intel-gfx, Matthew Rosato, Peter Oberparleiter, Halil Pasic, Vivi, Rodrigo, Vineeth Vijayan, Zhenyu Wang, Wang, Zhi A, kwankhede, kvm, linux-s390 On Wed, 2021-11-24 at 12:25 +0000, Liu, Yi L wrote: > > From: Jason Gunthorpe <jgg@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, 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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Intel-gfx] [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev @ 2021-11-29 17:18 ` Eric Farman 0 siblings, 0 replies; 21+ messages in thread From: Eric Farman @ 2021-11-29 17:18 UTC (permalink / raw) To: Liu, Yi L, Jason Gunthorpe Cc: Matthew Rosato, airlied, dri-devel, kwankhede, Vineeth Vijayan, linux-s390, kvm, corbet, Halil Pasic, borntraeger, intel-gfx, Jason Herne, Vasily Gorbik, Heiko Carstens, Harald Freudenberger, akrowiak, cohuck, Peter Oberparleiter On Wed, 2021-11-24 at 12:25 +0000, Liu, Yi L wrote: > > From: Jason Gunthorpe <jgg@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, 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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev @ 2021-11-29 17:18 ` Eric Farman 0 siblings, 0 replies; 21+ messages in thread From: Eric Farman @ 2021-11-29 17:18 UTC (permalink / raw) To: Liu, Yi L, Jason Gunthorpe Cc: Matthew Rosato, airlied, dri-devel, kwankhede, Vineeth Vijayan, linux-s390, kvm, corbet, Halil Pasic, borntraeger, intel-gfx, Wang, Zhi A, Jason Herne, Vasily Gorbik, Heiko Carstens, alex.williamson, Harald Freudenberger, Vivi, Rodrigo, akrowiak, cohuck, Peter Oberparleiter On Wed, 2021-11-24 at 12:25 +0000, Liu, Yi L wrote: > > From: Jason Gunthorpe <jgg@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, 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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev 2021-11-29 17:18 ` Eric Farman (?) @ 2021-11-30 11:32 ` Liu, Yi L -1 siblings, 0 replies; 21+ messages in thread From: Liu, Yi L @ 2021-11-30 11:32 UTC (permalink / raw) To: Eric Farman, Jason Gunthorpe Cc: airlied, akrowiak, alex.williamson, borntraeger, cohuck, corbet, Daniel Vetter, dri-devel, Harald Freudenberger, Vasily Gorbik, Heiko Carstens, Jani Nikula, Jason Herne, Joonas Lahtinen, intel-gfx, Matthew Rosato, Peter Oberparleiter, Halil Pasic, Vivi, Rodrigo, Vineeth Vijayan, Zhenyu Wang, Wang, Zhi A, kwankhede, kvm, linux-s390 > From: Eric Farman <farman@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@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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Intel-gfx] [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev @ 2021-11-30 11:32 ` Liu, Yi L 0 siblings, 0 replies; 21+ messages in thread From: Liu, Yi L @ 2021-11-30 11:32 UTC (permalink / raw) To: Eric Farman, Jason Gunthorpe Cc: Matthew Rosato, airlied, dri-devel, kwankhede, Vineeth Vijayan, linux-s390, kvm, corbet, Halil Pasic, borntraeger, intel-gfx, Jason Herne, Vasily Gorbik, Heiko Carstens, Harald Freudenberger, akrowiak, cohuck, Peter Oberparleiter > From: Eric Farman <farman@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@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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev @ 2021-11-30 11:32 ` Liu, Yi L 0 siblings, 0 replies; 21+ messages in thread From: Liu, Yi L @ 2021-11-30 11:32 UTC (permalink / raw) To: Eric Farman, Jason Gunthorpe Cc: Matthew Rosato, airlied, dri-devel, kwankhede, Vineeth Vijayan, linux-s390, kvm, corbet, Halil Pasic, borntraeger, intel-gfx, Wang, Zhi A, Jason Herne, Vasily Gorbik, Heiko Carstens, alex.williamson, Harald Freudenberger, Vivi, Rodrigo, akrowiak, cohuck, Peter Oberparleiter > From: Eric Farman <farman@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@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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev 2021-11-30 11:32 ` Liu, Yi L (?) @ 2022-01-14 12:28 ` Liu Yi L -1 siblings, 0 replies; 21+ messages in thread From: Liu Yi L @ 2022-01-14 12:28 UTC (permalink / raw) To: Eric Farman, Jason Gunthorpe Cc: airlied, akrowiak, alex.williamson, borntraeger, cohuck, corbet, Daniel Vetter, dri-devel, Harald Freudenberger, Vasily Gorbik, Heiko Carstens, Jani Nikula, Jason Herne, Joonas Lahtinen, intel-gfx, Matthew Rosato, Peter Oberparleiter, Halil Pasic, Vivi, Rodrigo, Vineeth Vijayan, Zhenyu Wang, Wang, Zhi A, kwankhede, kvm, linux-s390 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@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@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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Intel-gfx] [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev @ 2022-01-14 12:28 ` Liu Yi L 0 siblings, 0 replies; 21+ messages in thread From: Liu Yi L @ 2022-01-14 12:28 UTC (permalink / raw) To: Eric Farman, Jason Gunthorpe Cc: Matthew Rosato, airlied, dri-devel, kwankhede, Vineeth Vijayan, linux-s390, kvm, corbet, Halil Pasic, borntraeger, intel-gfx, Jason Herne, Vasily Gorbik, Heiko Carstens, Harald Freudenberger, akrowiak, cohuck, Peter Oberparleiter 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@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@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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev @ 2022-01-14 12:28 ` Liu Yi L 0 siblings, 0 replies; 21+ messages in thread From: Liu Yi L @ 2022-01-14 12:28 UTC (permalink / raw) To: Eric Farman, Jason Gunthorpe Cc: Matthew Rosato, airlied, dri-devel, kwankhede, Vineeth Vijayan, linux-s390, kvm, corbet, Halil Pasic, borntraeger, intel-gfx, Wang, Zhi A, Jason Herne, Vasily Gorbik, Heiko Carstens, alex.williamson, Harald Freudenberger, Vivi, Rodrigo, akrowiak, cohuck, Peter Oberparleiter 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@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@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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev 2022-01-14 12:28 ` Liu Yi L (?) @ 2022-01-14 16:30 ` Eric Farman -1 siblings, 0 replies; 21+ messages in thread From: Eric Farman @ 2022-01-14 16:30 UTC (permalink / raw) To: Liu Yi L, Jason Gunthorpe Cc: airlied, akrowiak, alex.williamson, borntraeger, cohuck, corbet, Daniel Vetter, dri-devel, Harald Freudenberger, Vasily Gorbik, Heiko Carstens, Jani Nikula, Jason Herne, Joonas Lahtinen, intel-gfx, Matthew Rosato, Peter Oberparleiter, Halil Pasic, Vivi, Rodrigo, Vineeth Vijayan, Zhenyu Wang, Wang, Zhi A, kwankhede, kvm, linux-s390 On Fri, 2022-01-14 at 20:28 +0800, Liu Yi L wrote: > Hi Eric, > > Hope you are back from new year holiday.:-) Have you got chane to > consider > this patch? Hi Yi Liu, It's coming up the list, but it's not there yet. Haven't forgotten. :) Eric > > Regards, > Yi Liu > > On 2021/11/30 19:32, Liu, Yi L wrote: > > > From: Eric Farman <farman@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@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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Intel-gfx] [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev @ 2022-01-14 16:30 ` Eric Farman 0 siblings, 0 replies; 21+ messages in thread From: Eric Farman @ 2022-01-14 16:30 UTC (permalink / raw) To: Liu Yi L, Jason Gunthorpe Cc: Matthew Rosato, airlied, dri-devel, kwankhede, Vineeth Vijayan, linux-s390, kvm, corbet, Halil Pasic, borntraeger, intel-gfx, Jason Herne, Vasily Gorbik, Heiko Carstens, Harald Freudenberger, akrowiak, cohuck, Peter Oberparleiter On Fri, 2022-01-14 at 20:28 +0800, Liu Yi L wrote: > Hi Eric, > > Hope you are back from new year holiday.:-) Have you got chane to > consider > this patch? Hi Yi Liu, It's coming up the list, but it's not there yet. Haven't forgotten. :) Eric > > Regards, > Yi Liu > > On 2021/11/30 19:32, Liu, Yi L wrote: > > > From: Eric Farman <farman@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@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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev @ 2022-01-14 16:30 ` Eric Farman 0 siblings, 0 replies; 21+ messages in thread From: Eric Farman @ 2022-01-14 16:30 UTC (permalink / raw) To: Liu Yi L, Jason Gunthorpe Cc: Matthew Rosato, airlied, dri-devel, kwankhede, Vineeth Vijayan, linux-s390, kvm, corbet, Halil Pasic, borntraeger, intel-gfx, Wang, Zhi A, Jason Herne, Vasily Gorbik, Heiko Carstens, alex.williamson, Harald Freudenberger, Vivi, Rodrigo, akrowiak, cohuck, Peter Oberparleiter On Fri, 2022-01-14 at 20:28 +0800, Liu Yi L wrote: > Hi Eric, > > Hope you are back from new year holiday.:-) Have you got chane to > consider > this patch? Hi Yi Liu, It's coming up the list, but it's not there yet. Haven't forgotten. :) Eric > > Regards, > Yi Liu > > On 2021/11/30 19:32, Liu, Yi L wrote: > > > From: Eric Farman <farman@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@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@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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev 2022-01-14 16:30 ` Eric Farman (?) @ 2022-01-17 15:35 ` Jason Gunthorpe -1 siblings, 0 replies; 21+ messages in thread From: Jason Gunthorpe @ 2022-01-17 15:35 UTC (permalink / raw) To: Eric Farman Cc: Liu Yi L, airlied, akrowiak, alex.williamson, borntraeger, cohuck, corbet, Daniel Vetter, dri-devel, Harald Freudenberger, Vasily Gorbik, Heiko Carstens, Jani Nikula, Jason Herne, Joonas Lahtinen, intel-gfx, Matthew Rosato, Peter Oberparleiter, Halil Pasic, Vivi, Rodrigo, Vineeth Vijayan, Zhenyu Wang, Wang, Zhi A, kwankhede, kvm, linux-s390 On Fri, Jan 14, 2022 at 11:30:36AM -0500, Eric Farman wrote: > On Fri, 2022-01-14 at 20:28 +0800, Liu Yi L wrote: > > Hi Eric, > > > > Hope you are back from new year holiday.:-) Have you got chane to > > consider > > this patch? > > Hi Yi Liu, > > It's coming up the list, but it's not there yet. Haven't forgotten. :) Liu would like to see ccw use a normal lifecycle for the vfio_device backing memory, is there a shorter path to achieve that then what I came up with? Jason ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Intel-gfx] [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev @ 2022-01-17 15:35 ` Jason Gunthorpe 0 siblings, 0 replies; 21+ messages in thread From: Jason Gunthorpe @ 2022-01-17 15:35 UTC (permalink / raw) To: Eric Farman Cc: Matthew Rosato, airlied, dri-devel, kwankhede, Vineeth Vijayan, linux-s390, Liu Yi L, kvm, corbet, Halil Pasic, borntraeger, intel-gfx, Jason Herne, Vasily Gorbik, Heiko Carstens, Harald Freudenberger, akrowiak, cohuck, Peter Oberparleiter On Fri, Jan 14, 2022 at 11:30:36AM -0500, Eric Farman wrote: > On Fri, 2022-01-14 at 20:28 +0800, Liu Yi L wrote: > > Hi Eric, > > > > Hope you are back from new year holiday.:-) Have you got chane to > > consider > > this patch? > > Hi Yi Liu, > > It's coming up the list, but it's not there yet. Haven't forgotten. :) Liu would like to see ccw use a normal lifecycle for the vfio_device backing memory, is there a shorter path to achieve that then what I came up with? Jason ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev @ 2022-01-17 15:35 ` Jason Gunthorpe 0 siblings, 0 replies; 21+ messages in thread From: Jason Gunthorpe @ 2022-01-17 15:35 UTC (permalink / raw) To: Eric Farman Cc: Matthew Rosato, airlied, dri-devel, kwankhede, Vineeth Vijayan, linux-s390, Liu Yi L, kvm, corbet, Halil Pasic, borntraeger, intel-gfx, Wang, Zhi A, Jason Herne, Vasily Gorbik, Heiko Carstens, alex.williamson, Harald Freudenberger, Vivi, Rodrigo, akrowiak, cohuck, Peter Oberparleiter On Fri, Jan 14, 2022 at 11:30:36AM -0500, Eric Farman wrote: > On Fri, 2022-01-14 at 20:28 +0800, Liu Yi L wrote: > > Hi Eric, > > > > Hope you are back from new year holiday.:-) Have you got chane to > > consider > > this patch? > > Hi Yi Liu, > > It's coming up the list, but it's not there yet. Haven't forgotten. :) Liu would like to see ccw use a normal lifecycle for the vfio_device backing memory, is there a shorter path to achieve that then what I came up with? Jason ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev 2022-01-17 15:35 ` Jason Gunthorpe (?) @ 2022-01-18 17:33 ` Eric Farman -1 siblings, 0 replies; 21+ messages in thread From: Eric Farman @ 2022-01-18 17:33 UTC (permalink / raw) To: Jason Gunthorpe Cc: Liu Yi L, airlied, akrowiak, alex.williamson, borntraeger, cohuck, corbet, Daniel Vetter, dri-devel, Harald Freudenberger, Vasily Gorbik, Heiko Carstens, Jani Nikula, Jason Herne, Joonas Lahtinen, intel-gfx, Matthew Rosato, Peter Oberparleiter, Halil Pasic, Vivi, Rodrigo, Vineeth Vijayan, Zhenyu Wang, Wang, Zhi A, kwankhede, kvm, linux-s390 On Mon, 2022-01-17 at 11:35 -0400, Jason Gunthorpe wrote: > On Fri, Jan 14, 2022 at 11:30:36AM -0500, Eric Farman wrote: > > On Fri, 2022-01-14 at 20:28 +0800, Liu Yi L wrote: > > > Hi Eric, > > > > > > Hope you are back from new year holiday.:-) Have you got chane to > > > consider > > > this patch? > > > > Hi Yi Liu, > > > > It's coming up the list, but it's not there yet. Haven't forgotten. > > :) > > Liu would like to see ccw use a normal lifecycle for the vfio_device > backing memory, is there a shorter path to achieve that then what I > came up with? Getting through your proposal is the task on our plate. Just not enough hours in the day at the moment, sorry. Eric > > Jason ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Intel-gfx] [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev @ 2022-01-18 17:33 ` Eric Farman 0 siblings, 0 replies; 21+ messages in thread From: Eric Farman @ 2022-01-18 17:33 UTC (permalink / raw) To: Jason Gunthorpe Cc: Matthew Rosato, airlied, dri-devel, kwankhede, Vineeth Vijayan, linux-s390, Liu Yi L, kvm, corbet, Halil Pasic, borntraeger, intel-gfx, Jason Herne, Vasily Gorbik, Heiko Carstens, Harald Freudenberger, akrowiak, cohuck, Peter Oberparleiter On Mon, 2022-01-17 at 11:35 -0400, Jason Gunthorpe wrote: > On Fri, Jan 14, 2022 at 11:30:36AM -0500, Eric Farman wrote: > > On Fri, 2022-01-14 at 20:28 +0800, Liu Yi L wrote: > > > Hi Eric, > > > > > > Hope you are back from new year holiday.:-) Have you got chane to > > > consider > > > this patch? > > > > Hi Yi Liu, > > > > It's coming up the list, but it's not there yet. Haven't forgotten. > > :) > > Liu would like to see ccw use a normal lifecycle for the vfio_device > backing memory, is there a shorter path to achieve that then what I > came up with? Getting through your proposal is the task on our plate. Just not enough hours in the day at the moment, sorry. Eric > > Jason ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev @ 2022-01-18 17:33 ` Eric Farman 0 siblings, 0 replies; 21+ messages in thread From: Eric Farman @ 2022-01-18 17:33 UTC (permalink / raw) To: Jason Gunthorpe Cc: Matthew Rosato, airlied, dri-devel, kwankhede, Vineeth Vijayan, linux-s390, Liu Yi L, kvm, corbet, Halil Pasic, borntraeger, intel-gfx, Wang, Zhi A, Jason Herne, Vasily Gorbik, Heiko Carstens, alex.williamson, Harald Freudenberger, Vivi, Rodrigo, akrowiak, cohuck, Peter Oberparleiter On Mon, 2022-01-17 at 11:35 -0400, Jason Gunthorpe wrote: > On Fri, Jan 14, 2022 at 11:30:36AM -0500, Eric Farman wrote: > > On Fri, 2022-01-14 at 20:28 +0800, Liu Yi L wrote: > > > Hi Eric, > > > > > > Hope you are back from new year holiday.:-) Have you got chane to > > > consider > > > this patch? > > > > Hi Yi Liu, > > > > It's coming up the list, but it's not there yet. Haven't forgotten. > > :) > > Liu would like to see ccw use a normal lifecycle for the vfio_device > backing memory, is there a shorter path to achieve that then what I > came up with? Getting through your proposal is the task on our plate. Just not enough hours in the day at the moment, sorry. Eric > > Jason ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2022-01-19 17:53 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-24 12:25 [RE]: [PATCH v3 10/10] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev Liu, Yi L 2021-11-24 12:25 ` Liu, Yi L 2021-11-24 12:25 ` [Intel-gfx] " Liu, Yi L 2021-11-29 17:18 ` Eric Farman 2021-11-29 17:18 ` [Intel-gfx] " Eric Farman 2021-11-29 17:18 ` Eric Farman 2021-11-30 11:32 ` Liu, Yi L 2021-11-30 11:32 ` [Intel-gfx] " Liu, Yi L 2021-11-30 11:32 ` Liu, Yi L 2022-01-14 12:28 ` Liu Yi L 2022-01-14 12:28 ` [Intel-gfx] " Liu Yi L 2022-01-14 12:28 ` Liu Yi L 2022-01-14 16:30 ` Eric Farman 2022-01-14 16:30 ` [Intel-gfx] " Eric Farman 2022-01-14 16:30 ` Eric Farman 2022-01-17 15:35 ` Jason Gunthorpe 2022-01-17 15:35 ` [Intel-gfx] " Jason Gunthorpe 2022-01-17 15:35 ` Jason Gunthorpe 2022-01-18 17:33 ` Eric Farman 2022-01-18 17:33 ` [Intel-gfx] " Eric Farman 2022-01-18 17:33 ` Eric Farman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.