Re: [RESEND PATCH v2 4/5] kvm: Remove ept_identity_pagetable from struct kvm_arch.

[Tang Chen <tangchen@cn.fujitsu.com>]

On 07/14/2014 10:27 PM, Gleb Natapov wrote:
......
>>>>   	if (likely(kvm->arch.ept_identity_pagetable_done))
>>>>   		return 1;
>>>> -	ret = 0;
>>>>   	identity_map_pfn = kvm->arch.ept_identity_map_addr>>   PAGE_SHIFT;
>>>> +
>>>> +	mutex_lock(&kvm->slots_lock);
>>> Why move this out of alloc_identity_pagetable()?
>>>
>>
>> Referring to the original code, I think mutex_lock(&kvm->slots_lock) is used
>> to protect kvm->arch.ept_identity_pagetable. If two or more threads try to
>> modify it at the same time, the mutex ensures that the identity table is
>> only
>> allocated once.
>>
>> Now, we dropped kvm->arch.ept_identity_pagetable. And use
>> kvm->arch.ept_identity_pagetable_done
>> to check if the identity table is allocated and initialized. So we should
>> protect
>> memory slot operation in alloc_identity_pagetable() and
>> kvm->arch.ept_identity_pagetable_done
>> with this mutex.
>>
>> Of course, I can see that the name "slots_lock" indicates that it may be
>> used
>> to protect the memory slot operation only. Maybe move it out here is not
>> suitable.
>>
>> If I'm wrong, please tell me.
>>
> No, you are right that besides memory slot creation slots_lock protects checking
> of ept_identity_pagetable here, but after you patch ept_identity_pagetable_done is
> tested outside of slots_lock so the allocation can happen twice, no?

Oh, yes. Will fix it in the next version.

Thanks.