Re: kvm-unit-test fail for split irqchip

[Paolo Bonzini]

On 14/09/2016 05:57, Wanpeng Li wrote:
> 2016-09-14 4:43 GMT+08:00 Paolo Bonzini <pbonzini@xxxxxxxxxx>:
>>
>>
>> On 13/09/2016 21:01, Radim Krcmar wrote:
>>> kvm_handle_interrupt() does
>>>
>>>   interrupt_request |= CPU_INTERRUPT_HARD
>>>
>>> which later calls cpu_get_pic_interrupt() in kvm_arch_pre_run(), but
>>> that function uses stale information from APIC and injects 62 again.
>>> If we synchronized the APIC, then the test would #GP, because there
>>> would be no injectable interrupt in LAPIC or PIC, so pic_read_irq()
>>> would return 15, thinking it was spurious.
>>>
>>> I think the bug starts in pic_irq_request(), which should not touch
>>> LAPIC.  The patch below makes it work (just the second hunk is
>>> sufficient), but it's still far from sane code ...
>>
>> This makes sense.  Most of the functions exported by hw/intc/apic.c
>> should only be used with a userspace APIC:
>>
>> 0000000000000b70 T apic_accept_pic_intr
>> 00000000000010f0 T apic_deliver_irq
>> 00000000000011e0 T apic_deliver_pic_intr
>> 0000000000001310 T apic_get_interrupt
>> 0000000000001270 T apic_poll_irq
>> 0000000000000a40 T apic_sipi
>>
>> The patch is okay, though for consistency with other code I'd use
>> !kvm_irqchip_in_kernel() rather than !kvm_irqchip_is_split().
>>
>> Wanpeng, can you do that,
> 
> Yeah, I just sent out a patch to fix the bug. Thanks for the long
> discussion with me and thanks Radim's proposal. :)
> 
>> and change hw/intc/apic.c to use a new casting
>> macro
>>
>>     APICCommonState *s = APIC(dev);
>>
>> instead of APIC_COMMON?
>>
> 
> I'm not familiar with QOM too much, what APIC macro do you like?

The macro would be defined like

#define TYPE_APIC "apic"
#define APIC(obj) \
     OBJECT_CHECK(APICCommonState, (obj), TYPE_APIC)

With this change and without your other patch, you should get an
assertion failure in eventinj.flat (which would have pointed immediately
at the bug!).

Thanks,

Paolo

> Regards,
> Wanpeng Li
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>