[PATCH] qemu-kvm: Fix segfault on -no-kvm startup

[PATCH] qemu-kvm: Fix segfault on -no-kvm startup

[Jan Kiszka]

The check for in-kernel irqchip must be protected by kvm_enabled, and we
have a different wrapper for it.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 hw/apic.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index 3a2e128..01ac174 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -509,9 +509,10 @@ void apic_init_reset(CPUState *env)
 
     env->halted = !(s->apicbase & MSR_IA32_APICBASE_BSP);
 #ifdef KVM_CAP_MP_STATE
-    if (kvm_irqchip_in_kernel(kvm_context))
+    if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
         env->mp_state
             = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
+    }
 #endif
 }
 

Re: [PATCH] qemu-kvm: Fix segfault on -no-kvm startup

[Gleb Natapov]

On Fri, Sep 25, 2009 at 06:05:49PM +0200, Jan Kiszka wrote:
> The check for in-kernel irqchip must be protected by kvm_enabled, and we
> have a different wrapper for it.
> 
Why not move kvm_enabled() into kvm_irqchip_in_kernel()? It will return
false if !kvm_enabled().

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
>  hw/apic.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/apic.c b/hw/apic.c
> index 3a2e128..01ac174 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -509,9 +509,10 @@ void apic_init_reset(CPUState *env)
>  
>      env->halted = !(s->apicbase & MSR_IA32_APICBASE_BSP);
>  #ifdef KVM_CAP_MP_STATE
> -    if (kvm_irqchip_in_kernel(kvm_context))
> +    if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
>          env->mp_state
>              = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
> +    }
>  #endif
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [PATCH] qemu-kvm: Fix segfault on -no-kvm startup

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Fri, Sep 25, 2009 at 06:05:49PM +0200, Jan Kiszka wrote:
>> The check for in-kernel irqchip must be protected by kvm_enabled, and we
>> have a different wrapper for it.
>>
> Why not move kvm_enabled() into kvm_irqchip_in_kernel()? It will return
> false if !kvm_enabled().

Yes, possible. But I'm not sure if it's worth to refactor at this level.

I think the whole irqchip interface has to go through some broader
refactoring when pushing it upstream. The result should either be a
specific, in-kernel-irqchip apic device or generic wrapper services that
cover all cases, is easily compiled away in the absence of KVM and avoid
#ifdefs like below.

Jan

> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>>  hw/apic.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/apic.c b/hw/apic.c
>> index 3a2e128..01ac174 100644
>> --- a/hw/apic.c
>> +++ b/hw/apic.c
>> @@ -509,9 +509,10 @@ void apic_init_reset(CPUState *env)
>>  
>>      env->halted = !(s->apicbase & MSR_IA32_APICBASE_BSP);
>>  #ifdef KVM_CAP_MP_STATE
>> -    if (kvm_irqchip_in_kernel(kvm_context))
>> +    if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
>>          env->mp_state
>>              = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
>> +    }
>>  #endif
>>  }
>>  


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

Re: [PATCH] qemu-kvm: Fix segfault on -no-kvm startup

[Avi Kivity]

On 09/25/2009 07:05 PM, Jan Kiszka wrote:
> The check for in-kernel irqchip must be protected by kvm_enabled, and we
> have a different wrapper for it.
>
>    

Applied, thanks.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] qemu-kvm: Fix segfault on -no-kvm startup

[Avi Kivity]

On 09/25/2009 10:03 PM, Jan Kiszka wrote:
> Gleb Natapov wrote:
>    
>> On Fri, Sep 25, 2009 at 06:05:49PM +0200, Jan Kiszka wrote:
>>      
>>> The check for in-kernel irqchip must be protected by kvm_enabled, and we
>>> have a different wrapper for it.
>>>
>>>        
>> Why not move kvm_enabled() into kvm_irqchip_in_kernel()? It will return
>> false if !kvm_enabled().
>>      
> Yes, possible. But I'm not sure if it's worth to refactor at this level.
>    

In any case, fix bugs first, refactor later.

> I think the whole irqchip interface has to go through some broader
> refactoring when pushing it upstream. The result should either be a
> specific, in-kernel-irqchip apic device or generic wrapper services that
> cover all cases, is easily compiled away in the absence of KVM and avoid
> #ifdefs like below.
>    

s/when/before/

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.