Re: [PATCH v2 08/10] s390x: smp: Wait for sigp completion

[Janosch Frank <frankja@linux.ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4928 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]