linux-kernel - Re: [PATCH v2 29/45] kvm/vmx: Use get/put_online_cpus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <51CAAD0D.8090601@redhat.com>
Date:	Wed, 26 Jun 2013 10:57:49 +0200
From:	Paolo Bonzini <pbonzini@...hat.com>
To:	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
CC:	Gleb Natapov <gleb@...hat.com>, peterz@...radead.org,
	fweisbec@...il.com, linux-kernel@...r.kernel.org,
	"H. Peter Anvin" <hpa@...or.com>, walken@...gle.com,
	mingo@...nel.org, linux-arch@...r.kernel.org,
	vincent.guittot@...aro.org, kvm@...r.kernel.org, x86@...nel.org,
	xiaoguangrong@...ux.vnet.ibm.com, Ingo Molnar <mingo@...hat.com>,
	wangyun@...ux.vnet.ibm.com, paulmck@...ux.vnet.ibm.com,
	nikunj@...ux.vnet.ibm.com, linux-pm@...r.kernel.org,
	rusty@...tcorp.com.au, rostedt@...dmis.org, namhyung@...nel.org,
	tglx@...utronix.de, laijs@...fujitsu.com, zhong@...ux.vnet.ibm.com,
	netdev@...r.kernel.org, oleg@...hat.com, sbw@....edu,
	tj@...nel.org, akpm@...ux-foundation.org,
	linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v2 29/45] kvm/vmx: Use get/put_online_cpus_atomic() to
 prevent CPU offline

Il 26/06/2013 10:41, Srivatsa S. Bhat ha scritto:
> On 06/26/2013 01:53 PM, Paolo Bonzini wrote:
>> Il 26/06/2013 10:06, Srivatsa S. Bhat ha scritto:
>>> On 06/26/2013 01:16 PM, Paolo Bonzini wrote:
>>>> Il 25/06/2013 22:30, Srivatsa S. Bhat ha scritto:
>>>>> -	cpu = get_cpu();
>>>>> +	cpu = get_online_cpus_atomic();
>>>>>  	vmx_vcpu_load(&vmx->vcpu, cpu);
>>>>>  	vmx->vcpu.cpu = cpu;
>>>>>  	err = vmx_vcpu_setup(vmx);
>>>>>  	vmx_vcpu_put(&vmx->vcpu);
>>>>> -	put_cpu();
>>>>> +	put_online_cpus_atomic();
>>>>
>>>> The new API has a weird name.  Why are you adding new functions instead
>>>> of just modifying get/put_cpu?
>>>>
>>>
>>> Because the purpose of those two functions are distinctly different
>>> from each other.
>>>
>>> get/put_cpu() is used to disable preemption on the local CPU. (Which
>>> also disables offlining the local CPU during that critical section).
>>
>> Ok, then I understood correctly... and I acked the other KVM patch.
>>
> 
> Thank you!
>  
>> However, keeping the code on the local CPU is exactly the point of this
>> particular use of get_cpu()/put_cpu().  Why does it need to synchronize
>> with offlining of other CPUs?
> 
> Now that I looked at it again, I think you are right, get/put_cpu() is
> good enough here.
> 
> But let me explain why I initially thought we needed full synchronization
> with CPU offline. In short, I wanted to synchronize the calls to
> __loaded_vmcs_clear(). We have the scenario shown below:
> 
> CPU offline:
> 	CPU_DYING:
> 		hardware_disable();
> 		->vmclear_local_loaded_vmcss();
> 		  ->__loaded_vmcs_clear(v);
> 
> 
> 
> And vmx_vcpu_load() (among others) can do:
>        vmx_vcpu_load();
>        -> loaded_vmcs_clear();
>           -> __loaded_vmcs_clear();
> 
> 
> So I wanted to avoid this race-condition and hence wrapped the code with
> get/put_online_cpus_atomic().
> 
> But the point I missed earlier is that loaded_vmcs_clear() calls
> __loaded_vmcs_clear() using smp_call_function_single(), which itself
> synchronizes properly with CPU hotplug. So there is no need to add full
> hotplug synchronization in the vmx code, as you noted above.

Makes sense, and I see now that it's patch 9 in this series.

In general, I would rather add an extra get_online_cpus_atomic pair
where it it actually needed (i.e. closer to where cpu_online is actually
used), and leave get_cpu/put_cpu as is in the caller... which is exactly
what happens in this case, since "where it is actually needed" is "in
smp_call_function_single()".

> So, please ignore this patch, and sorry for the noise!

No problem, thanks for the heads-up.

Paolo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/