[kvm-unit-tests 0/2] s390x/smp fix and enhancement

[kvm-unit-tests 0/2] s390x/smp fix and enhancement

[Christian Borntraeger]

Christian Borntraeger (2):
  s390x/smp: fix detection of "running"
  s390x/smp: add minimal test for sigp sense running status

 lib/s390x/smp.c |  4 ++--
 lib/s390x/smp.h |  2 +-
 s390x/smp.c     | 11 +++++++++++
 3 files changed, 14 insertions(+), 3 deletions(-)

-- 
2.25.1

[kvm-unit-tests 1/2] s390x/smp: fix detection of "running"

[Christian Borntraeger]

On s390x hosts with a single CPU, the smp test case hangs (loops).
The check is our restart has finished is wrong.
Sigp sense running status checks if the CPU is currently backed by a
real CPU. This means that on single CPU hosts a sigp sense running
will never claim that a target is running. We need to check for not
being stopped instead.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 lib/s390x/smp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 2555bf4..5ed8b7b 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -128,7 +128,7 @@ static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw)
 	 * The order has been accepted, but the actual restart may not
 	 * have been performed yet, so wait until the cpu is running.
 	 */
-	while (!smp_cpu_running(addr))
+	while (smp_cpu_stopped(addr))
 		mb();
 	cpu->active = true;
 	return 0;
-- 
2.25.1

Re: [kvm-unit-tests 1/2] s390x/smp: fix detection of "running"

[David Hildenbrand]

On 30.03.20 10:49, Christian Borntraeger wrote:
> On s390x hosts with a single CPU, the smp test case hangs (loops).
> The check is our restart has finished is wrong.
> Sigp sense running status checks if the CPU is currently backed by a
> real CPU. This means that on single CPU hosts a sigp sense running
> will never claim that a target is running. We need to check for not
> being stopped instead.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  lib/s390x/smp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index 2555bf4..5ed8b7b 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -128,7 +128,7 @@ static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw)
>  	 * The order has been accepted, but the actual restart may not
>  	 * have been performed yet, so wait until the cpu is running.
>  	 */
> -	while (!smp_cpu_running(addr))
> +	while (smp_cpu_stopped(addr))
>  		mb();
>  	cpu->active = true;
>  	return 0;
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests 1/2] s390x/smp: fix detection of "running"

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1126 bytes --]

On 3/30/20 10:49 AM, Christian Borntraeger wrote:
> On s390x hosts with a single CPU, the smp test case hangs (loops).
> The check is our restart has finished is wrong.
> Sigp sense running status checks if the CPU is currently backed by a
> real CPU. This means that on single CPU hosts a sigp sense running
> will never claim that a target is running. We need to check for not
> being stopped instead.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>

> ---
>  lib/s390x/smp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index 2555bf4..5ed8b7b 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -128,7 +128,7 @@ static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw)
>  	 * The order has been accepted, but the actual restart may not
>  	 * have been performed yet, so wait until the cpu is running.
>  	 */
> -	while (!smp_cpu_running(addr))
> +	while (smp_cpu_stopped(addr))
>  		mb();
>  	cpu->active = true;
>  	return 0;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests 1/2] s390x/smp: fix detection of "running"

[Cornelia Huck]

On Mon, 30 Mar 2020 04:49:10 -0400
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On s390x hosts with a single CPU, the smp test case hangs (loops).
> The check is our restart has finished is wrong.

s/is/if/

> Sigp sense running status checks if the CPU is currently backed by a
> real CPU. This means that on single CPU hosts a sigp sense running
> will never claim that a target is running. We need to check for not
> being stopped instead.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  lib/s390x/smp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index 2555bf4..5ed8b7b 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -128,7 +128,7 @@ static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw)
>  	 * The order has been accepted, but the actual restart may not
>  	 * have been performed yet, so wait until the cpu is running.
>  	 */
> -	while (!smp_cpu_running(addr))
> +	while (smp_cpu_stopped(addr))
>  		mb();
>  	cpu->active = true;
>  	return 0;

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [kvm-unit-tests 1/2] s390x/smp: fix detection of "running"

[David Hildenbrand]

On 31.03.20 08:49, Cornelia Huck wrote:
> On Mon, 30 Mar 2020 04:49:10 -0400
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On s390x hosts with a single CPU, the smp test case hangs (loops).
>> The check is our restart has finished is wrong.
> 
> s/is/if/

With that fixed up queued to

https://github.com/davidhildenbrand/kvm-unit-tests.git s390x-next

I'll wait with the other patch until we have a consent there regarding
possible races.

-- 
Thanks,

David / dhildenb

[kvm-unit-tests 2/2] s390x/smp: add minimal test for sigp sense running status

[Christian Borntraeger]

make sure that sigp sense running status returns a sane value for
stopped CPUs.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 lib/s390x/smp.c |  2 +-
 lib/s390x/smp.h |  2 +-
 s390x/smp.c     | 11 +++++++++++
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 5ed8b7b..492cb05 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -58,7 +58,7 @@ bool smp_cpu_stopped(uint16_t addr)
 	return !!(status & (SIGP_STATUS_CHECK_STOP|SIGP_STATUS_STOPPED));
 }
 
-bool smp_cpu_running(uint16_t addr)
+bool smp_sense_running_status(uint16_t addr)
 {
 	if (sigp(addr, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED)
 		return true;
diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
index a8b98c0..639ec92 100644
--- a/lib/s390x/smp.h
+++ b/lib/s390x/smp.h
@@ -40,7 +40,7 @@ struct cpu_status {
 int smp_query_num_cpus(void);
 struct cpu *smp_cpu_from_addr(uint16_t addr);
 bool smp_cpu_stopped(uint16_t addr);
-bool smp_cpu_running(uint16_t addr);
+bool smp_sense_running_status(uint16_t addr);
 int smp_cpu_restart(uint16_t addr);
 int smp_cpu_start(uint16_t addr, struct psw psw);
 int smp_cpu_stop(uint16_t addr);
diff --git a/s390x/smp.c b/s390x/smp.c
index 79cdc1f..f9f143d 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -210,6 +210,16 @@ static void test_emcall(void)
 	report_prefix_pop();
 }
 
+static void test_sense_running(void)
+{
+	report_prefix_push("sense_running");
+	/* make sure CPU is stopped */
+	smp_cpu_stop(1);
+	report(!smp_sense_running_status(1), "CPU1 sense claims not running");
+	report_prefix_pop();
+}
+
+
 /* Used to dirty registers of cpu #1 before it is reset */
 static void test_func_initial(void)
 {
@@ -319,6 +329,7 @@ int main(void)
 	test_store_status();
 	test_ecall();
 	test_emcall();
+	test_sense_running();
 	test_reset();
 	test_reset_initial();
 	smp_cpu_destroy(1);
-- 
2.25.1

Re: [kvm-unit-tests 2/2] s390x/smp: add minimal test for sigp sense running status

[David Hildenbrand]

On 30.03.20 10:49, Christian Borntraeger wrote:
> make sure that sigp sense running status returns a sane value for
> stopped CPUs.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  lib/s390x/smp.c |  2 +-
>  lib/s390x/smp.h |  2 +-
>  s390x/smp.c     | 11 +++++++++++
>  3 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index 5ed8b7b..492cb05 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -58,7 +58,7 @@ bool smp_cpu_stopped(uint16_t addr)
>  	return !!(status & (SIGP_STATUS_CHECK_STOP|SIGP_STATUS_STOPPED));
>  }
>  
> -bool smp_cpu_running(uint16_t addr)
> +bool smp_sense_running_status(uint16_t addr)
>  {
>  	if (sigp(addr, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED)
>  		return true;
> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
> index a8b98c0..639ec92 100644
> --- a/lib/s390x/smp.h
> +++ b/lib/s390x/smp.h
> @@ -40,7 +40,7 @@ struct cpu_status {
>  int smp_query_num_cpus(void);
>  struct cpu *smp_cpu_from_addr(uint16_t addr);
>  bool smp_cpu_stopped(uint16_t addr);
> -bool smp_cpu_running(uint16_t addr);
> +bool smp_sense_running_status(uint16_t addr);
>  int smp_cpu_restart(uint16_t addr);
>  int smp_cpu_start(uint16_t addr, struct psw psw);
>  int smp_cpu_stop(uint16_t addr);
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 79cdc1f..f9f143d 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -210,6 +210,16 @@ static void test_emcall(void)
>  	report_prefix_pop();
>  }
>  
> +static void test_sense_running(void)
> +{
> +	report_prefix_push("sense_running");
> +	/* make sure CPU is stopped */
> +	smp_cpu_stop(1);
> +	report(!smp_sense_running_status(1), "CPU1 sense claims not running");
> +	report_prefix_pop();
> +}
> +
> +
>  /* Used to dirty registers of cpu #1 before it is reset */
>  static void test_func_initial(void)
>  {
> @@ -319,6 +329,7 @@ int main(void)
>  	test_store_status();
>  	test_ecall();
>  	test_emcall();
> +	test_sense_running();
>  	test_reset();
>  	test_reset_initial();
>  	smp_cpu_destroy(1);
> 

In kvm, we set/clear via kvm_arch_vcpu_load/kvm_arch_vcpu_put. This
means, that a VCPU will also be indicated as running, in case we perform
certain VCPU IOCTLs, while the VCPU is already stopped.

Especially, there is a theoretical race between stopping a VCPU, and it
still being in the kernel, and the other thread sensing the running
status. This is the case with !kvm_s390_user_cpu_state_ctrl(), when
leaving handle_stop() but also with kvm_s390_user_cpu_state_ctrl(), when
setting the MP state via kvm_arch_vcpu_ioctl_set_mpstate().

-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests 2/2] s390x/smp: add minimal test for sigp sense running status

[Christian Borntraeger]

On 30.03.20 12:58, David Hildenbrand wrote:
> On 30.03.20 10:49, Christian Borntraeger wrote:
>> make sure that sigp sense running status returns a sane value for
>> stopped CPUs.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  lib/s390x/smp.c |  2 +-
>>  lib/s390x/smp.h |  2 +-
>>  s390x/smp.c     | 11 +++++++++++
>>  3 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
>> index 5ed8b7b..492cb05 100644
>> --- a/lib/s390x/smp.c
>> +++ b/lib/s390x/smp.c
>> @@ -58,7 +58,7 @@ bool smp_cpu_stopped(uint16_t addr)
>>  	return !!(status & (SIGP_STATUS_CHECK_STOP|SIGP_STATUS_STOPPED));
>>  }
>>  
>> -bool smp_cpu_running(uint16_t addr)
>> +bool smp_sense_running_status(uint16_t addr)
>>  {
>>  	if (sigp(addr, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED)
>>  		return true;
>> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
>> index a8b98c0..639ec92 100644
>> --- a/lib/s390x/smp.h
>> +++ b/lib/s390x/smp.h
>> @@ -40,7 +40,7 @@ struct cpu_status {
>>  int smp_query_num_cpus(void);
>>  struct cpu *smp_cpu_from_addr(uint16_t addr);
>>  bool smp_cpu_stopped(uint16_t addr);
>> -bool smp_cpu_running(uint16_t addr);
>> +bool smp_sense_running_status(uint16_t addr);
>>  int smp_cpu_restart(uint16_t addr);
>>  int smp_cpu_start(uint16_t addr, struct psw psw);
>>  int smp_cpu_stop(uint16_t addr);
>> diff --git a/s390x/smp.c b/s390x/smp.c
>> index 79cdc1f..f9f143d 100644
>> --- a/s390x/smp.c
>> +++ b/s390x/smp.c
>> @@ -210,6 +210,16 @@ static void test_emcall(void)
>>  	report_prefix_pop();
>>  }
>>  
>> +static void test_sense_running(void)
>> +{
>> +	report_prefix_push("sense_running");
>> +	/* make sure CPU is stopped */
>> +	smp_cpu_stop(1);
>> +	report(!smp_sense_running_status(1), "CPU1 sense claims not running");
>> +	report_prefix_pop();
>> +}
>> +
>> +
>>  /* Used to dirty registers of cpu #1 before it is reset */
>>  static void test_func_initial(void)
>>  {
>> @@ -319,6 +329,7 @@ int main(void)
>>  	test_store_status();
>>  	test_ecall();
>>  	test_emcall();
>> +	test_sense_running();
>>  	test_reset();
>>  	test_reset_initial();
>>  	smp_cpu_destroy(1);
>>
> 
> In kvm, we set/clear via kvm_arch_vcpu_load/kvm_arch_vcpu_put. This
> means, that a VCPU will also be indicated as running, in case we perform
> certain VCPU IOCTLs, while the VCPU is already stopped.
> 
> Especially, there is a theoretical race between stopping a VCPU, and it
> still being in the kernel, and the other thread sensing the running
> status. This is the case with !kvm_s390_user_cpu_state_ctrl(), when
> leaving handle_stop() but also with kvm_s390_user_cpu_state_ctrl(), when
> setting the MP state via kvm_arch_vcpu_ioctl_set_mpstate().

I think I will just busy wait until the target is not running. This must happen and a timeout indicates a failure.