[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 <far...@linux.ibm.com> wrote:
>
>> From: Farhan Ali <al...@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 <al...@linux.ibm.com>
>> Signed-off-by: Eric Farman <far...@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.
>
> (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?
>
>> + }
>> +
>> + /*
>> + * 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.
>
