| Message ID | 20200417023440.70514-4-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/vfio-ccw: Channel Path Handling [QEMU] | expand |
On Fri, 17 Apr 2020 04:34:36 +0200 Eric Farman <farman@linux.ibm.com> wrote: > 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. > > Signed-off-by: Farhan Ali <alifm@linux.ibm.com> > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > > 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.
On 4/22/20 7:36 AM, Cornelia Huck wrote: > On Fri, 17 Apr 2020 04:34:36 +0200 > Eric Farman <farman@linux.ibm.com> wrote: > >> 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. >> >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com> >> Signed-off-by: Eric Farman <farman@linux.ibm.com> >> --- >> >> 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. >
diff --git a/hw/s390x/css.c b/hw/s390x/css.c index 5d8e08667e..a44faa3549 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -1335,11 +1335,20 @@ 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; + + /* + * 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..0c619706a1 100644 --- a/hw/s390x/s390-ccw.c +++ b/hw/s390x/s390-ccw.c @@ -51,6 +51,34 @@ 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. + * + * 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.)" + */ + 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 ae9e396367..8aa224bf43 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; @@ -123,6 +126,51 @@ 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; + 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. + */ + 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; +} + static int vfio_ccw_handle_clear(SubchDev *sch) { S390CCWDevice *cdev = sch->driver_data; @@ -389,10 +437,23 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) vcdev->async_cmd_region = g_malloc0(info->size); } + ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW, + VFIO_REGION_SUBTYPE_CCW_SCHIB, &info); + if (!ret) { + vcdev->schib_region_size = info->size; + if (sizeof(*vcdev->schib_region) != vcdev->schib_region_size) { + error_setg(errp, "vfio: Unexpected size of the schib region"); + goto out_err; + } + vcdev->schib_region_offset = info->offset; + vcdev->schib_region = g_malloc(info->size); + } + g_free(info); return; out_err: + g_free(vcdev->schib_region); g_free(vcdev->async_cmd_region); g_free(vcdev->io_region); g_free(info); @@ -401,6 +462,7 @@ out_err: static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) { + g_free(vcdev->schib_region); g_free(vcdev->async_cmd_region); g_free(vcdev->io_region); } @@ -576,6 +638,7 @@ static void vfio_ccw_class_init(ObjectClass *klass, void *data) cdc->handle_request = vfio_ccw_handle_request; cdc->handle_halt = vfio_ccw_handle_halt; cdc->handle_clear = vfio_ccw_handle_clear; + cdc->handle_store = vfio_ccw_handle_store; } static const TypeInfo vfio_ccw_info = { diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h index f46bcafb16..7e3a5e7433 100644 --- a/include/hw/s390x/css.h +++ b/include/hw/s390x/css.h @@ -218,6 +218,7 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sub); int s390_ccw_halt(SubchDev *sch); int s390_ccw_clear(SubchDev *sch); +IOInstEnding s390_ccw_store(SubchDev *sch); typedef enum { CSS_IO_ADAPTER_VIRTIO = 0, @@ -242,7 +243,7 @@ SubchDev *css_find_subch(uint8_t m, uint8_t cssid, uint8_t ssid, uint16_t schid); bool css_subch_visible(SubchDev *sch); void css_conditional_io_interrupt(SubchDev *sch); -int css_do_stsch(SubchDev *sch, SCHIB *schib); +IOInstEnding css_do_stsch(SubchDev *sch, SCHIB *schib); bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid); IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *schib); IOInstEnding css_do_xsch(SubchDev *sch); diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h index fffb54562f..4a43803ef2 100644 --- a/include/hw/s390x/s390-ccw.h +++ b/include/hw/s390x/s390-ccw.h @@ -37,6 +37,7 @@ typedef struct S390CCWDeviceClass { IOInstEnding (*handle_request) (SubchDev *sch); int (*handle_halt) (SubchDev *sch); int (*handle_clear) (SubchDev *sch); + IOInstEnding (*handle_store) (SubchDev *sch); } S390CCWDeviceClass; #endif diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c index 0e840cc579..1b747a66d4 100644 --- a/target/s390x/ioinst.c +++ b/target/s390x/ioinst.c @@ -257,8 +257,7 @@ void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, sch = css_find_subch(m, cssid, ssid, schid); if (sch) { if (css_subch_visible(sch)) { - css_do_stsch(sch, &schib); - cc = 0; + cc = css_do_stsch(sch, &schib); } else { /* Indicate no more subchannels in this css/ss */ cc = 3;