| Message ID | 20161219160429.5237-1-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19/12/2016 17:04, Radim Krčmář wrote: > Broadcast to address 0xff is not exercised by seabios+linux so we better > test it. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > The bounded wait loop is bad ... time to introduce TSC magic? Yeah, the VMX tests already use the TSC (though in the other direction: they check that _at least_ some time passed). Roughly speaking, 1 billion TSC units should be plenty, even on a loaded hosts. Paolo > x86/apic.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/x86/apic.c b/x86/apic.c > index 39c7fd19913d..b25ba45e4b8f 100644 > --- a/x86/apic.c > +++ b/x86/apic.c > @@ -5,6 +5,7 @@ > #include "desc.h" > #include "isr.h" > #include "msr.h" > +#include "atomic.h" > > static void test_lapic_existence(void) > { > @@ -16,6 +17,7 @@ static void test_lapic_existence(void) > } > > #define TSC_DEADLINE_TIMER_VECTOR 0xef > +#define BROADCAST_VECTOR 0xcf > > static int tdt_count; > > @@ -411,6 +413,49 @@ static void test_apic_timer_one_shot(void) > (tsc2 - tsc1 >= interval)); > } > > +static atomic_t broadcast_counter; > + > +static void broadcast_handler(isr_regs_t *regs) > +{ > + atomic_inc(&broadcast_counter); > + eoi(); > +} > + > +static bool broadcast_received(unsigned ncpus) > +{ > + unsigned counter; > + > + for (int i = 123456789; i; i--) { > + counter = atomic_read(&broadcast_counter); > + if (counter >= ncpus) > + break; > + pause(); > + } > + > + atomic_set(&broadcast_counter, 0); > + > + return counter == ncpus; > +} > + > +static void test_physical_broadcast(void) > +{ > + unsigned ncpus = cpu_count(); > + unsigned long cr3 = read_cr3(); > + u32 broadcast_address = enable_x2apic() ? 0xffffffff : 0xff; > + > + handle_irq(BROADCAST_VECTOR, broadcast_handler); > + for (int c = 1; c < ncpus; c++) > + on_cpu(c, update_cr3, (void *)cr3); > + > + apic_icr_write(APIC_DEST_PHYSICAL | APIC_DM_FIXED | APIC_INT_ASSERT | > + BROADCAST_VECTOR, broadcast_address); > + report("APIC physical broadcast address", broadcast_received(ncpus)); > + > + apic_icr_write(APIC_DEST_PHYSICAL | APIC_DM_FIXED | APIC_INT_ASSERT | > + BROADCAST_VECTOR | APIC_DEST_ALLINC, 0); > + report("APIC physical broadcast shorthand", broadcast_received(ncpus)); > +} > + > int main() > { > setup_vm(); > @@ -425,6 +470,7 @@ int main() > test_apicbase(); > > test_self_ipi(); > + test_physical_broadcast(); > > test_sti_nmi(); > test_multiple_nmi(); > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19/12/2016 17:04, Radim Krčmář wrote: > Broadcast to address 0xff is not exercised by seabios+linux so we better > test it. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > The bounded wait loop is bad ... time to introduce TSC magic? > --- > x86/apic.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/x86/apic.c b/x86/apic.c > index 39c7fd19913d..b25ba45e4b8f 100644 > --- a/x86/apic.c > +++ b/x86/apic.c > @@ -5,6 +5,7 @@ > #include "desc.h" > #include "isr.h" > #include "msr.h" > +#include "atomic.h" > > static void test_lapic_existence(void) > { > @@ -16,6 +17,7 @@ static void test_lapic_existence(void) > } > > #define TSC_DEADLINE_TIMER_VECTOR 0xef > +#define BROADCAST_VECTOR 0xcf > > static int tdt_count; > > @@ -411,6 +413,49 @@ static void test_apic_timer_one_shot(void) > (tsc2 - tsc1 >= interval)); > } > > +static atomic_t broadcast_counter; > + > +static void broadcast_handler(isr_regs_t *regs) > +{ > + atomic_inc(&broadcast_counter); > + eoi(); > +} > + > +static bool broadcast_received(unsigned ncpus) > +{ > + unsigned counter; > + > + for (int i = 123456789; i; i--) { > + counter = atomic_read(&broadcast_counter); > + if (counter >= ncpus) > + break; > + pause(); > + } > + > + atomic_set(&broadcast_counter, 0); > + > + return counter == ncpus; > +} > + > +static void test_physical_broadcast(void) > +{ > + unsigned ncpus = cpu_count(); > + unsigned long cr3 = read_cr3(); > + u32 broadcast_address = enable_x2apic() ? 0xffffffff : 0xff; > + > + handle_irq(BROADCAST_VECTOR, broadcast_handler); > + for (int c = 1; c < ncpus; c++) > + on_cpu(c, update_cr3, (void *)cr3); > + > + apic_icr_write(APIC_DEST_PHYSICAL | APIC_DM_FIXED | APIC_INT_ASSERT | > + BROADCAST_VECTOR, broadcast_address); > + report("APIC physical broadcast address", broadcast_received(ncpus)); This test is broken by commit f11e8b051ef613670f9db77a79726534dbc667ec Author: Radim Krčmář <rkrcmar@redhat.com> Date: Tue Dec 13 17:30:00 2016 +0100 KVM: x86: make interrupt delivery fast and slow path behave the same Slow path tried to prevent IPIs from x2APIC VCPUs from being delivered to xAPIC VCPUs and vice-versa. Make slow path behave like fast path, which never distinguished that. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> Fixes: 682f732ecf73 ("KVM: x86: bump MAX_VCPUS to 288") Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> when run with "./x86/run x86/apic.flat -smp 2 -cpu Nehalem,-x2apic", so I'm removing that series from kvm/queue. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-12-22 11:45+0100, Paolo Bonzini: > On 19/12/2016 17:04, Radim Krčmář wrote: >> Broadcast to address 0xff is not exercised by seabios+linux so we better >> test it. >> >> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> >> --- > > This test is broken by > > commit f11e8b051ef613670f9db77a79726534dbc667ec > Author: Radim Krčmář <rkrcmar@redhat.com> > Date: Tue Dec 13 17:30:00 2016 +0100 > > KVM: x86: make interrupt delivery fast and slow path behave the same > > Slow path tried to prevent IPIs from x2APIC VCPUs from being delivered > to xAPIC VCPUs and vice-versa. Make slow path behave like fast path, > which never distinguished that. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > Fixes: 682f732ecf73 ("KVM: x86: bump MAX_VCPUS to 288") > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > when run with "./x86/run x86/apic.flat -smp 2 -cpu Nehalem,-x2apic", so I'm removing > that series from kvm/queue. Yes, kvm/queue had v2 of the series and I have added the test because David noticed the broadcast bug while reviewing v3 of that series. (http://www.spinics.net/lists/kvm/msg142614.html) The bug was fixed in v4. I have only posted v4 of this one patch as the rest seemed ok in v3. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-12-22 11:02+0100, Paolo Bonzini: > On 19/12/2016 17:04, Radim Krčmář wrote: >> Broadcast to address 0xff is not exercised by seabios+linux so we better >> test it. >> >> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> >> --- >> The bounded wait loop is bad ... time to introduce TSC magic? > > Yeah, the VMX tests already use the TSC (though in the other direction: > they check that _at least_ some time passed). Roughly speaking, 1 > billion TSC units should be plenty, even on a loaded hosts. I will convert it, thanks. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/12/2016 19:15, Radim Krčmář wrote: > 2016-12-22 11:45+0100, Paolo Bonzini: >> On 19/12/2016 17:04, Radim Krčmář wrote: >>> Broadcast to address 0xff is not exercised by seabios+linux so we better >>> test it. >>> >>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> >>> --- >> >> This test is broken by >> >> commit f11e8b051ef613670f9db77a79726534dbc667ec >> Author: Radim Krčmář <rkrcmar@redhat.com> >> Date: Tue Dec 13 17:30:00 2016 +0100 >> >> KVM: x86: make interrupt delivery fast and slow path behave the same >> >> Slow path tried to prevent IPIs from x2APIC VCPUs from being delivered >> to xAPIC VCPUs and vice-versa. Make slow path behave like fast path, >> which never distinguished that. >> >> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> >> Fixes: 682f732ecf73 ("KVM: x86: bump MAX_VCPUS to 288") >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> >> when run with "./x86/run x86/apic.flat -smp 2 -cpu Nehalem,-x2apic", so I'm removing >> that series from kvm/queue. > > Yes, kvm/queue had v2 of the series and I have added the test because > David noticed the broadcast bug while reviewing v3 of that series. > (http://www.spinics.net/lists/kvm/msg142614.html) > > The bug was fixed in v4. I have only posted v4 of this one patch as the > rest seemed ok in v3. Oh, okay, then I'll recheck and fix it. Currently I'm seeing WinXP/2003 failures with apicv=0 and Win2008r2 failures with apicv=0 ept=0. I guess I'll have to have some fun bisecting. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/x86/apic.c b/x86/apic.c index 39c7fd19913d..b25ba45e4b8f 100644 --- a/x86/apic.c +++ b/x86/apic.c @@ -5,6 +5,7 @@ #include "desc.h" #include "isr.h" #include "msr.h" +#include "atomic.h" static void test_lapic_existence(void) { @@ -16,6 +17,7 @@ static void test_lapic_existence(void) } #define TSC_DEADLINE_TIMER_VECTOR 0xef +#define BROADCAST_VECTOR 0xcf static int tdt_count; @@ -411,6 +413,49 @@ static void test_apic_timer_one_shot(void) (tsc2 - tsc1 >= interval)); } +static atomic_t broadcast_counter; + +static void broadcast_handler(isr_regs_t *regs) +{ + atomic_inc(&broadcast_counter); + eoi(); +} + +static bool broadcast_received(unsigned ncpus) +{ + unsigned counter; + + for (int i = 123456789; i; i--) { + counter = atomic_read(&broadcast_counter); + if (counter >= ncpus) + break; + pause(); + } + + atomic_set(&broadcast_counter, 0); + + return counter == ncpus; +} + +static void test_physical_broadcast(void) +{ + unsigned ncpus = cpu_count(); + unsigned long cr3 = read_cr3(); + u32 broadcast_address = enable_x2apic() ? 0xffffffff : 0xff; + + handle_irq(BROADCAST_VECTOR, broadcast_handler); + for (int c = 1; c < ncpus; c++) + on_cpu(c, update_cr3, (void *)cr3); + + apic_icr_write(APIC_DEST_PHYSICAL | APIC_DM_FIXED | APIC_INT_ASSERT | + BROADCAST_VECTOR, broadcast_address); + report("APIC physical broadcast address", broadcast_received(ncpus)); + + apic_icr_write(APIC_DEST_PHYSICAL | APIC_DM_FIXED | APIC_INT_ASSERT | + BROADCAST_VECTOR | APIC_DEST_ALLINC, 0); + report("APIC physical broadcast shorthand", broadcast_received(ncpus)); +} + int main() { setup_vm(); @@ -425,6 +470,7 @@ int main() test_apicbase(); test_self_ipi(); + test_physical_broadcast(); test_sti_nmi(); test_multiple_nmi();
Broadcast to address 0xff is not exercised by seabios+linux so we better test it. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- The bounded wait loop is bad ... time to introduce TSC magic? --- x86/apic.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)