[PATCH RFC 0/2] vfio-ccw: exploit halt/clear subchannel support

[PATCH RFC 0/2] vfio-ccw: exploit halt/clear subchannel support

[Cornelia Huck]

Hi,

this is the QEMU companion patchset to "vfio-ccw: support for {halt,clear}
subchannel", but is interface-wise independent of it.

With an old host kernel, things should work as before. With a new host
kernel, guest halt/clear requests should get passed through to the host.

I tested this with a hacked-up guest kernel to issue clears on demand.

Cornelia Huck (2):
  vfio-ccw: forward halt/clear to device if supported
  s390/css: add some tracing for pass-through handling

 hw/s390x/css.c         | 38 ++++++++++++++++++++++++++++++++++----
 hw/s390x/trace-events  |  4 ++++
 hw/vfio/ccw.c          | 11 +++++++++--
 include/hw/s390x/css.h | 10 +++++++---
 4 files changed, 54 insertions(+), 9 deletions(-)

-- 
2.14.3

[PATCH RFC 1/2] vfio-ccw: forward halt/clear to device if supported

[Cornelia Huck]

The initial version of vfio-ccw did not support forwarding of the
halt or clear functions to the device, and we had to emulate them
instead.

For versions of the vfio-ccw kernel implementation that indeed do
support halt/clear (by indicating them in the fctl of the scsw in
the io_region), we can simply start making use of it. If the kernel
does not support handling halt/clear, fall back to emulation.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/css.c         | 32 ++++++++++++++++++++++++++++----
 hw/vfio/ccw.c          | 11 +++++++++--
 include/hw/s390x/css.h | 10 +++++++---
 3 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 301bf1772f..b6727d0607 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1180,6 +1180,16 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
 
 }
 
+static IOInstEnding sch_handle_clear_func_passthrough(SubchDev *sch)
+{
+    return s390_ccw_cmd_request(sch);
+}
+
+static IOInstEnding sch_handle_halt_func_passthrough(SubchDev *sch)
+{
+    return s390_ccw_cmd_request(sch);
+}
+
 static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
 {
 
@@ -1233,13 +1243,27 @@ IOInstEnding do_subchannel_work_virtual(SubchDev *sch)
 IOInstEnding do_subchannel_work_passthrough(SubchDev *sch)
 {
     SCSW *s = &sch->curr_status.scsw;
+    static bool no_halt_clear;
 
+    /* if the kernel does not support halt/clear, fall back to emulation */
     if (s->ctrl & SCSW_FCTL_CLEAR_FUNC) {
-        /* TODO: Clear handling */
-        sch_handle_clear_func(sch);
+        if (no_halt_clear) {
+            sch_handle_clear_func(sch);
+        } else {
+            if (sch_handle_clear_func_passthrough(sch) == IOINST_OPNOTSUPP) {
+                no_halt_clear = true;
+                sch_handle_halt_func(sch);
+            }
+        }
     } else if (s->ctrl & SCSW_FCTL_HALT_FUNC) {
-        /* TODO: Halt handling */
-        sch_handle_halt_func(sch);
+        if (no_halt_clear) {
+            sch_handle_halt_func(sch);
+        } else {
+            if (sch_handle_halt_func_passthrough(sch) == IOINST_OPNOTSUPP) {
+                no_halt_clear = true;
+                sch_handle_halt_func(sch);
+            }
+        }
     } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
         return sch_handle_start_func_passthrough(sch);
     }
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index e67392c5f9..247901ae41 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -60,6 +60,7 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
 
     memset(region, 0, sizeof(*region));
 
+    /* orb is only valid for ssch, but does not hurt for other functions */
     memcpy(region->orb_area, &sch->orb, sizeof(ORB));
     memcpy(region->scsw_area, &sch->curr_status.scsw, sizeof(SCSW));
 
@@ -70,8 +71,12 @@ again:
         if (errno == EAGAIN) {
             goto again;
         }
-        error_report("vfio-ccw: wirte I/O region failed with errno=%d", errno);
-        ret = -errno;
+        /* handle not supported operations like halt/clear on older kernels */
+        if (ret != -EOPNOTSUPP) {
+            error_report("vfio-ccw: write I/O region failed with errno=%d",
+                         errno);
+            ret = -errno;
+        }
     } else {
         ret = region->ret_code;
     }
@@ -83,6 +88,8 @@ again:
     case -ENODEV:
     case -EACCES:
         return IOINST_CC_NOT_OPERATIONAL;
+    case -EOPNOTSUPP:
+        return IOINST_OPNOTSUPP;
     case -EFAULT:
     default:
         sch_gen_unit_exception(sch);
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 35facb47d2..e33f26882b 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -100,9 +100,11 @@ typedef struct CcwDataStream {
 } CcwDataStream;
 
 /*
- * IO instructions conclude according to this. Currently we have only
- * cc codes. Valid values are 0, 1, 2, 3 and the generic semantic for
+ * IO instructions conclude according to this. One class of values are
+ * cc codes: Valid values are 0, 1, 2, 3 and the generic semantic for
  * IO instructions is described briefly. For more details consult the PoP.
+ * Additionally, other endings may occur due to internal processing errors
+ * like lack of support for an operation.
  */
 typedef enum IOInstEnding {
     /* produced expected result */
@@ -112,7 +114,9 @@ typedef enum IOInstEnding {
     /* inst. ineffective because busy with previously initiated function */
     IOINST_CC_BUSY = 2,
     /* inst. ineffective because not operational */
-    IOINST_CC_NOT_OPERATIONAL = 3
+    IOINST_CC_NOT_OPERATIONAL = 3,
+    /* internal: operation not supported */
+    IOINST_OPNOTSUPP = 4
 } IOInstEnding;
 
 typedef struct SubchDev SubchDev;
-- 
2.14.3

[PATCH RFC 2/2] s390/css: add some tracing for pass-through handling

[Cornelia Huck]

...so we can get more easily an idea whether halt/clear is sent to
the device or emulated.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/css.c        | 6 ++++++
 hw/s390x/trace-events | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index b6727d0607..b6f3421380 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1182,11 +1182,13 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
 
 static IOInstEnding sch_handle_clear_func_passthrough(SubchDev *sch)
 {
+    trace_css_handle_clear_pt(sch->cssid, sch->ssid, sch->schid);
     return s390_ccw_cmd_request(sch);
 }
 
 static IOInstEnding sch_handle_halt_func_passthrough(SubchDev *sch)
 {
+    trace_css_handle_halt_pt(sch->cssid, sch->ssid, sch->schid);
     return s390_ccw_cmd_request(sch);
 }
 
@@ -1197,6 +1199,8 @@ static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
     SCSW *s = &sch->curr_status.scsw;
 
     ORB *orb = &sch->orb;
+
+    trace_css_handle_start_pt(sch->cssid, sch->ssid, sch->schid);
     if (!(s->ctrl & SCSW_ACTL_SUSP)) {
         assert(orb != NULL);
         p->intparm = orb->intparm;
@@ -1252,6 +1256,7 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sch)
         } else {
             if (sch_handle_clear_func_passthrough(sch) == IOINST_OPNOTSUPP) {
                 no_halt_clear = true;
+                trace_css_no_haltclear_pt();
                 sch_handle_halt_func(sch);
             }
         }
@@ -1260,6 +1265,7 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sch)
             sch_handle_halt_func(sch);
         } else {
             if (sch_handle_halt_func_passthrough(sch) == IOINST_OPNOTSUPP) {
+                trace_css_no_haltclear_pt();
                 no_halt_clear = true;
                 sch_handle_halt_func(sch);
             }
diff --git a/hw/s390x/trace-events b/hw/s390x/trace-events
index 0d3622ec6f..d4fd1b9da6 100644
--- a/hw/s390x/trace-events
+++ b/hw/s390x/trace-events
@@ -9,6 +9,10 @@ css_assign_subch(const char *do_assign, uint8_t cssid, uint8_t ssid, uint16_t sc
 css_io_interrupt(int cssid, int ssid, int schid, uint32_t intparm, uint8_t isc, const char *conditional) "CSS: I/O interrupt on sch %x.%x.%04x (intparm 0x%08x, isc 0x%x) %s"
 css_adapter_interrupt(uint8_t isc) "CSS: adapter I/O interrupt (isc 0x%x)"
 css_do_sic(uint16_t mode, uint8_t isc) "CSS: set interruption mode 0x%x on isc 0x%x"
+css_handle_clear_pt(int cssid, int ssid, int schid) "CSS: handling clear function for pass-through subchannel %x.%x.%04x"
+css_handle_halt_pt(int cssid, int ssid, int schid) "CSS: handling halt function for pass-through subchannel %x.%x.%04x"
+css_handle_start_pt(int cssid, int ssid, int schid) "CSS: handling start function for pass-through subchannel %x.%x.%04x"
+css_no_haltclear_pt(void) "CSS: no kernel support for halt/clear function passthrough handling, falling back to emulation"
 
 # hw/s390x/virtio-ccw.c
 virtio_ccw_interpret_ccw(int cssid, int ssid, int schid, int cmd_code) "VIRTIO-CCW: %x.%x.%04x: interpret command 0x%x"
-- 
2.14.3

Re: [PATCH RFC 1/2] vfio-ccw: forward halt/clear to device if supported

[Pierre Morel]

On 09/05/2018 17:49, Cornelia Huck wrote:
> The initial version of vfio-ccw did not support forwarding of the
> halt or clear functions to the device, and we had to emulate them
> instead.
>
> For versions of the vfio-ccw kernel implementation that indeed do
> support halt/clear (by indicating them in the fctl of the scsw in
> the io_region), we can simply start making use of it. If the kernel
> does not support handling halt/clear, fall back to emulation.
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   hw/s390x/css.c         | 32 ++++++++++++++++++++++++++++----
>   hw/vfio/ccw.c          | 11 +++++++++--
>   include/hw/s390x/css.h | 10 +++++++---
>   3 files changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 301bf1772f..b6727d0607 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1180,6 +1180,16 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
>   
>   }
>   
> +static IOInstEnding sch_handle_clear_func_passthrough(SubchDev *sch)
> +{
> +    return s390_ccw_cmd_request(sch);
> +}
> +
> +static IOInstEnding sch_handle_halt_func_passthrough(SubchDev *sch)
> +{
> +    return s390_ccw_cmd_request(sch);
> +}
> +
>   static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
>   {
>   
> @@ -1233,13 +1243,27 @@ IOInstEnding do_subchannel_work_virtual(SubchDev *sch)
>   IOInstEnding do_subchannel_work_passthrough(SubchDev *sch)
>   {
>       SCSW *s = &sch->curr_status.scsw;
> +    static bool no_halt_clear;
>   
> +    /* if the kernel does not support halt/clear, fall back to emulation */
>       if (s->ctrl & SCSW_FCTL_CLEAR_FUNC) {
> -        /* TODO: Clear handling */
> -        sch_handle_clear_func(sch);
> +        if (no_halt_clear) {
> +            sch_handle_clear_func(sch);
> +        } else {
> +            if (sch_handle_clear_func_passthrough(sch) == IOINST_OPNOTSUPP) {
> +                no_halt_clear = true;
> +                sch_handle_halt_func(sch);
> +            }
> +        }
>       } else if (s->ctrl & SCSW_FCTL_HALT_FUNC) {
> -        /* TODO: Halt handling */
> -        sch_handle_halt_func(sch);
> +        if (no_halt_clear) {
> +            sch_handle_halt_func(sch);
> +        } else {
> +            if (sch_handle_halt_func_passthrough(sch) == IOINST_OPNOTSUPP) {
> +                no_halt_clear = true;
> +                sch_handle_halt_func(sch);
> +            }
> +        }
>       } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
>           return sch_handle_start_func_passthrough(sch);
>       }
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index e67392c5f9..247901ae41 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -60,6 +60,7 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
>   
>       memset(region, 0, sizeof(*region));
>   
> +    /* orb is only valid for ssch, but does not hurt for other functions */
>       memcpy(region->orb_area, &sch->orb, sizeof(ORB));
>       memcpy(region->scsw_area, &sch->curr_status.scsw, sizeof(SCSW));
>   
> @@ -70,8 +71,12 @@ again:
>           if (errno == EAGAIN) {
>               goto again;
>           }
> -        error_report("vfio-ccw: wirte I/O region failed with errno=%d", errno);
> -        ret = -errno;
> +        /* handle not supported operations like halt/clear on older kernels */
> +        if (ret != -EOPNOTSUPP) {
> +            error_report("vfio-ccw: write I/O region failed with errno=%d",
> +                         errno);
> +            ret = -errno;
> +        }
>       } else {
>           ret = region->ret_code;
>       }
> @@ -83,6 +88,8 @@ again:
>       case -ENODEV:
>       case -EACCES:
>           return IOINST_CC_NOT_OPERATIONAL;
> +    case -EOPNOTSUPP:
> +        return IOINST_OPNOTSUPP;
>       case -EFAULT:
>       default:
>           sch_gen_unit_exception(sch);
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 35facb47d2..e33f26882b 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -100,9 +100,11 @@ typedef struct CcwDataStream {
>   } CcwDataStream;
>   
>   /*
> - * IO instructions conclude according to this. Currently we have only
> - * cc codes. Valid values are 0, 1, 2, 3 and the generic semantic for
> + * IO instructions conclude according to this. One class of values are
> + * cc codes: Valid values are 0, 1, 2, 3 and the generic semantic for
>    * IO instructions is described briefly. For more details consult the PoP.
> + * Additionally, other endings may occur due to internal processing errors
> + * like lack of support for an operation.
>    */
>   typedef enum IOInstEnding {
>       /* produced expected result */
> @@ -112,7 +114,9 @@ typedef enum IOInstEnding {
>       /* inst. ineffective because busy with previously initiated function */
>       IOINST_CC_BUSY = 2,
>       /* inst. ineffective because not operational */
> -    IOINST_CC_NOT_OPERATIONAL = 3
> +    IOINST_CC_NOT_OPERATIONAL = 3,
> +    /* internal: operation not supported */
> +    IOINST_OPNOTSUPP = 4
>   } IOInstEnding;
>   
>   typedef struct SubchDev SubchDev;


Couldn't we introduce ABI versioning ?


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH RFC 1/2] vfio-ccw: forward halt/clear to device if supported

[Cornelia Huck]

On Fri, 11 May 2018 11:53:52 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 09/05/2018 17:49, Cornelia Huck wrote:
> > The initial version of vfio-ccw did not support forwarding of the
> > halt or clear functions to the device, and we had to emulate them
> > instead.
> >
> > For versions of the vfio-ccw kernel implementation that indeed do
> > support halt/clear (by indicating them in the fctl of the scsw in
> > the io_region), we can simply start making use of it. If the kernel
> > does not support handling halt/clear, fall back to emulation.
> >
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >   hw/s390x/css.c         | 32 ++++++++++++++++++++++++++++----
> >   hw/vfio/ccw.c          | 11 +++++++++--
> >   include/hw/s390x/css.h | 10 +++++++---
> >   3 files changed, 44 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index 301bf1772f..b6727d0607 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -1180,6 +1180,16 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
> >   
> >   }
> >   
> > +static IOInstEnding sch_handle_clear_func_passthrough(SubchDev *sch)
> > +{
> > +    return s390_ccw_cmd_request(sch);
> > +}
> > +
> > +static IOInstEnding sch_handle_halt_func_passthrough(SubchDev *sch)
> > +{
> > +    return s390_ccw_cmd_request(sch);
> > +}
> > +
> >   static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
> >   {
> >   
> > @@ -1233,13 +1243,27 @@ IOInstEnding do_subchannel_work_virtual(SubchDev *sch)
> >   IOInstEnding do_subchannel_work_passthrough(SubchDev *sch)
> >   {
> >       SCSW *s = &sch->curr_status.scsw;
> > +    static bool no_halt_clear;
> >   
> > +    /* if the kernel does not support halt/clear, fall back to emulation */
> >       if (s->ctrl & SCSW_FCTL_CLEAR_FUNC) {
> > -        /* TODO: Clear handling */
> > -        sch_handle_clear_func(sch);
> > +        if (no_halt_clear) {
> > +            sch_handle_clear_func(sch);
> > +        } else {
> > +            if (sch_handle_clear_func_passthrough(sch) == IOINST_OPNOTSUPP) {
> > +                no_halt_clear = true;
> > +                sch_handle_halt_func(sch);
> > +            }
> > +        }
> >       } else if (s->ctrl & SCSW_FCTL_HALT_FUNC) {
> > -        /* TODO: Halt handling */
> > -        sch_handle_halt_func(sch);
> > +        if (no_halt_clear) {
> > +            sch_handle_halt_func(sch);
> > +        } else {
> > +            if (sch_handle_halt_func_passthrough(sch) == IOINST_OPNOTSUPP) {
> > +                no_halt_clear = true;
> > +                sch_handle_halt_func(sch);
> > +            }
> > +        }
> >       } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
> >           return sch_handle_start_func_passthrough(sch);
> >       }
> > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> > index e67392c5f9..247901ae41 100644
> > --- a/hw/vfio/ccw.c
> > +++ b/hw/vfio/ccw.c
> > @@ -60,6 +60,7 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
> >   
> >       memset(region, 0, sizeof(*region));
> >   
> > +    /* orb is only valid for ssch, but does not hurt for other functions */
> >       memcpy(region->orb_area, &sch->orb, sizeof(ORB));
> >       memcpy(region->scsw_area, &sch->curr_status.scsw, sizeof(SCSW));
> >   
> > @@ -70,8 +71,12 @@ again:
> >           if (errno == EAGAIN) {
> >               goto again;
> >           }
> > -        error_report("vfio-ccw: wirte I/O region failed with errno=%d", errno);
> > -        ret = -errno;
> > +        /* handle not supported operations like halt/clear on older kernels */
> > +        if (ret != -EOPNOTSUPP) {
> > +            error_report("vfio-ccw: write I/O region failed with errno=%d",
> > +                         errno);
> > +            ret = -errno;
> > +        }
> >       } else {
> >           ret = region->ret_code;
> >       }
> > @@ -83,6 +88,8 @@ again:
> >       case -ENODEV:
> >       case -EACCES:
> >           return IOINST_CC_NOT_OPERATIONAL;
> > +    case -EOPNOTSUPP:
> > +        return IOINST_OPNOTSUPP;
> >       case -EFAULT:
> >       default:
> >           sch_gen_unit_exception(sch);
> > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> > index 35facb47d2..e33f26882b 100644
> > --- a/include/hw/s390x/css.h
> > +++ b/include/hw/s390x/css.h
> > @@ -100,9 +100,11 @@ typedef struct CcwDataStream {
> >   } CcwDataStream;
> >   
> >   /*
> > - * IO instructions conclude according to this. Currently we have only
> > - * cc codes. Valid values are 0, 1, 2, 3 and the generic semantic for
> > + * IO instructions conclude according to this. One class of values are
> > + * cc codes: Valid values are 0, 1, 2, 3 and the generic semantic for
> >    * IO instructions is described briefly. For more details consult the PoP.
> > + * Additionally, other endings may occur due to internal processing errors
> > + * like lack of support for an operation.
> >    */
> >   typedef enum IOInstEnding {
> >       /* produced expected result */
> > @@ -112,7 +114,9 @@ typedef enum IOInstEnding {
> >       /* inst. ineffective because busy with previously initiated function */
> >       IOINST_CC_BUSY = 2,
> >       /* inst. ineffective because not operational */
> > -    IOINST_CC_NOT_OPERATIONAL = 3
> > +    IOINST_CC_NOT_OPERATIONAL = 3,
> > +    /* internal: operation not supported */
> > +    IOINST_OPNOTSUPP = 4
> >   } IOInstEnding;
> >   
> >   typedef struct SubchDev SubchDev;  
> 
> 
> Couldn't we introduce ABI versioning ?

Can you elaborate what you're referring to?

If you mean checking capabilities of the kernel or so: If we can avoid
that and just try (and stop if it does not work), I'd prefer that (no
dependencies).

The IOINST_OPNOTSUPP is a bit ugly, but I did not see a more elegant
way to pass 'not supported' up to the caller.

Re: [PATCH RFC 1/2] vfio-ccw: forward halt/clear to device if supported

[Pierre Morel]

On 15/05/2018 18:01, Cornelia Huck wrote:
> On Fri, 11 May 2018 11:53:52 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>> On 09/05/2018 17:49, Cornelia Huck wrote:
>>> The initial version of vfio-ccw did not support forwarding of the
>>> halt or clear functions to the device, and we had to emulate them
>>> instead.
>>>
>>> For versions of the vfio-ccw kernel implementation that indeed do
>>> support halt/clear (by indicating them in the fctl of the scsw in
>>> the io_region), we can simply start making use of it. If the kernel
>>> does not support handling halt/clear, fall back to emulation.
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>    hw/s390x/css.c         | 32 ++++++++++++++++++++++++++++----
>>>    hw/vfio/ccw.c          | 11 +++++++++--
>>>    include/hw/s390x/css.h | 10 +++++++---
>>>    3 files changed, 44 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>> index 301bf1772f..b6727d0607 100644
>>> --- a/hw/s390x/css.c
>>> +++ b/hw/s390x/css.c
>>> @@ -1180,6 +1180,16 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
>>>    
>>>    }
>>>    
>>> +static IOInstEnding sch_handle_clear_func_passthrough(SubchDev *sch)
>>> +{
>>> +    return s390_ccw_cmd_request(sch);
>>> +}
>>> +
>>> +static IOInstEnding sch_handle_halt_func_passthrough(SubchDev *sch)
>>> +{
>>> +    return s390_ccw_cmd_request(sch);
>>> +}
>>> +
>>>    static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
>>>    {
>>>    
>>> @@ -1233,13 +1243,27 @@ IOInstEnding do_subchannel_work_virtual(SubchDev *sch)
>>>    IOInstEnding do_subchannel_work_passthrough(SubchDev *sch)
>>>    {
>>>        SCSW *s = &sch->curr_status.scsw;
>>> +    static bool no_halt_clear;
>>>    
>>> +    /* if the kernel does not support halt/clear, fall back to emulation */
>>>        if (s->ctrl & SCSW_FCTL_CLEAR_FUNC) {
>>> -        /* TODO: Clear handling */
>>> -        sch_handle_clear_func(sch);
>>> +        if (no_halt_clear) {
>>> +            sch_handle_clear_func(sch);
>>> +        } else {
>>> +            if (sch_handle_clear_func_passthrough(sch) == IOINST_OPNOTSUPP) {
>>> +                no_halt_clear = true;
>>> +                sch_handle_halt_func(sch);
>>> +            }
>>> +        }
>>>        } else if (s->ctrl & SCSW_FCTL_HALT_FUNC) {
>>> -        /* TODO: Halt handling */
>>> -        sch_handle_halt_func(sch);
>>> +        if (no_halt_clear) {
>>> +            sch_handle_halt_func(sch);
>>> +        } else {
>>> +            if (sch_handle_halt_func_passthrough(sch) == IOINST_OPNOTSUPP) {
>>> +                no_halt_clear = true;
>>> +                sch_handle_halt_func(sch);
>>> +            }
>>> +        }
>>>        } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
>>>            return sch_handle_start_func_passthrough(sch);
>>>        }
>>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>>> index e67392c5f9..247901ae41 100644
>>> --- a/hw/vfio/ccw.c
>>> +++ b/hw/vfio/ccw.c
>>> @@ -60,6 +60,7 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
>>>    
>>>        memset(region, 0, sizeof(*region));
>>>    
>>> +    /* orb is only valid for ssch, but does not hurt for other functions */
>>>        memcpy(region->orb_area, &sch->orb, sizeof(ORB));
>>>        memcpy(region->scsw_area, &sch->curr_status.scsw, sizeof(SCSW));
>>>    
>>> @@ -70,8 +71,12 @@ again:
>>>            if (errno == EAGAIN) {
>>>                goto again;
>>>            }
>>> -        error_report("vfio-ccw: wirte I/O region failed with errno=%d", errno);
>>> -        ret = -errno;
>>> +        /* handle not supported operations like halt/clear on older kernels */
>>> +        if (ret != -EOPNOTSUPP) {
>>> +            error_report("vfio-ccw: write I/O region failed with errno=%d",
>>> +                         errno);
>>> +            ret = -errno;
>>> +        }
>>>        } else {
>>>            ret = region->ret_code;
>>>        }
>>> @@ -83,6 +88,8 @@ again:
>>>        case -ENODEV:
>>>        case -EACCES:
>>>            return IOINST_CC_NOT_OPERATIONAL;
>>> +    case -EOPNOTSUPP:
>>> +        return IOINST_OPNOTSUPP;
>>>        case -EFAULT:
>>>        default:
>>>            sch_gen_unit_exception(sch);
>>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>>> index 35facb47d2..e33f26882b 100644
>>> --- a/include/hw/s390x/css.h
>>> +++ b/include/hw/s390x/css.h
>>> @@ -100,9 +100,11 @@ typedef struct CcwDataStream {
>>>    } CcwDataStream;
>>>    
>>>    /*
>>> - * IO instructions conclude according to this. Currently we have only
>>> - * cc codes. Valid values are 0, 1, 2, 3 and the generic semantic for
>>> + * IO instructions conclude according to this. One class of values are
>>> + * cc codes: Valid values are 0, 1, 2, 3 and the generic semantic for
>>>     * IO instructions is described briefly. For more details consult the PoP.
>>> + * Additionally, other endings may occur due to internal processing errors
>>> + * like lack of support for an operation.
>>>     */
>>>    typedef enum IOInstEnding {
>>>        /* produced expected result */
>>> @@ -112,7 +114,9 @@ typedef enum IOInstEnding {
>>>        /* inst. ineffective because busy with previously initiated function */
>>>        IOINST_CC_BUSY = 2,
>>>        /* inst. ineffective because not operational */
>>> -    IOINST_CC_NOT_OPERATIONAL = 3
>>> +    IOINST_CC_NOT_OPERATIONAL = 3,
>>> +    /* internal: operation not supported */
>>> +    IOINST_OPNOTSUPP = 4
>>>    } IOInstEnding;
>>>    
>>>    typedef struct SubchDev SubchDev;
>>
>> Couldn't we introduce ABI versioning ?
> Can you elaborate what you're referring to?
>
> If you mean checking capabilities of the kernel or so: If we can avoid
> that and just try (and stop if it does not work), I'd prefer that (no
> dependencies).

VFIO_CHECK_EXTENSION is already used in different drivers
for this kind of interface extension.
We could use it to setup appropriate callbacks for scsh/csch/xsch/hsch
depending on the extension argument.

>
> The IOINST_OPNOTSUPP is a bit ugly, but I did not see a more elegant
> way to pass 'not supported' up to the caller.
>

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH RFC 1/2] vfio-ccw: forward halt/clear to device if supported

[Cornelia Huck]

On Wed, 16 May 2018 15:53:48 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 15/05/2018 18:01, Cornelia Huck wrote:
> > On Fri, 11 May 2018 11:53:52 +0200
> > Pierre Morel <pmorel@linux.ibm.com> wrote:

> >> Couldn't we introduce ABI versioning ?  
> > Can you elaborate what you're referring to?
> >
> > If you mean checking capabilities of the kernel or so: If we can avoid
> > that and just try (and stop if it does not work), I'd prefer that (no
> > dependencies).  
> 
> VFIO_CHECK_EXTENSION is already used in different drivers
> for this kind of interface extension.
> We could use it to setup appropriate callbacks for scsh/csch/xsch/hsch
> depending on the extension argument.

Hm, this might be useful for things like xsch that aren't really well
served by the current interface. Or we could try using the
device-specific capability interface.

Let me think and play with this a bit.