[Qemu-devel] Re: [PATCH 07/18] kvm: Add MCE signal support for !CONFIG_IOTHREAD

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3929 bytes --]

On 2011-01-26 13:01, Marcelo Tosatti wrote:
> On Wed, Jan 26, 2011 at 09:09:25AM +0100, Jan Kiszka wrote:
>> On 2011-01-24 13:36, Jan Kiszka wrote:
>>> On 2011-01-24 12:17, Marcelo Tosatti wrote:
>>>> On Mon, Jan 10, 2011 at 09:32:00AM +0100, Jan Kiszka wrote:
>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> Currently, we only configure and process MCE-related SIGBUS events if
>>>>> CONFIG_IOTHREAD is enabled. Fix this by factoring out the required
>>>>> handler registration and system configuration. Make sure that events
>>>>> happening over a VCPU context in non-threaded mode get dispatched as
>>>>> VCPU MCEs.
>>>>>
>>>>> We also need to call qemu_kvm_eat_signals in non-threaded mode now, so
>>>>> move it (unmodified) and add the required Windows stub.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> CC: Huang Ying <ying.huang@intel.com>
>>>>> ---
>>>>>  cpus.c |  200 +++++++++++++++++++++++++++++++++++++++------------------------
>>>>>  1 files changed, 124 insertions(+), 76 deletions(-)
>>>>>
>>>>> diff --git a/cpus.c b/cpus.c
>>>>> index 6da0f8f..b6f1cfb 100644
>>>>> --- a/cpus.c
>>>>> +++ b/cpus.c
>>>>> @@ -34,9 +34,6 @@
>>>>>  
>>>>>  #include "cpus.h"
>>>>>  #include "compatfd.h"
>>>>> -#ifdef CONFIG_LINUX
>>>>> -#include <sys/prctl.h>
>>>>> -#endif
>>>>>  
>>>>>  #ifdef SIGRTMIN
>>>>>  #define SIG_IPI (SIGRTMIN+4)
>>>>> @@ -44,10 +41,24 @@
>>>>>  #define SIG_IPI SIGUSR1
>>>>>  #endif
>>>>>  
>>>>
>>>>> @@ -912,6 +954,8 @@ static int qemu_cpu_exec(CPUState *env)
>>>>>  
>>>>>  bool cpu_exec_all(void)
>>>>>  {
>>>>> +    int r;
>>>>> +
>>>>>      if (next_cpu == NULL)
>>>>>          next_cpu = first_cpu;
>>>>>      for (; next_cpu != NULL && !exit_request; next_cpu = next_cpu->next_cpu) {
>>>>> @@ -923,7 +967,11 @@ bool cpu_exec_all(void)
>>>>>          if (qemu_alarm_pending())
>>>>>              break;
>>>>>          if (cpu_can_run(env)) {
>>>>> -            if (qemu_cpu_exec(env) == EXCP_DEBUG) {
>>>>> +            r = qemu_cpu_exec(env);
>>>>> +            if (kvm_enabled()) {
>>>>> +                qemu_kvm_eat_signals(env);
>>>>> +            }
>>>>> +            if (r == EXCP_DEBUG) {
>>>>>                  break;
>>>>>              }
>>>>
>>>> SIGBUS should be processed outside of vcpu execution context, think of a
>>>> non MCE SIGBUS while vm is stopped. Could use signalfd for that.
>>>
>>> signalfd - that's the missing bit. I was thinking of how to handle
>>> SIGBUS events raised outside the vcpu context. We need to handle them
>>> synchronously, and signalfd should allow this.
>>
>> This was straightforward. But now I wonder what actually makes this
>> pattern work. Doesn't the kernel force-inject SIGBUS, i.e. ignores any
>> blocking? Or does this only apply to BUS_MCEERR_AR?
> 
> SIGBUS is only forced if BUS_MCEERR_AR and the poisoned memory was not accessed 
> on behalf of the guest (say directly by qemu).

OK. I didn't find this detail in the kernel code on first glance, only
force_sig(SIGBUS, current).

> 
>>>> But the SIGBUS handler for !IOTHREAD case should not ignore Action
>>>> Required, since it might have been generated in vcpu context.
>>>>
>>>
>>> Yes, the sigbus handler will require some rework when we actually start
>>> using it for !IOTHREAD.
>>
>> And this no longer makes sense to me. The current version simply uses
>> the same sigbus handler for both modes, an that forwards the error code
>> properly. What did you mean?
> 
> There are two handlers, kvm_on_sigbus and kvm_on_sigbus_vcpu.
> kvm_on_sigbus, the handler for iothread, dies on BUS_MCEERR_AR (which
> will be generated if poisoned memory is accessed on behalf of vcpu). It
> should be handled with !CONFIG_IOTHREAD.

Just as with iothread, vcpu-triggered SIGBUS events are processes by
qemu_kvm_eat_signals, not the signal handler.

Jan


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