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-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(-)
> 
> (...)
> 
>> -int css_enable(int schid)
>> +/*
>> + * css_enable: enable Subchannel
>> + * @schid: Subchannel Identifier
>> + * @isc: Interruption subclass for this subchannel as a number
> 
> "number of the interruption subclass to use"?

Yes, thanks.

> 
>> + * Return value:
>> + *   On success: 0
>> + *   On error the CC of the faulty instruction
>> + *      or -1 if the retry count is exceeded.
>> + *
>> + */
>> +int css_enable(int schid, int isc)
>>   {
>>   	struct pmcw *pmcw = &schib.pmcw;
>>   	int retry_count = 0;
>> @@ -92,6 +103,9 @@ retry:
>>   	/* Update the SCHIB to enable the channel */
>>   	pmcw->flags |= PMCW_ENABLE;
>>   
>> +	/* Set Interruption Subclass to IO_SCH_ISC */
> 
> The specified isc, current callers just happen to pass that value.
> 

Forgot to remove this comment. Will do.

>> +	pmcw->flags |= (isc << PMCW_ISC_SHIFT);
>> +
>>   	/* Tell the CSS we want to modify the subchannel */
>>   	cc = msch(schid, &schib);
>>   	if (cc) {
>> @@ -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.

> 
>>   	if (pmcw->flags & PMCW_ENABLE) {
>>   		report_info("stsch: sch %08x enabled after %d retries",
>>   			    schid, retry_count);
>> @@ -129,3 +144,134 @@ retry:
>>   		    schid, retry_count, pmcw->flags);
>>   	return -1;
>>   }
>> +
>> +static struct irb irb;
>> +
>> +void css_irq_io(void)
>> +{
>> +	int ret = 0;
>> +	char *flags;
>> +	int sid;
>> +
>> +	report_prefix_push("Interrupt");
>> +	sid = lowcore_ptr->subsys_id_word;
>> +	/* Lowlevel set the SID as interrupt parameter. */
>> +	if (lowcore_ptr->io_int_param != sid) {
>> +		report(0,
>> +		       "io_int_param: %x differs from subsys_id_word: %x",
>> +		       lowcore_ptr->io_int_param, sid);
>> +		goto pop;
>> +	}
>> +	report_info("subsys_id_word: %08x io_int_param %08x io_int_word %08x",
>> +			lowcore_ptr->subsys_id_word,
>> +			lowcore_ptr->io_int_param,
>> +			lowcore_ptr->io_int_word);
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("tsch");
>> +	ret = tsch(sid, &irb);
>> +	switch (ret) {
>> +	case 1:
>> +		dump_irb(&irb);
>> +		flags = dump_scsw_flags(irb.scsw.ctrl);
>> +		report(0,
>> +		       "I/O interrupt, CC 1 but tsch reporting sch %08x as not status pending: %s",
> 
> "I/O interrupt, but tsch returns CC 1 for subchannel %08x" ?

Yes better, thanks

> 
>> +		       sid, flags);
>> +		break;
>> +	case 2:
>> +		report(0, "tsch returns unexpected CC 2");
>> +		break;
>> +	case 3:
>> +		report(0, "tsch reporting sch %08x as not operational", sid);
>> +		break;
>> +	case 0:
>> +		/* Stay humble on success */
>> +		break;
>> +	}
>> +pop:
>> +	report_prefix_pop();
>> +	lowcore_ptr->io_old_psw.mask &= ~PSW_MASK_WAIT;
>> +}
>> +
>> +int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw)
>> +{
>> +	struct orb orb = {
>> +		.intparm = sid,
>> +		.ctrl = ORB_CTRL_ISIC|ORB_CTRL_FMT|ORB_LPM_DFLT,
>> +		.cpa = (unsigned int) (unsigned long)ccw,
>> +	};
>> +
>> +	return ssch(sid, &orb);
>> +}
>> +
>> +/*
>> + * In the future, we want to implement support for CCW chains;
>> + * for that, we will need to work with ccw1 pointers.
>> + */
>> +static struct ccw1 unique_ccw;
>> +
>> +int start_single_ccw(unsigned int sid, int code, void *data, int count,
>> +		     unsigned char flags)
>> +{
>> +	int cc;
>> +	struct ccw1 *ccw = &unique_ccw;
>> +
>> +	report_prefix_push("start_subchannel");
>> +	/* Build the CCW chain with a single CCW */
>> +	ccw->code = code;
>> +	ccw->flags = flags; /* No flags need to be set */
> 
> s/No flags/No additional flags/

obviously :)

> 
>> +	ccw->count = count;
>> +	ccw->data_address = (int)(unsigned long)data;
>> +
>> +	cc = start_ccw1_chain(sid, ccw);
>> +	if (cc) {
>> +		report(0, "start_ccw_chain failed ret=%d", cc);
>> +		report_prefix_pop();
>> +		return cc;
>> +	}
>> +	report_prefix_pop();
>> +	return 0;
>> +}
>> +
>> +/*
>> + * css_residual_count
>> + * We expect no residual count when the ORB request was successful
> 
> If we have a short block, but have suppressed the incorrect length
> indication, we may have a successful request with a nonzero count.
> Maybe replace this with "Return the residual count, if it is valid."?


OK

> 
>> + * The residual count is valid when the subchannel is status pending
>> + * with primary status and device status only or device status and
>> + * subchannel status with PCI or incorrect length.
>> + * Return value:
>> + * Success: the residual count
>> + * Not meaningful: -1 (-1 can not be a valid count)
>> + */
>> +int css_residual_count(unsigned int schid)
>> +{
>> +
>> +	if (!(irb.scsw.ctrl & (SCSW_SC_PENDING | SCSW_SC_PRIMARY)))
>> +		goto fail;
> 
> s/fail/invalid/ ? It's not really a failure :)

yes

> 
>> +
>> +	if (irb.scsw.dev_stat)
>> +		if (irb.scsw.sch_stat & ~(SCSW_SCHS_PCI | SCSW_SCHS_IL))
>> +			goto fail;
>> +
>> +	return irb.scsw.count;
>> +
>> +fail:
>> +	report_info("sch  status %02x", irb.scsw.sch_stat);
>> +	report_info("dev  status %02x", irb.scsw.dev_stat);
>> +	report_info("ctrl status %08x", irb.scsw.ctrl);
>> +	report_info("count       %04x", irb.scsw.count);
>> +	report_info("ccw addr    %08x", irb.scsw.ccw_addr);
> 
> I don't understand why you dump this data if no valid residual count is
> available. But maybe I don't understand the purpose of this function
> correctly.

As debug information to facilitate the search why the function failed.
Would you prefer more accurate report_info inside the if tests?
or just return with error code?

> 

>>   
>> +/*
>> + * test_sense
>> + * Pre-requisits:
> 
> s/Pre-requisists/Pre-requisites/

OK

> 
>> + * - We need the test device as the first recognized
>> + *   device by the enumeration.
>> + */
>> +static void test_sense(void)
>> +{
>> +	int ret;
>> +	int len;
>> +
>> +	if (!test_device_sid) {
>> +		report_skip("No device");
>> +		return;
>> +	}
>> +
>> +	ret = css_enable(test_device_sid, IO_SCH_ISC);
>> +	if (ret) {
>> +		report(0,
>> +		       "Could not enable the subchannel: %08x",
>> +		       test_device_sid);
>> +		return;
>> +	}
>> +
>> +	ret = register_io_int_func(css_irq_io);
>> +	if (ret) {
>> +		report(0, "Could not register IRQ handler");
>> +		goto unreg_cb;
>> +	}
>> +
>> +	lowcore_ptr->io_int_param = 0;
>> +
>> +	memset(&senseid, 0, sizeof(senseid));
>> +	ret = start_single_ccw(test_device_sid, CCW_CMD_SENSE_ID,
>> +			       &senseid, sizeof(senseid), CCW_F_SLI);
>> +	if (ret) {
>> +		report(0, "ssch failed for SENSE ID on sch %08x with cc %d",
>> +		       test_device_sid, ret);
>> +		goto unreg_cb;
>> +	}
>> +
>> +	wait_for_interrupt(PSW_MASK_IO);
>> +
>> +	if (lowcore_ptr->io_int_param != test_device_sid) {
>> +		report(0, "ssch succeeded but interrupt parameter is wrong: expect %08x got %08x",
>> +		       test_device_sid, lowcore_ptr->io_int_param);
>> +		goto unreg_cb;
>> +	}
>> +
>> +	ret = css_residual_count(test_device_sid);
>> +	if (ret < 0) {
>> +		report(0, "ssch succeeded for SENSE ID but can not get a valid residual count");
>> +		goto unreg_cb;
>> +	}
> 
> I'm not sure what you're testing here. You should first test whether
> the I/O concluded normally (i.e., whether you actually get something
> like status pending with channel end/device end). If not, it does not
> make much sense to look either at the residual count or at the sense id
> data.
> 
> If css_residual_count does not return something >= 0 for that 'normal'
> case, something is definitely fishy, though :)

I will add the test before the call to get the residual count.
May be it leads to rework the css_residual_count too.

> 
>> +
>> +	len = sizeof(senseid) - ret;
>> +	if (ret && len < CSS_SENSEID_COMMON_LEN) {
>> +		report(0,
>> +		       "ssch succeeded for SENSE ID but report a too short length: %d",
> 
> s/report/transferred/ ?

OK

> 
>> +		       ret);
>> +		goto unreg_cb;
>> +	}
>> +
>> +	if (ret && len)
>> +		report_info("ssch succeeded for SENSE ID but report a shorter length: %d",
> 
> Same here.

OK

snip...


Thanks for review.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen