Re: [kvm-unit-tests PATCH v10 9/9] s390x: css: ssch/tsch with sense and interrupt

[Pierre Morel <pmorel@linux.ibm.com>]

On 2020-07-07 13:05, Cornelia Huck wrote:
> On Tue, 7 Jul 2020 12:57:03 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 2020-07-06 16:24, Cornelia Huck wrote:
>>> On Mon, 6 Jul 2020 15:01:50 +0200
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>    
>>>> On 2020-07-06 11:46, Cornelia Huck wrote:
>>>>> On Thu,  2 Jul 2020 18:31:20 +0200
>>>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>>>       
>>>>>> After a channel is enabled we start a SENSE_ID command using
>>>>>> the SSCH instruction to recognize the control unit and device.
>>>>>>
>>>>>> This tests the success of SSCH, the I/O interruption and the TSCH
>>>>>> instructions.
>>>>>>
>>>>>> The SENSE_ID command response is tested to report 0xff inside
>>>>>> its reserved field and to report the same control unit type
>>>>>> as the cu_type kernel argument.
>>>>>>
>>>>>> Without the cu_type kernel argument, the test expects a device
>>>>>> with a default control unit type of 0x3832, a.k.a virtio-net-ccw.
>>>>>>
>>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>>>> ---
>>>>>>     lib/s390x/asm/arch_def.h |   1 +
>>>>>>     lib/s390x/css.h          |  32 ++++++++-
>>>>>>     lib/s390x/css_lib.c      | 148 ++++++++++++++++++++++++++++++++++++++-
>>>>>>     s390x/css.c              |  94 ++++++++++++++++++++++++-
>>>>>>     4 files changed, 272 insertions(+), 3 deletions(-)
>>>
>>> (...)
>>>    
>>>>>> @@ -114,6 +128,7 @@ retry:
>>>>>>     		return cc;
>>>>>>     	}
>>>>>>     
>>>>>> +	report_info("stsch: flags: %04x", pmcw->flags);
>>>>>
>>>>> It feels like all of this already should have been included in the
>>>>> previous patch?
>>>>
>>>> Yes, I did not want to modify it since it was reviewed-by.
>>>
>>> It's not such a major change (the isc change and this here), though...
>>> what do the others think?
>> changed my mind:
>> What about keeping css_enable() to only do enable, in case we only want
>> to do this, and add a function to modify the ISC.
> 
> Hm, the isc is only really relevant while the subchannel is enabled, so
> this would be fine if we only ever modified the isc while the
> subchannel is disabled. On the other hand, we introduce an extra round
> of msch. No strong opinion on my side.
> 

Hum too, I fear we can not avoid the msch round:
After your comment I realize that we can not let the channel enabled 
without a valid ISC in the case we enable IRQ.
So unless other opinion, back to the css_enable(schid, isc).


-- 
Pierre Morel
IBM Lab Boeblingen