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

[Farhan Ali <alifm@linux.ibm.com>]

On 06/11/2019 07:37 AM, Cornelia Huck wrote:
> 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.
> 

Okay, now I get it. Thanks for the explanation, I really do appreciate 
it! :)

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

Yeah I agree, given the constraints I don't think any code juggling 
would look any prettier.

Thanks
Farhan