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:56:13PM +0200, Jan Kiszka wrote:
> Sheng Yang wrote:
> > 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.
> 
> OK, there is that IMCR register to enable/disable the PIC Mode - but
> neither KVM nor QEMU implement it (which may indicate they both want to
> provide Virtual Wire Mode?). When enabled, Virtual Wire Mode is
> effectively disabled (for the BSP at least) as the LAPIC is disconnected
> (from the BSP).

Please check the figure for the difference between PIC mode and virtual wire
mode.

> 
> Still, I find not trace in the spec that says only the BSP can receive
> PIC interrupts in Virtual Wire Mode (the LVT0 line is connected to _all_
> CPUs).
> 

Only the one to BSP works normally. And I am not meant to go throught PIC
when implement NMI Watchdog, otherwise I won't use that
apic_local_deliver(). NMI watchdog is too specific case, I don't want to mix
them up.

--
regards
Yang, Sheng

> Jan
>