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 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

>
> -	spin_lock(&kvm->mmu_lock);
> -	if (mem->slot>= kvm->memslots->nmemslots)
> -		kvm->memslots->nmemslots = mem->slot + 1;
> +#ifdef CONFIG_DMAR
> +	/* map the pages in iommu page table */
> +	if (npages)
> +		r = kvm_iommu_map_pages(kvm,&new);
> +		if (r)
> +			goto out_free;
>    

Bad indentation.

(still r == 0)

> +#endif
>
> -	*memslot = new;
> -	spin_unlock(&kvm->mmu_lock);
> +	slots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL);
> +	if (!slots)
> +		goto out_free;
>    

goto out_free with r == 0.

Best to assign r = -ENOMEM each time, to avoid these headaches.

>
> +int memslot_id(struct kvm *kvm, gfn_t gfn)
>    

Should be either static or kvm_memslot_id().  But the source is already 
like that, so leave it.

> @@ -807,23 +808,18 @@ static int kvm_handle_hva(struct kvm *kv
>   			  int (*handler)(struct kvm *kvm, unsigned long *rmapp,
>   					 unsigned long data))
>   {
> -	int i, j;
> +	int i, j, idx;
>   	int retval = 0;
> -	struct kvm_memslots *slots = kvm->memslots;
> +	struct kvm_memslots *slots;
> +
> +	idx = srcu_read_lock(&kvm->srcu);
>    

Maybe a better place is in the mmu notifiers, so the code is shared 
(once other archs start to use mmu notifiers).


> @@ -3020,16 +3017,20 @@ nomem:
>    */
>   unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm)
>   {
> -	int i;
> +	int i, idx;
>   	unsigned int nr_mmu_pages;
>   	unsigned int  nr_pages = 0;
> +	struct kvm_memslots *slots;
>
> -	for (i = 0; i<  kvm->memslots->nmemslots; i++)
> -		nr_pages += kvm->memslots->memslots[i].npages;
> +	idx = srcu_read_lock(&kvm->srcu);
> +	slots = rcu_dereference(kvm->memslots);
> +	for (i = 0; i<  slots->nmemslots; i++)
> +		nr_pages += slots->memslots[i].npages;
>
>   	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.

> @@ -1503,10 +1504,18 @@ static void enter_pmode(struct kvm_vcpu
>   static gva_t rmode_tss_base(struct kvm *kvm)
>   {
>   	if (!kvm->arch.tss_addr) {
> -		gfn_t base_gfn = kvm->memslots->memslots[0].base_gfn +
> -				 kvm->memslots->memslots[0].npages - 3;
> +		struct kvm_memslots *slots;
> +		gfn_t base_gfn;
> +		int idx;
> +
> +		idx = srcu_read_lock(&kvm->srcu);
> +		slots = rcu_dereference(kvm->memslots);
> + 		base_gfn = slots->memslots[0].base_gfn +
> +				 slots->memslots[0].npages - 3;
> +		srcu_read_unlock(&kvm->srcu, idx);
>   		return base_gfn<<  PAGE_SHIFT;
>   	}
> +
>   	return kvm->arch.tss_addr;
>   }
>    

Shouldn't we already hold the srcu_lock as part of normal guest exit 
(similar to down_read() we do today)?

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