Re: [patch 06/11] KVM: introduce kvm->srcu and convert kvm_set_memory_region to SRCU update

[Avi Kivity <avi@redhat.com>]

On 12/23/2009 04:19 PM, Marcelo Tosatti wrote:
> On Wed, Dec 23, 2009 at 02:32:55PM +0200, Avi Kivity wrote:
>    
>> On 12/23/2009 01:38 PM, Marcelo Tosatti wrote:
>>      
>>> Use two steps for memslot deletion: mark the slot invalid (which stops
>>> instantiation of new shadow pages for that slot, but allows destruction),
>>> then instantiate the new empty slot.
>>>
>>> Also simplifies kvm_handle_hva locking.
>>>
>>>
>>>    	r = kvm_arch_prepare_memory_region(kvm,&new, old, user_alloc);
>>>    	if (r)
>>>    		goto out_free;
>>>
>>>        
>> r == 0
>>      
> Huh? kvm_arch_prepare_memory_region returns a suitable error code on
> failure, 0 on success.
>    

That's fine here, just pointing out you're inheriting this r == 0 later on.

> kvm_iommu_map_pages returns 0 on success, error code otherwise? The
> error code of kvm_iommu_map_pages was returned before the patchset, BTW.
>    

Ok, then please fix it separately for easier backporting.


>>>    	nr_mmu_pages = nr_pages * KVM_PERMILLE_MMU_PAGES / 1000;
>>>    	nr_mmu_pages = max(nr_mmu_pages,
>>>    			(unsigned int) KVM_MIN_ALLOC_MMU_PAGES);
>>> +	srcu_read_unlock(&kvm->srcu, idx);
>>>
>>>
>>>        
>> Again, would like to move the srcu_locking to an outer scope.
>>      
> This one was for documentation purposes only, since
> kvm_mmu_calculate_mmu_pages is only called from commit_memory_region
> (which mutually excludes itself).
>
> I can remove it if you'd prefer.
>    

A comment should be sufficient.  Otherwise someone might read the code 
and point out it's unnecessary.



>> Shouldn't we already hold the srcu_lock as part of normal guest exit
>> (similar to down_read() we do today)?
>>      
> Yes, but:
>
> kvm_arch_vcpu_setup ->  vmx_vcpu_reset ->  vmx_set_cr0 ->  enter_pmode
>
> So its called outside guest context path (memslot info is used outside
> guest context in this case).
>
>    

Didn't we take slots_lock previously?  If so, that's a bug.  We should 
probably make all vcpu ioctls take slots_lock at top level, since many 
vcpu ioctls can be caused to access memory.

-- 
error compiling committee.c: too many arguments to function