Re: [PATCH v3 3/7] vfio-ccw: Add support for the schib region

[Eric Farman]

On 4/22/20 7:36 AM, Cornelia Huck wrote:
> On Fri, 17 Apr 2020 04:34:36 +0200
> Eric Farman <address@hidden> wrote:
> 
>> From: Farhan Ali <address@hidden>
>>
>> The schib region can be used to obtain the latest SCHIB from the host
>> passthrough subchannel. Since the guest SCHIB is virtualized,
>> we currently only update the path related information so that the
>> guest is aware of any path related changes when it issues the
>> 'stsch' instruction.
>>
>> Signed-off-by: Farhan Ali <address@hidden>
>> Signed-off-by: Eric Farman <address@hidden>
>> ---
>>
>> Notes:
>>     v1->v2:
>>      - Remove silly variable intialization, and add a block comment,
>>        to css_do_stsch() [CH]
>>      - Add a TODO statement to s390_ccw_store(), for myself to sort
>>        out while we go over kernel code more closely [CH/EF]
>>      - In vfio_ccw_handle_store(),
>>         - Set schib pointer once region is determined to be non-NULL [CH]
>>         - Return cc=0 if pread() fails, and log an error [CH]
>>     
>>     v0->v1: [EF]
>>      - Change various incarnations of "update chp status" to
>>        "handle_store", to reflect the STSCH instruction that will
>>        drive this code
>>      - Remove temporary variable for casting/testing purposes in
>>        s390_ccw_store(), and add a block comment of WHY its there.
>>      - Add a few comments to vfio_ccw_handle_store()
>>
>>  hw/s390x/css.c              | 13 ++++++--
>>  hw/s390x/s390-ccw.c         | 28 +++++++++++++++++
>>  hw/vfio/ccw.c               | 63 +++++++++++++++++++++++++++++++++++++
>>  include/hw/s390x/css.h      |  3 +-
>>  include/hw/s390x/s390-ccw.h |  1 +
>>  target/s390x/ioinst.c       |  3 +-
>>  6 files changed, 106 insertions(+), 5 deletions(-)
>>
> 
>> +IOInstEnding s390_ccw_store(SubchDev *sch)
>> +{
>> +    S390CCWDeviceClass *cdc = NULL;
>> +    int ret = IOINST_CC_EXPECTED;
>> +
>> +    /*
>> +     * This only applies to passthrough devices, so we can't unconditionally
>> +     * set this variable like we would for halt/clear.
>> +     *
>> +     * TODO from Conny on v1:
>> +     *   "We have a generic ccw_cb in the subchannel structure for ccw
>> +     *    interpretation; would it make sense to add a generic callback
>> +     *    for stsch there as well?
>> +     *
>> +     *   "(This works fine, though. Might want to add the check for
>> +     *    halt/clear as well, but that might be a bit overkill.)"
> 
> Do you plan to look into that for the next version? If not, don't feel
> pressured.

Ah, yes.  Clearly I missed this before v3.

> 
>> +     */
>> +    if (object_dynamic_cast(OBJECT(sch->driver_data), TYPE_S390_CCW)) {
>> +        cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data);
>> +    }
>> +
>> +    if (cdc && cdc->handle_store) {
>> +        ret = cdc->handle_store(sch);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>  static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
>>                                    char *sysfsdev,
>>                                    Error **errp)
> 
> Generally, looks sane.
>