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

[Nitesh Narayan Lal <nitesh@redhat.com>]

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();
>> +	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