Re: [PATCH 1/3] KVM: x86: Relax accept conditions of kvm_apic_accept_pic_intr

[Jan Kiszka <jan.kiszka@siemens.com>]

Sheng Yang wrote:
> On Wednesday 15 October 2008 22:27:49 Jan Kiszka wrote:
>> Aligning in-kernel kvm_apic_accept_pic_intr with its user space mate,
>> this patch relaxes the conditions under which PIC IRQs are accepted
>> by LVT0. This reflects reality and allows to reuse the service for the
>> NMI watchdog use case.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/x86/kvm/lapic.c |   13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> Index: b/arch/x86/kvm/lapic.c
>> ===================================================================
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -1072,16 +1072,11 @@ int kvm_apic_has_interrupt(struct kvm_vc
>>  int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
>>  {
>>  	u32 lvt0 = apic_get_reg(vcpu->arch.apic, APIC_LVT0);
>> -	int r = 0;
>>
>> -	if (vcpu->vcpu_id == 0) {
>> -		if (!apic_hw_enabled(vcpu->arch.apic))
>> -			r = 1;
>> -		if ((lvt0 & APIC_LVT_MASKED) == 0 &&
>> -		    GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
>> -			r = 1;
>> -	}
>> -	return r;
>> +	if (!apic_hw_enabled(vcpu->arch.apic) ||
>> +	    (lvt0 & APIC_LVT_MASKED) == 0)
>> +		return 1;
>> +	return 0;
>>  }
>>
>>  void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)
>>
> (sorry for late review...)
> 
> Thanks to find out the root cause of BSOD!
> 
> But I am a little concern about this change. As you know, PIC only connect to 
> cpu0. So I think it's not proper to make it generic. 

I don't think so - and if it were true, qemu would have a bug then, see
its corresponding code.

> 
> Maybe you can use kvm_apic_accept_pic_intr(vcpu0) in later patch?

Sorry, don't get what you mean with this (independent of the above).

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux