| Message ID | 20200423091013.11587-9-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: smp: Improve smp code part 2 | expand |
On 23.04.20 11:10, Janosch Frank wrote: > Sigp orders are not necessarily finished when the processor finished > the sigp instruction. We need to poll if the order has been finished > before we continue. > > For (re)start and stop we already use sigp sense running and sigp > sense loops. But we still lack completion checks for stop and store > status, as well as the cpu resets. > > Let's add them. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > --- > lib/s390x/smp.c | 8 ++++++++ > lib/s390x/smp.h | 1 + > s390x/smp.c | 4 ++++ > 3 files changed, 13 insertions(+) > > diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c > index 6ef0335..2555bf4 100644 > --- a/lib/s390x/smp.c > +++ b/lib/s390x/smp.c > @@ -154,6 +154,14 @@ int smp_cpu_start(uint16_t addr, struct psw psw) > return rc; > } > > +void smp_cpu_wait_for_completion(uint16_t addr) > +{ > + uint32_t status; > + > + /* Loops when cc == 2, i.e. when the cpu is busy with a sigp order */ > + sigp_retry(1, SIGP_SENSE, 0, &status); > +} > + > int smp_cpu_destroy(uint16_t addr) > { > struct cpu *cpu; > diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h > index ce63a89..a8b98c0 100644 > --- a/lib/s390x/smp.h > +++ b/lib/s390x/smp.h > @@ -45,6 +45,7 @@ int smp_cpu_restart(uint16_t addr); > int smp_cpu_start(uint16_t addr, struct psw psw); > int smp_cpu_stop(uint16_t addr); > int smp_cpu_stop_store_status(uint16_t addr); > +void smp_cpu_wait_for_completion(uint16_t addr); > int smp_cpu_destroy(uint16_t addr); > int smp_cpu_setup(uint16_t addr, struct psw psw); > void smp_teardown(void); > diff --git a/s390x/smp.c b/s390x/smp.c > index 7462211..48321f4 100644 > --- a/s390x/smp.c > +++ b/s390x/smp.c > @@ -75,6 +75,7 @@ static void test_stop_store_status(void) > lc->prefix_sa = 0; > lc->grs_sa[15] = 0; > smp_cpu_stop_store_status(1); > + smp_cpu_wait_for_completion(1); > mb(); > report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); > report(lc->grs_sa[15], "stack"); > @@ -85,6 +86,7 @@ static void test_stop_store_status(void) > lc->prefix_sa = 0; > lc->grs_sa[15] = 0; > smp_cpu_stop_store_status(1); Just curious: Would it make sense to add that inside smp_cpu_stop_store_status() instead?
On 4/24/20 12:11 PM, David Hildenbrand wrote: > On 23.04.20 11:10, Janosch Frank wrote: >> Sigp orders are not necessarily finished when the processor finished >> the sigp instruction. We need to poll if the order has been finished >> before we continue. >> >> For (re)start and stop we already use sigp sense running and sigp >> sense loops. But we still lack completion checks for stop and store >> status, as well as the cpu resets. >> >> Let's add them. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >> --- >> lib/s390x/smp.c | 8 ++++++++ >> lib/s390x/smp.h | 1 + >> s390x/smp.c | 4 ++++ >> 3 files changed, 13 insertions(+) >> >> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c >> index 6ef0335..2555bf4 100644 >> --- a/lib/s390x/smp.c >> +++ b/lib/s390x/smp.c >> @@ -154,6 +154,14 @@ int smp_cpu_start(uint16_t addr, struct psw psw) >> return rc; >> } >> >> +void smp_cpu_wait_for_completion(uint16_t addr) >> +{ >> + uint32_t status; >> + >> + /* Loops when cc == 2, i.e. when the cpu is busy with a sigp order */ >> + sigp_retry(1, SIGP_SENSE, 0, &status); >> +} >> + >> int smp_cpu_destroy(uint16_t addr) >> { >> struct cpu *cpu; >> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h >> index ce63a89..a8b98c0 100644 >> --- a/lib/s390x/smp.h >> +++ b/lib/s390x/smp.h >> @@ -45,6 +45,7 @@ int smp_cpu_restart(uint16_t addr); >> int smp_cpu_start(uint16_t addr, struct psw psw); >> int smp_cpu_stop(uint16_t addr); >> int smp_cpu_stop_store_status(uint16_t addr); >> +void smp_cpu_wait_for_completion(uint16_t addr); >> int smp_cpu_destroy(uint16_t addr); >> int smp_cpu_setup(uint16_t addr, struct psw psw); >> void smp_teardown(void); >> diff --git a/s390x/smp.c b/s390x/smp.c >> index 7462211..48321f4 100644 >> --- a/s390x/smp.c >> +++ b/s390x/smp.c >> @@ -75,6 +75,7 @@ static void test_stop_store_status(void) >> lc->prefix_sa = 0; >> lc->grs_sa[15] = 0; >> smp_cpu_stop_store_status(1); >> + smp_cpu_wait_for_completion(1); >> mb(); >> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); >> report(lc->grs_sa[15], "stack"); >> @@ -85,6 +86,7 @@ static void test_stop_store_status(void) >> lc->prefix_sa = 0; >> lc->grs_sa[15] = 0; >> smp_cpu_stop_store_status(1); > > Just curious: Would it make sense to add that inside > smp_cpu_stop_store_status() instead? > I think so, we also wait for stop and start to finish, so why not for this order code.
On 4/24/20 1:40 PM, Janosch Frank wrote: > On 4/24/20 12:11 PM, David Hildenbrand wrote: >> On 23.04.20 11:10, Janosch Frank wrote: >>> Sigp orders are not necessarily finished when the processor finished >>> the sigp instruction. We need to poll if the order has been finished >>> before we continue. >>> >>> For (re)start and stop we already use sigp sense running and sigp >>> sense loops. But we still lack completion checks for stop and store >>> status, as well as the cpu resets. >>> >>> Let's add them. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >>> --- >>> lib/s390x/smp.c | 8 ++++++++ >>> lib/s390x/smp.h | 1 + >>> s390x/smp.c | 4 ++++ >>> 3 files changed, 13 insertions(+) >>> >>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c >>> index 6ef0335..2555bf4 100644 >>> --- a/lib/s390x/smp.c >>> +++ b/lib/s390x/smp.c >>> @@ -154,6 +154,14 @@ int smp_cpu_start(uint16_t addr, struct psw psw) >>> return rc; >>> } >>> >>> +void smp_cpu_wait_for_completion(uint16_t addr) >>> +{ >>> + uint32_t status; >>> + >>> + /* Loops when cc == 2, i.e. when the cpu is busy with a sigp order */ >>> + sigp_retry(1, SIGP_SENSE, 0, &status); >>> +} >>> + >>> int smp_cpu_destroy(uint16_t addr) >>> { >>> struct cpu *cpu; >>> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h >>> index ce63a89..a8b98c0 100644 >>> --- a/lib/s390x/smp.h >>> +++ b/lib/s390x/smp.h >>> @@ -45,6 +45,7 @@ int smp_cpu_restart(uint16_t addr); >>> int smp_cpu_start(uint16_t addr, struct psw psw); >>> int smp_cpu_stop(uint16_t addr); >>> int smp_cpu_stop_store_status(uint16_t addr); >>> +void smp_cpu_wait_for_completion(uint16_t addr); >>> int smp_cpu_destroy(uint16_t addr); >>> int smp_cpu_setup(uint16_t addr, struct psw psw); >>> void smp_teardown(void); >>> diff --git a/s390x/smp.c b/s390x/smp.c >>> index 7462211..48321f4 100644 >>> --- a/s390x/smp.c >>> +++ b/s390x/smp.c >>> @@ -75,6 +75,7 @@ static void test_stop_store_status(void) >>> lc->prefix_sa = 0; >>> lc->grs_sa[15] = 0; >>> smp_cpu_stop_store_status(1); >>> + smp_cpu_wait_for_completion(1); >>> mb(); >>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); >>> report(lc->grs_sa[15], "stack"); >>> @@ -85,6 +86,7 @@ static void test_stop_store_status(void) >>> lc->prefix_sa = 0; >>> lc->grs_sa[15] = 0; >>> smp_cpu_stop_store_status(1); >> >> Just curious: Would it make sense to add that inside >> smp_cpu_stop_store_status() instead? >> > > I think so, we also wait for stop and start to finish, so why not for > this order code. > I've moved the waiting into the smp library and now the prefix check for stop and store status fails every so often if executed repeatedly. I've tried making the lc ptr volatile, a print of the prefix before the report seems to fix the issue, a print after the report still shows the issue but according to the print both values are the same. I'm currently at a loss...
On 29.04.20 10:57, Janosch Frank wrote: > On 4/24/20 1:40 PM, Janosch Frank wrote: >> On 4/24/20 12:11 PM, David Hildenbrand wrote: >>> On 23.04.20 11:10, Janosch Frank wrote: >>>> Sigp orders are not necessarily finished when the processor finished >>>> the sigp instruction. We need to poll if the order has been finished >>>> before we continue. >>>> >>>> For (re)start and stop we already use sigp sense running and sigp >>>> sense loops. But we still lack completion checks for stop and store >>>> status, as well as the cpu resets. >>>> >>>> Let's add them. >>>> >>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >>>> --- >>>> lib/s390x/smp.c | 8 ++++++++ >>>> lib/s390x/smp.h | 1 + >>>> s390x/smp.c | 4 ++++ >>>> 3 files changed, 13 insertions(+) >>>> >>>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c >>>> index 6ef0335..2555bf4 100644 >>>> --- a/lib/s390x/smp.c >>>> +++ b/lib/s390x/smp.c >>>> @@ -154,6 +154,14 @@ int smp_cpu_start(uint16_t addr, struct psw psw) >>>> return rc; >>>> } >>>> >>>> +void smp_cpu_wait_for_completion(uint16_t addr) >>>> +{ >>>> + uint32_t status; >>>> + >>>> + /* Loops when cc == 2, i.e. when the cpu is busy with a sigp order */ >>>> + sigp_retry(1, SIGP_SENSE, 0, &status); >>>> +} >>>> + >>>> int smp_cpu_destroy(uint16_t addr) >>>> { >>>> struct cpu *cpu; >>>> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h >>>> index ce63a89..a8b98c0 100644 >>>> --- a/lib/s390x/smp.h >>>> +++ b/lib/s390x/smp.h >>>> @@ -45,6 +45,7 @@ int smp_cpu_restart(uint16_t addr); >>>> int smp_cpu_start(uint16_t addr, struct psw psw); >>>> int smp_cpu_stop(uint16_t addr); >>>> int smp_cpu_stop_store_status(uint16_t addr); >>>> +void smp_cpu_wait_for_completion(uint16_t addr); >>>> int smp_cpu_destroy(uint16_t addr); >>>> int smp_cpu_setup(uint16_t addr, struct psw psw); >>>> void smp_teardown(void); >>>> diff --git a/s390x/smp.c b/s390x/smp.c >>>> index 7462211..48321f4 100644 >>>> --- a/s390x/smp.c >>>> +++ b/s390x/smp.c >>>> @@ -75,6 +75,7 @@ static void test_stop_store_status(void) >>>> lc->prefix_sa = 0; >>>> lc->grs_sa[15] = 0; >>>> smp_cpu_stop_store_status(1); >>>> + smp_cpu_wait_for_completion(1); >>>> mb(); >>>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); >>>> report(lc->grs_sa[15], "stack"); >>>> @@ -85,6 +86,7 @@ static void test_stop_store_status(void) >>>> lc->prefix_sa = 0; >>>> lc->grs_sa[15] = 0; >>>> smp_cpu_stop_store_status(1); >>> >>> Just curious: Would it make sense to add that inside >>> smp_cpu_stop_store_status() instead? >>> >> >> I think so, we also wait for stop and start to finish, so why not for >> this order code. >> > > I've moved the waiting into the smp library and now the prefix check for > stop and store status fails every so often if executed repeatedly. > > I've tried making the lc ptr volatile, a print of the prefix before the > report seems to fix the issue, a print after the report still shows the > issue but according to the print both values are the same. > > I'm currently at a loss... Are you missing a barrier() somewhere?
On 4/29/20 11:06 AM, David Hildenbrand wrote: > On 29.04.20 10:57, Janosch Frank wrote: >> On 4/24/20 1:40 PM, Janosch Frank wrote: >>> On 4/24/20 12:11 PM, David Hildenbrand wrote: >>>> On 23.04.20 11:10, Janosch Frank wrote: >>>>> Sigp orders are not necessarily finished when the processor finished >>>>> the sigp instruction. We need to poll if the order has been finished >>>>> before we continue. >>>>> >>>>> For (re)start and stop we already use sigp sense running and sigp >>>>> sense loops. But we still lack completion checks for stop and store >>>>> status, as well as the cpu resets. >>>>> >>>>> Let's add them. >>>>> >>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >>>>> --- >>>>> lib/s390x/smp.c | 8 ++++++++ >>>>> lib/s390x/smp.h | 1 + >>>>> s390x/smp.c | 4 ++++ >>>>> 3 files changed, 13 insertions(+) >>>>> >>>>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c >>>>> index 6ef0335..2555bf4 100644 >>>>> --- a/lib/s390x/smp.c >>>>> +++ b/lib/s390x/smp.c >>>>> @@ -154,6 +154,14 @@ int smp_cpu_start(uint16_t addr, struct psw psw) >>>>> return rc; >>>>> } >>>>> >>>>> +void smp_cpu_wait_for_completion(uint16_t addr) >>>>> +{ >>>>> + uint32_t status; >>>>> + >>>>> + /* Loops when cc == 2, i.e. when the cpu is busy with a sigp order */ >>>>> + sigp_retry(1, SIGP_SENSE, 0, &status); >>>>> +} >>>>> + >>>>> int smp_cpu_destroy(uint16_t addr) >>>>> { >>>>> struct cpu *cpu; >>>>> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h >>>>> index ce63a89..a8b98c0 100644 >>>>> --- a/lib/s390x/smp.h >>>>> +++ b/lib/s390x/smp.h >>>>> @@ -45,6 +45,7 @@ int smp_cpu_restart(uint16_t addr); >>>>> int smp_cpu_start(uint16_t addr, struct psw psw); >>>>> int smp_cpu_stop(uint16_t addr); >>>>> int smp_cpu_stop_store_status(uint16_t addr); >>>>> +void smp_cpu_wait_for_completion(uint16_t addr); >>>>> int smp_cpu_destroy(uint16_t addr); >>>>> int smp_cpu_setup(uint16_t addr, struct psw psw); >>>>> void smp_teardown(void); >>>>> diff --git a/s390x/smp.c b/s390x/smp.c >>>>> index 7462211..48321f4 100644 >>>>> --- a/s390x/smp.c >>>>> +++ b/s390x/smp.c >>>>> @@ -75,6 +75,7 @@ static void test_stop_store_status(void) >>>>> lc->prefix_sa = 0; >>>>> lc->grs_sa[15] = 0; >>>>> smp_cpu_stop_store_status(1); >>>>> + smp_cpu_wait_for_completion(1); >>>>> mb(); >>>>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); >>>>> report(lc->grs_sa[15], "stack"); >>>>> @@ -85,6 +86,7 @@ static void test_stop_store_status(void) >>>>> lc->prefix_sa = 0; >>>>> lc->grs_sa[15] = 0; >>>>> smp_cpu_stop_store_status(1); >>>> >>>> Just curious: Would it make sense to add that inside >>>> smp_cpu_stop_store_status() instead? >>>> >>> >>> I think so, we also wait for stop and start to finish, so why not for >>> this order code. >>> >> >> I've moved the waiting into the smp library and now the prefix check for >> stop and store status fails every so often if executed repeatedly. >> >> I've tried making the lc ptr volatile, a print of the prefix before the >> report seems to fix the issue, a print after the report still shows the >> issue but according to the print both values are the same. >> >> I'm currently at a loss... > > Are you missing a barrier() somewhere? > Maybe, but the question is where? There's already one before the report: smp_cpu_stop_store_status(1); mb(); report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
On 29.04.20 11:37, Janosch Frank wrote: > On 4/29/20 11:06 AM, David Hildenbrand wrote: >> On 29.04.20 10:57, Janosch Frank wrote: >>> On 4/24/20 1:40 PM, Janosch Frank wrote: >>>> On 4/24/20 12:11 PM, David Hildenbrand wrote: >>>>> On 23.04.20 11:10, Janosch Frank wrote: >>>>>> Sigp orders are not necessarily finished when the processor finished >>>>>> the sigp instruction. We need to poll if the order has been finished >>>>>> before we continue. >>>>>> >>>>>> For (re)start and stop we already use sigp sense running and sigp >>>>>> sense loops. But we still lack completion checks for stop and store >>>>>> status, as well as the cpu resets. >>>>>> >>>>>> Let's add them. >>>>>> >>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >>>>>> --- >>>>>> lib/s390x/smp.c | 8 ++++++++ >>>>>> lib/s390x/smp.h | 1 + >>>>>> s390x/smp.c | 4 ++++ >>>>>> 3 files changed, 13 insertions(+) >>>>>> >>>>>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c >>>>>> index 6ef0335..2555bf4 100644 >>>>>> --- a/lib/s390x/smp.c >>>>>> +++ b/lib/s390x/smp.c >>>>>> @@ -154,6 +154,14 @@ int smp_cpu_start(uint16_t addr, struct psw psw) >>>>>> return rc; >>>>>> } >>>>>> >>>>>> +void smp_cpu_wait_for_completion(uint16_t addr) >>>>>> +{ >>>>>> + uint32_t status; >>>>>> + >>>>>> + /* Loops when cc == 2, i.e. when the cpu is busy with a sigp order */ >>>>>> + sigp_retry(1, SIGP_SENSE, 0, &status); >>>>>> +} >>>>>> + >>>>>> int smp_cpu_destroy(uint16_t addr) >>>>>> { >>>>>> struct cpu *cpu; >>>>>> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h >>>>>> index ce63a89..a8b98c0 100644 >>>>>> --- a/lib/s390x/smp.h >>>>>> +++ b/lib/s390x/smp.h >>>>>> @@ -45,6 +45,7 @@ int smp_cpu_restart(uint16_t addr); >>>>>> int smp_cpu_start(uint16_t addr, struct psw psw); >>>>>> int smp_cpu_stop(uint16_t addr); >>>>>> int smp_cpu_stop_store_status(uint16_t addr); >>>>>> +void smp_cpu_wait_for_completion(uint16_t addr); >>>>>> int smp_cpu_destroy(uint16_t addr); >>>>>> int smp_cpu_setup(uint16_t addr, struct psw psw); >>>>>> void smp_teardown(void); >>>>>> diff --git a/s390x/smp.c b/s390x/smp.c >>>>>> index 7462211..48321f4 100644 >>>>>> --- a/s390x/smp.c >>>>>> +++ b/s390x/smp.c >>>>>> @@ -75,6 +75,7 @@ static void test_stop_store_status(void) >>>>>> lc->prefix_sa = 0; >>>>>> lc->grs_sa[15] = 0; >>>>>> smp_cpu_stop_store_status(1); >>>>>> + smp_cpu_wait_for_completion(1); >>>>>> mb(); >>>>>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); >>>>>> report(lc->grs_sa[15], "stack"); >>>>>> @@ -85,6 +86,7 @@ static void test_stop_store_status(void) >>>>>> lc->prefix_sa = 0; >>>>>> lc->grs_sa[15] = 0; >>>>>> smp_cpu_stop_store_status(1); >>>>> >>>>> Just curious: Would it make sense to add that inside >>>>> smp_cpu_stop_store_status() instead? >>>>> >>>> >>>> I think so, we also wait for stop and start to finish, so why not for >>>> this order code. >>>> >>> >>> I've moved the waiting into the smp library and now the prefix check for >>> stop and store status fails every so often if executed repeatedly. >>> >>> I've tried making the lc ptr volatile, a print of the prefix before the >>> report seems to fix the issue, a print after the report still shows the >>> issue but according to the print both values are the same. >>> >>> I'm currently at a loss... >> >> Are you missing a barrier() somewhere? >> > > Maybe, but the question is where? > > There's already one before the report: > smp_cpu_stop_store_status(1); > mb(); > report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); The issue here is: SIGP_SENSE is always handled in the kernel for KVM. Meaning, it will complete even before the target CPU executed the stop and store (in QEMU). Reading the PoP: "One of the following conditions exists at the addressed CPU: ... A previously issued stop- and-store-status ... has been accepted by the addressed CPU, and execution of the func- tion requested by the order has not yet been completed. "If the currently specified order is sense ... then the order is rejected, and condition code 2 is set." So, in case of KVM, SENSE does not wait for completion of the previous order. I remember that was a performance improvements, because we wanted to avoid going to user space just to sense if another CPU is running. (and I remember that the documentation was inconsistent) Let me guess, under TCG it works all the time?
On 4/29/20 11:55 AM, David Hildenbrand wrote: > On 29.04.20 11:37, Janosch Frank wrote: >> On 4/29/20 11:06 AM, David Hildenbrand wrote: >>> On 29.04.20 10:57, Janosch Frank wrote: >>>> On 4/24/20 1:40 PM, Janosch Frank wrote: >>>>> On 4/24/20 12:11 PM, David Hildenbrand wrote: >>>>>> On 23.04.20 11:10, Janosch Frank wrote: >>>>>>> Sigp orders are not necessarily finished when the processor finished >>>>>>> the sigp instruction. We need to poll if the order has been finished >>>>>>> before we continue. >>>>>>> >>>>>>> For (re)start and stop we already use sigp sense running and sigp >>>>>>> sense loops. But we still lack completion checks for stop and store >>>>>>> status, as well as the cpu resets. >>>>>>> >>>>>>> Let's add them. >>>>>>> >>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >>>>>>> --- >>>>>>> lib/s390x/smp.c | 8 ++++++++ >>>>>>> lib/s390x/smp.h | 1 + >>>>>>> s390x/smp.c | 4 ++++ >>>>>>> 3 files changed, 13 insertions(+) >>>>>>> >>>>>>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c >>>>>>> index 6ef0335..2555bf4 100644 >>>>>>> --- a/lib/s390x/smp.c >>>>>>> +++ b/lib/s390x/smp.c >>>>>>> @@ -154,6 +154,14 @@ int smp_cpu_start(uint16_t addr, struct psw psw) >>>>>>> return rc; >>>>>>> } >>>>>>> >>>>>>> +void smp_cpu_wait_for_completion(uint16_t addr) >>>>>>> +{ >>>>>>> + uint32_t status; >>>>>>> + >>>>>>> + /* Loops when cc == 2, i.e. when the cpu is busy with a sigp order */ >>>>>>> + sigp_retry(1, SIGP_SENSE, 0, &status); >>>>>>> +} >>>>>>> + >>>>>>> int smp_cpu_destroy(uint16_t addr) >>>>>>> { >>>>>>> struct cpu *cpu; >>>>>>> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h >>>>>>> index ce63a89..a8b98c0 100644 >>>>>>> --- a/lib/s390x/smp.h >>>>>>> +++ b/lib/s390x/smp.h >>>>>>> @@ -45,6 +45,7 @@ int smp_cpu_restart(uint16_t addr); >>>>>>> int smp_cpu_start(uint16_t addr, struct psw psw); >>>>>>> int smp_cpu_stop(uint16_t addr); >>>>>>> int smp_cpu_stop_store_status(uint16_t addr); >>>>>>> +void smp_cpu_wait_for_completion(uint16_t addr); >>>>>>> int smp_cpu_destroy(uint16_t addr); >>>>>>> int smp_cpu_setup(uint16_t addr, struct psw psw); >>>>>>> void smp_teardown(void); >>>>>>> diff --git a/s390x/smp.c b/s390x/smp.c >>>>>>> index 7462211..48321f4 100644 >>>>>>> --- a/s390x/smp.c >>>>>>> +++ b/s390x/smp.c >>>>>>> @@ -75,6 +75,7 @@ static void test_stop_store_status(void) >>>>>>> lc->prefix_sa = 0; >>>>>>> lc->grs_sa[15] = 0; >>>>>>> smp_cpu_stop_store_status(1); >>>>>>> + smp_cpu_wait_for_completion(1); >>>>>>> mb(); >>>>>>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); >>>>>>> report(lc->grs_sa[15], "stack"); >>>>>>> @@ -85,6 +86,7 @@ static void test_stop_store_status(void) >>>>>>> lc->prefix_sa = 0; >>>>>>> lc->grs_sa[15] = 0; >>>>>>> smp_cpu_stop_store_status(1); >>>>>> >>>>>> Just curious: Would it make sense to add that inside >>>>>> smp_cpu_stop_store_status() instead? >>>>>> >>>>> >>>>> I think so, we also wait for stop and start to finish, so why not for >>>>> this order code. >>>>> >>>> >>>> I've moved the waiting into the smp library and now the prefix check for >>>> stop and store status fails every so often if executed repeatedly. >>>> >>>> I've tried making the lc ptr volatile, a print of the prefix before the >>>> report seems to fix the issue, a print after the report still shows the >>>> issue but according to the print both values are the same. >>>> >>>> I'm currently at a loss... >>> >>> Are you missing a barrier() somewhere? >>> >> >> Maybe, but the question is where? >> >> There's already one before the report: >> smp_cpu_stop_store_status(1); >> mb(); >> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); > > The issue here is: > > SIGP_SENSE is always handled in the kernel for KVM. Meaning, it will > complete even before the target CPU executed the stop and store (in QEMU). > > Reading the PoP: > > "One of the following conditions exists at the > addressed CPU: ... A previously issued stop- > and-store-status ... has been accepted by the > addressed CPU, and execution of the func- > tion requested by the order has not yet been > completed. > > "If the currently specified order is sense ... then the order > is rejected, and condition code 2 is set." > > So, in case of KVM, SENSE does not wait for completion of the previous > order. I remember that was a performance improvements, because we wanted > to avoid going to user space just to sense if another CPU is running. > (and I remember that the documentation was inconsistent) So, KVM is not architectural compliant when it comes to SIGP SENSE? I guess I need to go back to looping until the prefix is > 0 > > Let me guess, under TCG it works all the time? > Looks like it
On 29.04.20 13:21, Janosch Frank wrote: > On 4/29/20 11:55 AM, David Hildenbrand wrote: >> On 29.04.20 11:37, Janosch Frank wrote: >>> On 4/29/20 11:06 AM, David Hildenbrand wrote: >>>> On 29.04.20 10:57, Janosch Frank wrote: >>>>> On 4/24/20 1:40 PM, Janosch Frank wrote: >>>>>> On 4/24/20 12:11 PM, David Hildenbrand wrote: >>>>>>> On 23.04.20 11:10, Janosch Frank wrote: >>>>>>>> Sigp orders are not necessarily finished when the processor finished >>>>>>>> the sigp instruction. We need to poll if the order has been finished >>>>>>>> before we continue. >>>>>>>> >>>>>>>> For (re)start and stop we already use sigp sense running and sigp >>>>>>>> sense loops. But we still lack completion checks for stop and store >>>>>>>> status, as well as the cpu resets. >>>>>>>> >>>>>>>> Let's add them. >>>>>>>> >>>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >>>>>>>> --- >>>>>>>> lib/s390x/smp.c | 8 ++++++++ >>>>>>>> lib/s390x/smp.h | 1 + >>>>>>>> s390x/smp.c | 4 ++++ >>>>>>>> 3 files changed, 13 insertions(+) >>>>>>>> >>>>>>>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c >>>>>>>> index 6ef0335..2555bf4 100644 >>>>>>>> --- a/lib/s390x/smp.c >>>>>>>> +++ b/lib/s390x/smp.c >>>>>>>> @@ -154,6 +154,14 @@ int smp_cpu_start(uint16_t addr, struct psw psw) >>>>>>>> return rc; >>>>>>>> } >>>>>>>> >>>>>>>> +void smp_cpu_wait_for_completion(uint16_t addr) >>>>>>>> +{ >>>>>>>> + uint32_t status; >>>>>>>> + >>>>>>>> + /* Loops when cc == 2, i.e. when the cpu is busy with a sigp order */ >>>>>>>> + sigp_retry(1, SIGP_SENSE, 0, &status); >>>>>>>> +} >>>>>>>> + >>>>>>>> int smp_cpu_destroy(uint16_t addr) >>>>>>>> { >>>>>>>> struct cpu *cpu; >>>>>>>> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h >>>>>>>> index ce63a89..a8b98c0 100644 >>>>>>>> --- a/lib/s390x/smp.h >>>>>>>> +++ b/lib/s390x/smp.h >>>>>>>> @@ -45,6 +45,7 @@ int smp_cpu_restart(uint16_t addr); >>>>>>>> int smp_cpu_start(uint16_t addr, struct psw psw); >>>>>>>> int smp_cpu_stop(uint16_t addr); >>>>>>>> int smp_cpu_stop_store_status(uint16_t addr); >>>>>>>> +void smp_cpu_wait_for_completion(uint16_t addr); >>>>>>>> int smp_cpu_destroy(uint16_t addr); >>>>>>>> int smp_cpu_setup(uint16_t addr, struct psw psw); >>>>>>>> void smp_teardown(void); >>>>>>>> diff --git a/s390x/smp.c b/s390x/smp.c >>>>>>>> index 7462211..48321f4 100644 >>>>>>>> --- a/s390x/smp.c >>>>>>>> +++ b/s390x/smp.c >>>>>>>> @@ -75,6 +75,7 @@ static void test_stop_store_status(void) >>>>>>>> lc->prefix_sa = 0; >>>>>>>> lc->grs_sa[15] = 0; >>>>>>>> smp_cpu_stop_store_status(1); >>>>>>>> + smp_cpu_wait_for_completion(1); >>>>>>>> mb(); >>>>>>>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); >>>>>>>> report(lc->grs_sa[15], "stack"); >>>>>>>> @@ -85,6 +86,7 @@ static void test_stop_store_status(void) >>>>>>>> lc->prefix_sa = 0; >>>>>>>> lc->grs_sa[15] = 0; >>>>>>>> smp_cpu_stop_store_status(1); >>>>>>> >>>>>>> Just curious: Would it make sense to add that inside >>>>>>> smp_cpu_stop_store_status() instead? >>>>>>> >>>>>> >>>>>> I think so, we also wait for stop and start to finish, so why not for >>>>>> this order code. >>>>>> >>>>> >>>>> I've moved the waiting into the smp library and now the prefix check for >>>>> stop and store status fails every so often if executed repeatedly. >>>>> >>>>> I've tried making the lc ptr volatile, a print of the prefix before the >>>>> report seems to fix the issue, a print after the report still shows the >>>>> issue but according to the print both values are the same. >>>>> >>>>> I'm currently at a loss... >>>> >>>> Are you missing a barrier() somewhere? >>>> >>> >>> Maybe, but the question is where? >>> >>> There's already one before the report: >>> smp_cpu_stop_store_status(1); >>> mb(); >>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); >> >> The issue here is: >> >> SIGP_SENSE is always handled in the kernel for KVM. Meaning, it will >> complete even before the target CPU executed the stop and store (in QEMU). >> >> Reading the PoP: >> >> "One of the following conditions exists at the >> addressed CPU: ... A previously issued stop- >> and-store-status ... has been accepted by the >> addressed CPU, and execution of the func- >> tion requested by the order has not yet been >> completed. >> >> "If the currently specified order is sense ... then the order >> is rejected, and condition code 2 is set." >> >> So, in case of KVM, SENSE does not wait for completion of the previous >> order. I remember that was a performance improvements, because we wanted >> to avoid going to user space just to sense if another CPU is running. >> (and I remember that the documentation was inconsistent) > > So, KVM is not architectural compliant when it comes to SIGP SENSE? > I guess I need to go back to looping until the prefix is > 0 Yeah, or fix SIGP_SENSE in KVM. Would need QEMU and KVM changes. I remember that a tricky part was checking if external calls are pending for a CPU from user space. We could pass that information along with the intercept to QEMU. AFAIKs, SIGP SENSE is not used on a hot path in Linux.
On 4/29/20 1:47 PM, David Hildenbrand wrote: > On 29.04.20 13:21, Janosch Frank wrote: >> On 4/29/20 11:55 AM, David Hildenbrand wrote: >>> On 29.04.20 11:37, Janosch Frank wrote: >>>> On 4/29/20 11:06 AM, David Hildenbrand wrote: >>>>> On 29.04.20 10:57, Janosch Frank wrote: >>>>>> On 4/24/20 1:40 PM, Janosch Frank wrote: >>>>>>> On 4/24/20 12:11 PM, David Hildenbrand wrote: >>>>>>>> On 23.04.20 11:10, Janosch Frank wrote: >>>>>>>>> Sigp orders are not necessarily finished when the processor finished >>>>>>>>> the sigp instruction. We need to poll if the order has been finished >>>>>>>>> before we continue. >>>>>>>>> >>>>>>>>> For (re)start and stop we already use sigp sense running and sigp >>>>>>>>> sense loops. But we still lack completion checks for stop and store >>>>>>>>> status, as well as the cpu resets. >>>>>>>>> >>>>>>>>> Let's add them. >>>>>>>>> >>>>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>>>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >>>>>>>>> --- >>>>>>>>> lib/s390x/smp.c | 8 ++++++++ >>>>>>>>> lib/s390x/smp.h | 1 + >>>>>>>>> s390x/smp.c | 4 ++++ >>>>>>>>> 3 files changed, 13 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c >>>>>>>>> index 6ef0335..2555bf4 100644 >>>>>>>>> --- a/lib/s390x/smp.c >>>>>>>>> +++ b/lib/s390x/smp.c >>>>>>>>> @@ -154,6 +154,14 @@ int smp_cpu_start(uint16_t addr, struct psw psw) >>>>>>>>> return rc; >>>>>>>>> } >>>>>>>>> >>>>>>>>> +void smp_cpu_wait_for_completion(uint16_t addr) >>>>>>>>> +{ >>>>>>>>> + uint32_t status; >>>>>>>>> + >>>>>>>>> + /* Loops when cc == 2, i.e. when the cpu is busy with a sigp order */ >>>>>>>>> + sigp_retry(1, SIGP_SENSE, 0, &status); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> int smp_cpu_destroy(uint16_t addr) >>>>>>>>> { >>>>>>>>> struct cpu *cpu; >>>>>>>>> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h >>>>>>>>> index ce63a89..a8b98c0 100644 >>>>>>>>> --- a/lib/s390x/smp.h >>>>>>>>> +++ b/lib/s390x/smp.h >>>>>>>>> @@ -45,6 +45,7 @@ int smp_cpu_restart(uint16_t addr); >>>>>>>>> int smp_cpu_start(uint16_t addr, struct psw psw); >>>>>>>>> int smp_cpu_stop(uint16_t addr); >>>>>>>>> int smp_cpu_stop_store_status(uint16_t addr); >>>>>>>>> +void smp_cpu_wait_for_completion(uint16_t addr); >>>>>>>>> int smp_cpu_destroy(uint16_t addr); >>>>>>>>> int smp_cpu_setup(uint16_t addr, struct psw psw); >>>>>>>>> void smp_teardown(void); >>>>>>>>> diff --git a/s390x/smp.c b/s390x/smp.c >>>>>>>>> index 7462211..48321f4 100644 >>>>>>>>> --- a/s390x/smp.c >>>>>>>>> +++ b/s390x/smp.c >>>>>>>>> @@ -75,6 +75,7 @@ static void test_stop_store_status(void) >>>>>>>>> lc->prefix_sa = 0; >>>>>>>>> lc->grs_sa[15] = 0; >>>>>>>>> smp_cpu_stop_store_status(1); >>>>>>>>> + smp_cpu_wait_for_completion(1); >>>>>>>>> mb(); >>>>>>>>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); >>>>>>>>> report(lc->grs_sa[15], "stack"); >>>>>>>>> @@ -85,6 +86,7 @@ static void test_stop_store_status(void) >>>>>>>>> lc->prefix_sa = 0; >>>>>>>>> lc->grs_sa[15] = 0; >>>>>>>>> smp_cpu_stop_store_status(1); >>>>>>>> >>>>>>>> Just curious: Would it make sense to add that inside >>>>>>>> smp_cpu_stop_store_status() instead? >>>>>>>> >>>>>>> >>>>>>> I think so, we also wait for stop and start to finish, so why not for >>>>>>> this order code. >>>>>>> >>>>>> >>>>>> I've moved the waiting into the smp library and now the prefix check for >>>>>> stop and store status fails every so often if executed repeatedly. >>>>>> >>>>>> I've tried making the lc ptr volatile, a print of the prefix before the >>>>>> report seems to fix the issue, a print after the report still shows the >>>>>> issue but according to the print both values are the same. >>>>>> >>>>>> I'm currently at a loss... >>>>> >>>>> Are you missing a barrier() somewhere? >>>>> >>>> >>>> Maybe, but the question is where? >>>> >>>> There's already one before the report: >>>> smp_cpu_stop_store_status(1); >>>> mb(); >>>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); >>> >>> The issue here is: >>> >>> SIGP_SENSE is always handled in the kernel for KVM. Meaning, it will >>> complete even before the target CPU executed the stop and store (in QEMU). >>> >>> Reading the PoP: >>> >>> "One of the following conditions exists at the >>> addressed CPU: ... A previously issued stop- >>> and-store-status ... has been accepted by the >>> addressed CPU, and execution of the func- >>> tion requested by the order has not yet been >>> completed. >>> >>> "If the currently specified order is sense ... then the order >>> is rejected, and condition code 2 is set." >>> >>> So, in case of KVM, SENSE does not wait for completion of the previous >>> order. I remember that was a performance improvements, because we wanted >>> to avoid going to user space just to sense if another CPU is running. >>> (and I remember that the documentation was inconsistent) >> >> So, KVM is not architectural compliant when it comes to SIGP SENSE? >> I guess I need to go back to looping until the prefix is > 0 > > Yeah, or fix SIGP_SENSE in KVM. Would need QEMU and KVM changes. I > remember that a tricky part was checking if external calls are pending > for a CPU from user space. > > We could pass that information along with the intercept to QEMU. > > AFAIKs, SIGP SENSE is not used on a hot path in Linux. > For now I'd rather have a workaround in the test until I can find cycles to find a solution in KVM/QEMU. SIGP SENSE has been working quite well for Linux for the last few years, so I won't start running around now frantically fixing stuff.
On 29.04.20 14:09, Janosch Frank wrote: > On 4/29/20 1:47 PM, David Hildenbrand wrote: >> On 29.04.20 13:21, Janosch Frank wrote: >>> On 4/29/20 11:55 AM, David Hildenbrand wrote: >>>> On 29.04.20 11:37, Janosch Frank wrote: >>>>> On 4/29/20 11:06 AM, David Hildenbrand wrote: >>>>>> On 29.04.20 10:57, Janosch Frank wrote: >>>>>>> On 4/24/20 1:40 PM, Janosch Frank wrote: >>>>>>>> On 4/24/20 12:11 PM, David Hildenbrand wrote: >>>>>>>>> On 23.04.20 11:10, Janosch Frank wrote: >>>>>>>>>> Sigp orders are not necessarily finished when the processor finished >>>>>>>>>> the sigp instruction. We need to poll if the order has been finished >>>>>>>>>> before we continue. >>>>>>>>>> >>>>>>>>>> For (re)start and stop we already use sigp sense running and sigp >>>>>>>>>> sense loops. But we still lack completion checks for stop and store >>>>>>>>>> status, as well as the cpu resets. >>>>>>>>>> >>>>>>>>>> Let's add them. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>>>>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >>>>>>>>>> --- >>>>>>>>>> lib/s390x/smp.c | 8 ++++++++ >>>>>>>>>> lib/s390x/smp.h | 1 + >>>>>>>>>> s390x/smp.c | 4 ++++ >>>>>>>>>> 3 files changed, 13 insertions(+) >>>>>>>>>> >>>>>>>>>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c >>>>>>>>>> index 6ef0335..2555bf4 100644 >>>>>>>>>> --- a/lib/s390x/smp.c >>>>>>>>>> +++ b/lib/s390x/smp.c >>>>>>>>>> @@ -154,6 +154,14 @@ int smp_cpu_start(uint16_t addr, struct psw psw) >>>>>>>>>> return rc; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> +void smp_cpu_wait_for_completion(uint16_t addr) >>>>>>>>>> +{ >>>>>>>>>> + uint32_t status; >>>>>>>>>> + >>>>>>>>>> + /* Loops when cc == 2, i.e. when the cpu is busy with a sigp order */ >>>>>>>>>> + sigp_retry(1, SIGP_SENSE, 0, &status); >>>>>>>>>> +} >>>>>>>>>> + >>>>>>>>>> int smp_cpu_destroy(uint16_t addr) >>>>>>>>>> { >>>>>>>>>> struct cpu *cpu; >>>>>>>>>> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h >>>>>>>>>> index ce63a89..a8b98c0 100644 >>>>>>>>>> --- a/lib/s390x/smp.h >>>>>>>>>> +++ b/lib/s390x/smp.h >>>>>>>>>> @@ -45,6 +45,7 @@ int smp_cpu_restart(uint16_t addr); >>>>>>>>>> int smp_cpu_start(uint16_t addr, struct psw psw); >>>>>>>>>> int smp_cpu_stop(uint16_t addr); >>>>>>>>>> int smp_cpu_stop_store_status(uint16_t addr); >>>>>>>>>> +void smp_cpu_wait_for_completion(uint16_t addr); >>>>>>>>>> int smp_cpu_destroy(uint16_t addr); >>>>>>>>>> int smp_cpu_setup(uint16_t addr, struct psw psw); >>>>>>>>>> void smp_teardown(void); >>>>>>>>>> diff --git a/s390x/smp.c b/s390x/smp.c >>>>>>>>>> index 7462211..48321f4 100644 >>>>>>>>>> --- a/s390x/smp.c >>>>>>>>>> +++ b/s390x/smp.c >>>>>>>>>> @@ -75,6 +75,7 @@ static void test_stop_store_status(void) >>>>>>>>>> lc->prefix_sa = 0; >>>>>>>>>> lc->grs_sa[15] = 0; >>>>>>>>>> smp_cpu_stop_store_status(1); >>>>>>>>>> + smp_cpu_wait_for_completion(1); >>>>>>>>>> mb(); >>>>>>>>>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); >>>>>>>>>> report(lc->grs_sa[15], "stack"); >>>>>>>>>> @@ -85,6 +86,7 @@ static void test_stop_store_status(void) >>>>>>>>>> lc->prefix_sa = 0; >>>>>>>>>> lc->grs_sa[15] = 0; >>>>>>>>>> smp_cpu_stop_store_status(1); >>>>>>>>> >>>>>>>>> Just curious: Would it make sense to add that inside >>>>>>>>> smp_cpu_stop_store_status() instead? >>>>>>>>> >>>>>>>> >>>>>>>> I think so, we also wait for stop and start to finish, so why not for >>>>>>>> this order code. >>>>>>>> >>>>>>> >>>>>>> I've moved the waiting into the smp library and now the prefix check for >>>>>>> stop and store status fails every so often if executed repeatedly. >>>>>>> >>>>>>> I've tried making the lc ptr volatile, a print of the prefix before the >>>>>>> report seems to fix the issue, a print after the report still shows the >>>>>>> issue but according to the print both values are the same. >>>>>>> >>>>>>> I'm currently at a loss... >>>>>> >>>>>> Are you missing a barrier() somewhere? >>>>>> >>>>> >>>>> Maybe, but the question is where? >>>>> >>>>> There's already one before the report: >>>>> smp_cpu_stop_store_status(1); >>>>> mb(); >>>>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); >>>> >>>> The issue here is: >>>> >>>> SIGP_SENSE is always handled in the kernel for KVM. Meaning, it will >>>> complete even before the target CPU executed the stop and store (in QEMU). >>>> >>>> Reading the PoP: >>>> >>>> "One of the following conditions exists at the >>>> addressed CPU: ... A previously issued stop- >>>> and-store-status ... has been accepted by the >>>> addressed CPU, and execution of the func- >>>> tion requested by the order has not yet been >>>> completed. >>>> >>>> "If the currently specified order is sense ... then the order >>>> is rejected, and condition code 2 is set." >>>> >>>> So, in case of KVM, SENSE does not wait for completion of the previous >>>> order. I remember that was a performance improvements, because we wanted >>>> to avoid going to user space just to sense if another CPU is running. >>>> (and I remember that the documentation was inconsistent) >>> >>> So, KVM is not architectural compliant when it comes to SIGP SENSE? >>> I guess I need to go back to looping until the prefix is > 0 >> >> Yeah, or fix SIGP_SENSE in KVM. Would need QEMU and KVM changes. I >> remember that a tricky part was checking if external calls are pending >> for a CPU from user space. >> >> We could pass that information along with the intercept to QEMU. >> >> AFAIKs, SIGP SENSE is not used on a hot path in Linux. >> > > For now I'd rather have a workaround in the test until I can find cycles > to find a solution in KVM/QEMU. > > SIGP SENSE has been working quite well for Linux for the last few years, > so I won't start running around now frantically fixing stuff. Huh. I thought that's why we have the SMP tests after all ;)
diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c index 6ef0335..2555bf4 100644 --- a/lib/s390x/smp.c +++ b/lib/s390x/smp.c @@ -154,6 +154,14 @@ int smp_cpu_start(uint16_t addr, struct psw psw) return rc; } +void smp_cpu_wait_for_completion(uint16_t addr) +{ + uint32_t status; + + /* Loops when cc == 2, i.e. when the cpu is busy with a sigp order */ + sigp_retry(1, SIGP_SENSE, 0, &status); +} + int smp_cpu_destroy(uint16_t addr) { struct cpu *cpu; diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h index ce63a89..a8b98c0 100644 --- a/lib/s390x/smp.h +++ b/lib/s390x/smp.h @@ -45,6 +45,7 @@ int smp_cpu_restart(uint16_t addr); int smp_cpu_start(uint16_t addr, struct psw psw); int smp_cpu_stop(uint16_t addr); int smp_cpu_stop_store_status(uint16_t addr); +void smp_cpu_wait_for_completion(uint16_t addr); int smp_cpu_destroy(uint16_t addr); int smp_cpu_setup(uint16_t addr, struct psw psw); void smp_teardown(void); diff --git a/s390x/smp.c b/s390x/smp.c index 7462211..48321f4 100644 --- a/s390x/smp.c +++ b/s390x/smp.c @@ -75,6 +75,7 @@ static void test_stop_store_status(void) lc->prefix_sa = 0; lc->grs_sa[15] = 0; smp_cpu_stop_store_status(1); + smp_cpu_wait_for_completion(1); mb(); report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); report(lc->grs_sa[15], "stack"); @@ -85,6 +86,7 @@ static void test_stop_store_status(void) lc->prefix_sa = 0; lc->grs_sa[15] = 0; smp_cpu_stop_store_status(1); + smp_cpu_wait_for_completion(1); mb(); report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix"); report(lc->grs_sa[15], "stack"); @@ -215,6 +217,7 @@ static void test_reset_initial(void) wait_for_flag(); sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL); + smp_cpu_wait_for_completion(1); sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL); report_prefix_push("clear"); @@ -264,6 +267,7 @@ static void test_reset(void) smp_cpu_start(1, psw); sigp_retry(1, SIGP_CPU_RESET, 0, NULL); + smp_cpu_wait_for_completion(1); report(smp_cpu_stopped(1), "cpu stopped"); set_flag(0);