Re: [RFC PATCH v1 5/8] vfio-ccw: Add support for the schib region

[Cornelia Huck <cohuck@redhat.com>]

On Fri, 31 Jan 2020 15:15:00 -0500
Eric Farman <farman@linux.ibm.com> wrote:

> [Had two un-addressed QEMU comments on my todo list; let's do that now.]
> 
> On 11/20/19 6:13 AM, Cornelia Huck wrote:
> > On Fri, 15 Nov 2019 04:34:34 +0100
> > Eric Farman <farman@linux.ibm.com> wrote:

Oh, that was some time ago... hope I can still recall what I was
thinking back then.

> >   
> >> From: Farhan Ali <alifm@linux.ibm.com>
> >>
> >> 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.  
> > 
> > One important information here is that you tweak the generic stsch
> > handling, although the behaviour remains the same for non-vfio
> > subchannels.
> >   
> >>
> >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> >> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> >> ---
> >>
> >> Notes:
> >>     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              |  8 ++++--
> >>  hw/s390x/s390-ccw.c         | 20 +++++++++++++
> >>  hw/vfio/ccw.c               | 57 +++++++++++++++++++++++++++++++++++++
> >>  include/hw/s390x/css.h      |  3 +-
> >>  include/hw/s390x/s390-ccw.h |  1 +
> >>  target/s390x/ioinst.c       |  3 +-
> >>  6 files changed, 87 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index 844caab408..6ee6a96d75 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -1335,11 +1335,15 @@ static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src)
> >>      }
> >>  }
> >>  
> >> -int css_do_stsch(SubchDev *sch, SCHIB *schib)
> >> +IOInstEnding css_do_stsch(SubchDev *sch, SCHIB *schib)
> >>  {
> >> +    int ret = IOINST_CC_EXPECTED;  
> > 
> > It's a bit useless initializing ret here, given that you use it right
> > below :)
> >   
> >> +  
> > 
> > Maybe add a comment here:
> > 
> >        /*
> >         * For some subchannels, we may want to update parts of
> >         * the schib (e.g. update path masks from the host device
> >         * for passthrough subchannels).
> >         */
> >   
> >> +    ret = s390_ccw_store(sch);
> >> +
> >>      /* Use current status. */
> >>      copy_schib_to_guest(schib, &sch->curr_status);
> >> -    return 0;
> >> +    return ret;
> >>  }
> >>  
> >>  static void copy_pmcw_from_guest(PMCW *dest, const PMCW *src)
> >> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> >> index 0c5a5b60bd..be0b379338 100644
> >> --- a/hw/s390x/s390-ccw.c
> >> +++ b/hw/s390x/s390-ccw.c
> >> @@ -51,6 +51,26 @@ int s390_ccw_clear(SubchDev *sch)
> >>      return cdc->handle_clear(sch);
> >>  }
> >>  
> >> +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.  
> > 
> > Hm, can we maybe handle this differently? 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?  
> 
> Might make things a little cleaner, but I am not sure.  I'm going to
> leave a todo here while we sort out the rest of the kernel code, so I
> can try to put something more beneficial together.

Yes, this looks like something that can be sorted out later.

What bothers me most here, I guess, is that it is not very obvious why
the functions above can assume the presence of the cdc object, while
this one can't. The secret sauce is in the different
do_subchannel_work_* callbacks, which reside in another file... maybe
amend the comment here to mention that? That would help and doesn't
involve code refactoring :)

> 
> > 
> > (This works fine, though. Might want to add the check for halt/clear as
> > well, but that might be a bit overkill.)
> >   
> >> +     */
> >> +    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)
> >> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> >> index 3e32bc1819..2ff223470f 100644
> >> --- a/hw/vfio/ccw.c
> >> +++ b/hw/vfio/ccw.c
> >> @@ -41,6 +41,9 @@ struct VFIOCCWDevice {
> >>      uint64_t async_cmd_region_size;
> >>      uint64_t async_cmd_region_offset;
> >>      struct ccw_cmd_region *async_cmd_region;
> >> +    uint64_t schib_region_size;
> >> +    uint64_t schib_region_offset;
> >> +    struct ccw_schib_region *schib_region;
> >>      EventNotifier io_notifier;
> >>      bool force_orb_pfch;
> >>      bool warned_orb_pfch;
> >> @@ -124,6 +127,45 @@ again:
> >>      }
> >>  }
> >>  
> >> +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 = (SCHIB *)region->schib_area;  
> > 
> > You probably don't want to access region until after you checked it's
> > not NULL :)  
> 
> :)
> 
> >   
> >> +    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) {
> >> +        /* Assume device is damaged, regardless of errno */
> >> +        return IOINST_CC_NOT_OPERATIONAL;  
> > 
> > Not sure that's correct; cc 3 for stsch indicates that there are no
> > more subchannels with higher ids?  
> 
> POPS says cc=3 on stsch if the subchannel is not operational, and it's
> not operational if it's not provided to the channel subsystem.  So,
> basically yes.  I guess his idea was to call out a problem on the kvm
> side, so should we just return CC_EXPECTED here, as if there's no region?

Yes, that looks like the better choice (out of the two possible
condition codes...) And also log a moan somewhere?

> 
> >   
> >> +    }
> >> +
> >> +    /*
> >> +     * Selectively copy path-related bits of the SCHIB,
> >> +     * rather than copying the entire struct.
> >> +     */
> >> +    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;
> >> +}
> >> +  
> > 
> > The rest of the patch looks good to me.
> >   
>