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 07:35:01PM +0200, Jan Kiszka wrote:
> Sheng Yang wrote:
> > 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.
> 
> Ah, good reference.
> 
> > 
> > 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.
> 
> According to my understanding of the spec, the virtual wire mode means
> that the PIC signal is delivered via LVT0, and thus can be received by
> _all_ CPUs in the system. However, only the BSP usually enables LVT0,
> thus is receiving the IRQ. When Linux switches to NMI watchdog mode 1,
> it also unmasks the other CPUs (and reprograms all to deliver NMIs
> instead of EXTINTs).
> 
> Then there is also the "PIC Mode", ie. direct delivery to the BSP, and
> only the latter. That mode is obviously target by the current
> kvm_apic_accept_pic_intr implementation. But I find no indication in the
> spec yet that both modes cannot exists in the same system. But I also
> fail to understand how one could switch between both modes (via software).

No. If so, we don't need to check LVT0.

--
regards
Yang, Sheng

> 
> > 
> > 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.
> 
> The key problem for Windows was most probably not NMI, but the fact that
> we forwarded _any_ PIC IRQ (emulating virtual wire mode) without
> checking for the LAPICs' mask state.
> 
> OK, this requires a few more thoughts and a bit more reading.
> 
> Jan
>