[v1] s390x: Protected Virtualization support

[PATCH 15/15] s390x: protvirt: Handle SIGP store status correctly

Posted by Janosch Frank 6 years, 2 months ago

Status storing is obviously not done by qemu anymore.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/sigp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index 2ce22d4dc1..68634d694a 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -144,7 +144,9 @@ static void sigp_stop_and_store_status(CPUState *cs, run_on_cpu_data arg)
     case S390_CPU_STATE_STOPPED:
         /* already stopped, just store the status */
         cpu_synchronize_state(cs);
-        s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
+        if (!cpu->env.pv) {
+            s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
+        }
         break;
     }
     si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
@@ -497,7 +499,8 @@ void do_stop_interrupt(CPUS390XState *env)
     if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
         qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
     }
-    if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
+    /* Storing will occur on next SIE entry for fmt 4 */
+    if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS && !env->pv) {
         s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
     }
     env->sigp_order = 0;
-- 
2.20.1

Re: [PATCH 15/15] s390x: protvirt: Handle SIGP store status correctly

Posted by David Hildenbrand 6 years, 2 months ago

On 20.11.19 12:43, Janosch Frank wrote:
> Status storing is obviously not done by qemu anymore.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   target/s390x/sigp.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
> index 2ce22d4dc1..68634d694a 100644
> --- a/target/s390x/sigp.c
> +++ b/target/s390x/sigp.c
> @@ -144,7 +144,9 @@ static void sigp_stop_and_store_status(CPUState *cs, run_on_cpu_data arg)
>       case S390_CPU_STATE_STOPPED:
>           /* already stopped, just store the status */
>           cpu_synchronize_state(cs);
> -        s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
> +        if (!cpu->env.pv) {
> +            s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
> +        }

Confused, how is that case handled? The CPU is already stopped, so it 
won't be run again (consequently, next SIE entry can't store it). Who 
will store the status?

>           break;
>       }
>       si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
> @@ -497,7 +499,8 @@ void do_stop_interrupt(CPUS390XState *env)
>       if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
>           qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>       }
> -    if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
> +    /* Storing will occur on next SIE entry for fmt 4 */
> +    if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS && !env->pv) {
>           s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
>       }
>       env->sigp_order = 0;
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH 15/15] s390x: protvirt: Handle SIGP store status correctly

Posted by Janosch Frank 6 years, 2 months ago

On 11/21/19 12:24 PM, David Hildenbrand wrote:
> On 20.11.19 12:43, Janosch Frank wrote:
>> Status storing is obviously not done by qemu anymore.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   target/s390x/sigp.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
>> index 2ce22d4dc1..68634d694a 100644
>> --- a/target/s390x/sigp.c
>> +++ b/target/s390x/sigp.c
>> @@ -144,7 +144,9 @@ static void sigp_stop_and_store_status(CPUState *cs, run_on_cpu_data arg)
>>       case S390_CPU_STATE_STOPPED:
>>           /* already stopped, just store the status */
>>           cpu_synchronize_state(cs);
>> -        s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
>> +        if (!cpu->env.pv) {
>> +            s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
>> +        }
> 
> Confused, how is that case handled? The CPU is already stopped, so it 
> won't be run again (consequently, next SIE entry can't store it). Who 
> will store the status?

Firmware does some magic

> 
>>           break;
>>       }
>>       si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>> @@ -497,7 +499,8 @@ void do_stop_interrupt(CPUS390XState *env)
>>       if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
>>           qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>>       }
>> -    if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
>> +    /* Storing will occur on next SIE entry for fmt 4 */
>> +    if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS && !env->pv) {
>>           s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
>>       }
>>       env->sigp_order = 0;
>>
> 
>

Re: [PATCH 15/15] s390x: protvirt: Handle SIGP store status correctly

Posted by Thomas Huth 6 years, 2 months ago

On 20/11/2019 12.43, Janosch Frank wrote:
> Status storing is obviously not done by qemu anymore.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/sigp.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
> index 2ce22d4dc1..68634d694a 100644
> --- a/target/s390x/sigp.c
> +++ b/target/s390x/sigp.c
> @@ -144,7 +144,9 @@ static void sigp_stop_and_store_status(CPUState *cs, run_on_cpu_data arg)
>      case S390_CPU_STATE_STOPPED:
>          /* already stopped, just store the status */
>          cpu_synchronize_state(cs);
> -        s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
> +        if (!cpu->env.pv) {
> +            s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
> +        }
>          break;
>      }
>      si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
> @@ -497,7 +499,8 @@ void do_stop_interrupt(CPUS390XState *env)
>      if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
>          qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>      }
> -    if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
> +    /* Storing will occur on next SIE entry for fmt 4 */
> +    if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS && !env->pv) {
>          s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
>      }
>      env->sigp_order = 0;
> 

Maybe move the check into the s390_store_status() function instead?

Anyway,
Reviewed-by: Thomas Huth <thuth@redhat.com>