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

[Cornelia Huck]

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.)

> 
> >   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);
> > +}
> > +  
>