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

Cornelia Huck
2020-07-09T12:14:01Z

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.

> +
> /* 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/ ?

> + 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)?

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

> + report_info("Unexpected Subch. status %02x", irb.scsw.sch_stat);
> + ret = -1;
> + goto end;
> + }
> +
> +end:
> + report_prefix_pop();
> + return ret;

(...)