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@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?
>>
>
> 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?
--
Thanks,
David / dhildenb
