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

Pierre Morel
2020-07-09T13:42:02Z

On 2020-07-09 15:30, Cornelia Huck wrote:
> 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?

hum, right.
If it is OK I remove this one.
And I must rework the same test I do later
in this patch.

Thanks,
Pierre

--
Pierre Morel
IBM Lab Boeblingen