[Qemu-devel] [PATCH] cpu: smp_wmb before lauching cpus.

[Qemu-devel] [PATCH] cpu: smp_wmb before lauching cpus.

[Liu Ping Fan]

Vcpu state must be set completely before receiving INIT-IPI,SIPI

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 kvm.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/kvm.h b/kvm.h
index 9c7b0ea..5b3c228 100644
--- a/kvm.h
+++ b/kvm.h
@@ -198,6 +198,7 @@ static inline void cpu_synchronize_post_init(CPUArchState *env)
 {
     if (kvm_enabled()) {
         kvm_cpu_synchronize_post_init(env);
+        smp_wmb();
     }
 }
 
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH] cpu: smp_wmb before lauching cpus.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

On 2012-07-05 04:18, Liu Ping Fan wrote:
> Vcpu state must be set completely before receiving INIT-IPI,SIPI
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  kvm.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/kvm.h b/kvm.h
> index 9c7b0ea..5b3c228 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -198,6 +198,7 @@ static inline void cpu_synchronize_post_init(CPUArchState *env)
>  {
>      if (kvm_enabled()) {
>          kvm_cpu_synchronize_post_init(env);
> +        smp_wmb();
>      }
>  }
>  
> 

In theory, there should be no vcpu kick-off after this without some
locking operations involved that imply barriers. Did you see real
inconsistencies without this explicit one?

Jan


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

Re: [Qemu-devel] [PATCH] cpu: smp_wmb before lauching cpus.

[liu ping fan]

On Thu, Jul 5, 2012 at 2:46 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2012-07-05 04:18, Liu Ping Fan wrote:
>> Vcpu state must be set completely before receiving INIT-IPI,SIPI
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  kvm.h |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/kvm.h b/kvm.h
>> index 9c7b0ea..5b3c228 100644
>> --- a/kvm.h
>> +++ b/kvm.h
>> @@ -198,6 +198,7 @@ static inline void cpu_synchronize_post_init(CPUArchState *env)
>>  {
>>      if (kvm_enabled()) {
>>          kvm_cpu_synchronize_post_init(env);
>> +        smp_wmb();
>>      }
>>  }
>>
>>
>
> In theory, there should be no vcpu kick-off after this without some
> locking operations involved that imply barriers. Did you see real

Yeah, but what if it is non-x86?
> inconsistencies without this explicit one?
>
> Jan
>

Re: [Qemu-devel] [PATCH] cpu: smp_wmb before lauching cpus.

[Jan Kiszka]

On 2012-07-05 12:10, liu ping fan wrote:
> On Thu, Jul 5, 2012 at 2:46 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2012-07-05 04:18, Liu Ping Fan wrote:
>>> Vcpu state must be set completely before receiving INIT-IPI,SIPI
>>>
>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>> ---
>>>  kvm.h |    1 +
>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/kvm.h b/kvm.h
>>> index 9c7b0ea..5b3c228 100644
>>> --- a/kvm.h
>>> +++ b/kvm.h
>>> @@ -198,6 +198,7 @@ static inline void cpu_synchronize_post_init(CPUArchState *env)
>>>  {
>>>      if (kvm_enabled()) {
>>>          kvm_cpu_synchronize_post_init(env);
>>> +        smp_wmb();
>>>      }
>>>  }
>>>
>>>
>>
>> In theory, there should be no vcpu kick-off after this without some
>> locking operations involved that imply barriers. Did you see real
> 
> Yeah, but what if it is non-x86?

The locking I'm referring to is arch independent.

>> inconsistencies without this explicit one?

Again: Did you see real issues or is this based on static analysis?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] cpu: smp_wmb before lauching cpus.

[liu ping fan]

On Thu, Jul 5, 2012 at 6:16 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2012-07-05 12:10, liu ping fan wrote:
>> On Thu, Jul 5, 2012 at 2:46 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> On 2012-07-05 04:18, Liu Ping Fan wrote:
>>>> Vcpu state must be set completely before receiving INIT-IPI,SIPI
>>>>
>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>> ---
>>>>  kvm.h |    1 +
>>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/kvm.h b/kvm.h
>>>> index 9c7b0ea..5b3c228 100644
>>>> --- a/kvm.h
>>>> +++ b/kvm.h
>>>> @@ -198,6 +198,7 @@ static inline void cpu_synchronize_post_init(CPUArchState *env)
>>>>  {
>>>>      if (kvm_enabled()) {
>>>>          kvm_cpu_synchronize_post_init(env);
>>>> +        smp_wmb();
>>>>      }
>>>>  }
>>>>
>>>>
>>>
>>> In theory, there should be no vcpu kick-off after this without some
>>> locking operations involved that imply barriers. Did you see real
>>
>> Yeah, but what if it is non-x86?
>
> The locking I'm referring to is arch independent.
>
>>> inconsistencies without this explicit one?
>
> Again: Did you see real issues or is this based on static analysis?
>
Just on  static analysis

Regards,
pingfan
> Jan
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux
>

Re: [Qemu-devel] [PATCH] cpu: smp_wmb before lauching cpus.

[Jan Kiszka]

On 2012-07-05 13:02, liu ping fan wrote:
> On Thu, Jul 5, 2012 at 6:16 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2012-07-05 12:10, liu ping fan wrote:
>>> On Thu, Jul 5, 2012 at 2:46 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> On 2012-07-05 04:18, Liu Ping Fan wrote:
>>>>> Vcpu state must be set completely before receiving INIT-IPI,SIPI
>>>>>
>>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>>> ---
>>>>>  kvm.h |    1 +
>>>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/kvm.h b/kvm.h
>>>>> index 9c7b0ea..5b3c228 100644
>>>>> --- a/kvm.h
>>>>> +++ b/kvm.h
>>>>> @@ -198,6 +198,7 @@ static inline void cpu_synchronize_post_init(CPUArchState *env)
>>>>>  {
>>>>>      if (kvm_enabled()) {
>>>>>          kvm_cpu_synchronize_post_init(env);
>>>>> +        smp_wmb();
>>>>>      }
>>>>>  }
>>>>>
>>>>>
>>>>
>>>> In theory, there should be no vcpu kick-off after this without some
>>>> locking operations involved that imply barriers. Did you see real
>>>
>>> Yeah, but what if it is non-x86?
>>
>> The locking I'm referring to is arch independent.
>>
>>>> inconsistencies without this explicit one?
>>
>> Again: Did you see real issues or is this based on static analysis?
>>
> Just on  static analysis

Then please describe - also for the changelog - at least one case in
details where this is needed.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] cpu: smp_wmb before lauching cpus.

[liu ping fan]

On Thu, Jul 5, 2012 at 7:58 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2012-07-05 13:02, liu ping fan wrote:
>> On Thu, Jul 5, 2012 at 6:16 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2012-07-05 12:10, liu ping fan wrote:
>>>> On Thu, Jul 5, 2012 at 2:46 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>> On 2012-07-05 04:18, Liu Ping Fan wrote:
>>>>>> Vcpu state must be set completely before receiving INIT-IPI,SIPI
>>>>>>
>>>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>  kvm.h |    1 +
>>>>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>>>>
>>>>>> diff --git a/kvm.h b/kvm.h
>>>>>> index 9c7b0ea..5b3c228 100644
>>>>>> --- a/kvm.h
>>>>>> +++ b/kvm.h
>>>>>> @@ -198,6 +198,7 @@ static inline void cpu_synchronize_post_init(CPUArchState *env)
>>>>>>  {
>>>>>>      if (kvm_enabled()) {
>>>>>>          kvm_cpu_synchronize_post_init(env);
>>>>>> +        smp_wmb();
>>>>>>      }
>>>>>>  }
>>>>>>
>>>>>>
>>>>>
>>>>> In theory, there should be no vcpu kick-off after this without some
>>>>> locking operations involved that imply barriers. Did you see real
>>>>
>>>> Yeah, but what if it is non-x86?
>>>
>>> The locking I'm referring to is arch independent.
>>>
>>>>> inconsistencies without this explicit one?
>>>
>>> Again: Did you see real issues or is this based on static analysis?
>>>
>> Just on  static analysis
>
> Then please describe - also for the changelog - at least one case in
> details where this is needed.
>
I dived into code. And yes, as you said, ACPI eject does involve some
locking operation. So the only thing left is for starting up.  There
seems no potential lock operation from cpu_synchronize_all_post_init()
to  resume_all_vcpus().
If correct, I will update this info for the changelog

Regards,
pingfa

> Thanks,
> Jan
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux
>
>

Re: [Qemu-devel] [PATCH] cpu: smp_wmb before lauching cpus.

[Jan Kiszka]

On 2012-07-06 09:46, liu ping fan wrote:
> On Thu, Jul 5, 2012 at 7:58 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2012-07-05 13:02, liu ping fan wrote:
>>> On Thu, Jul 5, 2012 at 6:16 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> On 2012-07-05 12:10, liu ping fan wrote:
>>>>> On Thu, Jul 5, 2012 at 2:46 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>>> On 2012-07-05 04:18, Liu Ping Fan wrote:
>>>>>>> Vcpu state must be set completely before receiving INIT-IPI,SIPI
>>>>>>>
>>>>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>>>>> ---
>>>>>>>  kvm.h |    1 +
>>>>>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>>>>>
>>>>>>> diff --git a/kvm.h b/kvm.h
>>>>>>> index 9c7b0ea..5b3c228 100644
>>>>>>> --- a/kvm.h
>>>>>>> +++ b/kvm.h
>>>>>>> @@ -198,6 +198,7 @@ static inline void cpu_synchronize_post_init(CPUArchState *env)
>>>>>>>  {
>>>>>>>      if (kvm_enabled()) {
>>>>>>>          kvm_cpu_synchronize_post_init(env);
>>>>>>> +        smp_wmb();
>>>>>>>      }
>>>>>>>  }
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> In theory, there should be no vcpu kick-off after this without some
>>>>>> locking operations involved that imply barriers. Did you see real
>>>>>
>>>>> Yeah, but what if it is non-x86?
>>>>
>>>> The locking I'm referring to is arch independent.
>>>>
>>>>>> inconsistencies without this explicit one?
>>>>
>>>> Again: Did you see real issues or is this based on static analysis?
>>>>
>>> Just on  static analysis
>>
>> Then please describe - also for the changelog - at least one case in
>> details where this is needed.
>>
> I dived into code. And yes, as you said, ACPI eject does involve some
> locking operation. So the only thing left is for starting up.  There
> seems no potential lock operation from cpu_synchronize_all_post_init()
> to  resume_all_vcpus().

There is no difference in this scenario as well: the vcpu is waiting on
a condvar for the initial kick-off and will synchronize with the main
thread via the global mutex. When in doubt, try using a debugger.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux