Re: [PATCH 18/20] KVM: arm64: Add PKVM_HYP_REQ_SPLIT

[Vincent Donnefort <vdonnefort@google.com>]

[...]

> > +static void pkvm_mapping_split(struct pkvm_mapping *mapping, struct kvm_pgtable *pgt,
> > +			       struct list_head *spares)
> > +{
> > +	struct kvm *kvm = kvm_s2_mmu_to_kvm(pgt->mmu);
> > +	u64 nr_pages = mapping->nr_pages - 1;
> > +	gfn_t gfn = mapping->gfn + 1;
> > +	u64 pfn = mapping->pfn + 1;
> > +
> > +	lockdep_assert_held_write(&kvm->mmu_lock);
> > +
> > +	pkvm_mapping_remove(mapping, &pgt->pkvm_mappings);
> > +	mapping->nr_pages = 1;
> > +	pkvm_mapping_insert(mapping, &pgt->pkvm_mappings);
> > +
> > +	while (nr_pages--) {
> > +		struct pkvm_mapping *m;
> > +
> > +		if (WARN_ON(list_empty(spares)))
> > +			break;
> > +
> > +		m = list_first_entry(spares, struct pkvm_mapping, list);
> > +		list_del(&m->list);
> > +
> > +		m->nr_pages = 1;
> > +		m->gfn = gfn++;
> > +		m->pfn = pfn++;
> > +
> > +		pkvm_mapping_insert(m, &pgt->pkvm_mappings);
> > +	}
> > +}
> > +
> 
> I don't see anything wrong with this linked list approach, but have you
> considered just allocating an array of pointers for the allocated
> pkvm_mappings? The union in pkvm_mappings can be avoided.

I have, but it doesn't fit the reclaim path which frees pkvm_mapping as a single
object.

> 
> >  int pkvm_pgtable_stage2_init(struct kvm_pgtable *pgt, struct kvm_s2_mmu *mmu,
> >  			     struct kvm_pgtable_mm_ops *mm_ops)
> >  {
> > @@ -619,6 +682,91 @@ int pkvm_pgtable_stage2_split(struct kvm_pgtable *pgt, u64 addr, u64 size, void
> >  	return -EINVAL;
> >  }
> >  
> > +/*
> > + * Splitting is only expected on the back of a guest HVC, while
> > + * pkvm_pgtable_stage2_split() can be called with dirty logging.
> > + */
> > +static int __pkvm_pgtable_stage2_split(struct kvm_vcpu *vcpu, phys_addr_t ipa, u64 size)
> > +{
> > +	struct kvm_hyp_memcache *mc = &vcpu->arch.pkvm_memcache;
> > +	struct kvm_pgtable *pgt = vcpu->arch.hw_mmu->pgt;
> > +	struct page **pages __free(kfree) = NULL;
> > +	struct mm_struct *mm = current->mm;
> > +	struct kvm_memory_slot *memslot;
> > +	struct pkvm_mapping *mapping;
> > +	struct kvm *kvm = vcpu->kvm;
> > +	struct list_head spares;
> > +	unsigned long hva;
> > +	bool writable;
> > +	u64 nr_pages;
> > +	int ret, idx;
> > +	gfn_t gfn;
> > +
> > +	if (WARN_ON(!kvm_vm_is_protected(kvm)))
> > +		return -EINVAL;
> > +
> > +	if (!IS_ALIGNED(ipa, PMD_SIZE) || size != PMD_SIZE)
> > +		return -EINVAL;
> > +
> > +	ret = topup_hyp_memcache(mc, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* We already have 1 pin on the huge-page */
> > +	gfn = gpa_to_gfn(ipa) + 1;
> > +	nr_pages = (size / PAGE_SIZE) - 1;
> > +	pages = kmalloc_objs(struct page *, nr_pages);
> > +	if (!pages)
> > +		return -ENOMEM;
> > +
> > +	INIT_LIST_HEAD(&spares);
> > +	ret = pkvm_mapping_alloc_spares(&spares, nr_pages);
> > +	if (ret)
> > +		return ret;
> > +
> > +	idx = srcu_read_lock(&kvm->srcu);
> > +	memslot = gfn_to_memslot(kvm, gfn);
> > +	hva = gfn_to_hva_memslot_prot(memslot, gfn, &writable);
> > +	if (kvm_is_error_hva(hva) || !writable) {
> > +		ret = -EFAULT;
> > +		goto unlock_srcu;
> > +	}
> > +
> > +	mmap_read_lock(mm);
> > +	ret = pin_user_pages(hva, nr_pages, FOLL_HWPOISON | FOLL_LONGTERM | FOLL_WRITE, pages);
> 
> If I am not mistaken this is for getting additional pins only. hva walk
> and pages[] are just by-product.
> 
> Can we use folio_add_pins() here instead? Then I think we won't need
> pages[], the hva walk, and unpin_user_pages().

Good point, folio_add_pins() sounds way simpler.

-- 
Vincent

> 
> Thanks,
> Wei-Lin Chang
> 
> > +	mmap_read_unlock(mm);
> > +	if (ret != nr_pages) {
> > +		if (ret > 0)
> > +			unpin_user_pages(pages, ret);
> > +		ret = -EFAULT;
> > +		goto unlock_srcu;
> > +	}
> > +
> > +	write_lock(&kvm->mmu_lock);
> > +	mapping = pkvm_mapping_iter_first(&pgt->pkvm_mappings, ipa, ipa + size - 1);
> > +	if (!pkvm_mapping_can_split(mapping)) {
> > +		ret = -EINVAL;
> > +		goto unlock_mmu;
> > +	}
> > +
> > +	ret = kvm_call_hyp_nvhe(__pkvm_host_split_guest, gpa_to_gfn(ipa), size / PAGE_SIZE);
> > +	if (ret)
> > +		goto unlock_mmu;
> > +
> > +	pkvm_mapping_split(mapping, pgt, &spares);
> > +
> > +unlock_mmu:
> > +	write_unlock(&kvm->mmu_lock);
> > +	if (ret)
> > +		unpin_user_pages(pages, nr_pages);
> > +
> > +unlock_srcu:
> > +	srcu_read_unlock(&kvm->srcu, idx);
> > +	pkvm_mapping_free_spares(&spares);
> > +
> > +	return ret;
> > +}
> > +
> 
> [...]
>