Re: [Qemu-devel] [PATCH 2/2] vfio-ccw: support async command subregion

[Cornelia Huck <cohuck@redhat.com>]

On Mon, 26 Nov 2018 19:01:55 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 26/11/2018 10:58, Cornelia Huck wrote:
> > On Fri, 23 Nov 2018 15:13:12 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >   
> >> On 22/11/2018 17:54, Cornelia Huck wrote:  
> >>> A vfio-ccw device may provide an async command subregion for
> >>> issuing halt/clear subchannel requests. If it is present, use
> >>> it for sending halt/clear request to the device; if not, fall
> >>> back to emulation (as done today).
> >>>
> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> >>> ---
> >>>    hw/s390x/css.c              |  27 +++++++--
> >>>    hw/vfio/ccw.c               | 109 +++++++++++++++++++++++++++++++++++-
> >>>    include/hw/s390x/s390-ccw.h |   3 +
> >>>    3 files changed, 133 insertions(+), 6 deletions(-)
> >>>  
> >   
> >>> @@ -114,6 +120,87 @@ again:
> >>>        }
> >>>    }
> >>>    
> >>> +int vfio_ccw_handle_clear(SubchDev *sch)
> >>> +{
> >>> +    S390CCWDevice *cdev = sch->driver_data;
> >>> +    VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
> >>> +    struct ccw_cmd_region *region = vcdev->async_cmd_region;
> >>> +    int ret;
> >>> +
> >>> +    if (!vcdev->async_cmd_region) {
> >>> +        /* Async command region not available, fall back to emulation */
> >>> +        return -ENOSYS;
> >>> +    }
> >>> +
> >>> +    memset(region, 0, sizeof(*region));
> >>> +    region->command = VFIO_CCW_ASYNC_CMD_CSCH;
> >>> +
> >>> +again:
> >>> +    ret = pwrite(vcdev->vdev.fd, region,
> >>> +                 vcdev->async_cmd_region_size, vcdev->async_cmd_region_offset);
> >>> +    if (ret != vcdev->async_cmd_region_size) {
> >>> +        if (errno == EAGAIN) {  
> >>
> >>
> >> Where do the EAGAIN come from?  
> > 
> > It might be set by pwrite.  
> 
> I saw that the man indicate this, and so we are legitimate to handle the 
> fail case, but I did not find EAGAIN in the path of the write for 
> accessing devices and I did not find it in the access to the CSS.
> 
> If we do not set it explicitly from the driver, the concern I have is: 
> isn't it dangerous to try again and shouldn't we better abort?

If it is set by the reason stated in the man page, retry sounds like
the sensible thing, doesn't it? I don't think I've yet seen it in
practice, though.

[I don't think we should need to comb through the whole code path to
find out what might happen or not, at some point we'll just have to
trust the documentation.]