Re: [PATCH] s390x: sigp: Fix sense running reporting

[David Hildenbrand]

On 24.01.20 11:05, Cornelia Huck wrote:
> On Fri, 24 Jan 2020 05:01:37 -0500
> Janosch Frank <address@hidden> wrote:
> 
>> The logic was inversed and reported running if the cpu was stopped.
> 
> s/inversed/inverted/ ?
> 
>> Let's fix that.
>>
> 
> Fixes: d1b468bc8869 ("s390x/tcg: implement SIGP SENSE RUNNING STATUS")
> 
>> Signed-off-by: Janosch Frank <address@hidden>
>> ---
>>  target/s390x/sigp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
>> index 727875bb4a..286c0d6c9c 100644
>> --- a/target/s390x/sigp.c
>> +++ b/target/s390x/sigp.c
>> @@ -347,7 +347,7 @@ static void sigp_sense_running(S390CPU *dst_cpu, 
>> SigpInfo *si)
>>      }
>>  
>>      /* If halted (which includes also STOPPED), it is not running */
>> -    if (CPU(dst_cpu)->halted) {
>> +    if (!CPU(dst_cpu)->halted) {
>>          si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>>      } else {
>>          set_sigp_status(si, SIGP_STAT_NOT_RUNNING);
> 
> I'm wondering why nobody noticed this before...

AFAIR, it "SENSE RUNNING" allows you to test if the target CPU is
scheduled by the hypervisor. So it is used for performance optimization,
but correctness of a program barely depends on it.

a) You can always return "not running" and it would be totally fine

b) Return "running" would also most probably always valid (although it
does not make too much sense for STOPPED CPUs).

E.g., in KVM we set CPUSTAT_RUNNING whenever we load the CPU. This can
also happen (AFAIR) when the CPU state is already stopped (e.g.,
currently getting stopped) or still sleeping (e.g., about to wake up, or
in kvm_vcpu_block()).


Long story short: There is no trusting on these values.


But yeah, the heuristic we are using is sub-optimal. :)

Reviewed-by: David Hildenbrand <address@hidden>

-- 
Thanks,

David / dhildenb