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

Marcelo Tosatti
2020-03-10T17:02:11Z

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