Re: [kvm-unit-tests PATCH v11 8/9] s390x: css: msch, enable test

[Cornelia Huck <cohuck@redhat.com>]

On Thu, 9 Jul 2020 15:12:05 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2020-07-09 13:40, Cornelia Huck wrote:
> > On Thu,  9 Jul 2020 10:07:47 +0200
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >   
> >> A second step when testing the channel subsystem is to prepare a channel
> >> for use.
> >> This includes:
> >> - Get the current subchannel Information Block (SCHIB) using STSCH
> >> - Update it in memory to set the ENABLE bit and the specified ISC
> >> - Tell the CSS that the SCHIB has been modified using MSCH
> >> - Get the SCHIB from the CSS again to verify that the subchannel is
> >>    enabled and uses the specified ISC.
> >> - If the command succeeds but subchannel is not enabled or the ISC
> >>    field is not as expected, retry a predefined retries count.
> >> - If the command fails, report the failure and do not retry, even
> >>    if cc indicates a busy/status pending as we do not expect this.
> >>
> >> This tests the MSCH instruction to enable a channel successfully.
> >> Retries are done and in case of error, and if the retries count
> >> is exceeded, a report is made.
> >>
> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> >> Acked-by: Thomas Huth <thuth@redhat.com>
> >> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >> ---
> >>   lib/s390x/css.h     |  8 +++--
> >>   lib/s390x/css_lib.c | 72 +++++++++++++++++++++++++++++++++++++++++++++
> >>   s390x/css.c         | 15 ++++++++++
> >>   3 files changed, 92 insertions(+), 3 deletions(-)  
> > 
> > (...)
> >   
> >> +/*
> >> + * css_msch: enable subchannel and set with specified ISC  
> > 
> > "css_enable: enable the subchannel with the specified ISC"
> > 
> > ?
> >   
> >> + * @schid: Subchannel Identifier
> >> + * @isc  : number of the interruption subclass to use
> >> + * 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;
> >> +	uint16_t flags;
> >> +	int cc;
> >> +
> >> +	/* Read the SCHIB for this subchannel */
> >> +	cc = stsch(schid, &schib);
> >> +	if (cc) {
> >> +		report_info("stsch: sch %08x failed with cc=%d", schid, cc);
> >> +		return cc;
> >> +	}
> >> +
> >> +	flags = PMCW_ENABLE | (isc << PMCW_ISC_SHIFT);
> >> +	if ((pmcw->flags & flags) == flags) {  
> > 
> > I think you want (pmcw->flags & PMCW_ENABLE) == PMCW_ENABLE -- this
> > catches the case of "subchannel has been enabled before, but with a
> > different isc".  
> 
> If with a different ISC, we need to modify the ISC.
> Don't we ?

I think that's a policy decision (I would probably fail and require a
disable before setting another isc, but that's a matter of taste).

Regardless, I think the current check doesn't even catch the 'different
isc' case?

> 
> >   
> >> +		report_info("stsch: sch %08x already enabled", schid);
> >> +		return 0;
> >> +	}  
> 
> 
> 
> > 
> > (...)
> >   
> 
>