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

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

On 2020-07-09 14:13, Cornelia Huck wrote:
> On Thu,  9 Jul 2020 10:07:48 +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          |  35 ++++++++
>>   lib/s390x/css_lib.c      | 183 +++++++++++++++++++++++++++++++++++++++
>>   s390x/css.c              |  80 +++++++++++++++++
>>   4 files changed, 299 insertions(+)
> 
> (...)
> 
>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index eda68a4..c64edd5 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -16,6 +16,7 @@
>>   #include <interrupt.h>
>>   #include <asm/arch_def.h>
>>   #include <asm/time.h>
>> +#include <asm/arch_def.h>
>>   
>>   #include <css.h>
>>   
>> @@ -103,6 +104,9 @@ retry:
>>   	/* Update the SCHIB to enable the channel and set the ISC */
>>   	pmcw->flags |= flags;
>>   
>> +	/* Set Interruption Subclass to IO_SCH_ISC */
>> +	pmcw->flags |= (isc << PMCW_ISC_SHIFT);
> 
> But isn't the isc already contained in 'flags'? I think you should just
> delete these two lines.

right.

> 
>> +
>>   	/* Tell the CSS we want to modify the subchannel */
>>   	cc = msch(schid, &schib);
>>   	if (cc) {
> 
> (...)
> 
>> +/* wait_and_check_io_completion:
>> + * @schid: the subchannel ID
>> + *
>> + * Makes the most common check to validate a successful I/O
>> + * completion.
>> + * Only report failures.
>> + */
>> +int wait_and_check_io_completion(int schid)
>> +{
>> +	int ret = 0;
>> +
>> +	wait_for_interrupt(PSW_MASK_IO);
>> +
>> +	report_prefix_push("check I/O completion");
>> +
>> +	if (lowcore_ptr->io_int_param != schid) {
>> +		report(0, "interrupt parameter: expected %08x got %08x",
>> +		       schid, lowcore_ptr->io_int_param);
>> +		ret = -1;
>> +		goto end;
>> +	}
>> +
>> +	/* Verify that device status is valid */
>> +	if (!(irb.scsw.ctrl & SCSW_SC_PENDING)) {
>> +		report(0, "No status pending after interrupt. Subch Ctrl: %08x",
>> +		       irb.scsw.ctrl);
>> +		ret = -1;
>> +		goto end;
>> +	}
>> +
>> +	if (!(irb.scsw.ctrl & (SCSW_SC_SECONDARY | SCSW_SC_PRIMARY))) {
>> +		report(0, "Primary or secondary status missing. Subch Ctrl: %08x",
>> +		       irb.scsw.ctrl);
>> +		ret = -1;
>> +		goto end;
>> +	}
>> +
>> +	if (!(irb.scsw.dev_stat & (SCSW_DEVS_DEV_END | SCSW_DEVS_SCH_END))) {
>> +		report(0, "No device end nor sch end. Dev. status: %02x",
> 
> s/nor/or/ ?

OK

> 
>> +		       irb.scsw.dev_stat);
>> +		ret = -1;
>> +		goto end;
>> +	}
>> +
>> +	if (irb.scsw.sch_stat & !(SCSW_SCHS_PCI | SCSW_SCHS_IL)) {
> 
> Did you mean ~(SCSW_SCHS_PCI | SCSW_SCHS_IL)?

grrr... yes, thanks.

> 
> If yes, why do think a PCI may show up?

Should not in the current implementation.
I thought I can add it as a general test.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen