| Message ID | 20220726153725.2573294-1-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] vfio/ccw: Move mdev stuff out of struct subchannel | expand |
On Tue, Jul 26, 2022 at 05:37:25PM +0200, Eric Farman wrote: > Here's my swipe at a cleanup patch that can be folded in > to this series, to get the mdev stuff in a more proper > location for vfio-ccw. > > As previously described, the subchannel is a device-agnostic > structure that does/should not need to know about specific > nuances such as mediated devices. This is why things like > struct vfio_ccw_private exist, so move these details there. Should I resend the series with that folded in? At this point we're probably not talking about 5.20 anyway.
On Tue, 2022-07-26 at 19:48 +0200, Christoph Hellwig wrote: > On Tue, Jul 26, 2022 at 05:37:25PM +0200, Eric Farman wrote: > > Here's my swipe at a cleanup patch that can be folded in > > to this series, to get the mdev stuff in a more proper > > location for vfio-ccw. > > > > As previously described, the subchannel is a device-agnostic > > structure that does/should not need to know about specific > > nuances such as mediated devices. This is why things like > > struct vfio_ccw_private exist, so move these details there. > > Should I resend the series with that folded in? That would be great. I'll give it another spin and can look over the ccw changes without the smattering of fixups I have. > At this point we're > probably not talking about 5.20 anyway.
Hi, Eric, > From: Eric Farman > Sent: Tuesday, July 26, 2022 11:37 PM > > --- a/drivers/s390/cio/vfio_ccw_private.h > +++ b/drivers/s390/cio/vfio_ccw_private.h > @@ -111,6 +111,10 @@ struct vfio_ccw_private { > struct eventfd_ctx *req_trigger; > struct work_struct io_work; > struct work_struct crw_work; > + > + struct mdev_parent parent; > + struct mdev_type mdev_type; > + struct mdev_type *mdev_types[1]; > } __aligned(8); > IMHO creating a separate structure to encapsulate parent related information is far cleaner than mixing both mdev and parent into one structure. mdev and parent have different life cycles. mdev is between mdev probe/remove while parent is between css probe/remove. Mixing them together prevents further cleanup in vfio core [1] which you posted in earlier series and also other upcoming improvements [2]. Thanks Kevin [1] https://lore.kernel.org/all/20220602171948.2790690-16-farman@linux.ibm.com/ [2] https://listman.redhat.com/archives/libvir-list/2022-August/233482.html
On Thu, 2022-08-18 at 06:53 +0000, Tian, Kevin wrote: > Hi, Eric, > > > From: Eric Farman > > Sent: Tuesday, July 26, 2022 11:37 PM > > > > --- a/drivers/s390/cio/vfio_ccw_private.h > > +++ b/drivers/s390/cio/vfio_ccw_private.h > > @@ -111,6 +111,10 @@ struct vfio_ccw_private { > > struct eventfd_ctx *req_trigger; > > struct work_struct io_work; > > struct work_struct crw_work; > > + > > + struct mdev_parent parent; > > + struct mdev_type mdev_type; > > + struct mdev_type *mdev_types[1]; > > } __aligned(8); > > > > IMHO creating a separate structure to encapsulate parent related > information is far cleaner than mixing both mdev and parent into > one structure. > > mdev and parent have different life cycles. mdev is between > mdev probe/remove while parent is between css probe/remove. I don't disagree with any of that. My point with the suggested patch was a way to get this working without disrupting the cio's subchannel (which is used for many non-mdev things). Un-mixing the mdev from the parent stuff wouldn't be impossible, but requires consideration I haven't had the bandwidth to do (which is why the cleanup you reference below was dropped from later versions of that series [3]). Thanks, Eric [3] https://lore.kernel.org/all/20220615203318.3830778-1-farman@linux.ibm.com/ > > Mixing them together prevents further cleanup in vfio core [1] > which you posted in earlier series and also other upcoming > improvements [2]. > > Thanks > Kevin > > [1] > https://lore.kernel.org/all/20220602171948.2790690-16-farman@linux.ibm.com/ > [2] > https://listman.redhat.com/archives/libvir-list/2022-August/233482.html
> From: Eric Farman <farman@linux.ibm.com> > Sent: Thursday, August 18, 2022 9:24 PM > > On Thu, 2022-08-18 at 06:53 +0000, Tian, Kevin wrote: > > Hi, Eric, > > > > > From: Eric Farman > > > Sent: Tuesday, July 26, 2022 11:37 PM > > > > > > --- a/drivers/s390/cio/vfio_ccw_private.h > > > +++ b/drivers/s390/cio/vfio_ccw_private.h > > > @@ -111,6 +111,10 @@ struct vfio_ccw_private { > > > struct eventfd_ctx *req_trigger; > > > struct work_struct io_work; > > > struct work_struct crw_work; > > > + > > > + struct mdev_parent parent; > > > + struct mdev_type mdev_type; > > > + struct mdev_type *mdev_types[1]; > > > } __aligned(8); > > > > > > > IMHO creating a separate structure to encapsulate parent related > > information is far cleaner than mixing both mdev and parent into > > one structure. > > > > mdev and parent have different life cycles. mdev is between > > mdev probe/remove while parent is between css probe/remove. > > I don't disagree with any of that. My point with the suggested patch > was a way to get this working without disrupting the cio's subchannel > (which is used for many non-mdev things). ok > > Un-mixing the mdev from the parent stuff wouldn't be impossible, but > requires consideration I haven't had the bandwidth to do (which is why > the cleanup you reference below was dropped from later versions of that > series [3]). > Could you help take a look at [1] which introduces a workaround for ccw so struct device can be introduced to vfio_device now instead of being blocked by this un-mixing task? [1] https://lore.kernel.org/lkml/20220827171037.30297-14-kevin.tian@intel.com/
diff --git a/drivers/s390/cio/cio.h b/drivers/s390/cio/cio.h index 1da45307a186..2ad8833653e9 100644 --- a/drivers/s390/cio/cio.h +++ b/drivers/s390/cio/cio.h @@ -109,9 +109,6 @@ struct subchannel { * frees it. Use driver_set_override() to set or clear it. */ const char *driver_override; - struct mdev_parent parent; - struct mdev_type mdev_type; - struct mdev_type *mdev_types[1]; } __attribute__ ((aligned(8))); DECLARE_PER_CPU_ALIGNED(struct irb, cio_irb); diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c index 6605b4239f88..bdf76805d175 100644 --- a/drivers/s390/cio/vfio_ccw_drv.c +++ b/drivers/s390/cio/vfio_ccw_drv.c @@ -142,6 +142,10 @@ static struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch) INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo); INIT_WORK(&private->crw_work, vfio_ccw_crw_todo); + private->mdev_type.sysfs_name = "io"; + private->mdev_type.pretty_name = "I/O subchannel (Non-QDIO)"; + private->mdev_types[0] = &private->mdev_type; + private->cp.guest_cp = kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1), GFP_KERNEL); if (!private->cp.guest_cp) @@ -219,12 +223,9 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) dev_set_drvdata(&sch->dev, private); - sch->mdev_type.sysfs_name = "io"; - sch->mdev_type.pretty_name = "I/O subchannel (Non-QDIO)"; - sch->mdev_types[0] = &sch->mdev_type; - ret = mdev_register_parent(&sch->parent, &sch->dev, + ret = mdev_register_parent(&private->parent, &sch->dev, &vfio_ccw_mdev_driver, - sch->mdev_types, 1); + private->mdev_types, 1); if (ret) goto out_free; @@ -244,7 +245,7 @@ static void vfio_ccw_sch_remove(struct subchannel *sch) struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev); vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE); - mdev_unregister_parent(&sch->parent); + mdev_unregister_parent(&private->parent); dev_set_drvdata(&sch->dev, NULL); diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h index 4aa806530974..358996897efc 100644 --- a/drivers/s390/cio/vfio_ccw_private.h +++ b/drivers/s390/cio/vfio_ccw_private.h @@ -111,6 +111,10 @@ struct vfio_ccw_private { struct eventfd_ctx *req_trigger; struct work_struct io_work; struct work_struct crw_work; + + struct mdev_parent parent; + struct mdev_type mdev_type; + struct mdev_type *mdev_types[1]; } __aligned(8); int vfio_ccw_sch_quiesce(struct subchannel *sch);
Here's my swipe at a cleanup patch that can be folded in to this series, to get the mdev stuff in a more proper location for vfio-ccw. As previously described, the subchannel is a device-agnostic structure that does/should not need to know about specific nuances such as mediated devices. This is why things like struct vfio_ccw_private exist, so move these details there. Signed-off-by: Eric Farman <farman@linux.ibm.com> --- drivers/s390/cio/cio.h | 3 --- drivers/s390/cio/vfio_ccw_drv.c | 13 +++++++------ drivers/s390/cio/vfio_ccw_private.h | 4 ++++ 3 files changed, 11 insertions(+), 9 deletions(-)