[PATCH v2 0/2] s390x: improve subchannel error handling (vfio)

[PATCH v2 0/2] s390x: improve subchannel error handling (vfio)

[Cornelia Huck]

This is a followup on the first version (which I had sent out in May,
and which kind of fell through the cracks.) While the first patch
is mostly unchanged, I added a second patch to address some possible
problems with the generated unit exceptions; non-vfio subchannels
are not affected by this.

As before, this works on the good path, and I have not managed to
actually get my system to exercise the error path :(

v1->v2:
- add comments regarding -ENODEV/-EACCES handling
- add second patch

Cornelia Huck (2):
  vfio-ccw: forward halt/clear errors
  css: fix actl handling for unit exceptions

 hw/s390x/css.c         | 38 ++++++++++++++++++++++++++++++++++----
 hw/vfio/ccw.c          |  4 ++--
 include/hw/s390x/css.h |  3 ++-
 3 files changed, 38 insertions(+), 7 deletions(-)

-- 
2.31.1

[PATCH v2 1/2] vfio-ccw: forward halt/clear errors

[Cornelia Huck]

hsch and csch basically have two parts: execute the command,
and perform the halt/clear function. For fully emulated
subchannels, it is pretty clear how it will work: check the
subchannel state, and actually 'perform the halt/clear function'
and set cc 0 if everything looks good.

For passthrough subchannels, some of the checking is done
within QEMU, but some has to be done within the kernel. QEMU's
subchannel state may be such that we can perform the async
function, but the kernel may still get a cc != 0 when it is
actually executing the instruction. In that case, we need to
set the condition actually encountered by the kernel; if we
set cc 0 on error, we would actually need to inject an interrupt
as well.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/css.c | 38 ++++++++++++++++++++++++++++++++++----
 hw/vfio/ccw.c  |  4 ++--
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 133ddea5757e..7d9523f81138 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1206,23 +1206,53 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
 
 }
 
-static void sch_handle_halt_func_passthrough(SubchDev *sch)
+static IOInstEnding sch_handle_halt_func_passthrough(SubchDev *sch)
 {
     int ret;
 
     ret = s390_ccw_halt(sch);
     if (ret == -ENOSYS) {
         sch_handle_halt_func(sch);
+        return IOINST_CC_EXPECTED;
+    }
+    /*
+     * Some conditions may have been detected prior to starting the halt
+     * function; map them to the correct cc.
+     * Note that we map both -ENODEV and -EACCES to cc 3 (there's not really
+     * anything else we can do.)
+     */
+    switch (ret) {
+    case -EBUSY:
+        return IOINST_CC_BUSY;
+    case -ENODEV:
+    case -EACCES:
+        return IOINST_CC_NOT_OPERATIONAL;
+    default:
+        return IOINST_CC_EXPECTED;
     }
 }
 
-static void sch_handle_clear_func_passthrough(SubchDev *sch)
+static IOInstEnding sch_handle_clear_func_passthrough(SubchDev *sch)
 {
     int ret;
 
     ret = s390_ccw_clear(sch);
     if (ret == -ENOSYS) {
         sch_handle_clear_func(sch);
+        return IOINST_CC_EXPECTED;
+    }
+    /*
+     * Some conditions may have been detected prior to starting the clear
+     * function; map them to the correct cc.
+     * Note that we map both -ENODEV and -EACCES to cc 3 (there's not really
+     * anything else we can do.)
+     */
+    switch (ret) {
+    case -ENODEV:
+    case -EACCES:
+        return IOINST_CC_NOT_OPERATIONAL;
+    default:
+        return IOINST_CC_EXPECTED;
     }
 }
 
@@ -1265,9 +1295,9 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sch)
     SCHIB *schib = &sch->curr_status;
 
     if (schib->scsw.ctrl & SCSW_FCTL_CLEAR_FUNC) {
-        sch_handle_clear_func_passthrough(sch);
+        return sch_handle_clear_func_passthrough(sch);
     } else if (schib->scsw.ctrl & SCSW_FCTL_HALT_FUNC) {
-        sch_handle_halt_func_passthrough(sch);
+        return sch_handle_halt_func_passthrough(sch);
     } else if (schib->scsw.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 000992fb9fb6..0354737666a1 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -199,7 +199,7 @@ again:
     case 0:
     case -ENODEV:
     case -EACCES:
-        return 0;
+        return ret;
     case -EFAULT:
     default:
         sch_gen_unit_exception(sch);
@@ -240,7 +240,7 @@ again:
     case -EBUSY:
     case -ENODEV:
     case -EACCES:
-        return 0;
+        return ret;
     case -EFAULT:
     default:
         sch_gen_unit_exception(sch);
-- 
2.31.1

[PATCH v2 2/2] css: fix actl handling for unit exceptions

[Cornelia Huck]

When a subchannel becomes pending with unit exception, start
pending (and for that matter, halt or clear pending) are not
removed in the actl. Device active and subchannel active,
however, are (due to the subchannel becoming status pending
with primary respectively secondary status).

The other conditions in the actl are only cleared when the
guest executes tsch on the subchannel.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 include/hw/s390x/css.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 10ed1df1bb74..75e53816135a 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -146,7 +146,8 @@ struct SubchDev {
 
 static inline void sch_gen_unit_exception(SubchDev *sch)
 {
-    sch->curr_status.scsw.ctrl &= ~SCSW_ACTL_START_PEND;
+    sch->curr_status.scsw.ctrl &= ~(SCSW_ACTL_DEVICE_ACTIVE |
+                                    SCSW_ACTL_SUBCH_ACTIVE);
     sch->curr_status.scsw.ctrl |= SCSW_STCTL_PRIMARY |
                                   SCSW_STCTL_SECONDARY |
                                   SCSW_STCTL_ALERT |
-- 
2.31.1

Re: [PATCH v2 0/2] s390x: improve subchannel error handling (vfio)

[Matthew Rosato]

On 7/5/21 12:39 PM, Cornelia Huck wrote:
> This is a followup on the first version (which I had sent out in May,
> and which kind of fell through the cracks.) While the first patch
> is mostly unchanged, I added a second patch to address some possible
> problems with the generated unit exceptions; non-vfio subchannels
> are not affected by this.
> 
> As before, this works on the good path, and I have not managed to
> actually get my system to exercise the error path :(

Sorry for the silence, was out of office for a bit and Eric is 
unavailable -- Anyway the code LGTM and matches what I see in the POPs, 
I'd be willing to ACK but I'd feel better if we could exercise the error 
paths before merging.

@Jared/@Mike, you've both had eyes on this area of code recently, would 
one of you be willing to take a crack at a tested-by (non-zero CCs on 
HSCH/CSCH + also drive the sch_gen_unit_exception path)?

> 
> v1->v2:
> - add comments regarding -ENODEV/-EACCES handling
> - add second patch
> 
> Cornelia Huck (2):
>    vfio-ccw: forward halt/clear errors
>    css: fix actl handling for unit exceptions
> 
>   hw/s390x/css.c         | 38 ++++++++++++++++++++++++++++++++++----
>   hw/vfio/ccw.c          |  4 ++--
>   include/hw/s390x/css.h |  3 ++-
>   3 files changed, 38 insertions(+), 7 deletions(-)
> 

Re: [PATCH v2 0/2] s390x: improve subchannel error handling (vfio)

[Jared Rossi]

I will take a look and see if I can exercise the error paths.

Regards,

Jared Rossi

On 7/19/21 10:16 AM, Matthew Rosato wrote:
> On 7/5/21 12:39 PM, Cornelia Huck wrote:
>> This is a followup on the first version (which I had sent out in May,
>> and which kind of fell through the cracks.) While the first patch
>> is mostly unchanged, I added a second patch to address some possible
>> problems with the generated unit exceptions; non-vfio subchannels
>> are not affected by this.
>>
>> As before, this works on the good path, and I have not managed to
>> actually get my system to exercise the error path :(
>
> Sorry for the silence, was out of office for a bit and Eric is 
> unavailable -- Anyway the code LGTM and matches what I see in the 
> POPs, I'd be willing to ACK but I'd feel better if we could exercise 
> the error paths before merging.
>
> @Jared/@Mike, you've both had eyes on this area of code recently, 
> would one of you be willing to take a crack at a tested-by (non-zero 
> CCs on HSCH/CSCH + also drive the sch_gen_unit_exception path)?
>
>>
>> v1->v2:
>> - add comments regarding -ENODEV/-EACCES handling
>> - add second patch
>>
>> Cornelia Huck (2):
>>    vfio-ccw: forward halt/clear errors
>>    css: fix actl handling for unit exceptions
>>
>>   hw/s390x/css.c         | 38 ++++++++++++++++++++++++++++++++++----
>>   hw/vfio/ccw.c          |  4 ++--
>>   include/hw/s390x/css.h |  3 ++-
>>   3 files changed, 38 insertions(+), 7 deletions(-)
>>
>

Re: [PATCH v2 0/2] s390x: improve subchannel error handling (vfio)

[Jared Rossi]

I've exercised the error paths and it appears to all work correctly.

On 7/19/21 11:09 AM, Jared Rossi wrote:
> I will take a look and see if I can exercise the error paths.
>
> Regards,
>
> Jared Rossi
>
> On 7/19/21 10:16 AM, Matthew Rosato wrote:
>> On 7/5/21 12:39 PM, Cornelia Huck wrote:
>>> This is a followup on the first version (which I had sent out in May,
>>> and which kind of fell through the cracks.) While the first patch
>>> is mostly unchanged, I added a second patch to address some possible
>>> problems with the generated unit exceptions; non-vfio subchannels
>>> are not affected by this.
>>>
>>> As before, this works on the good path, and I have not managed to
>>> actually get my system to exercise the error path :(
>>
>> Sorry for the silence, was out of office for a bit and Eric is 
>> unavailable -- Anyway the code LGTM and matches what I see in the 
>> POPs, I'd be willing to ACK but I'd feel better if we could exercise 
>> the error paths before merging.
>>
>> @Jared/@Mike, you've both had eyes on this area of code recently, 
>> would one of you be willing to take a crack at a tested-by (non-zero 
>> CCs on HSCH/CSCH + also drive the sch_gen_unit_exception path)?
>>
>>>
>>> v1->v2:
>>> - add comments regarding -ENODEV/-EACCES handling
>>> - add second patch
>>>
>>> Cornelia Huck (2):
>>>    vfio-ccw: forward halt/clear errors
>>>    css: fix actl handling for unit exceptions
>>>
>>>   hw/s390x/css.c         | 38 ++++++++++++++++++++++++++++++++++----
>>>   hw/vfio/ccw.c          |  4 ++--
>>>   include/hw/s390x/css.h |  3 ++-
>>>   3 files changed, 38 insertions(+), 7 deletions(-)
>>>
>>

Re: [PATCH v2 0/2] s390x: improve subchannel error handling (vfio)

[Matthew Rosato]

On 8/4/21 8:30 PM, Jared Rossi wrote:
> I've exercised the error paths and it appears to all work correctly.
> 
> On 7/19/21 11:09 AM, Jared Rossi wrote:
>> I will take a look and see if I can exercise the error paths.
>>
>> Regards,
>>
>> Jared Rossi

Thanks Jared!  So, with that I'd suggest a

Tested-by: Jared Rossi <jrossi@linux.ibm.com>

and as I said earlier the code LGTM -- so for the series:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

>>
>> On 7/19/21 10:16 AM, Matthew Rosato wrote:
>>> On 7/5/21 12:39 PM, Cornelia Huck wrote:
>>>> This is a followup on the first version (which I had sent out in May,
>>>> and which kind of fell through the cracks.) While the first patch
>>>> is mostly unchanged, I added a second patch to address some possible
>>>> problems with the generated unit exceptions; non-vfio subchannels
>>>> are not affected by this.
>>>>
>>>> As before, this works on the good path, and I have not managed to
>>>> actually get my system to exercise the error path :(
>>>
>>> Sorry for the silence, was out of office for a bit and Eric is 
>>> unavailable -- Anyway the code LGTM and matches what I see in the 
>>> POPs, I'd be willing to ACK but I'd feel better if we could exercise 
>>> the error paths before merging.
>>>
>>> @Jared/@Mike, you've both had eyes on this area of code recently, 
>>> would one of you be willing to take a crack at a tested-by (non-zero 
>>> CCs on HSCH/CSCH + also drive the sch_gen_unit_exception path)?
>>>
>>>>
>>>> v1->v2:
>>>> - add comments regarding -ENODEV/-EACCES handling
>>>> - add second patch
>>>>
>>>> Cornelia Huck (2):
>>>>    vfio-ccw: forward halt/clear errors
>>>>    css: fix actl handling for unit exceptions
>>>>
>>>>   hw/s390x/css.c         | 38 ++++++++++++++++++++++++++++++++++----
>>>>   hw/vfio/ccw.c          |  4 ++--
>>>>   include/hw/s390x/css.h |  3 ++-
>>>>   3 files changed, 38 insertions(+), 7 deletions(-)
>>>>
>>>

Re: [PATCH v2 0/2] s390x: improve subchannel error handling (vfio)

[Cornelia Huck]

On Wed, Aug 04 2021, Matthew Rosato <mjrosato@linux.ibm.com> wrote:

> On 8/4/21 8:30 PM, Jared Rossi wrote:
>> I've exercised the error paths and it appears to all work correctly.
>> 
>> On 7/19/21 11:09 AM, Jared Rossi wrote:
>>> I will take a look and see if I can exercise the error paths.
>>>
>>> Regards,
>>>
>>> Jared Rossi
>
> Thanks Jared!  So, with that I'd suggest a
>
> Tested-by: Jared Rossi <jrossi@linux.ibm.com>

I took the liberty to include that.

>
> and as I said earlier the code LGTM -- so for the series:
>
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

Thanks to you both!

>
>>>
>>> On 7/19/21 10:16 AM, Matthew Rosato wrote:
>>>> On 7/5/21 12:39 PM, Cornelia Huck wrote:
>>>>> This is a followup on the first version (which I had sent out in May,
>>>>> and which kind of fell through the cracks.) While the first patch
>>>>> is mostly unchanged, I added a second patch to address some possible
>>>>> problems with the generated unit exceptions; non-vfio subchannels
>>>>> are not affected by this.
>>>>>
>>>>> As before, this works on the good path, and I have not managed to
>>>>> actually get my system to exercise the error path :(
>>>>
>>>> Sorry for the silence, was out of office for a bit and Eric is 
>>>> unavailable -- Anyway the code LGTM and matches what I see in the 
>>>> POPs, I'd be willing to ACK but I'd feel better if we could exercise 
>>>> the error paths before merging.
>>>>
>>>> @Jared/@Mike, you've both had eyes on this area of code recently, 
>>>> would one of you be willing to take a crack at a tested-by (non-zero 
>>>> CCs on HSCH/CSCH + also drive the sch_gen_unit_exception path)?
>>>>
>>>>>
>>>>> v1->v2:
>>>>> - add comments regarding -ENODEV/-EACCES handling
>>>>> - add second patch
>>>>>
>>>>> Cornelia Huck (2):
>>>>>    vfio-ccw: forward halt/clear errors
>>>>>    css: fix actl handling for unit exceptions
>>>>>
>>>>>   hw/s390x/css.c         | 38 ++++++++++++++++++++++++++++++++++----
>>>>>   hw/vfio/ccw.c          |  4 ++--
>>>>>   include/hw/s390x/css.h |  3 ++-
>>>>>   3 files changed, 38 insertions(+), 7 deletions(-)
>>>>>
>>>>

Queued to s390-next.