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

[kvm-unit-tests PATCH v4 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 | 69 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 59 insertions(+), 10 deletions(-)

-- 
2.31.1

since v3:

- added initial patch and merge with comments
  Sorry for the noise.

since v2:

- use tabs in first patch
  (Nico)

- Added comments

- Added SCLP_RC_INSUFFICIENT_SCCB_LENGTH handling

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

[Pierre Morel]

A test would hang if an abort happens before SCLP Read SCP
Information has completed.

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

diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index acdc8a9..34a31da 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -119,8 +119,15 @@ void sclp_read_info(void)
 
 int sclp_get_cpu_num(void)
 {
-	assert(read_info);
-	return read_info->entries_cpu;
+	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;
 }
 
 CPUEntry *sclp_get_cpu_entries(void)
-- 
2.31.1

[kvm-unit-tests PATCH v4 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 v4 2/2] s390x: sclp: Implement SCLP_RC_INSUFFICIENT_SCCB_LENGTH

[Claudio Imbrenda]

On Tue, 30 May 2023 14:52:43 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

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

the idea is good, but I wonder if the code can be simplified (see below)

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

this is ok ^

[skip everything else]

>  void sclp_read_info(void)
>  {
> -	sclp_read_scp_info((void *)_read_info, SCCB_SIZE);

	sclp_read_scp_info((void *)_read_info,
		test_facility(140) ? sizeof(_read_info) : SCCB_SIZE;

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

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

[Nico Boehr]

Quoting Pierre Morel (2023-05-30 14:52:42)
> A test would hang if an abort happens before SCLP Read SCP
> Information has completed.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

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

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

[Pierre Morel]

On 5/30/23 17:35, Claudio Imbrenda wrote:
> On Tue, 30 May 2023 14:52:43 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>> If SCLP_CMDW_READ_SCP_INFO fails due to a short buffer, retry
>> with a greater buffer.
> the idea is good, but I wonder if the code can be simplified (see below)
>
>> 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)));
> this is ok ^
>
> [skip everything else]
>
>>   void sclp_read_info(void)
>>   {
>> -	sclp_read_scp_info((void *)_read_info, SCCB_SIZE);
> 	sclp_read_scp_info((void *)_read_info,
> 		test_facility(140) ? sizeof(_read_info) : SCCB_SIZE;
>
>> +	sclp_read_scp_info((void *)_read_info);
>>   	read_info = (ReadInfo *)_read_info;
>>   }
>>   

You are right, no need to begin with a short buffer if we can go with a 
big one at first try.

I take it

thx