On Mon, 6 Apr 2020 17:43:42 -0400
Eric Farman <farman@xxxxxxxxxxxxx> wrote:
> (Ah, didn't see this come in earlier.)
>
> On 4/6/20 9:40 AM, Cornelia Huck wrote:
> > On Thu, 6 Feb 2020 22:38:21 +0100
> > Eric Farman <farman@xxxxxxxxxxxxx> wrote:
> >
> >> From: Farhan Ali <alifm@xxxxxxxxxxxxx>
> >>
> >> 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.
> >>
> >> Signed-off-by: Farhan Ali <alifm@xxxxxxxxxxxxx>
> >> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
> >> ---
> >>
> >> Notes:
> >> 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 | 15 ++++++++
> >> drivers/s390/cio/vfio_ccw_chp.c | 56 +++++++++++++++++++++++++++++
> >> 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 | 9 +++++
> >> 7 files changed, 108 insertions(+)
> >>
> >
> > (...)
> >
> >> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> >> index 826d08379fe3..8fde94552149 100644
> >> --- a/drivers/s390/cio/vfio_ccw_chp.c
> >> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> >> @@ -73,3 +73,59 @@ 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;
> >> +
> >> + if (list_empty(&private->crw))
> >> + return 0;
>
> This is actually nonsense, because the list isn't introduced for a
> couple of patches.
>
> >> +
> >> + 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;
> >> +}
> >
> > Would it make sense to clear out the crw after it has been read by
> > userspace?
>
> Yes, I fixed this up in my in-progress v3 last week. Sorry to waste
> some time, I should have mentioned it when I changed my branch locally.
>
> I changed the list_empty() check above to bail out if the region is
> already zero, which I guess is why the userspace check looks for both.
> But if we only look at the contents of the region, then checking both is
> unnecessary. Just do the copy and be done with it.
Yes, that makes sense.
>
> >
> > In patch 7, you add a notification for a new crw via eventfd, but
> > nothing is preventing userspace from reading this even if not
> > triggered. I also don't see the region being updated there until a new
> > crw is posted.
> >
> > Or am I missing something?
> >
>
