[RFC,v1,5/8] vfio-ccw: Add support for the schib region
diff mbox series

Message ID 20191115033437.37926-6-farman@linux.ibm.com
State New
Headers show
Series
  • s390x/vfio-ccw: Channel Path Handling
Related show

Commit Message

Eric Farman Nov. 15, 2019, 3:34 a.m. UTC
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:
    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(-)

Comments

Cornelia Huck Nov. 20, 2019, 11:13 a.m. UTC | #1
On Fri, 15 Nov 2019 04:34:34 +0100
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.

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?

(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?

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

Patch
diff mbox series

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;
+
+    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.
+     */
+    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;
+    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;
+    }
+
+    /*
+     * 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;
+}
+
 static int vfio_ccw_handle_clear(SubchDev *sch)
 {
     S390CCWDevice *cdev = sch->driver_data;
@@ -395,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);
@@ -407,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);
 }
@@ -582,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 c437a1d8c6..63b4c84215 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;