From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:2982 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1729124AbgGFNFW (ORCPT ); Mon, 6 Jul 2020 09:05:22 -0400 Subject: Re: [kvm-unit-tests PATCH v10 9/9] s390x: css: ssch/tsch with sense and interrupt References: <1593707480-23921-1-git-send-email-pmorel@linux.ibm.com> <1593707480-23921-10-git-send-email-pmorel@linux.ibm.com> <20200706114655.5088b6b7.cohuck@redhat.com> From: Pierre Morel Message-ID: <02eb7a70-7a74-6f09-334f-004e69aaa198@linux.ibm.com> Date: Mon, 6 Jul 2020 15:01:50 +0200 MIME-Version: 1.0 In-Reply-To: <20200706114655.5088b6b7.cohuck@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-s390-owner@vger.kernel.org List-ID: To: Cornelia Huck Cc: kvm@vger.kernel.org, linux-s390@vger.kernel.org, frankja@linux.ibm.com, david@redhat.com, thuth@redhat.com, drjones@redhat.com On 2020-07-06 11:46, Cornelia Huck wrote: > On Thu, 2 Jul 2020 18:31:20 +0200 > Pierre Morel 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 >> --- >> 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