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

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

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