Re: [kvm-unit-tests PATCH v6 09/10] s390x: css: ssch/tsch with sense and interrupt

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

On 2020-05-14 14:24, Cornelia Huck wrote:
> On Fri, 24 Apr 2020 12:45:51 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We add a new css_lib file to contain the I/O function we may
> 
> s/function/functions/

Yes, thanks.


>> +	report_prefix_push("tsch");
>> +	sid = lowcore->subsys_id_word;
>> +	ret = tsch(sid, &irb);
>> +	switch (ret) {
>> +	case 1:
>> +		dump_irb(&irb);
>> +		flags = dump_scsw_flags(irb.scsw.ctrl);
>> +		report(0,
>> +		       "I/O interrupt, but sch not status pending: %s", flags);
> 
> "...but tsch reporting sch as not status pending" ?

Yes, better, Thx

> 
> A buggy implementation might give the wrong cc for tsch, but still
> indicate status pending in the control block.

OK, I will write the status for other error cases too.
> 
>> +		break;
>> +	case 2:
>> +		report(0, "TSCH returns unexpected CC 2");

will also s/TSCH/tsch/ here

>> +		break;
>> +	case 3:
>> +		report(0, "Subchannel %08x not operational", sid);
> 
> "tsch reporting subchannel %08x as not operational" ?

Yes, better.
and I will standardize these three reports.

> 
>> +		break;
>> +	case 0:
>> +		/* Stay humble on success */
>> +		break;
>> +	}
>> +pop:
>> +	report_prefix_pop();
>> +	lowcore->io_old_psw.mask &= ~PSW_MASK_WAIT;
>> +}
> 
...snip...

>> +	lowcore->io_int_param = 0;
>> +
>> +	ret = start_subchannel(CCW_CMD_SENSE_ID, &senseid, sizeof(senseid));
> 
> You're always send the full (extended) sense id block. What if the the
> machine you're running on doesn't support extended sense id? Would the
> SLI ccw flag help?

Yes, you are right, since I only use senseid for recognition of the PONG 
subchannel, I can accept non extended senseid

> 
>> +	if (!ret) {
>> +		report(0, "start_senseid failed");
> 
> "ssch failed for SENSE ID on sch <sch>" ?

Better. Thx

> 
>> +		goto unreg_cb;
>> +	}
>> +
>> +	wfi(PSW_MASK_IO);
>> +
>> +	if (lowcore->io_int_param != test_device_sid) {
>> +		report(0,
>> +		       "No interrupts. io_int_param: expect 0x%08x, got 0x%08x",
>> +		       test_device_sid, lowcore->io_int_param);
> 
> Doesn't irq_io() already moan here?

Yes, right, I kept this from last version with delays.
It has no sense here:
- We are the only user of the CSS
- If the interrupt did not fire we are stuck in wfi()

Thanks.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen