[RFC PATCH v5 0/1] s390x: Improvements to SIGP handling [QEMU]

[RFC PATCH v5 0/1] s390x: Improvements to SIGP handling [QEMU]

[Eric Farman]

Here is an update to the SIGP handling series, to correspond to
version 5 of the KVM series [1].

Unlike earlier versions, there is no new interface to exploit.
So this simply rearranges processing to mirror expectations
on the kernel side.

[1] https://lore.kernel.org/r/20211213210550.856213-1-farman@linux.ibm.com/

Previous RFCs:
v1: https://lore.kernel.org/r/20211008203811.1980478-1-farman@linux.ibm.com/
v2: https://lore.kernel.org/r/20211102201122.3188108-1-farman@linux.ibm.com/
v3: https://lore.kernel.org/r/20211110204528.1378524-1-farman@linux.ibm.com/
v4: https://lore.kernel.org/r/20211119214327.2367026-1-farman@linux.ibm.com/

Eric Farman (1):
  s390x: sigp: Reorder the SIGP STOP code

 target/s390x/sigp.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

-- 
2.32.0

[RFC PATCH v5 1/1] s390x: sigp: Reorder the SIGP STOP code

[Eric Farman]

Let's wait to mark the VCPU STOPPED until the possible
STORE STATUS operation is completed, so that we know the
CPU is fully stopped and done doing anything. (When we
also clear the possible sigp_order field for STOP orders.)

Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 target/s390x/sigp.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index 51c727834c..9dd977349a 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -139,7 +139,7 @@ static void sigp_stop_and_store_status(CPUState *cs, run_on_cpu_data arg)
     case S390_CPU_STATE_OPERATING:
         cpu->env.sigp_order = SIGP_STOP_STORE_STATUS;
         cpu_inject_stop(cpu);
-        /* store will be performed in do_stop_interrup() */
+        /* store will be performed in do_stop_interrupt() */
         break;
     case S390_CPU_STATE_STOPPED:
         /* already stopped, just store the status */
@@ -479,13 +479,17 @@ void do_stop_interrupt(CPUS390XState *env)
 {
     S390CPU *cpu = env_archcpu(env);
 
-    if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
-        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
-    }
+    /*
+     * Complete the STOP operation before exposing the CPU as
+     * STOPPED to the system.
+     */
     if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
         s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
     }
     env->sigp_order = 0;
+    if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
+        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+    }
     env->pending_int &= ~INTERRUPT_STOP;
 }
 
-- 
2.32.0

Re: [RFC PATCH v5 1/1] s390x: sigp: Reorder the SIGP STOP code

[David Hildenbrand]

On 13.12.21 22:09, Eric Farman wrote:
> Let's wait to mark the VCPU STOPPED until the possible
> STORE STATUS operation is completed, so that we know the
> CPU is fully stopped and done doing anything. (When we
> also clear the possible sigp_order field for STOP orders.)
> 
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  target/s390x/sigp.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
> index 51c727834c..9dd977349a 100644
> --- a/target/s390x/sigp.c
> +++ b/target/s390x/sigp.c
> @@ -139,7 +139,7 @@ static void sigp_stop_and_store_status(CPUState *cs, run_on_cpu_data arg)
>      case S390_CPU_STATE_OPERATING:
>          cpu->env.sigp_order = SIGP_STOP_STORE_STATUS;
>          cpu_inject_stop(cpu);
> -        /* store will be performed in do_stop_interrup() */
> +        /* store will be performed in do_stop_interrupt() */
>          break;
>      case S390_CPU_STATE_STOPPED:
>          /* already stopped, just store the status */
> @@ -479,13 +479,17 @@ void do_stop_interrupt(CPUS390XState *env)
>  {
>      S390CPU *cpu = env_archcpu(env);
>  
> -    if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
> -        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> -    }
> +    /*
> +     * Complete the STOP operation before exposing the CPU as
> +     * STOPPED to the system.
> +     */
>      if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
>          s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
>      }
>      env->sigp_order = 0;
> +    if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
> +        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +    }
>      env->pending_int &= ~INTERRUPT_STOP;
>  }
>  

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

-- 
Thanks,

David / dhildenb

Re: [RFC PATCH v5 1/1] s390x: sigp: Reorder the SIGP STOP code

[Thomas Huth]

On 18/01/2022 14.34, David Hildenbrand wrote:
> On 13.12.21 22:09, Eric Farman wrote:
>> Let's wait to mark the VCPU STOPPED until the possible
>> STORE STATUS operation is completed, so that we know the
>> CPU is fully stopped and done doing anything. (When we
>> also clear the possible sigp_order field for STOP orders.)
>>
>> Suggested-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>   target/s390x/sigp.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
>> index 51c727834c..9dd977349a 100644
>> --- a/target/s390x/sigp.c
>> +++ b/target/s390x/sigp.c
>> @@ -139,7 +139,7 @@ static void sigp_stop_and_store_status(CPUState *cs, run_on_cpu_data arg)
>>       case S390_CPU_STATE_OPERATING:
>>           cpu->env.sigp_order = SIGP_STOP_STORE_STATUS;
>>           cpu_inject_stop(cpu);
>> -        /* store will be performed in do_stop_interrup() */
>> +        /* store will be performed in do_stop_interrupt() */
>>           break;
>>       case S390_CPU_STATE_STOPPED:
>>           /* already stopped, just store the status */
>> @@ -479,13 +479,17 @@ void do_stop_interrupt(CPUS390XState *env)
>>   {
>>       S390CPU *cpu = env_archcpu(env);
>>   
>> -    if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
>> -        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>> -    }
>> +    /*
>> +     * Complete the STOP operation before exposing the CPU as
>> +     * STOPPED to the system.
>> +     */
>>       if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
>>           s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
>>       }
>>       env->sigp_order = 0;
>> +    if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
>> +        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>> +    }
>>       env->pending_int &= ~INTERRUPT_STOP;
>>   }
>>   
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 

Thanks, queued to my s390x-next branch now:

  https://gitlab.com/thuth/qemu/-/commits/s390x-next/

  Thomas