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

Pierre Morel
2019-12-12T14:01:15Z

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