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

Pierre Morel
2020-07-09T13:12:12Z

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 ?

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

>
> (...)
>

--
Pierre Morel
IBM Lab Boeblingen