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@xxxxxxxxxxxxx>
>>>> Reviewed-by: Cornelia Huck <cohuck@xxxxxxxxxx>
>>>> ---
>>>> 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?
--
Thanks,
David / dhildenb
