diff mbox series

[kvm-unit-tests,1/1] s390x: css: check the CSS is working with any ISC

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

Commit Message

Pierre Morel Aug. 12, 2021, 11:53 a.m. UTC
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(-)

Comments

Cornelia Huck Aug. 12, 2021, 12:31 p.m. UTC | #1
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();
>  
>
Pierre Morel Aug. 12, 2021, 2:59 p.m. UTC | #2
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
Thomas Huth Aug. 18, 2021, 7:40 a.m. UTC | #3
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();
>   
>
Pierre Morel Aug. 23, 2021, 9:16 a.m. UTC | #4
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 mbox series

Patch

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();