Re: [qemu-s390x] [PATCH v5] vfio-ccw: support async command subregion

[Farhan Ali]

On 06/07/2019 11:09 AM, Cornelia Huck wrote:
> On Fri, 7 Jun 2019 11:02:36 -0400
> Farhan Ali <address@hidden> wrote:
>
> > On 06/07/2019 10:53 AM, 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 <address@hidden>
> > > ---
> > >
> > > v4->v5:
> > > - It seems we need to take the indirection via the class for the
> > >     callbacks after all :(
> > > - Dropped Eric's R-b: for that reason
> > >
> > > ---
> > >    hw/s390x/css.c              |  27 +++++++--
> > >    hw/s390x/s390-ccw.c         |  20 +++++++
> > >    hw/vfio/ccw.c               | 112 +++++++++++++++++++++++++++++++++++-
> > >    include/hw/s390x/css.h      |   3 +
> > >    include/hw/s390x/s390-ccw.h |   2 +
> > >    5 files changed, 158 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > > index ad310b9f94bc..b92395f165e6 100644
> > > --- a/hw/s390x/css.c
> > > +++ b/hw/s390x/css.c
> > > @@ -22,6 +22,7 @@
> > >    #include "trace.h"
> > >    #include "hw/s390x/s390_flic.h"
> > >    #include "hw/s390x/s390-virtio-ccw.h"
> > > +#include "hw/s390x/s390-ccw.h"
> > >    
> > >    typedef struct CrwContainer {
> > >        CRW crw;
> > > @@ -1205,6 +1206,26 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
> > >    
> > >    }
> > >    
> > > +static void sch_handle_halt_func_passthrough(SubchDev *sch)
> > > +{
> > > +    int ret;
> > > +
> > > +    ret = s390_ccw_halt(sch);
> > > +    if (ret == -ENOSYS) {
> > > +        sch_handle_halt_func(sch);
> > > +    }
> > > +}
> > > +
> > > +static void sch_handle_clear_func_passthrough(SubchDev *sch)
> > > +{
> > > +    int ret;
> > > +
> > > +    ret = s390_ccw_clear(sch);
> > > +    if (ret == -ENOSYS) {
> > > +        sch_handle_clear_func(sch);
> > > +    }
> > > +}
> > > +
> >
> > do we need an extra s390_ccw_clear/halt functions? can't we just call
> > cdc->clear/halt in the passthrough functions?
>
> I mostly added them for symmetry reasons... we still need to check for
> presence of the callback in any case, though.
>
> (vfio is not always built, e.g. on windows or os x.)


right, but if we are calling do_subchannel_work_passthrough, then we 
know for sure we are building the S390CCWDevice which is the vfio 
device, no?

So we could just add checks for callbacks in 
sch_handle_clear/halt_func_passthrough, no?

I would even like to get rid of the s390_ccw_cmd_request if we can, but 
that is out of scope for this patch. :)


> > >    static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
> > >    {
> > >        SCHIB *schib = &sch->curr_status;
> > > @@ -1244,11 +1265,9 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev 
> > > *sch)
> > >        SCHIB *schib = &sch->curr_status;
> > >    
> > >        if (schib->scsw.ctrl & SCSW_FCTL_CLEAR_FUNC) {
> > > -        /* TODO: Clear handling */
> > > -        sch_handle_clear_func(sch);
> > > +        sch_handle_clear_func_passthrough(sch);
> > >        } else if (schib->scsw.ctrl & SCSW_FCTL_HALT_FUNC) {
> > > -        /* TODO: Halt handling */
> > > -        sch_handle_halt_func(sch);
> > > +        sch_handle_halt_func_passthrough(sch);
> > >        } else if (schib->scsw.ctrl & SCSW_FCTL_START_FUNC) {
> > >            return sch_handle_start_func_passthrough(sch);
> > >        }
> > > diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> > > index f5f025d1b6ca..951be5ab0245 100644
> > > --- a/hw/s390x/s390-ccw.c
> > > +++ b/hw/s390x/s390-ccw.c
> > > @@ -29,6 +29,26 @@ IOInstEnding s390_ccw_cmd_request(SubchDev *sch)
> > >        return cdc->handle_request(sch);
> > >    }
> > >    
> > > +int s390_ccw_halt(SubchDev *sch)
> > > +{
> > > +    S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data);
> > > +
> > > +    if (!cdc->handle_halt) {
> > > +        return -ENOSYS;
> > > +    }
> > > +    return cdc->handle_halt(sch);
> > > +}
> > > +
> > > +int s390_ccw_clear(SubchDev *sch)
> > > +{
> > > +    S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data);
> > > +
> > > +    if (!cdc->handle_clear) {
> > > +        return -ENOSYS;
> > > +    }
> > > +    return cdc->handle_clear(sch);
> > > +}
> > > +