Re: [Xen-devel] [PATCH] X86/vmx: Dump PIR and vIRR before ASSERT()

[Chao Gao <chao.gao@xxxxxxxxx>]

On Wed, Feb 08, 2017 at 04:06:53PM +0800, Tian, Kevin wrote:
>> From: Gao, Chao
>> Sent: Wednesday, February 08, 2017 8:12 AM
>> 
>> On Tue, Feb 07, 2017 at 09:18:56AM -0700, Jan Beulich wrote:
>> >>>> On 07.02.17 at 08:28, <chao.gao@xxxxxxxxx> wrote:
>> >> On Tue, Feb 07, 2017 at 06:46:16AM -0700, Jan Beulich wrote:
>> >>>>>> On 07.02.17 at 07:32, <chao.gao@xxxxxxxxx> wrote:
>> >>>> On Tue, Feb 07, 2017 at 03:04:32AM -0700, Jan Beulich wrote:
>> >>>>>>>> On 07.02.17 at 07:48, <xuquan8@xxxxxxxxxx> wrote:
>> >>>>>> Some comment from QEMU/KVM code, in
>> <linux-kernel>/arch/x86/kvm/lapic.c,
>> >>>>>>
>> >>>>>> int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
>> >>>>>> {
>> >>>>>>       /* This may race with setting of irr in __apic_accept_irq() and
>> >>>>>>        * value returned may be wrong, but kvm_vcpu_kick() in 
>> >>>>>> __apic_accept_irq
>> >>>>>>        * will cause vmexit immediately and the value will be 
>> >>>>>> recalculated
>> >>>>>>        * on the next vmentry.
>> >>>>>>        */
>> >>>>>> ...
>> >>>>>> }
>> >>>>>>
>> >>>>>> I am afraid, there may be a similar race with setting of vIRR..
>> >>>>>
>> >>>>>I think this is unrelated: If another interrupt came in, the highest
>> >>>>>set bit in vIRR can only increase. Plus pt_update_irq(), before
>> >>>>>returning, calls vlapic_set_irq(), which ought to result in pt_vector's
>> >>>>>vIRR bit to be set (either directly or via setting its PIR bit). I.e. 
>> >>>>>the
>> >>>>>highest priority interrupt should still have a vector >= pt_vector.
>> >>>>
>> >>>> I have noticed that pt_vector was 0x38 and the hightest vector was 0x30
>> >>>> when the assertion failed. In the process of debugging pt_update_irq()
>> >>>> when I was working on another feature, I noticed that 0x30 is always
>> >>>> the vector of IRQ2. I suspect that the source of the periodic timer
>> >> interrupt
>> >>>> is not lapic. If that, pt_update_irq() reads the vioapic entry twice,
>> >>>> one in hvm_isa_irq_assert() and the other in pt_irq_vector().
>> >>>> If guest changed the content in viopaic entry between the two read
>> >>>> (ie. change the vector from 0x30 to 0x38), the assertion would fail.
>> >>>> Do you think it is a reasonable explanation?
>> >>>
>> >>>IRQ2? If this was going via the PIC, that would be impossible, as
>> >>>that's the cascade pin. IRQ0, otoh, normally arrives at pin2 of the
>> >>>IO-APIC (and we follow this model - see tools/libacpi/build.c). How
>> >>
>> >> Sorry, I meaned IRQ0. I was confused with IRQ0 and pin2 of the IO-APIC.
>> >>
>> >>>did you determine that 0x30 is the vector of IRQ2?
>> >>
>> >> By monitoring the write operation to IO-APIC redirection entry.
>> >> From experimental data, linux kernel always writes vector 0x30 to
>> >> redirection entry of pin2 of IO-APIC.
>> >
>> >But if it goes through the IO-APIC, it'll also go through the LAPIC
>> >(and in the ExtInt case you wouldn't be able to read a valid vector
>> >from the RTE).
>> 
>> Yes. If a pt interrupt goes through IO-APIC, pt_update_irq() will read
>> the RTE twice. One is in hvm_isa_irq_assert() to determine which bit
>> in vIRR should be set. The other is in pt_irq_vector() to get the bit
>> we have set and return it. Is it possible that the return vector is
>> different from the bit set in vIRR of LAPIC, since guest has changed
>> RTE of IO-APIC pin2.
>> 
>
>In theory it could happen. There is no lock to prevent vIOAPIC RTE
>being modified between above two operations. But I'm not sure
>whether it's the reason for this bug. My feeling is that vector for IRQ0
>should be fixed one. Did you capture such vector change in your side?

I agree. I don't see guest changes the vector for IRQ0 in hundreds of tests
(simply create and shutdown guest).

Thanks
chao
>
>Thanks
>Kevin

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel