| Message ID | 1628769189-10699-2-git-send-email-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: css: check the CSS is working with any ISC | expand |
On Thu, Aug 12 2021, Pierre Morel <pmorel@linux.ibm.com> wrote: > In the previous version we did only check that one ISC dedicated by > Linux for I/O is working fine. > > However, there is no reason to prefer one ISC to another ISC, we are > free to take anyone. > > Let's check all possible ISC to verify that QEMU/KVM is really ISC > independent. It's probably a good idea to test for a non-standard isc. Not sure whether we need all of them, but it doesn't hurt. Do you also have plans for a test to verify the priority handling for the different iscs? > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > s390x/css.c | 25 +++++++++++++++++-------- > 1 file changed, 17 insertions(+), 8 deletions(-) > (...) > @@ -142,7 +143,6 @@ static void sense_id(void) > > static void css_init(void) > { > - assert(register_io_int_func(css_irq_io) == 0); > lowcore_ptr->io_int_param = 0; > > report(get_chsc_scsc(), "Store Channel Characteristics"); > @@ -351,11 +351,20 @@ int main(int argc, char *argv[]) > int i; > > report_prefix_push("Channel Subsystem"); > - enable_io_isc(0x80 >> IO_SCH_ISC); > - for (i = 0; tests[i].name; i++) { > - report_prefix_push(tests[i].name); > - tests[i].func(); > - report_prefix_pop(); > + > + for (io_isc = 0; io_isc < 8; io_isc++) { > + report_info("ISC: %d\n", io_isc); > + > + enable_io_isc(0x80 >> io_isc); > + assert(register_io_int_func(css_irq_io) == 0); Why are you registering/deregistering the irq handler multiple times? It should be the same, regardless of the isc? > + > + for (i = 0; tests[i].name; i++) { > + report_prefix_push(tests[i].name); > + tests[i].func(); > + report_prefix_pop(); > + } > + > + unregister_io_int_func(css_irq_io); > } > report_prefix_pop(); > >
On 8/12/21 2:31 PM, Cornelia Huck wrote: > On Thu, Aug 12 2021, Pierre Morel <pmorel@linux.ibm.com> wrote: > >> In the previous version we did only check that one ISC dedicated by >> Linux for I/O is working fine. >> >> However, there is no reason to prefer one ISC to another ISC, we are >> free to take anyone. >> >> Let's check all possible ISC to verify that QEMU/KVM is really ISC >> independent. > > It's probably a good idea to test for a non-standard isc. Not sure > whether we need all of them, but it doesn't hurt. > > Do you also have plans for a test to verify the priority handling for > the different iscs? No, I did not think about this yet. > >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> s390x/css.c | 25 +++++++++++++++++-------- >> 1 file changed, 17 insertions(+), 8 deletions(-) >> > > (...) > >> @@ -142,7 +143,6 @@ static void sense_id(void) >> >> static void css_init(void) >> { >> - assert(register_io_int_func(css_irq_io) == 0); >> lowcore_ptr->io_int_param = 0; >> >> report(get_chsc_scsc(), "Store Channel Characteristics"); >> @@ -351,11 +351,20 @@ int main(int argc, char *argv[]) >> int i; >> >> report_prefix_push("Channel Subsystem"); >> - enable_io_isc(0x80 >> IO_SCH_ISC); >> - for (i = 0; tests[i].name; i++) { >> - report_prefix_push(tests[i].name); >> - tests[i].func(); >> - report_prefix_pop(); >> + >> + for (io_isc = 0; io_isc < 8; io_isc++) { >> + report_info("ISC: %d\n", io_isc); >> + >> + enable_io_isc(0x80 >> io_isc); >> + assert(register_io_int_func(css_irq_io) == 0); > > Why are you registering/deregistering the irq handler multiple times? It > should be the same, regardless of the isc? Yes, right, did not pay attention when pushing all in the loop, I will get it out of the loop. Thanks, Pierre
On 12/08/2021 13.53, Pierre Morel wrote: > In the previous version we did only check that one ISC dedicated by > Linux for I/O is working fine. > > However, there is no reason to prefer one ISC to another ISC, we are > free to take anyone. > > Let's check all possible ISC to verify that QEMU/KVM is really ISC > independent. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > s390x/css.c | 25 +++++++++++++++++-------- > 1 file changed, 17 insertions(+), 8 deletions(-) > > diff --git a/s390x/css.c b/s390x/css.c > index c340c539..aa005309 100644 > --- a/s390x/css.c > +++ b/s390x/css.c > @@ -22,6 +22,7 @@ > > #define DEFAULT_CU_TYPE 0x3832 /* virtio-ccw */ > static unsigned long cu_type = DEFAULT_CU_TYPE; > +static int io_isc; > > static int test_device_sid; > static struct senseid *senseid; > @@ -46,7 +47,7 @@ static void test_enable(void) > return; > } > > - cc = css_enable(test_device_sid, IO_SCH_ISC); > + cc = css_enable(test_device_sid, io_isc); > > report(cc == 0, "Enable subchannel %08x", test_device_sid); > } > @@ -67,7 +68,7 @@ static void test_sense(void) > return; > } > > - ret = css_enable(test_device_sid, IO_SCH_ISC); > + ret = css_enable(test_device_sid, io_isc); > if (ret) { > report(0, "Could not enable the subchannel: %08x", > test_device_sid); > @@ -142,7 +143,6 @@ static void sense_id(void) > > static void css_init(void) > { > - assert(register_io_int_func(css_irq_io) == 0); > lowcore_ptr->io_int_param = 0; > > report(get_chsc_scsc(), "Store Channel Characteristics"); > @@ -351,11 +351,20 @@ int main(int argc, char *argv[]) > int i; > > report_prefix_push("Channel Subsystem"); > - enable_io_isc(0x80 >> IO_SCH_ISC); > - for (i = 0; tests[i].name; i++) { > - report_prefix_push(tests[i].name); > - tests[i].func(); > - report_prefix_pop(); > + > + for (io_isc = 0; io_isc < 8; io_isc++) { > + report_info("ISC: %d\n", io_isc); Would it make sense to add the "ISC" string as a prefix with report_prefix_push() instead, so that the tests get individual test names? Thomas > + enable_io_isc(0x80 >> io_isc); > + assert(register_io_int_func(css_irq_io) == 0); > + > + for (i = 0; tests[i].name; i++) { > + report_prefix_push(tests[i].name); > + tests[i].func(); > + report_prefix_pop(); > + } > + > + unregister_io_int_func(css_irq_io); > } > report_prefix_pop(); > >
On 8/18/21 9:40 AM, Thomas Huth wrote: > On 12/08/2021 13.53, Pierre Morel wrote: >> In the previous version we did only check that one ISC dedicated by >> Linux for I/O is working fine. >> >> However, there is no reason to prefer one ISC to another ISC, we are >> free to take anyone. >> >> Let's check all possible ISC to verify that QEMU/KVM is really ISC >> independent. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> s390x/css.c | 25 +++++++++++++++++-------- >> 1 file changed, 17 insertions(+), 8 deletions(-) >> >> diff --git a/s390x/css.c b/s390x/css.c >> index c340c539..aa005309 100644 >> --- a/s390x/css.c >> +++ b/s390x/css.c >> @@ -22,6 +22,7 @@ >> #define DEFAULT_CU_TYPE 0x3832 /* virtio-ccw */ >> static unsigned long cu_type = DEFAULT_CU_TYPE; >> +static int io_isc; >> static int test_device_sid; >> static struct senseid *senseid; >> @@ -46,7 +47,7 @@ static void test_enable(void) >> return; >> } >> - cc = css_enable(test_device_sid, IO_SCH_ISC); >> + cc = css_enable(test_device_sid, io_isc); >> report(cc == 0, "Enable subchannel %08x", test_device_sid); >> } >> @@ -67,7 +68,7 @@ static void test_sense(void) >> return; >> } >> - ret = css_enable(test_device_sid, IO_SCH_ISC); >> + ret = css_enable(test_device_sid, io_isc); >> if (ret) { >> report(0, "Could not enable the subchannel: %08x", >> test_device_sid); >> @@ -142,7 +143,6 @@ static void sense_id(void) >> static void css_init(void) >> { >> - assert(register_io_int_func(css_irq_io) == 0); >> lowcore_ptr->io_int_param = 0; >> report(get_chsc_scsc(), "Store Channel Characteristics"); >> @@ -351,11 +351,20 @@ int main(int argc, char *argv[]) >> int i; >> report_prefix_push("Channel Subsystem"); >> - enable_io_isc(0x80 >> IO_SCH_ISC); >> - for (i = 0; tests[i].name; i++) { >> - report_prefix_push(tests[i].name); >> - tests[i].func(); >> - report_prefix_pop(); >> + >> + for (io_isc = 0; io_isc < 8; io_isc++) { >> + report_info("ISC: %d\n", io_isc); > > Would it make sense to add the "ISC" string as a prefix with > report_prefix_push() instead, so that the tests get individual test names? > > Thomas > Yes, this would make a better description I think. Thanks, Pierre
diff --git a/s390x/css.c b/s390x/css.c index c340c539..aa005309 100644 --- a/s390x/css.c +++ b/s390x/css.c @@ -22,6 +22,7 @@ #define DEFAULT_CU_TYPE 0x3832 /* virtio-ccw */ static unsigned long cu_type = DEFAULT_CU_TYPE; +static int io_isc; static int test_device_sid; static struct senseid *senseid; @@ -46,7 +47,7 @@ static void test_enable(void) return; } - cc = css_enable(test_device_sid, IO_SCH_ISC); + cc = css_enable(test_device_sid, io_isc); report(cc == 0, "Enable subchannel %08x", test_device_sid); } @@ -67,7 +68,7 @@ static void test_sense(void) return; } - ret = css_enable(test_device_sid, IO_SCH_ISC); + ret = css_enable(test_device_sid, io_isc); if (ret) { report(0, "Could not enable the subchannel: %08x", test_device_sid); @@ -142,7 +143,6 @@ static void sense_id(void) static void css_init(void) { - assert(register_io_int_func(css_irq_io) == 0); lowcore_ptr->io_int_param = 0; report(get_chsc_scsc(), "Store Channel Characteristics"); @@ -351,11 +351,20 @@ int main(int argc, char *argv[]) int i; report_prefix_push("Channel Subsystem"); - enable_io_isc(0x80 >> IO_SCH_ISC); - for (i = 0; tests[i].name; i++) { - report_prefix_push(tests[i].name); - tests[i].func(); - report_prefix_pop(); + + for (io_isc = 0; io_isc < 8; io_isc++) { + report_info("ISC: %d\n", io_isc); + + enable_io_isc(0x80 >> io_isc); + assert(register_io_int_func(css_irq_io) == 0); + + for (i = 0; tests[i].name; i++) { + report_prefix_push(tests[i].name); + tests[i].func(); + report_prefix_pop(); + } + + unregister_io_int_func(css_irq_io); } report_prefix_pop();
In the previous version we did only check that one ISC dedicated by Linux for I/O is working fine. However, there is no reason to prefer one ISC to another ISC, we are free to take anyone. Let's check all possible ISC to verify that QEMU/KVM is really ISC independent. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- s390x/css.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)