| Message ID | 20200324081251.28810-5-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: smp: Improve smp code part 2 | expand |
On Tue, 24 Mar 2020 04:12:45 -0400 Janosch Frank <frankja@linux.ibm.com> wrote: > Local interrupts (external and emergency call) should be cleared after > any cpu reset. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > s390x/smp.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 24.03.20 09:12, Janosch Frank wrote: > Local interrupts (external and emergency call) should be cleared after > any cpu reset. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > s390x/smp.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/s390x/smp.c b/s390x/smp.c > index 8a6cd1d8b17d76c6..a8e3dd7aac0c788c 100644 > --- a/s390x/smp.c > +++ b/s390x/smp.c > @@ -243,6 +243,20 @@ static void test_reset_initial(void) > report_prefix_pop(); > } > > +static void test_local_ints(void) > +{ > + unsigned long mask; > + > + expect_ext_int(); > + /* Open masks for ecall and emcall */ > + ctl_set_bit(0, 13); > + ctl_set_bit(0, 14); > + mask = extract_psw_mask(); > + mask |= PSW_MASK_EXT; > + load_psw_mask(mask); > + set_flag(1); > +} > + > static void test_reset(void) > { > struct psw psw; > @@ -251,10 +265,18 @@ static void test_reset(void) > psw.addr = (unsigned long)test_func; > > report_prefix_push("cpu reset"); > + sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL); > + sigp(1, SIGP_EXTERNAL_CALL, 0, NULL); > smp_cpu_start(1, psw); > > sigp_retry(1, SIGP_CPU_RESET, 0, NULL); > report(smp_cpu_stopped(1), "cpu stopped"); > + > + set_flag(0); > + psw.addr = (unsigned long)test_local_ints; > + smp_cpu_start(1, psw); > + wait_for_flag(); > + report(true, "local interrupts cleared"); How can you be sure they were actually cleared/delivered?
On 3/31/20 11:07 AM, David Hildenbrand wrote: > On 24.03.20 09:12, Janosch Frank wrote: >> Local interrupts (external and emergency call) should be cleared after >> any cpu reset. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> s390x/smp.c | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/s390x/smp.c b/s390x/smp.c >> index 8a6cd1d8b17d76c6..a8e3dd7aac0c788c 100644 >> --- a/s390x/smp.c >> +++ b/s390x/smp.c >> @@ -243,6 +243,20 @@ static void test_reset_initial(void) >> report_prefix_pop(); >> } >> >> +static void test_local_ints(void) >> +{ >> + unsigned long mask; >> + >> + expect_ext_int(); >> + /* Open masks for ecall and emcall */ >> + ctl_set_bit(0, 13); >> + ctl_set_bit(0, 14); >> + mask = extract_psw_mask(); >> + mask |= PSW_MASK_EXT; >> + load_psw_mask(mask); >> + set_flag(1); >> +} >> + >> static void test_reset(void) >> { >> struct psw psw; >> @@ -251,10 +265,18 @@ static void test_reset(void) >> psw.addr = (unsigned long)test_func; >> >> report_prefix_push("cpu reset"); >> + sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL); >> + sigp(1, SIGP_EXTERNAL_CALL, 0, NULL); >> smp_cpu_start(1, psw); >> >> sigp_retry(1, SIGP_CPU_RESET, 0, NULL); >> report(smp_cpu_stopped(1), "cpu stopped"); >> + >> + set_flag(0); >> + psw.addr = (unsigned long)test_local_ints; >> + smp_cpu_start(1, psw); >> + wait_for_flag(); >> + report(true, "local interrupts cleared"); > > > How can you be sure they were actually cleared/delivered? > Because cpu 1 would get a ext int it didn't expect and would do a report_abort() as pecified in lib/s390x/interrupt.c
On 31.03.20 11:28, Janosch Frank wrote: > On 3/31/20 11:07 AM, David Hildenbrand wrote: >> On 24.03.20 09:12, Janosch Frank wrote: >>> Local interrupts (external and emergency call) should be cleared after >>> any cpu reset. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> --- >>> s390x/smp.c | 22 ++++++++++++++++++++++ >>> 1 file changed, 22 insertions(+) >>> >>> diff --git a/s390x/smp.c b/s390x/smp.c >>> index 8a6cd1d8b17d76c6..a8e3dd7aac0c788c 100644 >>> --- a/s390x/smp.c >>> +++ b/s390x/smp.c >>> @@ -243,6 +243,20 @@ static void test_reset_initial(void) >>> report_prefix_pop(); >>> } >>> >>> +static void test_local_ints(void) >>> +{ >>> + unsigned long mask; >>> + >>> + expect_ext_int(); >>> + /* Open masks for ecall and emcall */ >>> + ctl_set_bit(0, 13); >>> + ctl_set_bit(0, 14); >>> + mask = extract_psw_mask(); >>> + mask |= PSW_MASK_EXT; >>> + load_psw_mask(mask); >>> + set_flag(1); >>> +} >>> + >>> static void test_reset(void) >>> { >>> struct psw psw; >>> @@ -251,10 +265,18 @@ static void test_reset(void) >>> psw.addr = (unsigned long)test_func; >>> >>> report_prefix_push("cpu reset"); >>> + sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL); >>> + sigp(1, SIGP_EXTERNAL_CALL, 0, NULL); >>> smp_cpu_start(1, psw); >>> >>> sigp_retry(1, SIGP_CPU_RESET, 0, NULL); >>> report(smp_cpu_stopped(1), "cpu stopped"); >>> + >>> + set_flag(0); >>> + psw.addr = (unsigned long)test_local_ints; >>> + smp_cpu_start(1, psw); >>> + wait_for_flag(); >>> + report(true, "local interrupts cleared"); >> >> >> How can you be sure they were actually cleared/delivered? >> > Because cpu 1 would get a ext int it didn't expect and would do a > report_abort() as pecified in lib/s390x/interrupt.c But what if it *didn't* get the interrupts delivered. Then cpu 1 will simply (test_local_ints()) unlock interrupts, load the psw mask, set the flag and be done with it. What am I missing? How can you be sure the interrupts on cpu 1 were actually delivered?
On 3/31/20 7:27 PM, David Hildenbrand wrote: > On 31.03.20 11:28, Janosch Frank wrote: >> On 3/31/20 11:07 AM, David Hildenbrand wrote: >>> On 24.03.20 09:12, Janosch Frank wrote: >>>> Local interrupts (external and emergency call) should be cleared after >>>> any cpu reset. >>>> >>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>> --- >>>> s390x/smp.c | 22 ++++++++++++++++++++++ >>>> 1 file changed, 22 insertions(+) >>>> >>>> diff --git a/s390x/smp.c b/s390x/smp.c >>>> index 8a6cd1d8b17d76c6..a8e3dd7aac0c788c 100644 >>>> --- a/s390x/smp.c >>>> +++ b/s390x/smp.c >>>> @@ -243,6 +243,20 @@ static void test_reset_initial(void) >>>> report_prefix_pop(); >>>> } >>>> >>>> +static void test_local_ints(void) >>>> +{ >>>> + unsigned long mask; >>>> + >>>> + expect_ext_int(); >>>> + /* Open masks for ecall and emcall */ >>>> + ctl_set_bit(0, 13); >>>> + ctl_set_bit(0, 14); >>>> + mask = extract_psw_mask(); >>>> + mask |= PSW_MASK_EXT; >>>> + load_psw_mask(mask); >>>> + set_flag(1); >>>> +} >>>> + >>>> static void test_reset(void) >>>> { >>>> struct psw psw; >>>> @@ -251,10 +265,18 @@ static void test_reset(void) >>>> psw.addr = (unsigned long)test_func; >>>> >>>> report_prefix_push("cpu reset"); >>>> + sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL); >>>> + sigp(1, SIGP_EXTERNAL_CALL, 0, NULL); >>>> smp_cpu_start(1, psw); >>>> >>>> sigp_retry(1, SIGP_CPU_RESET, 0, NULL); >>>> report(smp_cpu_stopped(1), "cpu stopped"); >>>> + >>>> + set_flag(0); >>>> + psw.addr = (unsigned long)test_local_ints; >>>> + smp_cpu_start(1, psw); >>>> + wait_for_flag(); >>>> + report(true, "local interrupts cleared"); >>> >>> >>> How can you be sure they were actually cleared/delivered? >>> >> Because cpu 1 would get a ext int it didn't expect and would do a >> report_abort() as pecified in lib/s390x/interrupt.c > > But what if it *didn't* get the interrupts delivered. Well, the target is still looping until the reset initial test is executed and from personal experience I can say that this test bites rather fast. We could execute an instruction with a mandatory exit which will prompt KVM to inject any remaining IRQs before setting the flag to 1. Unfortunately we do not have a polling instruction for non-IO interrupts at the moment to verify this test. > > Then cpu 1 will simply (test_local_ints()) unlock interrupts, load the > psw mask, set the flag and be done with it. What am I missing? How can > you be sure the interrupts on cpu 1 were actually delivered? >
On 01.04.20 09:19, Janosch Frank wrote: > On 3/31/20 7:27 PM, David Hildenbrand wrote: >> On 31.03.20 11:28, Janosch Frank wrote: >>> On 3/31/20 11:07 AM, David Hildenbrand wrote: >>>> On 24.03.20 09:12, Janosch Frank wrote: >>>>> Local interrupts (external and emergency call) should be cleared after >>>>> any cpu reset. >>>>> >>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>>> --- >>>>> s390x/smp.c | 22 ++++++++++++++++++++++ >>>>> 1 file changed, 22 insertions(+) >>>>> >>>>> diff --git a/s390x/smp.c b/s390x/smp.c >>>>> index 8a6cd1d8b17d76c6..a8e3dd7aac0c788c 100644 >>>>> --- a/s390x/smp.c >>>>> +++ b/s390x/smp.c >>>>> @@ -243,6 +243,20 @@ static void test_reset_initial(void) >>>>> report_prefix_pop(); >>>>> } >>>>> >>>>> +static void test_local_ints(void) >>>>> +{ >>>>> + unsigned long mask; >>>>> + >>>>> + expect_ext_int(); >>>>> + /* Open masks for ecall and emcall */ >>>>> + ctl_set_bit(0, 13); >>>>> + ctl_set_bit(0, 14); >>>>> + mask = extract_psw_mask(); >>>>> + mask |= PSW_MASK_EXT; >>>>> + load_psw_mask(mask); >>>>> + set_flag(1); >>>>> +} >>>>> + >>>>> static void test_reset(void) >>>>> { >>>>> struct psw psw; >>>>> @@ -251,10 +265,18 @@ static void test_reset(void) >>>>> psw.addr = (unsigned long)test_func; >>>>> >>>>> report_prefix_push("cpu reset"); >>>>> + sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL); >>>>> + sigp(1, SIGP_EXTERNAL_CALL, 0, NULL); >>>>> smp_cpu_start(1, psw); >>>>> >>>>> sigp_retry(1, SIGP_CPU_RESET, 0, NULL); >>>>> report(smp_cpu_stopped(1), "cpu stopped"); >>>>> + >>>>> + set_flag(0); >>>>> + psw.addr = (unsigned long)test_local_ints; >>>>> + smp_cpu_start(1, psw); >>>>> + wait_for_flag(); >>>>> + report(true, "local interrupts cleared"); >>>> >>>> >>>> How can you be sure they were actually cleared/delivered? >>>> >>> Because cpu 1 would get a ext int it didn't expect and would do a >>> report_abort() as pecified in lib/s390x/interrupt.c >> >> But what if it *didn't* get the interrupts delivered. > > Well, the target is still looping until the reset initial test is > executed and from personal experience I can say that this test bites > rather fast. > > We could execute an instruction with a mandatory exit which will prompt > KVM to inject any remaining IRQs before setting the flag to 1. > > Unfortunately we do not have a polling instruction for non-IO interrupts > at the moment to verify this test. I'd expect you should do something like this in your test instead static void test_local_ints(void) { unsigned long mask; /* Enable external interrupts */ mask = extract_psw_mask(); mask |= PSW_MASK_EXT; load_psw_mask(mask); expect_ext_int(); /* Open masks for external call */ ctl_set_bit(0, 13); if (lc->ext_int_code != $TODO1) return; expect_ext_int(); /* Open masks for emergency call */ ctl_set_bit(0, 14); if (lc->ext_int_code != $TODO2) return; set_flag(1); }
diff --git a/s390x/smp.c b/s390x/smp.c index 8a6cd1d8b17d76c6..a8e3dd7aac0c788c 100644 --- a/s390x/smp.c +++ b/s390x/smp.c @@ -243,6 +243,20 @@ static void test_reset_initial(void) report_prefix_pop(); } +static void test_local_ints(void) +{ + unsigned long mask; + + expect_ext_int(); + /* Open masks for ecall and emcall */ + ctl_set_bit(0, 13); + ctl_set_bit(0, 14); + mask = extract_psw_mask(); + mask |= PSW_MASK_EXT; + load_psw_mask(mask); + set_flag(1); +} + static void test_reset(void) { struct psw psw; @@ -251,10 +265,18 @@ static void test_reset(void) psw.addr = (unsigned long)test_func; report_prefix_push("cpu reset"); + sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL); + sigp(1, SIGP_EXTERNAL_CALL, 0, NULL); smp_cpu_start(1, psw); sigp_retry(1, SIGP_CPU_RESET, 0, NULL); report(smp_cpu_stopped(1), "cpu stopped"); + + set_flag(0); + psw.addr = (unsigned long)test_local_ints; + smp_cpu_start(1, psw); + wait_for_flag(); + report(true, "local interrupts cleared"); report_prefix_pop(); }
Local interrupts (external and emergency call) should be cleared after any cpu reset. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- s390x/smp.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)