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

[Cornelia Huck]

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.

> +     */
> +    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.