Re: [Patch v1] x86: Fix the logical destination mode test

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Nitesh Narayan Lal <nitesh@redhat.com> 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@redhat.com>
>> 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.

Why do we move it from under 'if (cpu_count() > 1)' ?

Also, this patch could've been split.

>>> +	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

-- 
Vitaly