| Message ID | 20200417023001.65006-6-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/vfio-ccw: Channel Path Handling [KVM] | expand |
On Fri, 17 Apr 2020 04:29:58 +0200 Eric Farman <farman@linux.ibm.com> wrote: > From: Farhan Ali <alifm@linux.ibm.com> > > This region provides a mechanism to pass Channel Report Word(s) > that affect vfio-ccw devices, and need to be passed to the guest > for its awareness and/or processing. > > The base driver (see crw_collect_info()) provides space for two > CRWs, as a subchannel event may have two CRWs chained together > (one for the ssid, one for the subcahnnel). All other CRWs will > only occupy the first one. Even though this support will also > only utilize the first one, we'll provide space for two also. This is no longer true? > > Signed-off-by: Farhan Ali <alifm@linux.ibm.com> > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > > Notes: > v2->v3: > - Remove "if list-empty" check, since there's no list yet [EF] > - Reduce the CRW region to one fullword, instead of two [CH] > > v1->v2: > - Add new region info to Documentation/s390/vfio-ccw.rst [CH] > - Add a block comment to struct ccw_crw_region [CH] > > v0->v1: [EF] > - Clean up checkpatch (whitespace) errors > - Add ret=-ENOMEM in error path for new region > - Add io_mutex for region read (originally in last patch) > - Change crw1/crw2 to crw0/crw1 > - Reorder cleanup of regions > > Documentation/s390/vfio-ccw.rst | 16 +++++++++ > drivers/s390/cio/vfio_ccw_chp.c | 53 +++++++++++++++++++++++++++++ > drivers/s390/cio/vfio_ccw_drv.c | 20 +++++++++++ > drivers/s390/cio/vfio_ccw_ops.c | 4 +++ > drivers/s390/cio/vfio_ccw_private.h | 3 ++ > include/uapi/linux/vfio.h | 1 + > include/uapi/linux/vfio_ccw.h | 8 +++++ > 7 files changed, 105 insertions(+) > > diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst > index 98832d95f395..3338551ef642 100644 > --- a/Documentation/s390/vfio-ccw.rst > +++ b/Documentation/s390/vfio-ccw.rst > @@ -247,6 +247,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB. > Reading this region triggers a STORE SUBCHANNEL to be issued to the > associated hardware. > > +vfio-ccw crw region > +--------------------- > + > +The vfio-ccw crw region is used to return Channel Report Word (CRW) > +data to userspace:: > + > + struct ccw_crw_region { > + __u32 crw; > + } __packed; > + > +This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_CRW. > + > +Currently, space is provided for a single CRW. Handling of chained > +CRWs (not implemented in vfio-ccw) can be accomplished by re-reading > +the region for additional CRW data. What about the following instead: "Reading this region returns a CRW if one that is relevant for this subchannel (e.g. one reporting changes in channel path state) is pending, or all zeroes if not. If multiple CRWs are pending (including possibly chained CRWs), reading this region again will return the next one, until no more CRWs are pending and zeroes are returned. This is similar to how STORE CHANNEL REPORT WORD works." > + > vfio-ccw operation details > -------------------------- > > diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c > index 18f3b3e873a9..c1362aae61f5 100644 > --- a/drivers/s390/cio/vfio_ccw_chp.c > +++ b/drivers/s390/cio/vfio_ccw_chp.c > @@ -74,3 +74,56 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private) > VFIO_REGION_INFO_FLAG_READ, > private->schib_region); > } > + > +static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private, > + char __user *buf, size_t count, > + loff_t *ppos) > +{ > + unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS; > + loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK; > + struct ccw_crw_region *region; > + int ret; > + > + if (pos + count > sizeof(*region)) > + return -EINVAL; > + > + mutex_lock(&private->io_mutex); > + region = private->region[i].data; > + > + if (copy_to_user(buf, (void *)region + pos, count)) > + ret = -EFAULT; > + else > + ret = count; I see that you implemented it a bit differently in patch 7, but I think resetting crw to 0 immediately after the copy_to_user() is cleaner. It also can be done in this patch already :) > + > + mutex_unlock(&private->io_mutex); > + return ret; > +} (...) > diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h > index 758bf214898d..faf57e691d4a 100644 > --- a/include/uapi/linux/vfio_ccw.h > +++ b/include/uapi/linux/vfio_ccw.h > @@ -44,4 +44,12 @@ struct ccw_schib_region { > __u8 schib_area[SCHIB_AREA_SIZE]; > } __packed; > > +/* > + * Used for returning Channel Report Word(s) to userspace. s/Channel Report Word(s)/a Channel Report Word/ ? > + * Note: this is controlled by a capability > + */ > +struct ccw_crw_region { > + __u32 crw; > +} __packed; > + > #endif
On 4/21/20 5:41 AM, Cornelia Huck wrote: > On Fri, 17 Apr 2020 04:29:58 +0200 > Eric Farman <farman@linux.ibm.com> wrote: > >> From: Farhan Ali <alifm@linux.ibm.com> >> >> This region provides a mechanism to pass Channel Report Word(s) >> that affect vfio-ccw devices, and need to be passed to the guest >> for its awareness and/or processing. >> >> The base driver (see crw_collect_info()) provides space for two >> CRWs, as a subchannel event may have two CRWs chained together >> (one for the ssid, one for the subcahnnel). All other CRWs will s/subcahnnel/subchannel/ >> only occupy the first one. Even though this support will also >> only utilize the first one, we'll provide space for two also. > > This is no longer true? Oops, yes. > >> >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com> >> Signed-off-by: Eric Farman <farman@linux.ibm.com> >> --- >> >> Notes: >> v2->v3: >> - Remove "if list-empty" check, since there's no list yet [EF] >> - Reduce the CRW region to one fullword, instead of two [CH] >> >> v1->v2: >> - Add new region info to Documentation/s390/vfio-ccw.rst [CH] >> - Add a block comment to struct ccw_crw_region [CH] >> >> v0->v1: [EF] >> - Clean up checkpatch (whitespace) errors >> - Add ret=-ENOMEM in error path for new region >> - Add io_mutex for region read (originally in last patch) >> - Change crw1/crw2 to crw0/crw1 >> - Reorder cleanup of regions >> >> Documentation/s390/vfio-ccw.rst | 16 +++++++++ >> drivers/s390/cio/vfio_ccw_chp.c | 53 +++++++++++++++++++++++++++++ >> drivers/s390/cio/vfio_ccw_drv.c | 20 +++++++++++ >> drivers/s390/cio/vfio_ccw_ops.c | 4 +++ >> drivers/s390/cio/vfio_ccw_private.h | 3 ++ >> include/uapi/linux/vfio.h | 1 + >> include/uapi/linux/vfio_ccw.h | 8 +++++ >> 7 files changed, 105 insertions(+) >> >> diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst >> index 98832d95f395..3338551ef642 100644 >> --- a/Documentation/s390/vfio-ccw.rst >> +++ b/Documentation/s390/vfio-ccw.rst >> @@ -247,6 +247,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB. >> Reading this region triggers a STORE SUBCHANNEL to be issued to the >> associated hardware. >> >> +vfio-ccw crw region >> +--------------------- >> + >> +The vfio-ccw crw region is used to return Channel Report Word (CRW) >> +data to userspace:: >> + >> + struct ccw_crw_region { >> + __u32 crw; >> + } __packed; >> + >> +This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_CRW. >> + >> +Currently, space is provided for a single CRW. Handling of chained >> +CRWs (not implemented in vfio-ccw) can be accomplished by re-reading >> +the region for additional CRW data. > > What about the following instead: > > "Reading this region returns a CRW if one that is relevant for this > subchannel (e.g. one reporting changes in channel path state) is > pending, or all zeroes if not. If multiple CRWs are pending (including > possibly chained CRWs), reading this region again will return the next > one, until no more CRWs are pending and zeroes are returned. This is > similar to how STORE CHANNEL REPORT WORD works." Sounds good to me. Hrm... Maybe coffee hasn't hit yet. Should I wire STCRW into this, or just rely on the notification from the host to trigger the read? > >> + >> vfio-ccw operation details >> -------------------------- >> >> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c >> index 18f3b3e873a9..c1362aae61f5 100644 >> --- a/drivers/s390/cio/vfio_ccw_chp.c >> +++ b/drivers/s390/cio/vfio_ccw_chp.c >> @@ -74,3 +74,56 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private) >> VFIO_REGION_INFO_FLAG_READ, >> private->schib_region); >> } >> + >> +static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private, >> + char __user *buf, size_t count, >> + loff_t *ppos) >> +{ >> + unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS; >> + loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK; >> + struct ccw_crw_region *region; >> + int ret; >> + >> + if (pos + count > sizeof(*region)) >> + return -EINVAL; >> + >> + mutex_lock(&private->io_mutex); >> + region = private->region[i].data; >> + >> + if (copy_to_user(buf, (void *)region + pos, count)) >> + ret = -EFAULT; >> + else >> + ret = count; > > I see that you implemented it a bit differently in patch 7, but I think > resetting crw to 0 immediately after the copy_to_user() is cleaner. It > also can be done in this patch already :) Ha, yes. I actually had it set that way, but didn't like the result in patch 7. I'll revisit that. > >> + >> + mutex_unlock(&private->io_mutex); >> + return ret; >> +} > > (...) > >> diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h >> index 758bf214898d..faf57e691d4a 100644 >> --- a/include/uapi/linux/vfio_ccw.h >> +++ b/include/uapi/linux/vfio_ccw.h >> @@ -44,4 +44,12 @@ struct ccw_schib_region { >> __u8 schib_area[SCHIB_AREA_SIZE]; >> } __packed; >> >> +/* >> + * Used for returning Channel Report Word(s) to userspace. > > s/Channel Report Word(s)/a Channel Report Word/ ? Nod. > >> + * Note: this is controlled by a capability >> + */ >> +struct ccw_crw_region { >> + __u32 crw; >> +} __packed; >> + >> #endif >
On Tue, 21 Apr 2020 07:02:03 -0400 Eric Farman <farman@linux.ibm.com> wrote: > On 4/21/20 5:41 AM, Cornelia Huck wrote: > > On Fri, 17 Apr 2020 04:29:58 +0200 > > Eric Farman <farman@linux.ibm.com> wrote: > >> diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst > >> index 98832d95f395..3338551ef642 100644 > >> --- a/Documentation/s390/vfio-ccw.rst > >> +++ b/Documentation/s390/vfio-ccw.rst > >> @@ -247,6 +247,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB. > >> Reading this region triggers a STORE SUBCHANNEL to be issued to the > >> associated hardware. > >> > >> +vfio-ccw crw region > >> +--------------------- > >> + > >> +The vfio-ccw crw region is used to return Channel Report Word (CRW) > >> +data to userspace:: > >> + > >> + struct ccw_crw_region { > >> + __u32 crw; > >> + } __packed; > >> + > >> +This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_CRW. > >> + > >> +Currently, space is provided for a single CRW. Handling of chained > >> +CRWs (not implemented in vfio-ccw) can be accomplished by re-reading > >> +the region for additional CRW data. > > > > What about the following instead: > > > > "Reading this region returns a CRW if one that is relevant for this > > subchannel (e.g. one reporting changes in channel path state) is > > pending, or all zeroes if not. If multiple CRWs are pending (including > > possibly chained CRWs), reading this region again will return the next > > one, until no more CRWs are pending and zeroes are returned. This is > > similar to how STORE CHANNEL REPORT WORD works." > > Sounds good to me. > > Hrm... Maybe coffee hasn't hit yet. Should I wire STCRW into this, or > just rely on the notification from the host to trigger the read? Userspace is supposed to use this to get crws to inject into the guest, no stcrw involved until the guest actually got the machine check for it.
On 4/21/20 7:08 AM, Cornelia Huck wrote: > On Tue, 21 Apr 2020 07:02:03 -0400 > Eric Farman <farman@linux.ibm.com> wrote: > >> On 4/21/20 5:41 AM, Cornelia Huck wrote: >>> On Fri, 17 Apr 2020 04:29:58 +0200 >>> Eric Farman <farman@linux.ibm.com> wrote: > >>>> diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst >>>> index 98832d95f395..3338551ef642 100644 >>>> --- a/Documentation/s390/vfio-ccw.rst >>>> +++ b/Documentation/s390/vfio-ccw.rst >>>> @@ -247,6 +247,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB. >>>> Reading this region triggers a STORE SUBCHANNEL to be issued to the >>>> associated hardware. >>>> >>>> +vfio-ccw crw region >>>> +--------------------- >>>> + >>>> +The vfio-ccw crw region is used to return Channel Report Word (CRW) >>>> +data to userspace:: >>>> + >>>> + struct ccw_crw_region { >>>> + __u32 crw; >>>> + } __packed; >>>> + >>>> +This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_CRW. >>>> + >>>> +Currently, space is provided for a single CRW. Handling of chained >>>> +CRWs (not implemented in vfio-ccw) can be accomplished by re-reading >>>> +the region for additional CRW data. >>> >>> What about the following instead: >>> >>> "Reading this region returns a CRW if one that is relevant for this >>> subchannel (e.g. one reporting changes in channel path state) is >>> pending, or all zeroes if not. If multiple CRWs are pending (including >>> possibly chained CRWs), reading this region again will return the next >>> one, until no more CRWs are pending and zeroes are returned. This is >>> similar to how STORE CHANNEL REPORT WORD works." >> >> Sounds good to me. >> >> Hrm... Maybe coffee hasn't hit yet. Should I wire STCRW into this, or >> just rely on the notification from the host to trigger the read? > > Userspace is supposed to use this to get crws to inject into the guest, > no stcrw involved until the guest actually got the machine check for it. > Ah, yes. Thanks!
diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst index 98832d95f395..3338551ef642 100644 --- a/Documentation/s390/vfio-ccw.rst +++ b/Documentation/s390/vfio-ccw.rst @@ -247,6 +247,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB. Reading this region triggers a STORE SUBCHANNEL to be issued to the associated hardware. +vfio-ccw crw region +--------------------- + +The vfio-ccw crw region is used to return Channel Report Word (CRW) +data to userspace:: + + struct ccw_crw_region { + __u32 crw; + } __packed; + +This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_CRW. + +Currently, space is provided for a single CRW. Handling of chained +CRWs (not implemented in vfio-ccw) can be accomplished by re-reading +the region for additional CRW data. + vfio-ccw operation details -------------------------- diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c index 18f3b3e873a9..c1362aae61f5 100644 --- a/drivers/s390/cio/vfio_ccw_chp.c +++ b/drivers/s390/cio/vfio_ccw_chp.c @@ -74,3 +74,56 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private) VFIO_REGION_INFO_FLAG_READ, private->schib_region); } + +static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private, + char __user *buf, size_t count, + loff_t *ppos) +{ + unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS; + loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK; + struct ccw_crw_region *region; + int ret; + + if (pos + count > sizeof(*region)) + return -EINVAL; + + mutex_lock(&private->io_mutex); + region = private->region[i].data; + + if (copy_to_user(buf, (void *)region + pos, count)) + ret = -EFAULT; + else + ret = count; + + mutex_unlock(&private->io_mutex); + return ret; +} + +static ssize_t vfio_ccw_crw_region_write(struct vfio_ccw_private *private, + const char __user *buf, size_t count, + loff_t *ppos) +{ + return -EINVAL; +} + +static void vfio_ccw_crw_region_release(struct vfio_ccw_private *private, + struct vfio_ccw_region *region) +{ + +} + +const struct vfio_ccw_regops vfio_ccw_crw_region_ops = { + .read = vfio_ccw_crw_region_read, + .write = vfio_ccw_crw_region_write, + .release = vfio_ccw_crw_region_release, +}; + +int vfio_ccw_register_crw_dev_regions(struct vfio_ccw_private *private) +{ + return vfio_ccw_register_dev_region(private, + VFIO_REGION_SUBTYPE_CCW_CRW, + &vfio_ccw_crw_region_ops, + sizeof(struct ccw_crw_region), + VFIO_REGION_INFO_FLAG_READ, + private->crw_region); +} diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c index 8e05ed611c10..7893027c3a8f 100644 --- a/drivers/s390/cio/vfio_ccw_drv.c +++ b/drivers/s390/cio/vfio_ccw_drv.c @@ -28,6 +28,7 @@ struct workqueue_struct *vfio_ccw_work_q; static struct kmem_cache *vfio_ccw_io_region; static struct kmem_cache *vfio_ccw_cmd_region; static struct kmem_cache *vfio_ccw_schib_region; +static struct kmem_cache *vfio_ccw_crw_region; debug_info_t *vfio_ccw_debug_msg_id; debug_info_t *vfio_ccw_debug_trace_id; @@ -120,6 +121,8 @@ static void vfio_ccw_sch_irq(struct subchannel *sch) static void vfio_ccw_free_regions(struct vfio_ccw_private *private) { + if (private->crw_region) + kmem_cache_free(vfio_ccw_crw_region, private->crw_region); if (private->schib_region) kmem_cache_free(vfio_ccw_schib_region, private->schib_region); if (private->cmd_region) @@ -165,6 +168,12 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) if (!private->schib_region) goto out_free; + private->crw_region = kmem_cache_zalloc(vfio_ccw_crw_region, + GFP_KERNEL | GFP_DMA); + + if (!private->crw_region) + goto out_free; + private->sch = sch; dev_set_drvdata(&sch->dev, private); mutex_init(&private->io_mutex); @@ -364,6 +373,7 @@ static void vfio_ccw_debug_exit(void) static void vfio_ccw_destroy_regions(void) { + kmem_cache_destroy(vfio_ccw_crw_region); kmem_cache_destroy(vfio_ccw_schib_region); kmem_cache_destroy(vfio_ccw_cmd_region); kmem_cache_destroy(vfio_ccw_io_region); @@ -411,6 +421,16 @@ static int __init vfio_ccw_sch_init(void) goto out_err; } + vfio_ccw_crw_region = kmem_cache_create_usercopy("vfio_ccw_crw_region", + sizeof(struct ccw_crw_region), 0, + SLAB_ACCOUNT, 0, + sizeof(struct ccw_crw_region), NULL); + + if (!vfio_ccw_crw_region) { + ret = -ENOMEM; + goto out_err; + } + isc_register(VFIO_CCW_ISC); ret = css_driver_register(&vfio_ccw_sch_driver); if (ret) { diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c index 22988d67b6bb..edec0fb7ace8 100644 --- a/drivers/s390/cio/vfio_ccw_ops.c +++ b/drivers/s390/cio/vfio_ccw_ops.c @@ -178,6 +178,10 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev) if (ret) goto out_unregister; + ret = vfio_ccw_register_crw_dev_regions(private); + if (ret) + goto out_unregister; + return ret; out_unregister: diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h index d6601a8adf13..8289b6850e59 100644 --- a/drivers/s390/cio/vfio_ccw_private.h +++ b/drivers/s390/cio/vfio_ccw_private.h @@ -57,6 +57,7 @@ void vfio_ccw_unregister_dev_regions(struct vfio_ccw_private *private); int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private); int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private); +int vfio_ccw_register_crw_dev_regions(struct vfio_ccw_private *private); /** * struct vfio_ccw_private @@ -71,6 +72,7 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private); * @region: additional regions for other subchannel operations * @cmd_region: MMIO region for asynchronous I/O commands other than START * @schib_region: MMIO region for SCHIB information + * @crw_region: MMIO region for getting channel report words * @num_regions: number of additional regions * @cp: channel program for the current I/O operation * @irb: irb info received from interrupt @@ -90,6 +92,7 @@ struct vfio_ccw_private { struct vfio_ccw_region *region; struct ccw_cmd_region *cmd_region; struct ccw_schib_region *schib_region; + struct ccw_crw_region *crw_region; int num_regions; struct channel_program cp; diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index 7a1abbd889bd..469f813749f1 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -379,6 +379,7 @@ struct vfio_region_gfx_edid { /* sub-types for VFIO_REGION_TYPE_CCW */ #define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD (1) #define VFIO_REGION_SUBTYPE_CCW_SCHIB (2) +#define VFIO_REGION_SUBTYPE_CCW_CRW (3) /* * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h index 758bf214898d..faf57e691d4a 100644 --- a/include/uapi/linux/vfio_ccw.h +++ b/include/uapi/linux/vfio_ccw.h @@ -44,4 +44,12 @@ struct ccw_schib_region { __u8 schib_area[SCHIB_AREA_SIZE]; } __packed; +/* + * Used for returning Channel Report Word(s) to userspace. + * Note: this is controlled by a capability + */ +struct ccw_crw_region { + __u32 crw; +} __packed; + #endif