From mboxrd@z Thu Jan  1 00:00:00 1970
From: Anthony Liguori <anthony@codemonkey.ws>
Subject: Re: [PATCH 08/10] MCE: Relay UCR MCE to guest
Date: Wed, 20 Oct 2010 15:59:29 -0500
Message-ID: <4CBF5831.7030709@codemonkey.ws>
References: <cover.1287596626.git.mtosatti@redhat.com>  <10ae5833ff9de153c311917d532f3e84e5b00387.1287596626.git.mtosatti@redhat.com> <4CBF485C.9060808@linux.vnet.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org,
	Avi Kivity <avi@redhat.com>
To: Anthony Liguori <aliguori@linux.vnet.ibm.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mail-qw0-f46.google.com ([209.85.216.46]:46436 "EHLO
	mail-qw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751885Ab0JTU7b (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 20 Oct 2010 16:59:31 -0400
Received: by qwa26 with SMTP id 26so2770615qwa.19
        for <kvm@vger.kernel.org>; Wed, 20 Oct 2010 13:59:30 -0700 (PDT)
In-Reply-To: <4CBF485C.9060808@linux.vnet.ibm.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 10/20/2010 02:51 PM, Anthony Liguori wrote:
>
>> +}
>> +
>>   static void qemu_kvm_eat_signal(CPUState *env, int timeout)
>>   {
>>       struct timespec ts;
>>       int r, e;
>>       siginfo_t siginfo;
>>       sigset_t waitset;
>> +    sigset_t chkset;
>>
>>       ts.tv_sec = timeout / 1000;
>>       ts.tv_nsec = (timeout % 1000) * 1000000;
>>
>>       sigemptyset(&waitset);
>>       sigaddset(&waitset, SIG_IPI);
>> +    sigaddset(&waitset, SIGBUS);
>>
>> -    qemu_mutex_unlock(&qemu_global_mutex);
>> -    r = sigtimedwait(&waitset,&siginfo,&ts);
>> -    e = errno;
>> -    qemu_mutex_lock(&qemu_global_mutex);
>> +    do {
>> +        qemu_mutex_unlock(&qemu_global_mutex);
>>
>> -    if (r == -1&&  !(e == EAGAIN || e == EINTR)) {
>> -        fprintf(stderr, "sigtimedwait: %s\n", strerror(e));
>> -        exit(1);
>> -    }
>> +        r = sigtimedwait(&waitset,&siginfo,&ts);
>> +        e = errno;
>> +
>> +        qemu_mutex_lock(&qemu_global_mutex);
>> +
>> +        if (r == -1&&  !(e == EAGAIN || e == EINTR)) {
>> +            fprintf(stderr, "sigtimedwait: %s\n", strerror(e));
>> +            exit(1);
>> +        }
>> +
>> +        switch (r) {
>> +        case SIGBUS:
>> +#ifdef TARGET_I386
>> +            if (kvm_on_sigbus_vcpu(env, siginfo.si_code, 
>> siginfo.si_addr))
>> +#endif
>> +                sigbus_reraise();
>> +            break;
>> +        default:
>> +            break;
>> +        }
>> +
>> +        r = sigpending(&chkset);
>> +        if (r == -1) {
>> +            fprintf(stderr, "sigpending: %s\n", strerror(e));
>> +            exit(1);
>> +        }
>> +    } while (sigismember(&chkset, SIG_IPI) || sigismember(&chkset, 
>> SIGBUS));
>>   }
>
> I don't understand why this loop is needed but we specifically wait 
> for a signal to get delivered that's either SIG_IPI or SIGBUS.  We 
> then check whether a SIG_IPI or SIGBUS is pending and loop waiting for 
> signals again.
>
> Shouldn't we be looping on just sigismember(SIGBUS)?
>
> BTW, we're no longer respecting timeout because we're not adjusting ts 
> after each iteration.

I think this is important too.  The last time I went through the code 
and played around here, it wasn't possible to set timeout to a very, 
very large value because there are still things that we poll for (like 
whether shutdown has occurred).   If we loop indefinitely without 
reducing ts, we can potentially recreate an infinite timeout which means 
we won't catch any of the events we poll for.  This would be a very, 
very subtle bug to track down.

Regards,

Anthony Liguori