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

[Cornelia Huck <cohuck@redhat.com>]

On Fri, 7 Jun 2019 11:19:09 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> On 06/07/2019 11:09 AM, Cornelia Huck wrote:
> > On Fri, 7 Jun 2019 11:02:36 -0400
> > Farhan Ali <alifm@linux.ibm.com> wrote:
> >   
> >> On 06/07/2019 10:53 AM, Cornelia Huck wrote:  

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

Ok, I just walked through various source files (some of which are a bit
confusingly named) again and now I understand again why it was done
that way in the first place.

- hw/s390x/s390-ccw.c provides some interfaces for pass-through ccw
  devices. It is built unconditionally, and its interfaces are called
  unconditionally from the css code.
  It also provides a device class where code can hook up callbacks.
- hw/vfio/ccw.c (which is not built for !KVM) actually hooks up
  callbacks in that device class.

So, s390-ccw.c (not to be confused with ccw-device.c...) provides a
layer that makes it possible to call things unconditionally, regardless
whether we have vfio-ccw available or not. Not that the code ends up
being called without vfio-ccw support; but the class indirection
enables the code to be built.

There's possibly a way to make this work without the class indirection
as well, but I think we'd end up doing code juggling before ending up
with something that's not really nicer than the code we have now.
Therefore, I'd prefer to keep the class handling and hook up the
halt/clear callbacks there as well.