| Message ID | 20211202095843.41162-3-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: firq: floating interrupt test | expand |
On Thu, 2 Dec 2021 10:58:43 +0100 David Hildenbrand <david@redhat.com> wrote: > We had a KVM BUG fixed by kernel commit a3e03bc1368c ("KVM: s390: index > kvm->arch.idle_mask by vcpu_idx"), whereby a floating interrupt might get > stuck forever because a CPU in the wait state would not get woken up. > > The issue can be triggered when CPUs are created in a nonlinear fashion, > such that the CPU address ("core-id") and the KVM cpu id don't match. > > So let's start with a floating interrupt test that will trigger a > floating interrupt (via SCLP) to be delivered to a CPU in the wait state. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > s390x/Makefile | 1 + > s390x/firq.c | 141 ++++++++++++++++++++++++++++++++++++++++++++ > s390x/unittests.cfg | 10 ++++ > 3 files changed, 152 insertions(+) > create mode 100644 s390x/firq.c > > diff --git a/s390x/Makefile b/s390x/Makefile > index f95f2e6..1e567c1 100644 > --- a/s390x/Makefile > +++ b/s390x/Makefile > @@ -25,6 +25,7 @@ tests += $(TEST_DIR)/uv-host.elf > tests += $(TEST_DIR)/edat.elf > tests += $(TEST_DIR)/mvpg-sie.elf > tests += $(TEST_DIR)/spec_ex-sie.elf > +tests += $(TEST_DIR)/firq.elf > > tests_binary = $(patsubst %.elf,%.bin,$(tests)) > ifneq ($(HOST_KEY_DOCUMENT),) > diff --git a/s390x/firq.c b/s390x/firq.c > new file mode 100644 > index 0000000..3e60681 > --- /dev/null > +++ b/s390x/firq.c > @@ -0,0 +1,141 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Floating interrupt tests. > + * > + * Copyright 2021 Red Hat Inc > + * > + * Authors: > + * David Hildenbrand <david@redhat.com> > + */ > +#include <libcflat.h> > +#include <asm/asm-offsets.h> > +#include <asm/interrupt.h> > +#include <asm/page.h> > +#include <asm-generic/barrier.h> > + > +#include <sclp.h> > +#include <smp.h> > +#include <alloc_page.h> > + > +static int testflag = 0; > + > +static void wait_for_flag(void) > +{ > + while (!testflag) > + mb(); > +} > + > +static void set_flag(int val) > +{ > + mb(); > + testflag = val; > + mb(); > +} > + > +static void wait_for_sclp_int(void) > +{ > + /* Enable SCLP interrupts on this CPU only. */ > + ctl_set_bit(0, CTL0_SERVICE_SIGNAL); > + > + set_flag(1); why not just WRITE_ONCE/READ_ONCE? > + > + /* Enable external interrupts and go to the wait state. */ > + wait_for_interrupt(PSW_MASK_EXT); > +} > + > +/* > + * Some KVM versions might mix CPUs when looking for a floating IRQ target, > + * accidentially detecting a stopped CPU as waiting and resulting in the actually > + * waiting CPU not getting woken up for the interrupt. > + */ > +static void test_wait_state_delivery(void) > +{ > + struct psw psw; > + SCCBHeader *h; > + int ret; > + > + report_prefix_push("wait state delivery"); > + > + if (smp_query_num_cpus() < 3) { > + report_skip("need at least 3 CPUs for this test"); > + goto out; > + } > + > + if (stap()) { > + report_skip("need to start on CPU #0"); > + goto out; > + } > + > + /* > + * We want CPU #2 to be stopped. This should be the case at this > + * point, however, we want to sense if it even exists as well. > + */ > + ret = smp_cpu_stop(2); > + if (ret) { > + report_skip("CPU #2 not found"); > + goto out; > + } > + > + /* > + * We're going to perform an SCLP service call but expect > + * the interrupt on CPU #1 while it is in the wait state. > + */ > + sclp_mark_busy(); this means that now no SCLP command can happen as long as the flag stays set.... > + set_flag(0); > + > + /* Start CPU #1 and let it wait for the interrupt. */ > + psw.mask = extract_psw_mask(); > + psw.addr = (unsigned long)wait_for_sclp_int; > + ret = smp_cpu_setup(1, psw); > + if (ret) { > + report_skip("cpu #1 not found"); ...which means that this will hang, and so will all the other report* functions. maybe you should manually unset the flag before calling the various report* functions. > + goto out; > + } > + > + /* Wait until the CPU #1 at least enabled SCLP interrupts. */ > + wait_for_flag(); > + > + /* > + * We'd have to jump trough some hoops to sense e.g., via SIGP > + * CONDITIONAL EMERGENCY SIGNAL if CPU #1 is already in the > + * wait state. > + * > + * Although not completely reliable, use SIGP SENSE RUNNING STATUS > + * until not reported as running -- after all, our SCLP processing > + * will take some time as well and make races very rare. > + */ > + while(smp_sense_running_status(1)); > + > + h = alloc_page(); do you really need to dynamically allocate one page? is there a reason for not using a simple static buffer? (which you can have aligned and statically initialized) > + memset(h, 0, sizeof(*h)); otherwise, if you really want to allocate the memory, get rid of the memset; the allocator always returns zeroed memory (unless you explicitly ask not to by using flags) > + h->length = 4096; > + ret = servc(SCLP_CMDW_READ_CPU_INFO, __pa(h)); > + if (ret) { > + report_fail("SCLP_CMDW_READ_CPU_INFO failed"); > + goto out_destroy; > + } > + > + /* > + * Wait until the interrupt gets delivered on CPU #1, marking the why do you expect the interrupt to be delivered on CPU1? could it not be delivered on CPU0? > + * SCLP requests as done. > + */ > + sclp_wait_busy(); this is logically not wrong (and should stay, because it makes clear what you are trying to do), but strictly speaking it's not needed since the report below will hang as long as the SCLP busy flag is set. > + > + report(true, "firq delivered"); > + > +out_destroy: > + smp_cpu_destroy(1); > + free_page(h); > +out: > + report_prefix_pop(); > +} > + > +int main(void) > +{ > + report_prefix_push("firq"); > + > + test_wait_state_delivery(); > + > + report_prefix_pop(); > + return report_summary(); > +} > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg > index 3b454b7..054560c 100644 > --- a/s390x/unittests.cfg > +++ b/s390x/unittests.cfg > @@ -112,3 +112,13 @@ file = mvpg-sie.elf > > [spec_ex-sie] > file = spec_ex-sie.elf > + > +[firq-linear-cpu-ids] > +file = firq.elf > +timeout = 20 > +extra_params = -smp 1,maxcpus=3 -cpu qemu -device qemu-s390x-cpu,core-id=1 -device qemu-s390x-cpu,core-id=2 > + > +[firq-nonlinear-cpu-ids] > +file = firq.elf > +timeout = 20 > +extra_params = -smp 1,maxcpus=3 -cpu qemu -device qemu-s390x-cpu,core-id=2 -device qemu-s390x-cpu,core-id=1
>> +static void wait_for_sclp_int(void) >> +{ >> + /* Enable SCLP interrupts on this CPU only. */ >> + ctl_set_bit(0, CTL0_SERVICE_SIGNAL); >> + >> + set_flag(1); > > why not just WRITE_ONCE/READ_ONCE? Because I shamelessly copied that from s390x/smp.c ;) >> + set_flag(0); >> + >> + /* Start CPU #1 and let it wait for the interrupt. */ >> + psw.mask = extract_psw_mask(); >> + psw.addr = (unsigned long)wait_for_sclp_int; >> + ret = smp_cpu_setup(1, psw); >> + if (ret) { >> + report_skip("cpu #1 not found"); > > ...which means that this will hang, and so will all the other report* > functions. maybe you should manually unset the flag before calling the > various report* functions. Good point, thanks! > >> + goto out; >> + } >> + >> + /* Wait until the CPU #1 at least enabled SCLP interrupts. */ >> + wait_for_flag(); >> + >> + /* >> + * We'd have to jump trough some hoops to sense e.g., via SIGP >> + * CONDITIONAL EMERGENCY SIGNAL if CPU #1 is already in the >> + * wait state. >> + * >> + * Although not completely reliable, use SIGP SENSE RUNNING STATUS >> + * until not reported as running -- after all, our SCLP processing >> + * will take some time as well and make races very rare. >> + */ >> + while(smp_sense_running_status(1)); >> + >> + h = alloc_page(); > > do you really need to dynamically allocate one page? > is there a reason for not using a simple static buffer? (which you can > have aligned and statically initialized) I don't really have a strong opinion. I do prefer dynamic alloctions, though, if there isn't a good reason not to use them. No need to mess with page alignments manually. > >> + memset(h, 0, sizeof(*h)); > > otherwise, if you really want to allocate the memory, get rid of the > memset; the allocator always returns zeroed memory (unless you > explicitly ask not to by using flags) Right. "special" FLAG_DONTZERO in that semantics in that allocator. > >> + h->length = 4096; >> + ret = servc(SCLP_CMDW_READ_CPU_INFO, __pa(h)); >> + if (ret) { >> + report_fail("SCLP_CMDW_READ_CPU_INFO failed"); >> + goto out_destroy; >> + } >> + >> + /* >> + * Wait until the interrupt gets delivered on CPU #1, marking the > > why do you expect the interrupt to be delivered on CPU1? could it not > be delivered on CPU0? We don't enable SCLP interrupts + external interrupts on CPU #0 because we'll only call sclp_setup_int() on CPU #1. > >> + * SCLP requests as done. >> + */ >> + sclp_wait_busy(); > > this is logically not wrong (and should stay, because it makes clear > what you are trying to do), but strictly speaking it's not needed since > the report below will hang as long as the SCLP busy flag is set. Right. But it's really clearer to just have this in the code.
On 02.12.21 12:13, David Hildenbrand wrote: >>> +static void wait_for_sclp_int(void) >>> +{ >>> + /* Enable SCLP interrupts on this CPU only. */ >>> + ctl_set_bit(0, CTL0_SERVICE_SIGNAL); >>> + >>> + set_flag(1); >> >> why not just WRITE_ONCE/READ_ONCE? > > Because I shamelessly copied that from s390x/smp.c ;) Oh, and also because WRITE_ONCE/READ_ONCE are semantically the wrong thing to use: "Prevent the compiler from merging or refetching reads or writes. The compiler is also forbidden from reordering successive instances of READ_ONCE and WRITE_ONCE" We need memory barriers to prevent reordering with surrounding code.
On Thu, 2 Dec 2021 12:13:08 +0100 David Hildenbrand <david@redhat.com> wrote: > >> +static void wait_for_sclp_int(void) > >> +{ > >> + /* Enable SCLP interrupts on this CPU only. */ > >> + ctl_set_bit(0, CTL0_SERVICE_SIGNAL); > >> + > >> + set_flag(1); > > > > why not just WRITE_ONCE/READ_ONCE? > > Because I shamelessly copied that from s390x/smp.c ;) > > >> + set_flag(0); > >> + > >> + /* Start CPU #1 and let it wait for the interrupt. */ > >> + psw.mask = extract_psw_mask(); > >> + psw.addr = (unsigned long)wait_for_sclp_int; > >> + ret = smp_cpu_setup(1, psw); > >> + if (ret) { > >> + report_skip("cpu #1 not found"); > > > > ...which means that this will hang, and so will all the other report* > > functions. maybe you should manually unset the flag before calling the > > various report* functions. > > Good point, thanks! > > > > >> + goto out; > >> + } > >> + > >> + /* Wait until the CPU #1 at least enabled SCLP interrupts. */ > >> + wait_for_flag(); > >> + > >> + /* > >> + * We'd have to jump trough some hoops to sense e.g., via SIGP > >> + * CONDITIONAL EMERGENCY SIGNAL if CPU #1 is already in the > >> + * wait state. > >> + * > >> + * Although not completely reliable, use SIGP SENSE RUNNING STATUS > >> + * until not reported as running -- after all, our SCLP processing > >> + * will take some time as well and make races very rare. > >> + */ > >> + while(smp_sense_running_status(1)); if you wait here for CPU1 to be in wait state, then why did you need to wait until it has set the flag earlier? can't you just wait here and not use the whole wait_for_flag logic? smp_cpu_setup only returns after the new CPU has started running. I guess this was also inspired by smp.c :) > >> + > >> + h = alloc_page(); > > > > do you really need to dynamically allocate one page? > > is there a reason for not using a simple static buffer? (which you can > > have aligned and statically initialized) > > I don't really have a strong opinion. I do prefer dynamic alloctions, > though, if there isn't a good reason not to use them. No need to mess > with page alignments manually. fair enough, I also do not have a strong opinion > > > >> + memset(h, 0, sizeof(*h)); > > > > otherwise, if you really want to allocate the memory, get rid of the > > memset; the allocator always returns zeroed memory (unless you > > explicitly ask not to by using flags) > > Right. "special" FLAG_DONTZERO in that semantics in that allocator. > > > > >> + h->length = 4096; > >> + ret = servc(SCLP_CMDW_READ_CPU_INFO, __pa(h)); > >> + if (ret) { > >> + report_fail("SCLP_CMDW_READ_CPU_INFO failed"); > >> + goto out_destroy; > >> + } > >> + > >> + /* > >> + * Wait until the interrupt gets delivered on CPU #1, marking the > > > > why do you expect the interrupt to be delivered on CPU1? could it not > > be delivered on CPU0? > > We don't enable SCLP interrupts + external interrupts on CPU #0 because > we'll only call sclp_setup_int() on CPU #1. oh right, I had missed that > > > > >> + * SCLP requests as done. > >> + */ > >> + sclp_wait_busy(); > > > > this is logically not wrong (and should stay, because it makes clear > > what you are trying to do), but strictly speaking it's not needed since > > the report below will hang as long as the SCLP busy flag is set. > > Right. But it's really clearer to just have this in the code. yep, which is why I said that this should stay :) > >
On 02.12.21 13:07, Claudio Imbrenda wrote: > On Thu, 2 Dec 2021 12:13:08 +0100 > David Hildenbrand <david@redhat.com> wrote: > >>>> +static void wait_for_sclp_int(void) >>>> +{ >>>> + /* Enable SCLP interrupts on this CPU only. */ >>>> + ctl_set_bit(0, CTL0_SERVICE_SIGNAL); >>>> + >>>> + set_flag(1); >>> >>> why not just WRITE_ONCE/READ_ONCE? >> >> Because I shamelessly copied that from s390x/smp.c ;) >> >>>> + set_flag(0); >>>> + >>>> + /* Start CPU #1 and let it wait for the interrupt. */ >>>> + psw.mask = extract_psw_mask(); >>>> + psw.addr = (unsigned long)wait_for_sclp_int; >>>> + ret = smp_cpu_setup(1, psw); >>>> + if (ret) { >>>> + report_skip("cpu #1 not found"); >>> >>> ...which means that this will hang, and so will all the other report* >>> functions. maybe you should manually unset the flag before calling the >>> various report* functions. >> >> Good point, thanks! >> >>> >>>> + goto out; >>>> + } >>>> + >>>> + /* Wait until the CPU #1 at least enabled SCLP interrupts. */ >>>> + wait_for_flag(); >>>> + >>>> + /* >>>> + * We'd have to jump trough some hoops to sense e.g., via SIGP >>>> + * CONDITIONAL EMERGENCY SIGNAL if CPU #1 is already in the >>>> + * wait state. >>>> + * >>>> + * Although not completely reliable, use SIGP SENSE RUNNING STATUS >>>> + * until not reported as running -- after all, our SCLP processing >>>> + * will take some time as well and make races very rare. >>>> + */ >>>> + while(smp_sense_running_status(1)); > > if you wait here for CPU1 to be in wait state, then why did you need to > wait until it has set the flag earlier? can't you just wait here and not > use the whole wait_for_flag logic? smp_cpu_setup only returns after the > new CPU has started running. I use the flag right now as a mechanism to make the race window as short as possible. But you're right, we might not need the flag logic as knowing that we processed the setup part and will jump/jumped to the target code might be good enough. Thanks!
diff --git a/s390x/Makefile b/s390x/Makefile index f95f2e6..1e567c1 100644 --- a/s390x/Makefile +++ b/s390x/Makefile @@ -25,6 +25,7 @@ tests += $(TEST_DIR)/uv-host.elf tests += $(TEST_DIR)/edat.elf tests += $(TEST_DIR)/mvpg-sie.elf tests += $(TEST_DIR)/spec_ex-sie.elf +tests += $(TEST_DIR)/firq.elf tests_binary = $(patsubst %.elf,%.bin,$(tests)) ifneq ($(HOST_KEY_DOCUMENT),) diff --git a/s390x/firq.c b/s390x/firq.c new file mode 100644 index 0000000..3e60681 --- /dev/null +++ b/s390x/firq.c @@ -0,0 +1,141 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Floating interrupt tests. + * + * Copyright 2021 Red Hat Inc + * + * Authors: + * David Hildenbrand <david@redhat.com> + */ +#include <libcflat.h> +#include <asm/asm-offsets.h> +#include <asm/interrupt.h> +#include <asm/page.h> +#include <asm-generic/barrier.h> + +#include <sclp.h> +#include <smp.h> +#include <alloc_page.h> + +static int testflag = 0; + +static void wait_for_flag(void) +{ + while (!testflag) + mb(); +} + +static void set_flag(int val) +{ + mb(); + testflag = val; + mb(); +} + +static void wait_for_sclp_int(void) +{ + /* Enable SCLP interrupts on this CPU only. */ + ctl_set_bit(0, CTL0_SERVICE_SIGNAL); + + set_flag(1); + + /* Enable external interrupts and go to the wait state. */ + wait_for_interrupt(PSW_MASK_EXT); +} + +/* + * Some KVM versions might mix CPUs when looking for a floating IRQ target, + * accidentially detecting a stopped CPU as waiting and resulting in the actually + * waiting CPU not getting woken up for the interrupt. + */ +static void test_wait_state_delivery(void) +{ + struct psw psw; + SCCBHeader *h; + int ret; + + report_prefix_push("wait state delivery"); + + if (smp_query_num_cpus() < 3) { + report_skip("need at least 3 CPUs for this test"); + goto out; + } + + if (stap()) { + report_skip("need to start on CPU #0"); + goto out; + } + + /* + * We want CPU #2 to be stopped. This should be the case at this + * point, however, we want to sense if it even exists as well. + */ + ret = smp_cpu_stop(2); + if (ret) { + report_skip("CPU #2 not found"); + goto out; + } + + /* + * We're going to perform an SCLP service call but expect + * the interrupt on CPU #1 while it is in the wait state. + */ + sclp_mark_busy(); + set_flag(0); + + /* Start CPU #1 and let it wait for the interrupt. */ + psw.mask = extract_psw_mask(); + psw.addr = (unsigned long)wait_for_sclp_int; + ret = smp_cpu_setup(1, psw); + if (ret) { + report_skip("cpu #1 not found"); + goto out; + } + + /* Wait until the CPU #1 at least enabled SCLP interrupts. */ + wait_for_flag(); + + /* + * We'd have to jump trough some hoops to sense e.g., via SIGP + * CONDITIONAL EMERGENCY SIGNAL if CPU #1 is already in the + * wait state. + * + * Although not completely reliable, use SIGP SENSE RUNNING STATUS + * until not reported as running -- after all, our SCLP processing + * will take some time as well and make races very rare. + */ + while(smp_sense_running_status(1)); + + h = alloc_page(); + memset(h, 0, sizeof(*h)); + h->length = 4096; + ret = servc(SCLP_CMDW_READ_CPU_INFO, __pa(h)); + if (ret) { + report_fail("SCLP_CMDW_READ_CPU_INFO failed"); + goto out_destroy; + } + + /* + * Wait until the interrupt gets delivered on CPU #1, marking the + * SCLP requests as done. + */ + sclp_wait_busy(); + + report(true, "firq delivered"); + +out_destroy: + smp_cpu_destroy(1); + free_page(h); +out: + report_prefix_pop(); +} + +int main(void) +{ + report_prefix_push("firq"); + + test_wait_state_delivery(); + + report_prefix_pop(); + return report_summary(); +} diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg index 3b454b7..054560c 100644 --- a/s390x/unittests.cfg +++ b/s390x/unittests.cfg @@ -112,3 +112,13 @@ file = mvpg-sie.elf [spec_ex-sie] file = spec_ex-sie.elf + +[firq-linear-cpu-ids] +file = firq.elf +timeout = 20 +extra_params = -smp 1,maxcpus=3 -cpu qemu -device qemu-s390x-cpu,core-id=1 -device qemu-s390x-cpu,core-id=2 + +[firq-nonlinear-cpu-ids] +file = firq.elf +timeout = 20 +extra_params = -smp 1,maxcpus=3 -cpu qemu -device qemu-s390x-cpu,core-id=2 -device qemu-s390x-cpu,core-id=1
We had a KVM BUG fixed by kernel commit a3e03bc1368c ("KVM: s390: index kvm->arch.idle_mask by vcpu_idx"), whereby a floating interrupt might get stuck forever because a CPU in the wait state would not get woken up. The issue can be triggered when CPUs are created in a nonlinear fashion, such that the CPU address ("core-id") and the KVM cpu id don't match. So let's start with a floating interrupt test that will trigger a floating interrupt (via SCLP) to be delivered to a CPU in the wait state. Signed-off-by: David Hildenbrand <david@redhat.com> --- s390x/Makefile | 1 + s390x/firq.c | 141 ++++++++++++++++++++++++++++++++++++++++++++ s390x/unittests.cfg | 10 ++++ 3 files changed, 152 insertions(+) create mode 100644 s390x/firq.c