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

[Cornelia Huck]

On Tue,  5 May 2020 14:57:54 +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         | 21 +++++++++++++
>  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, 99 insertions(+), 5 deletions(-)
> 
(...)
>  
> +static IOInstEnding vfio_ccw_handle_store(SubchDev *sch)
> +{
> +    S390CCWDevice *cdev = sch->driver_data;
> +    VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
> +    SCHIB *schib = &sch->curr_status;
> +    struct ccw_schib_region *region = vcdev->schib_region;
> +    SCHIB *s;
> +    int ret;
> +
> +    /* schib region not available so nothing else to do */
> +    if (!region) {
> +        return IOINST_CC_EXPECTED;
> +    }
> +
> +    memset(region, 0, sizeof(*region));
> +    ret = pread(vcdev->vdev.fd, region, vcdev->schib_region_size,
> +                vcdev->schib_region_offset);
> +
> +    if (ret == -1) {
> +        /*
> +         * Device is probably damaged, but store subchannel does not
> +         * have a nonzero cc defined for this scenario.  Log an error,
> +         * and presume things are otherwise fine.

One thing we might do is set the device number valid bit to 0 in our
local copy and inject a (subchannel) crw. Maybe as a patch on top later.

> +         */
> +        error_report("vfio-ccw: store region read failed with errno=%d", 
> errno);
> +        return IOINST_CC_EXPECTED;
> +    }
> +
> +    /*
> +     * Selectively copy path-related bits of the SCHIB,
> +     * rather than copying the entire struct.
> +     */
> +    s = (SCHIB *)region->schib_area;
> +    schib->pmcw.pnom = s->pmcw.pnom;
> +    schib->pmcw.lpum = s->pmcw.lpum;
> +    schib->pmcw.pam = s->pmcw.pam;
> +    schib->pmcw.pom = s->pmcw.pom;
> +
> +    if (s->scsw.flags & SCSW_FLAGS_MASK_PNO) {
> +        schib->scsw.flags |= SCSW_FLAGS_MASK_PNO;
> +    }
> +
> +    return IOINST_CC_EXPECTED;
> +}
> +

Reviewed-by: Cornelia Huck <address@hidden>