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

[Pierre Morel <pmorel@linux.ibm.com>]

On 2019-12-12 13:01, Cornelia Huck wrote:
> On Wed, 11 Dec 2019 16:46:08 +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 | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 65 insertions(+)
>>
>> diff --git a/s390x/css.c b/s390x/css.c
>> index dfab35f..b8824ad 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -19,12 +19,24 @@
>>   #include <asm/time.h>
>>   
>>   #include <css.h>
>> +#include <asm/time.h>
>>   
>>   #define SID_ONE		0x00010000
>>   
>>   static struct schib schib;
>>   static int test_device_sid;
>>   
>> +static inline void delay(unsigned long ms)
>> +{
>> +	unsigned long startclk;
>> +
>> +	startclk = get_clock_ms();
>> +	for (;;) {
>> +		if (get_clock_ms() - startclk > ms)
>> +			break;
>> +	}
>> +} >
> Would this function be useful for other callers as well? I.e., should
> it go into a common header?

Yes, I wanted to put it in the new time.h with the get_clock_ms()  but 
did not since I already got the RB.
I also did not want to add a patch to the series, but since you ask, I 
can put it in a separate patch to keep the RB and to add it in the time.h

> 
>> +
>>   static void test_enumerate(void)
>>   {
>>   	struct pmcw *pmcw = &schib.pmcw;
>> @@ -64,11 +76,64 @@ out:
>>   	report(1, "Devices, tested: %d, I/O type: %d", scn, scn_found);
>>   }
>>   
>> +static void test_enable(void)
>> +{
>> +	struct pmcw *pmcw = &schib.pmcw;
>> +		int count = 0;
> 
> Odd indentation.

indeed!

> 
>> +	int cc;
>> +
>> +	if (!test_device_sid) {
>> +		report_skip("No device");
>> +		return;
>> +	}
>> +	/* Read the SCHIB for this subchannel */
>> +	cc = stsch(test_device_sid, &schib);
>> +	if (cc) {
>> +		report(0, "stsch cc=%d", 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) {
>> +		/*
>> +		 * If the subchannel is status pending or
>> +		 * if a function is in progress,
>> +		 * we consider both cases as errors.
>> +		 */
>> +		report(0, "msch cc=%d", cc);
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * Read the SCHIB again to verify the enablement
>> +	 * insert a little delay and try 5 times.
>> +	 */
>> +	do {
>> +		cc = stsch(test_device_sid, &schib);
>> +		if (cc) {
>> +			report(0, "stsch cc=%d", cc);
>> +			return;
>> +		}
>> +		delay(10);
> 
> That's just a short delay to avoid a busy loop, right? msch should be
> immediate,

Thought you told to me that it may not be immediate in zVM did I 
misunderstand?

> and you probably should not delay on success?

yes, it is not optimized, I can test PMCW_ENABLE in the loop this way we 
can see if, in the zVM case we need to do retries or not.


> 
>> +	} while (!(pmcw->flags & PMCW_ENABLE) && count++ < 5);
> 
> How is this supposed to work? Doesn't the stsch overwrite the control
> block again, so you need to re-set the enable bit before you retry?

I do not think so, there is no msch() in the loop.
Do I miss something?

Thanks for the review,
Regards,

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen