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

[Sheng Yang <yasker@gmail.com>]

On Fri, Oct 17, 2008 at 10:10:54AM +0200, Jan Kiszka wrote:
> 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.

You can refer to Intel MP spec, virtual wire mode. Google
"MP spec" can find it.

Normally PIC is only used in BSP boot up for SMP guest(PIC can't afford SMP,
otherwise we won't need IOAPIC/LAPIC). After that, it should be disabled.
And virtual wire mode works with APIC_MODE_EXTINT on LVT0 of BSP lapic, so
that's why you see

    GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT

KVM follow virtual wire mode exactly.

For QEmu, it just check if lapic LVT0 is masked, and don't check vcpu0.
That's indeed a little problematic, for it's not that sufficient to
determine if it's programmed as virtual wire mode and used for deliver
interrupts from PIC. Well, in most condition, it can work. But maybe
it's not clean in logic.

For NMI watchdog here, we use a little more tricky way other than normal
PIC/LAPIC interaction. IIRC, NMI watchdog don't mask PIC after enable
IOAPIC, it also don't mask LVT0 of every LAPIC. It use physical connection
of PIT to PIC then to LAPIC LVT0 to send NMI. Program LVT0 to NMI, then
every PIT interrupt would go through PIC, arrive at LVT0, trig a NMI.

So I think the key problem for Windows is, they don't need it, but we send
the NMIs. We send the NMI when LVT0 is masked. Base on this, I think your
optimize patch also can resolve this issue? It's already including necessary
judgment. We will try it next week.

--
regards
Yang, Sheng

> 
> > 
> > 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
> --
> 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