| Message ID | 1576079170-7244-8-git-send-email-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Testing the Channel Subsystem I/O | expand |
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? > + > 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. > + 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, and you probably should not delay on success? > + } 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? > + > + if (!(pmcw->flags & PMCW_ENABLE)) { > + report(0, "Enable failed. pmcw: %x", pmcw->flags); > + return; > + } > + report(1, "Tested"); > +} > + > static struct { > const char *name; > void (*func)(void); > } tests[] = { > { "enumerate (stsch)", test_enumerate }, > + { "enable (msch)", test_enable }, > { NULL, NULL } > }; >
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
On Thu, 12 Dec 2019 15:01:07 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > 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(+) > >> + /* 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? Maybe I have been confusing... what I'm referring to is this programming note for msch: "It is recommended that the program inspect the contents of the subchannel by subsequently issuing STORE SUBCHANNEL when MODIFY SUBCHANNEL sets condition code 0. Use of STORE SUBCHANNEL is a method for deter- mining if the designated subchannel was changed or not. Failure to inspect the subchan- nel following the setting of condition code 0 by MODIFY SUBCHANNEL may result in conditions that the program does not expect to occur." That's exactly what we had to do under z/VM back then: do the msch, check via stsch, redo the msch if needed, check again via stsch. It usually worked with the second msch the latest. > > > 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? Well, _I_ missed that the msch() was missing :) You need it (see above); just waiting and re-doing the stsch is useless, as msch is a synchronous instruction which has finished its processing after the cc has been set.
On 2019-12-12 15:10, Cornelia Huck wrote: > On Thu, 12 Dec 2019 15:01:07 +0100 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> 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(+) > >>>> + /* 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? > > Maybe I have been confusing... what I'm referring to is this > programming note for msch: > > "It is recommended that the program inspect the > contents of the subchannel by subsequently > issuing STORE SUBCHANNEL when MODIFY > SUBCHANNEL sets condition code 0. Use of > STORE SUBCHANNEL is a method for deter- > mining if the designated subchannel was > changed or not. Failure to inspect the subchan- > nel following the setting of condition code 0 by > MODIFY SUBCHANNEL may result in conditions > that the program does not expect to occur." > > That's exactly what we had to do under z/VM back then: do the msch, > check via stsch, redo the msch if needed, check again via stsch. It > usually worked with the second msch the latest. OK, I understand, then it is a bug in zVM that this test could enlighten. I think we should keep it so, it allows to recognize 3 cases (after I change to test ENABLE in the loop as I said I will): - immediate ENABLE - asynchrone ENABLE - failure to ENABLE > >> >>> 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? > > Well, _I_ missed that the msch() was missing :) You need it (see above); > just waiting and re-doing the stsch is useless, as msch is a > synchronous instruction which has finished its processing after the cc > has been set. > Since kvm-unit-test is a test system, not an OS so I think that here we have one more point to leverage the enable function: - We need to test the enable (what I did (partially)) - We need the enable to work (your proposition) to further test the I/O OK, I rework this part with your comment in mind. Thanks Pierre
On Thu, 12 Dec 2019 15:21:21 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 2019-12-12 15:10, Cornelia Huck wrote: > > On Thu, 12 Dec 2019 15:01:07 +0100 > > Pierre Morel <pmorel@linux.ibm.com> wrote: > > > >> 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(+) > > > >>>> + /* 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? > > > > Maybe I have been confusing... what I'm referring to is this > > programming note for msch: > > > > "It is recommended that the program inspect the > > contents of the subchannel by subsequently > > issuing STORE SUBCHANNEL when MODIFY > > SUBCHANNEL sets condition code 0. Use of > > STORE SUBCHANNEL is a method for deter- > > mining if the designated subchannel was > > changed or not. Failure to inspect the subchan- > > nel following the setting of condition code 0 by > > MODIFY SUBCHANNEL may result in conditions > > that the program does not expect to occur." > > > > That's exactly what we had to do under z/VM back then: do the msch, > > check via stsch, redo the msch if needed, check again via stsch. It > > usually worked with the second msch the latest. > > OK, I understand, then it is a bug in zVM that this test could enlighten. Probably more a quirk than a bug... the explanation there is not explicit about that :) > > I think we should keep it so, it allows to recognize 3 cases (after I > change to test ENABLE in the loop as I said I will): > - immediate ENABLE This is the good case. > - asynchrone ENABLE This one I would consider an architecture violation. > - failure to ENABLE This is the quirk above. But I'm not quite sure how you would be able to distinguish the last two cases? > > > >> > >>> 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? > > > > Well, _I_ missed that the msch() was missing :) You need it (see above); > > just waiting and re-doing the stsch is useless, as msch is a > > synchronous instruction which has finished its processing after the cc > > has been set. > > > > Since kvm-unit-test is a test system, not an OS so I think that here we > have one more point to leverage the enable function: > - We need to test the enable (what I did (partially)) Maybe also log if you needed to retry? Not as an error, but as additional information? > - We need the enable to work (your proposition) to further test the I/O > > OK, I rework this part with your comment in mind. > > Thanks > Pierre > >
On 2019-12-12 15:33, Cornelia Huck wrote: > On Thu, 12 Dec 2019 15:21:21 +0100 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> On 2019-12-12 15:10, Cornelia Huck wrote: >>> On Thu, 12 Dec 2019 15:01:07 +0100 >>> Pierre Morel <pmorel@linux.ibm.com> wrote: >>> >>>> 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(+) >>> >>>>>> + /* 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? >>> >>> Maybe I have been confusing... what I'm referring to is this >>> programming note for msch: >>> >>> "It is recommended that the program inspect the >>> contents of the subchannel by subsequently >>> issuing STORE SUBCHANNEL when MODIFY >>> SUBCHANNEL sets condition code 0. Use of >>> STORE SUBCHANNEL is a method for deter- >>> mining if the designated subchannel was >>> changed or not. Failure to inspect the subchan- >>> nel following the setting of condition code 0 by >>> MODIFY SUBCHANNEL may result in conditions >>> that the program does not expect to occur." >>> >>> That's exactly what we had to do under z/VM back then: do the msch, >>> check via stsch, redo the msch if needed, check again via stsch. It >>> usually worked with the second msch the latest. >> >> OK, I understand, then it is a bug in zVM that this test could enlighten. > > Probably more a quirk than a bug... the explanation there is not > explicit about that :) > >> >> I think we should keep it so, it allows to recognize 3 cases (after I >> change to test ENABLE in the loop as I said I will): >> - immediate ENABLE > > This is the good case. > >> - asynchrone ENABLE > > This one I would consider an architecture violation. > >> - failure to ENABLE > > This is the quirk above. > > But I'm not quite sure how you would be able to distinguish the last > two cases? This is where the delay can help. But yes, not sure that we can differentiate this without to know how long we should delay. > >>> >>>> >>>>> 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? >>> >>> Well, _I_ missed that the msch() was missing :) You need it (see above); >>> just waiting and re-doing the stsch is useless, as msch is a >>> synchronous instruction which has finished its processing after the cc >>> has been set. >>> >> >> Since kvm-unit-test is a test system, not an OS so I think that here we >> have one more point to leverage the enable function: >> - We need to test the enable (what I did (partially)) > > Maybe also log if you needed to retry? Not as an error, but as > additional information? Yes. Regards, Pierre
On 2019-12-12 17:05, Pierre Morel wrote: > > > On 2019-12-12 15:33, Cornelia Huck wrote: >> On Thu, 12 Dec 2019 15:21:21 +0100 >> Pierre Morel <pmorel@linux.ibm.com> wrote: >> >>> On 2019-12-12 15:10, Cornelia Huck wrote: >>>> On Thu, 12 Dec 2019 15:01:07 +0100 >>>> Pierre Morel <pmorel@linux.ibm.com> wrote: >>>>> 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(+) >>>>>>> + /* 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? >>>> >>>> Maybe I have been confusing... what I'm referring to is this >>>> programming note for msch: >>>> >>>> "It is recommended that the program inspect the >>>> contents of the subchannel by subsequently >>>> issuing STORE SUBCHANNEL when MODIFY >>>> SUBCHANNEL sets condition code 0. Use of >>>> STORE SUBCHANNEL is a method for deter- >>>> mining if the designated subchannel was >>>> changed or not. Failure to inspect the subchan- >>>> nel following the setting of condition code 0 by >>>> MODIFY SUBCHANNEL may result in conditions >>>> that the program does not expect to occur." >>>> >>>> That's exactly what we had to do under z/VM back then: do the msch, >>>> check via stsch, redo the msch if needed, check again via stsch. It >>>> usually worked with the second msch the latest. >>> >>> OK, I understand, then it is a bug in zVM that this test could >>> enlighten. >> >> Probably more a quirk than a bug... the explanation there is not >> explicit about that :) >> >>> >>> I think we should keep it so, it allows to recognize 3 cases (after I >>> change to test ENABLE in the loop as I said I will): >>> - immediate ENABLE >> >> This is the good case. >> >>> - asynchrone ENABLE >> >> This one I would consider an architecture violation. >> >>> - failure to ENABLE >> >> This is the quirk above. >> >> But I'm not quite sure how you would be able to distinguish the last >> two cases? > > This is where the delay can help. > But yes, not sure that we can differentiate this without to know how > long we should delay. > > >> >>>>>> 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? >>>> >>>> Well, _I_ missed that the msch() was missing :) You need it (see >>>> above); >>>> just waiting and re-doing the stsch is useless, as msch is a >>>> synchronous instruction which has finished its processing after the cc >>>> has been set. >>> >>> Since kvm-unit-test is a test system, not an OS so I think that here we >>> have one more point to leverage the enable function: >>> - We need to test the enable (what I did (partially)) >> >> Maybe also log if you needed to retry? Not as an error, but as >> additional information? > > Yes. > > Regards, > Pierre > After all, I make it simple by testing if the MSCH works as expected, no retry, no delay. This is just a test. I will add a new patch to add a library function to enable the channel, with retry to serve when we really need to enable the channel, not to test. Regards, Pierre
On Thu, 12 Dec 2019 18:33:15 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > After all, I make it simple by testing if the MSCH works as expected, no > retry, no delay. > This is just a test. That's probably fine if you only run under kvm (not sure what your further plans here are). > > I will add a new patch to add a library function to enable the channel, > with retry to serve when we really need to enable the channel, not to test. A simple enable should be enough for kvm-only usage, we can add a retry easily if needed. We probably can also defer adding the library function until after we get another user :)
On 2019-12-13 10:33, Cornelia Huck wrote: > On Thu, 12 Dec 2019 18:33:15 +0100 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> After all, I make it simple by testing if the MSCH works as expected, no >> retry, no delay. >> This is just a test. > > That's probably fine if you only run under kvm (not sure what your > further plans here are). If the test fails, it fails. However, for other tests we need the enable to work. For example before making a SENSE_ID, in this case we will make retries. > >> >> I will add a new patch to add a library function to enable the channel, >> with retry to serve when we really need to enable the channel, not to test. > > A simple enable should be enough for kvm-only usage, we can add a retry > easily if needed. > > We probably can also defer adding the library function until after we > get another user :) > I already work on it in the prepared next series, I have - the enable test and - a enable_sch to use as a librairy function Regards, Pierre
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; + } +} + 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; + 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); + } while (!(pmcw->flags & PMCW_ENABLE) && count++ < 5); + + if (!(pmcw->flags & PMCW_ENABLE)) { + report(0, "Enable failed. pmcw: %x", pmcw->flags); + return; + } + report(1, "Tested"); +} + static struct { const char *name; void (*func)(void); } tests[] = { { "enumerate (stsch)", test_enumerate }, + { "enable (msch)", test_enable }, { NULL, NULL } };
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(+)