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

Pierre Morel
2019-12-12T14:10:24Z

On 2019-12-12 13:26, Cornelia Huck wrote:
> On Wed, 11 Dec 2019 16:46:09 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>> When a channel is enabled we can start a SENSE command using the SSCH
>
> s/SENSE/SENSE ID/
>
> SENSE is for getting sense data after a unit check :)

Yes, thanks.

>
>> instruction to recognize the control unit and device.
>>
>> This tests the success of SSCH, the I/O interruption and the TSCH
>> instructions.
>>
>> The test expects a device with a control unit type of 0xC0CA as the
>> first subchannel of the CSS.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>> lib/s390x/css.h | 13 ++++
>> s390x/css.c | 175 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 188 insertions(+)
>
>> +static void irq_io(void)
>> +{
>> + int ret = 0;
>> + char *flags;
>> + int sid;
>> +
>> + report_prefix_push("Interrupt");
>> + if (lowcore->io_int_param != CSS_TEST_INT_PARAM) {
>> + report(0, "Bad io_int_param: %x", lowcore->io_int_param);
>> + report_prefix_pop();
>> + return;
>> + }
>> + report_prefix_pop();
>> +
>> + 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, "IRB scsw flags: %s", flags);
>
> I guess that should only happen if the I/O interrupt was for another
> subchannel, and we only enable one subchannel, right?
>
> Maybe log "I/O interrupt, but sch not status pending: <flags>"? (No
> idea how log the logged messages can be for kvm unit tests.)

Yes, the log message I had was not very useful at first sight.

>
>> + goto pop;
>> + case 2:
>> + report(0, "TSCH return unexpected CC 2");
>
> s/return/returns/
>
>> + goto pop;
>> + case 3:
>> + report(0, "Subchannel %08x not operational", sid);
>> + goto pop;
>> + case 0:
>> + /* Stay humble on success */
>
> :)
>
>> + break;
>> + }
>> +pop:
>> + report_prefix_pop();
>> +}
>> +
>> +static int start_subchannel(int code, char *data, int count)
>> +{
>> + int ret;
>> + struct pmcw *p = &schib.pmcw;
>> + struct orb *orb_p = &orb[0];
>> +
>> + /* Verify that a test subchannel has been set */
>> + if (!test_device_sid) {
>> + report_skip("No device");
>> + return 0;
>> + }
>> +
>> + if ((unsigned long)data >= 0x80000000UL) {
>> + report(0, "Data above 2G! %p", data);
>> + return 0;
>> + }
>> +
>> + /* Verify that the subchannel has been enabled */
>> + ret = stsch(test_device_sid, &schib);
>> + if (ret) {
>> + report(0, "Err %d on stsch on sid %08x", ret, test_device_sid);
>> + return 0;
>> + }
>> + if (!(p->flags & PMCW_ENABLE)) {
>> + report_skip("Device (sid %08x) not enabled", test_device_sid);
>> + return 0;
>> + }
>> +
>> + report_prefix_push("ssch");
>> + /* Build the CCW chain with a single CCW */
>> + ccw[0].code = code;
>> + ccw[0].flags = 0; /* No flags need to be set */
>> + ccw[0].count = count;
>> + ccw[0].data_address = (int)(unsigned long)data;
>> + orb_p->intparm = CSS_TEST_INT_PARAM;
>> + orb_p->ctrl = ORB_F_INIT_IRQ|ORB_F_FORMAT|ORB_F_LPM_DFLT;
>> + if ((unsigned long)&ccw[0] >= 0x80000000UL) {
>> + report(0, "CCW above 2G! %016lx", (unsigned long)&ccw[0]);
>
> Maybe check before filling out the ccw?

Yes. Also I wonder if we should not make sure the all kvm-test-text and
data are under 2G by construct, because I am quite sure that this sort
of tests will repeat all over the kvm-unit-test code.

Will provide a separate patch for this, in between just do as you said,
it is the logical thing to do here.

>
>> + report_prefix_pop();
>> + return 0;
>> + }
>> + orb_p->cpa = (unsigned int) (unsigned long)&ccw[0];
>> +
>> + ret = ssch(test_device_sid, orb_p);
>> + if (ret) {
>> + report(0, "ssch cc=%d", ret);
>> + report_prefix_pop();
>> + return 0;
>> + }
>> + report_prefix_pop();
>> + return 1;
>> +}
>> +
>> +/*
>> + * test_sense
>> + * Pre-requisits:
>> + * - We need the QEMU PONG device as the first recognized
>> + * - device by the enumeration.
>> + * - ./s390x-run s390x/css.elf -device ccw-pong,cu_type=0xc0ca
>> + */
>> +static void test_sense(void)
>> +{
>> + int ret;
>> +
>> + ret = register_io_int_func(irq_io);
>> + if (ret) {
>> + report(0, "Could not register IRQ handler");
>> + goto unreg_cb;
>> + }
>> +
>> + enable_io_irq();
>> + lowcore->io_int_param = 0;
>> +
>> + ret = start_subchannel(CCW_CMD_SENSE_ID, buffer, sizeof(senseid));
>> + if (!ret) {
>> + report(0, "start_subchannel failed");
>> + goto unreg_cb;
>> + }
>> +
>> + delay(100);
>> + if (lowcore->io_int_param != CSS_TEST_INT_PARAM) {
>> + report(0, "cu_type: expect 0x%08x, got 0x%08x",
>> + CSS_TEST_INT_PARAM, lowcore->io_int_param);
>> + goto unreg_cb;
>> + }
>
> This still looks like that odd "delay and hope an interrupt has arrived
> in the mean time" pattern.

yes.

>
> Also, doesn't the interrupt handler check for the intparm already?

Yes, if the interrupt fires.

>
>> +
>> + senseid.cu_type = buffer[2] | (buffer[1] << 8);
>
> This still looks odd; why not have the ccw fill out the senseid
> structure directly?

Oh sorry, you already said and I forgot to modify this.
thanks

>
>> +
>> + /* Sense ID is non packed cut_type is at offset +1 byte */
>
> I have trouble parsing this sentence...
>
>> + if (senseid.cu_type == PONG_CU)
>> + report(1, "cu_type: expect 0x%04x got 0x%04x",
>> + PONG_CU_TYPE, senseid.cu_type);
>> + else
>> + report(0, "cu_type: expect 0x%04x got 0x%04x",
>> + PONG_CU_TYPE, senseid.cu_type);
>
> Didn't you want to check for ff in the reserved field as well?

It was not intended as a check for SENSE_ID but for STSCH/READ.
But, while at this... why not.

Thanks for the review.
Regards,

Pierre

--
Pierre Morel
IBM Lab Boeblingen