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 <fran...@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 <fran...@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 <da...@redhat.com> Thanks!
signature.asc
Description: OpenPGP digital signature
