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

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

[Nitesh Narayan Lal]

On 3/9/20 7:15 PM, 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().

The above were the reasons which were causing the ioapic logical destination
mode test to fail/hang.

The previous discussion can be found at:
https://lore.kernel.org/kvm/20191205151610.19299-1-thuth@redhat.com/

Following is a test run with smp = 4 and -machine kernel_irqchip=split.

[root@virtlab420 kvm-unit-tests]# git diff
diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index d658bc8..348953b 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,+x2apic
+extra_params = -cpu qemu64,+x2apic -machine kernel_irqchip=split
 arch = x86_64
 
 [cmpxchg8b]
[root@virtlab420 kvm-unit-tests]# ./tests/ioapic
BUILD_HEAD=83a3b429
timeout -k 1s --foreground 90s /usr/libexec/qemu-kvm -nodefaults -device
pc-testdev -device isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc none -serial stdio
-device pci-testdev -machine accel=kvm -kernel /tmp/tmp.cS40xFvt0U -smp 4 -cpu
qemu64,+x2apic -machine kernel_irqchip=split # -initrd /tmp/tmp.69rw56TppH
enabling apic
enabling apic
enabling apic
enabling apic
paging enabled
cr0 = 80010011
cr3 = 61d000
cr4 = 20
x2apic enabled
PASS: version register read only test
PASS: id register only bits [24:27] writable
PASS: arbitration register set by id
PASS: arbtration register read only
PASS: edge triggered intr
PASS: level triggered intr
PASS: ioapic simultaneous edge interrupts
PASS: coalesce simultaneous level interrupts
PASS: sequential level interrupts
PASS: retriggered level interrupts without masking
PASS: masked level interrupt
PASS: unmasked level interrupt
PASS: masked level interrupt
PASS: unmasked level interrupt
PASS: retriggered level interrupts with mask
PASS: TMR for ioapic edge interrupts (expected false)
PASS: TMR for ioapic level interrupts (expected false)
PASS: TMR for ioapic level interrupts (expected true)
PASS: TMR for ioapic edge interrupts (expected true)
PASS: ioapic physical destination mode
PASS: ioapic logical destination mode
1012375 iterations before interrupt received
PASS: TMR for ioapic edge interrupts (expected false)
832021 iterations before interrupt received
PASS: TMR for ioapic level interrupts (expected false)
1009498 iterations before interrupt received
PASS: TMR for ioapic level interrupts (expected true)
994532 iterations before interrupt received
PASS: TMR for ioapic edge interrupts (expected true)
FAIL: Reconfigure self
SUMMARY: 26 tests, 1 unexpected failures
FAIL ioapic (26 tests, 1 unexpected failures)

Please let me know if there are any more suggestions/comments.

[...]

-- 
Nitesh

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

[Marcelo Tosatti]

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.

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

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

[Nitesh Narayan Lal]

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

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

[Paolo Bonzini]

On 16/03/20 23:27, Nitesh Narayan Lal wrote:
> 
> 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.

Thanks Nitesh, I have queued it now.

Paolo

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

[Vitaly Kuznetsov]

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

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

[Nitesh Narayan Lal]

On 4/14/20 10:01 AM, Vitaly Kuznetsov wrote:
> 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.

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

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

[Vitaly Kuznetsov]

Nitesh Narayan Lal <nitesh@redhat.com> writes:

> On 4/14/20 10:01 AM, Vitaly Kuznetsov wrote:
>>
>> 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?

Too late, it's already commited :-) I just meant to say that
e.g. spinlock part could've been split into its own patch, unittests.cfg
- another one,...

-- 
Vitaly

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

[Nitesh Narayan Lal]

On 4/14/20 11:30 AM, Vitaly Kuznetsov wrote:
> Nitesh Narayan Lal <nitesh@redhat.com> writes:
>
>> On 4/14/20 10:01 AM, Vitaly Kuznetsov wrote:
>>> 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?
> Too late, it's already commited :-) I just meant to say that
> e.g. spinlock part could've been split into its own patch, unittests.cfg
> - another one,...

Ah, I see. I will be more careful.
For now,  I will just move the physical destination mode test back under
the check. Will that be acceptable as a standalone patch?
In between I have a question is it normal for test_ioapic_self_reconfigure()
to fail when executed with irqchip split?
If so do we expect that it will leave the VM in some sort of dirty state
that causes the following test to fail?

>
-- 
Nitesh

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

[Vitaly Kuznetsov]

Nitesh Narayan Lal <nitesh@redhat.com> writes:

> On 4/14/20 11:30 AM, Vitaly Kuznetsov wrote:
>> Nitesh Narayan Lal <nitesh@redhat.com> writes:
>>
>>> On 4/14/20 10:01 AM, Vitaly Kuznetsov wrote:
>>>> 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?
>> Too late, it's already commited :-) I just meant to say that
>> e.g. spinlock part could've been split into its own patch, unittests.cfg
>> - another one,...
>
> Ah, I see. I will be more careful.
> For now,  I will just move the physical destination mode test back under
> the check. Will that be acceptable as a standalone patch?

This is already in Paolo's patch:
https://lore.kernel.org/kvm/87zhbexh3v.fsf@vitty.brq.redhat.com/T/#m9791cd50a9d82fabdaddcb9259d14df3b89ed250

> In between I have a question is it normal for test_ioapic_self_reconfigure()
> to fail when executed with irqchip split?
> If so do we expect that it will leave the VM in some sort of dirty state
> that causes the following test to fail?

Not sure I got your question but IMO when someone does
./run_tests.sh
all tests are supposed to pass -- unless there is a bug in KVM (e.g. the
person is running an old kernel). In case we're seeing failures (or,
even worse, hangs) with the latest upstream kernel -- something is
broken, either KVM or kvm-unit-tests.

-- 
Vitaly

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

[Nitesh Narayan Lal]

On 4/14/20 12:11 PM, Vitaly Kuznetsov wrote:
> Nitesh Narayan Lal <nitesh@redhat.com> writes:
>
>> On 4/14/20 11:30 AM, Vitaly Kuznetsov wrote:
>>> Nitesh Narayan Lal <nitesh@redhat.com> writes:
>>>
>>>> On 4/14/20 10:01 AM, Vitaly Kuznetsov wrote:
>>>>> 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?
>>> Too late, it's already commited :-) I just meant to say that
>>> e.g. spinlock part could've been split into its own patch, unittests.cfg
>>> - another one,...
>> Ah, I see. I will be more careful.
>> For now,  I will just move the physical destination mode test back under
>> the check. Will that be acceptable as a standalone patch?
> This is already in Paolo's patch:
> https://lore.kernel.org/kvm/87zhbexh3v.fsf@vitty.brq.redhat.com/T/#m9791cd50a9d82fabdaddcb9259d14df3b89ed250

Yeap, this patch is already in the tree.
I was referring to a new patch in which I would move the physical destination
mode test
under 'if (cpu_count() > 1)', that should fixed the ioapic-split hang.
Sorry about the confusion.

>
>> In between I have a question is it normal for test_ioapic_self_reconfigure()
>> to fail when executed with irqchip split?
>> If so do we expect that it will leave the VM in some sort of dirty state
>> that causes the following test to fail?
> Not sure I got your question but IMO when someone does
> ./run_tests.sh
> all tests are supposed to pass -- unless there is a bug in KVM (e.g. the
> person is running an old kernel). In case we're seeing failures (or,
> even worse, hangs) with the latest upstream kernel -- something is
> broken, either KVM or kvm-unit-tests.

Yeap, I am not sure what is the exact issue that causes
test_ioapic_self_reconfigure() to
fail when used with kernel_irqchip=split.
I can again take a look to find out the root cause later.

-- 
Nitesh