On 4/14/20 10:01 AM, Vitaly Kuznetsov wrote:
> Nitesh Narayan Lal <nitesh@xxxxxxxxxx> writes:
>
>> On 3/10/20 10:03 AM, Marcelo Tosatti wrote:
>>> On Mon, Mar 09, 2020 at 07:15:50PM -0400, Nitesh Narayan Lal wrote:
>>>> There are following issues with the ioapic logical destination mode test:
>>>>
>>>> - A race condition that is triggered when the interrupt handler
>>>> ioapic_isr_86() is called at the same time by multiple vCPUs. Due to this
>>>> the g_isr_86 is not correctly incremented. To prevent this a spinlock is
>>>> added around ‘g_isr_86++’.
>>>>
>>>> - On older QEMU versions initial x2APIC ID is not set, that is why
>>>> the local APIC IDs of each vCPUs are not configured. Hence the logical
>>>> destination mode test fails/hangs. Adding ‘+x2apic’ to the qemu -cpu params
>>>> ensures that the local APICs are configured every time, irrespective of the
>>>> QEMU version.
>>>>
>>>> - With ‘-machine kernel_irqchip=split’ included in the ioapic test
>>>> test_ioapic_self_reconfigure() always fails and somehow leads to a state where
>>>> after submitting IOAPIC fixed delivery - logical destination mode request we
>>>> never receive an interrupt back. For now, the physical and logical destination
>>>> mode tests are moved above test_ioapic_self_reconfigure().
>>>>
>>>> Fixes: b2a1ee7e ("kvm-unit-test: x86: ioapic: Test physical and logical destination mode")
>>>> Signed-off-by: Nitesh Narayan Lal <nitesh@xxxxxxxxxx>
>>> Looks good to me.
>> Hi,
>>
>> I just wanted to follow up and see if there are any more suggestions for me to
>> improve this patch before it can be merged.
>>
>>
>>>> ---
>>>> x86/ioapic.c | 11 +++++++----
>>>> x86/unittests.cfg | 2 +-
>>>> 2 files changed, 8 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/x86/ioapic.c b/x86/ioapic.c
>>>> index 742c711..3106531 100644
>>>> --- a/x86/ioapic.c
>>>> +++ b/x86/ioapic.c
>>>> @@ -432,10 +432,13 @@ static void test_ioapic_physical_destination_mode(void)
>>>> }
>>>>
>>>> static volatile int g_isr_86;
>>>> +struct spinlock ioapic_lock;
>>>>
>>>> static void ioapic_isr_86(isr_regs_t *regs)
>>>> {
>>>> + spin_lock(&ioapic_lock);
>>>> ++g_isr_86;
>>>> + spin_unlock(&ioapic_lock);
>>>> set_irq_line(0x0e, 0);
>>>> eoi();
>>>> }
>>>> @@ -501,6 +504,10 @@ int main(void)
>>>> test_ioapic_level_tmr(true);
>>>> test_ioapic_edge_tmr(true);
>>>>
>>>> + test_ioapic_physical_destination_mode();
> I just found out that this particular change causes 'ioapic-split' test
> to hang reliably:
>
> # ./run_tests.sh ioapic-split
> FAIL ioapic-split (timeout; duration=90s)
> PASS ioapic (26 tests)
>
> unlike 'ioapic' test we run it with '-smp 1' and
> 'test_ioapic_physical_destination_mode' requires > 1 CPU to work AFAICT.
Yes, that is correct.
It's my bad as I didn't realize that I introduced this bug while submitting this
patch.
>
> Why do we move it from under 'if (cpu_count() > 1)' ?
I should have just moved it above test_ioapic_self_reconfigure().
>
> Also, this patch could've been split.
I can divide it 2 parts:
1. support for logical destination mode.
2. support for physical destination mode. I can also fix the above issue in
this patch itself.
Does that make sense?
>
>>>> + if (cpu_count() > 3)
>>>> + test_ioapic_logical_destination_mode();
>>>> +
>>>> if (cpu_count() > 1) {
>>>> test_ioapic_edge_tmr_smp(false);
>>>> test_ioapic_level_tmr_smp(false);
>>>> @@ -508,11 +515,7 @@ int main(void)
>>>> test_ioapic_edge_tmr_smp(true);
>>>>
>>>> test_ioapic_self_reconfigure();
>>>> - test_ioapic_physical_destination_mode();
>>>> }
>>>>
>>>> - if (cpu_count() > 3)
>>>> - test_ioapic_logical_destination_mode();
>>>> -
>>>> return report_summary();
>>>> }
>>>> diff --git a/x86/unittests.cfg b/x86/unittests.cfg
>>>> index f2401eb..d658bc8 100644
>>>> --- a/x86/unittests.cfg
>>>> +++ b/x86/unittests.cfg
>>>> @@ -46,7 +46,7 @@ timeout = 30
>>>> [ioapic]
>>>> file = ioapic.flat
>>>> smp = 4
>>>> -extra_params = -cpu qemu64
>>>> +extra_params = -cpu qemu64,+x2apic
>>>> arch = x86_64
>>>>
>>>> [cmpxchg8b]
>>>> --
>>>> 1.8.3.1
--
Nitesh
