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

[Janosch Frank <frankja@linux.ibm.com>]

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

On 1/24/20 1:03 PM, David Hildenbrand wrote:
> On 24.01.20 11:05, Cornelia Huck wrote:
>> On Fri, 24 Jan 2020 05:01:37 -0500
>> Janosch Frank <frankja@linux.ibm.com> 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 <frankja@linux.ibm.com>
>>> ---
>>>  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.

That answer makes me highly uncomfortable...

> 
> 
> But yeah, the heuristic we are using is sub-optimal. :)
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!



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