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

Pierre Morel
2019-12-10T17:35:48Z

On 2019-12-10 10:09, Cornelia Huck wrote:
> On Tue, 10 Dec 2019 10:01:46 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>> On 2019-12-09 17:54, Cornelia Huck wrote:
>>> On Fri, 6 Dec 2019 17:26:26 +0100
>>> 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
>>>> - 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.
>>>>
>>>> This tests the success of the MSCH instruction by enabling a channel.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>> s390x/css.c | 39 +++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 39 insertions(+)
>>>>
>>>> diff --git a/s390x/css.c b/s390x/css.c
>>>> index 3d4a986..4c0031c 100644
>>>> --- a/s390x/css.c
>>>> +++ b/s390x/css.c
>>>> @@ -58,11 +58,50 @@ static void test_enumerate(void)
>>>> report("Tested %d devices, %d found", 1, scn, found);
>>>> }
>>>>
>>>> +static void test_enable(void)
>>>> +{
>>>> + struct pmcw *pmcw = &schib.pmcw;
>>>> + int cc;
>>>> +
>>>> + if (!test_device_sid) {
>>>> + report_skip("No device");
>>>> + return;
>>>> + }
>>>> + /* Read the SCIB for this subchannel */
>>>
>>> s/SCIB/SCHIB/
>>
>> yes
>>
>>>
>>>> + cc = stsch(test_device_sid, &schib);
>>>> + if (cc) {
>>>> + report("stsch cc=%d", 0, cc);
>>>> + return;
>>>> + }
>>>> + /* Update the SCHIB to enable the channel */
>>>> + pmcw->flags |= PMCW_ENABLE;
>>>> +
>>>> + /* Tell the CSS we want to modify the subchannel */
>>>> + cc = msch(test_device_sid, &schib);
>>>> + if (cc) {
>>>> + report("msch cc=%d", 0, cc);
>>>
>>> So you expect the subchannel to be idle? Probably true, especially as
>>> QEMU has no reason to post an unsolicited interrupt for a test device.
>>>
>>>> + return;
>>>> + }
>>>> +
>>>> + /* Read the SCHIB again to verify the enablement */
>>>> + cc = stsch(test_device_sid, &schib);
>>>> + if (cc) {
>>>> + report("stsch cc=%d", 0, cc);
>>>> + return;
>>>> + }
>>>> + if (!(pmcw->flags & PMCW_ENABLE)) {
>>>> + report("Enable failed. pmcw: %x", 0, pmcw->flags);
>>>
>>> This check is fine when running under KVM. If this test is modified to
>>> run under z/VM in the future, you probably should retry here: I've seen
>>> the enable bit 'stick' only after the second msch() there.
>>
>> Oh. Thanks, may be I can loop with a delay and count.
>
> FWIW, the Linux kernel code is trying 5 times.
>
>> If I need to do this may be I need to create dedicated sub-functions to
>> include the sanity tests.
>
> I'm not sure how worthwhile investing time here is, actually: If you
> don't plan to run under anything but KVM, you won't need it. I'm not
> sure if current versions of z/VM still display the same behaviour (it
> has been some time...); on the other hand, it is compliant with the
> architecture...

OK, I just insert the retry.

Thanks,
Pierre

--
Pierre Morel
IBM Lab Boeblingen