[kvm-unit-tests PATCH v3 0/2] Fixing infinite loop on SCLP READ SCP INFO error

[kvm-unit-tests PATCH v3 0/2] Fixing infinite loop on SCLP READ SCP INFO error

[Pierre Morel]

Aborting on SCLP READ SCP INFO error leads to a deadloop.

The loop is:
abort() -> exit() -> smp_teardown() -> smp_query_num_cpus() ->
sclp_get_cpu_num() -> assert() -> abort()

Since smp_setup() is done after sclp_read_info() inside setup() this
loop only happens when only the start processor is running.
Let sclp_get_cpu_num() return 1 in this case.

Also implement the SCLP_RC_INSUFFICIENT_SCCB_LENGTH handling and
repeat the sclp command.

Pierre Morel (2):
  s390x: sclp: consider monoprocessor on read_info error
  s390x: sclp: Implement SCLP_RC_INSUFFICIENT_SCCB_LENGTH

 lib/s390x/sclp.c | 64 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 56 insertions(+), 8 deletions(-)

-- 
2.31.1

since v2:

- use tabs in first patch
  (Nico)

- Added comments

- Added SCLP_RC_INSUFFICIENT_SCCB_LENGTH handling

[kvm-unit-tests PATCH v3 1/2] s390x: sclp: consider monoprocessor on read_info error

[Pierre Morel]

A kvm-unit-test would hang if an abort happens before SCLP Read SCP
Information has completed if sclp_get_cpu_num() does not report at
least one CPU.
Since we obviously have one, report it.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/sclp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index 12919ca..34a31da 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -121,6 +121,12 @@ int sclp_get_cpu_num(void)
 {
 	if (read_info)
 		return read_info->entries_cpu;
+	/*
+	 * If we fail here and read_info has not being set,
+	 * it means we failed early and we try to abort the test.
+	 * We need to return at least one CPU, and obviously we have
+	 * at least one, for the smp_teardown to correctly work.
+	 */
 	return 1;
 }
 
-- 
2.31.1

Re: [kvm-unit-tests PATCH v3 1/2] s390x: sclp: consider monoprocessor on read_info error

[Nico Boehr]

Quoting Pierre Morel (2023-05-30 14:40:55)
> A kvm-unit-test would hang if an abort happens before SCLP Read SCP
> Information has completed if sclp_get_cpu_num() does not report at
> least one CPU.
> Since we obviously have one, report it.

Sorry for complaining again, in a discussion with Janosch we found that the
description and commit below can be easily misunderstood. I suggest the
following wording in the commit description:

s390x: sclp: treat system as single processor when read_info is NULL

When a test abort()s before SCLP read info is completed, the assertion on
read_info in sclp_read_info() will fail. Since abort() eventually calls
smp_teardown() which in turn calls sclp_get_cpu_num(), this will cause an
infinite abort() chain, causing the test to hang.

Fix this by considering the system single processor when read_info is missing.

[...]
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index 12919ca..34a31da 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -121,6 +121,12 @@ int sclp_get_cpu_num(void)
>  {
>         if (read_info)
>                 return read_info->entries_cpu;
> +       /*
> +        * If we fail here and read_info has not being set,
> +        * it means we failed early and we try to abort the test.
> +        * We need to return at least one CPU, and obviously we have
> +        * at least one, for the smp_teardown to correctly work.
> +        */

Please make this:

Don't abort here if read_info is NULL since abort() calls smp_teardown() which
eventually calls this function and thus causes an infinite abort() chain,
causing the test to hang. Since we obviously have at least one CPU, just return
one.

With these changes:

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Sorry for the back and forth.

Re: [kvm-unit-tests PATCH v3 1/2] s390x: sclp: consider monoprocessor on read_info error

[Pierre Morel]

On 6/1/23 13:52, Nico Boehr wrote:
> Quoting Pierre Morel (2023-05-30 14:40:55)
>> A kvm-unit-test would hang if an abort happens before SCLP Read SCP
>> Information has completed if sclp_get_cpu_num() does not report at
>> least one CPU.
>> Since we obviously have one, report it.
> Sorry for complaining again, in a discussion with Janosch we found that the
> description and commit below can be easily misunderstood. I suggest the
> following wording in the commit description:
>
> s390x: sclp: treat system as single processor when read_info is NULL
>
> When a test abort()s before SCLP read info is completed, the assertion on
> read_info in sclp_read_info() will fail. Since abort() eventually calls
> smp_teardown() which in turn calls sclp_get_cpu_num(), this will cause an
> infinite abort() chain, causing the test to hang.
>
> Fix this by considering the system single processor when read_info is missing.
>
> [...]

better, I take it

thx



>> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
>> index 12919ca..34a31da 100644
>> --- a/lib/s390x/sclp.c
>> +++ b/lib/s390x/sclp.c
>> @@ -121,6 +121,12 @@ int sclp_get_cpu_num(void)
>>   {
>>          if (read_info)
>>                  return read_info->entries_cpu;
>> +       /*
>> +        * If we fail here and read_info has not being set,
>> +        * it means we failed early and we try to abort the test.
>> +        * We need to return at least one CPU, and obviously we have
>> +        * at least one, for the smp_teardown to correctly work.
>> +        */
> Please make this:
>
> Don't abort here if read_info is NULL since abort() calls smp_teardown() which
> eventually calls this function and thus causes an infinite abort() chain,
> causing the test to hang. Since we obviously have at least one CPU, just return
> one.
>
> With these changes:
>
> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
>
> Sorry for the back and forth.

[kvm-unit-tests PATCH v3 2/2] s390x: sclp: Implement SCLP_RC_INSUFFICIENT_SCCB_LENGTH

[Pierre Morel]

If SCLP_CMDW_READ_SCP_INFO fails due to a short buffer, retry
with a greater buffer.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/sclp.c | 58 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 8 deletions(-)

diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index 34a31da..9d51ca4 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -17,13 +17,14 @@
 #include "sclp.h"
 #include <alloc_phys.h>
 #include <alloc_page.h>
+#include <asm/facility.h>
 
 extern unsigned long stacktop;
 
 static uint64_t storage_increment_size;
 static uint64_t max_ram_size;
 static uint64_t ram_size;
-char _read_info[PAGE_SIZE] __attribute__((__aligned__(PAGE_SIZE)));
+char _read_info[2 * PAGE_SIZE] __attribute__((__aligned__(PAGE_SIZE)));
 static ReadInfo *read_info;
 struct sclp_facilities sclp_facilities;
 
@@ -89,10 +90,41 @@ void sclp_clear_busy(void)
 	spin_unlock(&sclp_lock);
 }
 
-static void sclp_read_scp_info(ReadInfo *ri, int length)
+static bool sclp_read_scp_info_extended(unsigned int command, ReadInfo *ri)
+{
+	int cc;
+
+	if (!test_facility(140)) {
+		report_abort("S390_FEAT_EXTENDED_LENGTH_SCCB missing");
+		return false;
+	}
+	if (ri->h.length > (2 * PAGE_SIZE)) {
+		report_abort("SCLP_READ_INFO expected size too big");
+		return false;
+	}
+
+	sclp_mark_busy();
+	memset(&ri->h, 0, sizeof(ri->h));
+	ri->h.length = 2 * PAGE_SIZE;
+
+	cc = sclp_service_call(command, ri);
+	if (cc) {
+		report_abort("SCLP_READ_INFO error");
+		return false;
+	}
+	if (ri->h.response_code != SCLP_RC_NORMAL_READ_COMPLETION) {
+		report_abort("SCLP_READ_INFO error %02x", ri->h.response_code);
+		return false;
+	}
+
+	return true;
+}
+
+static void sclp_read_scp_info(ReadInfo *ri)
 {
 	unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED,
 				    SCLP_CMDW_READ_SCP_INFO };
+	int length = PAGE_SIZE;
 	int i, cc;
 
 	for (i = 0; i < ARRAY_SIZE(commands); i++) {
@@ -101,19 +133,29 @@ static void sclp_read_scp_info(ReadInfo *ri, int length)
 		ri->h.length = length;
 
 		cc = sclp_service_call(commands[i], ri);
-		if (cc)
-			break;
-		if (ri->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION)
+		if (cc) {
+			report_abort("SCLP_READ_INFO error");
 			return;
-		if (ri->h.response_code != SCLP_RC_INVALID_SCLP_COMMAND)
+		}
+
+		switch (ri->h.response_code) {
+		case SCLP_RC_NORMAL_READ_COMPLETION:
+			return;
+		case SCLP_RC_INVALID_SCLP_COMMAND:
 			break;
+		case SCLP_RC_INSUFFICIENT_SCCB_LENGTH:
+			sclp_read_scp_info_extended(commands[i], ri);
+			return;
+		default:
+			report_abort("READ_SCP_INFO failed");
+			return;
+		}
 	}
-	report_abort("READ_SCP_INFO failed");
 }
 
 void sclp_read_info(void)
 {
-	sclp_read_scp_info((void *)_read_info, SCCB_SIZE);
+	sclp_read_scp_info((void *)_read_info);
 	read_info = (ReadInfo *)_read_info;
 }
 
-- 
2.31.1

Re: [kvm-unit-tests PATCH v3 2/2] s390x: sclp: Implement SCLP_RC_INSUFFICIENT_SCCB_LENGTH

[Janosch Frank]

On 5/30/23 14:40, Pierre Morel wrote:
> If SCLP_CMDW_READ_SCP_INFO fails due to a short buffer, retry
> with a greater buffer.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

You've been testing using all possible cpus, haven't you?

>   }
>   
> -static void sclp_read_scp_info(ReadInfo *ri, int length)
> +static bool sclp_read_scp_info_extended(unsigned int command, ReadInfo *ri)
> +{
> +	int cc;
> +
> +	if (!test_facility(140)) {
> +		report_abort("S390_FEAT_EXTENDED_LENGTH_SCCB missing");

That's the QEMU name for the facility, isn't it?
"extended-length-SCCB facility is missing" might be better since that's 
the name that the architecture specifies for that feature.

> +		return false;
> +	}
> +	if (ri->h.length > (2 * PAGE_SIZE)) {

sizeof() would reduce the locations that we have to touch if we ever 
want to increase the buffer in the future.

> +		report_abort("SCLP_READ_INFO expected size too big");
> +		return false;
> +	}
> +
> +	sclp_mark_busy();
> +	memset(&ri->h, 0, sizeof(ri->h));
> +	ri->h.length = 2 * PAGE_SIZE;

Same here

> +
> +	cc = sclp_service_call(command, ri);
> +	if (cc) {
> +		report_abort("SCLP_READ_INFO error");
> +		return false;
> +	}
> +	if (ri->h.response_code != SCLP_RC_NORMAL_READ_COMPLETION) {
> +		report_abort("SCLP_READ_INFO error %02x", ri->h.response_code);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static void sclp_read_scp_info(ReadInfo *ri)
>   {
>   	unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED,
>   				    SCLP_CMDW_READ_SCP_INFO };
> +	int length = PAGE_SIZE;
>   	int i, cc;
>   
>   	for (i = 0; i < ARRAY_SIZE(commands); i++) {
> @@ -101,19 +133,29 @@ static void sclp_read_scp_info(ReadInfo *ri, int length)
>   		ri->h.length = length;
>   
>   		cc = sclp_service_call(commands[i], ri);
> -		if (cc)
> -			break;
> -		if (ri->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION)
> +		if (cc) {
> +			report_abort("SCLP_READ_INFO error");
>   			return;
> -		if (ri->h.response_code != SCLP_RC_INVALID_SCLP_COMMAND)
> +		}
> +
> +		switch (ri->h.response_code) {
> +		case SCLP_RC_NORMAL_READ_COMPLETION:
> +			return;
> +		case SCLP_RC_INVALID_SCLP_COMMAND:
>   			break;
> +		case SCLP_RC_INSUFFICIENT_SCCB_LENGTH:
> +			sclp_read_scp_info_extended(commands[i], ri);
> +			return;
> +		default:
> +			report_abort("READ_SCP_INFO failed");
> +			return;
> +		}
>   	}
> -	report_abort("READ_SCP_INFO failed");
>   }
>   
>   void sclp_read_info(void)
>   {
> -	sclp_read_scp_info((void *)_read_info, SCCB_SIZE);

Why did you remove that?
You could have re-tried with the extended-length in 
sclp_read_scp_info(). Or you could return the rc and introduce a tiny 
function that tries both lengths depending on the rc.

> +	sclp_read_scp_info((void *)_read_info);
>   	read_info = (ReadInfo *)_read_info;
>   }
>   

Re: [kvm-unit-tests PATCH v3 2/2] s390x: sclp: Implement SCLP_RC_INSUFFICIENT_SCCB_LENGTH

[Pierre Morel]

On 6/1/23 10:03, Janosch Frank wrote:
> On 5/30/23 14:40, Pierre Morel wrote:
>> If SCLP_CMDW_READ_SCP_INFO fails due to a short buffer, retry
>> with a greater buffer.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>
> You've been testing using all possible cpus, haven't you?


yes up to 248


>
>>   }
>>   -static void sclp_read_scp_info(ReadInfo *ri, int length)
>> +static bool sclp_read_scp_info_extended(unsigned int command, 
>> ReadInfo *ri)
>> +{
>> +    int cc;
>> +
>> +    if (!test_facility(140)) {
>> +        report_abort("S390_FEAT_EXTENDED_LENGTH_SCCB missing");
>
> That's the QEMU name for the facility, isn't it?
> "extended-length-SCCB facility is missing" might be better since 
> that's the name that the architecture specifies for that feature.


yes


>
>> +        return false;
>> +    }
>> +    if (ri->h.length > (2 * PAGE_SIZE)) {
>
> sizeof() would reduce the locations that we have to touch if we ever 
> want to increase the buffer in the future.


yes


>
>> +        report_abort("SCLP_READ_INFO expected size too big");
>> +        return false;
>> +    }
>> +
>> +    sclp_mark_busy();
>> +    memset(&ri->h, 0, sizeof(ri->h));
>> +    ri->h.length = 2 * PAGE_SIZE;
>
> Same here


OK


>
>> +
>> +    cc = sclp_service_call(command, ri);
>> +    if (cc) {
>> +        report_abort("SCLP_READ_INFO error");
>> +        return false;
>> +    }
>> +    if (ri->h.response_code != SCLP_RC_NORMAL_READ_COMPLETION) {
>> +        report_abort("SCLP_READ_INFO error %02x", ri->h.response_code);
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +static void sclp_read_scp_info(ReadInfo *ri)
>>   {
>>       unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED,
>>                       SCLP_CMDW_READ_SCP_INFO };
>> +    int length = PAGE_SIZE;
>>       int i, cc;
>>         for (i = 0; i < ARRAY_SIZE(commands); i++) {
>> @@ -101,19 +133,29 @@ static void sclp_read_scp_info(ReadInfo *ri, 
>> int length)
>>           ri->h.length = length;
>>             cc = sclp_service_call(commands[i], ri);
>> -        if (cc)
>> -            break;
>> -        if (ri->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION)
>> +        if (cc) {
>> +            report_abort("SCLP_READ_INFO error");
>>               return;
>> -        if (ri->h.response_code != SCLP_RC_INVALID_SCLP_COMMAND)
>> +        }
>> +
>> +        switch (ri->h.response_code) {
>> +        case SCLP_RC_NORMAL_READ_COMPLETION:
>> +            return;
>> +        case SCLP_RC_INVALID_SCLP_COMMAND:
>>               break;
>> +        case SCLP_RC_INSUFFICIENT_SCCB_LENGTH:
>> +            sclp_read_scp_info_extended(commands[i], ri);
>> +            return;
>> +        default:
>> +            report_abort("READ_SCP_INFO failed");
>> +            return;
>> +        }
>>       }
>> -    report_abort("READ_SCP_INFO failed");
>>   }
>>     void sclp_read_info(void)
>>   {
>> -    sclp_read_scp_info((void *)_read_info, SCCB_SIZE);
>
> Why did you remove that?
> You could have re-tried with the extended-length in 
> sclp_read_scp_info(). Or you could return the rc and introduce a tiny 
> function that tries both lengths depending on the rc.


Yes, I can let it here. I found it has little sense to give the length 
as parameter.

Retrying with extended length in sclp_read_scp_info() is what is done 
isn'it?

It does not change a lot to let the first used size here so I will let 
it here.


>
>> +    sclp_read_scp_info((void *)_read_info);
>>       read_info = (ReadInfo *)_read_info;
>>   }
>

Re: [kvm-unit-tests PATCH v3 2/2] s390x: sclp: Implement SCLP_RC_INSUFFICIENT_SCCB_LENGTH

[Nico Boehr]

Quoting Janosch Frank (2023-06-01 10:03:06)
> On 5/30/23 14:40, Pierre Morel wrote:
> > If SCLP_CMDW_READ_SCP_INFO fails due to a short buffer, retry
> > with a greater buffer.
> > 
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

Janosch, I think it makes sense if Pierre picks up Claudios suggestion from here:
https://lore.kernel.org/all/20230530173544.378a63c6@p-imbrenda/

Do you agree?

Re: [kvm-unit-tests PATCH v3 2/2] s390x: sclp: Implement SCLP_RC_INSUFFICIENT_SCCB_LENGTH

[Pierre Morel]

On 6/1/23 13:59, Nico Boehr wrote:
> Quoting Janosch Frank (2023-06-01 10:03:06)
>> On 5/30/23 14:40, Pierre Morel wrote:
>>> If SCLP_CMDW_READ_SCP_INFO fails due to a short buffer, retry
>>> with a greater buffer.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Janosch, I think it makes sense if Pierre picks up Claudios suggestion from here:
> https://lore.kernel.org/all/20230530173544.378a63c6@p-imbrenda/
>
> Do you agree?

from my side:

It simplifies greatly the code and tested without problem.

The documentation says the SCCB length is "at least"... so we can use a 
greater size from the beginning.

Re: [kvm-unit-tests PATCH v3 2/2] s390x: sclp: Implement SCLP_RC_INSUFFICIENT_SCCB_LENGTH

[Janosch Frank]

On 6/1/23 14:55, Pierre Morel wrote:
> 
> On 6/1/23 13:59, Nico Boehr wrote:
>> Quoting Janosch Frank (2023-06-01 10:03:06)
>>> On 5/30/23 14:40, Pierre Morel wrote:
>>>> If SCLP_CMDW_READ_SCP_INFO fails due to a short buffer, retry
>>>> with a greater buffer.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Janosch, I think it makes sense if Pierre picks up Claudios suggestion from here:
>> https://lore.kernel.org/all/20230530173544.378a63c6@p-imbrenda/
>>
>> Do you agree?
> 
> from my side:
> 
> It simplifies greatly the code and tested without problem.
> 
> The documentation says the SCCB length is "at least"... so we can use a
> greater size from the beginning.
> 
> 

Sounds good