Re: [PATCH v7 1/7] kvmppc: Driver to manage pages of secure guest

From
Sukadev Bhattiprolu <hidden>
Date
2019-08-29 03:02:32
Also in
linux-mm
Reply to
Bharata B Rao, 2019-08-22
Some minor comments/questions below. Overall, the patches look
fine to me.
quoted hunk ↗ jump to hunk
+#include <linux/pagemap.h>
+#include <linux/migrate.h>
+#include <linux/kvm_host.h>
+#include <asm/ultravisor.h>
+
+static struct dev_pagemap kvmppc_devm_pgmap;
+static unsigned long *kvmppc_devm_pfn_bitmap;
+static DEFINE_SPINLOCK(kvmppc_devm_pfn_lock);
Is this lock protecting just the pfn_bitmap?
quoted hunk ↗ jump to hunk
+
+struct kvmppc_devm_page_pvt {
+	unsigned long *rmap;
+	unsigned int lpid;
+	unsigned long gpa;
+};
+
+/*
+ * Get a free device PFN from the pool
+ *
+ * Called when a normal page is moved to secure memory (UV_PAGE_IN). Device
+ * PFN will be used to keep track of the secure page on HV side.
+ *
+ * @rmap here is the slot in the rmap array that corresponds to @gpa.
+ * Thus a non-zero rmap entry indicates that the corresponding guest
+ * page has become secure, and is not mapped on the HV side.
+ *
+ * NOTE: In this and subsequent functions, we pass around and access
+ * individual elements of kvm_memory_slot->arch.rmap[] without any
+ * protection. Should we use lock_rmap() here?
+ */
+static struct page *kvmppc_devm_get_page(unsigned long *rmap, unsigned long gpa,
+					 unsigned int lpid)
+{
+	struct page *dpage = NULL;
+	unsigned long bit, devm_pfn;
+	unsigned long flags;
+	struct kvmppc_devm_page_pvt *pvt;
+	unsigned long pfn_last, pfn_first;
+
+	if (kvmppc_rmap_is_devm_pfn(*rmap))
+		return NULL;
+
+	pfn_first = kvmppc_devm_pgmap.res.start >> PAGE_SHIFT;
+	pfn_last = pfn_first +
+		   (resource_size(&kvmppc_devm_pgmap.res) >> PAGE_SHIFT);
+	spin_lock_irqsave(&kvmppc_devm_pfn_lock, flags);
Blank lines around spin_lock() would help.
quoted hunk ↗ jump to hunk
+	bit = find_first_zero_bit(kvmppc_devm_pfn_bitmap, pfn_last - pfn_first);
+	if (bit >= (pfn_last - pfn_first))
+		goto out;
+
+	bitmap_set(kvmppc_devm_pfn_bitmap, bit, 1);
+	devm_pfn = bit + pfn_first;
Can we drop the &kvmppc_devm_pfn_lock here or after the trylock_page()?
Or does it also protect the ->zone_device_data' assignment below as well?
If so, maybe drop the 'pfn_' from the name of the lock?

Besides, we don't seem to hold this lock when accessing ->zone_device_data
in kvmppc_share_page(). Maybe &kvmppc_devm_pfn_lock just protects the bitmap?

+	dpage = pfn_to_page(devm_pfn);
Does this code and hence CONFIG_PPC_UV depend on a specific model like
CONFIG_SPARSEMEM_VMEMMAP?
quoted hunk ↗ jump to hunk
+
+	if (!trylock_page(dpage))
+		goto out_clear;
+
+	*rmap = devm_pfn | KVMPPC_RMAP_DEVM_PFN;
+	pvt = kzalloc(sizeof(*pvt), GFP_ATOMIC);
+	if (!pvt)
+		goto out_unlock;
+	pvt->rmap = rmap;
+	pvt->gpa = gpa;
+	pvt->lpid = lpid;
+	dpage->zone_device_data = pvt;
->zone_device_data is set after locking the dpage here, but in
kvmppc_share_page() and kvmppc_devm_fault_migrate_alloc_and_copy()
it is accessed without locking the page?
quoted hunk ↗ jump to hunk
+	spin_unlock_irqrestore(&kvmppc_devm_pfn_lock, flags);
+
+	get_page(dpage);
+	return dpage;
+
+out_unlock:
+	unlock_page(dpage);
+out_clear:
+	bitmap_clear(kvmppc_devm_pfn_bitmap, devm_pfn - pfn_first, 1);
+out:
+	spin_unlock_irqrestore(&kvmppc_devm_pfn_lock, flags);
+	return NULL;
+}
+
+/*
+ * Alloc a PFN from private device memory pool and copy page from normal
+ * memory to secure memory.
+ */
+static int
+kvmppc_devm_migrate_alloc_and_copy(struct migrate_vma *mig,
+				   unsigned long *rmap, unsigned long gpa,
+				   unsigned int lpid, unsigned long page_shift)
+{
+	struct page *spage = migrate_pfn_to_page(*mig->src);
+	unsigned long pfn = *mig->src >> MIGRATE_PFN_SHIFT;
+	struct page *dpage;
+
+	*mig->dst = 0;
+	if (!spage || !(*mig->src & MIGRATE_PFN_MIGRATE))
+		return 0;
+
+	dpage = kvmppc_devm_get_page(rmap, gpa, lpid);
+	if (!dpage)
+		return -EINVAL;
+
+	if (spage)
+		uv_page_in(lpid, pfn << page_shift, gpa, 0, page_shift);
+
+	*mig->dst = migrate_pfn(page_to_pfn(dpage)) | MIGRATE_PFN_LOCKED;
+	return 0;
+}
+
+/*
+ * Move page from normal memory to secure memory.
+ */
+unsigned long
+kvmppc_h_svm_page_in(struct kvm *kvm, unsigned long gpa,
+		     unsigned long flags, unsigned long page_shift)
+{
+	unsigned long addr, end;
+	unsigned long src_pfn, dst_pfn;
These are the host frame numbers correct? Trying to distinguish them
from 'gfn' and 'gpa' used in the function.
quoted hunk ↗ jump to hunk
+	struct migrate_vma mig;
+	struct vm_area_struct *vma;
+	int srcu_idx;
+	unsigned long gfn = gpa >> page_shift;
+	struct kvm_memory_slot *slot;
+	unsigned long *rmap;
+	int ret;
+
+	if (page_shift != PAGE_SHIFT)
+		return H_P3;
+
+	if (flags)
+		return H_P2;
+
+	ret = H_PARAMETER;
+	down_read(&kvm->mm->mmap_sem);
+	srcu_idx = srcu_read_lock(&kvm->srcu);
+	slot = gfn_to_memslot(kvm, gfn);
Can slot be NULL? could be a bug in UV...
+	rmap = &slot->arch.rmap[gfn - slot->base_gfn];
+	addr = gfn_to_hva(kvm, gpa >> page_shift);
Use 'gfn' as the second parameter? 

Nit. for consistency with gpa and gfn, maybe rename 'addr' to
'hva' or to match 'end' maybe to 'start'.

Also, can we check 'kvmppc_rmap_is_devm_pfn(*rmap)' here and bail out
if its already shared? We currently do it further down the call chain
in kvmppc_devm_get_page() after doing more work.

quoted hunk ↗ jump to hunk
+	if (kvm_is_error_hva(addr))
+		goto out;
+
+	end = addr + (1UL << page_shift);
+	vma = find_vma_intersection(kvm->mm, addr, end);
+	if (!vma || vma->vm_start > addr || vma->vm_end < end)
+		goto out;
+
+	memset(&mig, 0, sizeof(mig));
+	mig.vma = vma;
+	mig.start = addr;
+	mig.end = end;
+	mig.src = &src_pfn;
+	mig.dst = &dst_pfn;
+
+	if (migrate_vma_setup(&mig))
+		goto out;
+
+	if (kvmppc_devm_migrate_alloc_and_copy(&mig, rmap, gpa,
+					       kvm->arch.lpid, page_shift))
+		goto out_finalize;
+
+	migrate_vma_pages(&mig);
+	ret = H_SUCCESS;
+out_finalize:
+	migrate_vma_finalize(&mig);
+out:
+	srcu_read_unlock(&kvm->srcu, srcu_idx);
+	up_read(&kvm->mm->mmap_sem);
+	return ret;
+}
+
+/*
+ * Provision a new page on HV side and copy over the contents
+ * from secure memory.
+ */
+static int
+kvmppc_devm_fault_migrate_alloc_and_copy(struct migrate_vma *mig,
+					 unsigned long page_shift)
+{
+	struct page *dpage, *spage;
+	struct kvmppc_devm_page_pvt *pvt;
+	unsigned long pfn;
+	int ret;
+
+	spage = migrate_pfn_to_page(*mig->src);
+	if (!spage || !(*mig->src & MIGRATE_PFN_MIGRATE))
+		return 0;
+	if (!is_zone_device_page(spage))
+		return 0;
What does it mean if its not a zone_device page at this point? Caller
would then proceed to migrage_vma_pages() if we return 0 right?
quoted hunk ↗ jump to hunk
+
+	dpage = alloc_page_vma(GFP_HIGHUSER, mig->vma, mig->start);
+	if (!dpage)
+		return -EINVAL;
+	lock_page(dpage);
+	pvt = spage->zone_device_data;
+
+	pfn = page_to_pfn(dpage);
+	ret = uv_page_out(pvt->lpid, pfn << page_shift, pvt->gpa, 0,
+			  page_shift);
+	if (ret == U_SUCCESS)
+		*mig->dst = migrate_pfn(pfn) | MIGRATE_PFN_LOCKED;
+	else {
+		unlock_page(dpage);
+		__free_page(dpage);
+	}
+	return ret;
+}
+
+/*
+ * Fault handler callback when HV touches any page that has been
+ * moved to secure memory, we ask UV to give back the page by
+ * issuing a UV_PAGE_OUT uvcall.
+ *
+ * This eventually results in dropping of device PFN and the newly
+ * provisioned page/PFN gets populated in QEMU page tables.
+ */
+static vm_fault_t kvmppc_devm_migrate_to_ram(struct vm_fault *vmf)
+{
+	unsigned long src_pfn, dst_pfn = 0;
+	struct migrate_vma mig;
+	int ret = 0;
+
+	memset(&mig, 0, sizeof(mig));
+	mig.vma = vmf->vma;
+	mig.start = vmf->address;
+	mig.end = vmf->address + PAGE_SIZE;
+	mig.src = &src_pfn;
+	mig.dst = &dst_pfn;
+
+	if (migrate_vma_setup(&mig)) {
+		ret = VM_FAULT_SIGBUS;
+		goto out;
+	}
+
+	if (kvmppc_devm_fault_migrate_alloc_and_copy(&mig, PAGE_SHIFT)) {
+		ret = VM_FAULT_SIGBUS;
+		goto out_finalize;
+	}
+
+	migrate_vma_pages(&mig);
+out_finalize:
+	migrate_vma_finalize(&mig);
+out:
+	return ret;
+}
+
+/*
+ * Release the device PFN back to the pool
+ *
+ * Gets called when secure page becomes a normal page during UV_PAGE_OUT.
Nit: Should that be H_SVM_PAGE_OUT?
quoted hunk ↗ jump to hunk
+ */
+static void kvmppc_devm_page_free(struct page *page)
+{
+	unsigned long pfn = page_to_pfn(page);
+	unsigned long flags;
+	struct kvmppc_devm_page_pvt *pvt;
+
+	spin_lock_irqsave(&kvmppc_devm_pfn_lock, flags);
+	pvt = page->zone_device_data;
+	page->zone_device_data = NULL;
If the pfn_lock only protects the bitmap, would be better to move
it here?
quoted hunk ↗ jump to hunk
+
+	bitmap_clear(kvmppc_devm_pfn_bitmap,
+		     pfn - (kvmppc_devm_pgmap.res.start >> PAGE_SHIFT), 1);
+	*pvt->rmap = 0;
+	spin_unlock_irqrestore(&kvmppc_devm_pfn_lock, flags);
+	kfree(pvt);
+}
+
+static const struct dev_pagemap_ops kvmppc_devm_ops = {
+	.page_free = kvmppc_devm_page_free,
+	.migrate_to_ram	= kvmppc_devm_migrate_to_ram,
+};
+
+/*
+ * Move page from secure memory to normal memory.
+ */
+unsigned long
+kvmppc_h_svm_page_out(struct kvm *kvm, unsigned long gpa,
+		      unsigned long flags, unsigned long page_shift)
+{
+	struct migrate_vma mig;
+	unsigned long addr, end;
+	struct vm_area_struct *vma;
+	unsigned long src_pfn, dst_pfn = 0;
+	int srcu_idx;
+	int ret;
Nit: Not sure its a coding style requirement, but many functions seem
to "sort" these local variables in descending order of line length for
appearance :-)  (eg: migrate_vma* functions).
quoted hunk ↗ jump to hunk
+
+	if (page_shift != PAGE_SHIFT)
+		return H_P3;
+
+	if (flags)
+		return H_P2;
+
+	ret = H_PARAMETER;
+	down_read(&kvm->mm->mmap_sem);
+	srcu_idx = srcu_read_lock(&kvm->srcu);
+	addr = gfn_to_hva(kvm, gpa >> page_shift);
+	if (kvm_is_error_hva(addr))
+		goto out;
+
+	end = addr + (1UL << page_shift);
+	vma = find_vma_intersection(kvm->mm, addr, end);
+	if (!vma || vma->vm_start > addr || vma->vm_end < end)
+		goto out;
+
+	memset(&mig, 0, sizeof(mig));
+	mig.vma = vma;
+	mig.start = addr;
+	mig.end = end;
+	mig.src = &src_pfn;
+	mig.dst = &dst_pfn;
+	if (migrate_vma_setup(&mig))
+		goto out;
+
+	ret = kvmppc_devm_fault_migrate_alloc_and_copy(&mig, page_shift);
+	if (ret)
+		goto out_finalize;
+
+	migrate_vma_pages(&mig);
+	ret = H_SUCCESS;
Nit: Blank line here?
quoted hunk ↗ jump to hunk
+out_finalize:
+	migrate_vma_finalize(&mig);
+out:
+	srcu_read_unlock(&kvm->srcu, srcu_idx);
+	up_read(&kvm->mm->mmap_sem);
+	return ret;
+}
+
+static u64 kvmppc_get_secmem_size(void)
+{
+	struct device_node *np;
+	int i, len;
+	const __be32 *prop;
+	u64 size = 0;
+
+	np = of_find_compatible_node(NULL, NULL, "ibm,uv-firmware");
+	if (!np)
+		goto out;
+
+	prop = of_get_property(np, "secure-memory-ranges", &len);
+	if (!prop)
+		goto out_put;
+
+	for (i = 0; i < len / (sizeof(*prop) * 4); i++)
+		size += of_read_number(prop + (i * 4) + 2, 2);
+
+out_put:
+	of_node_put(np);
+out:
+	return size;
+}
+
+int kvmppc_devm_init(void)
+{
+	int ret = 0;
+	unsigned long size;
+	struct resource *res;
+	void *addr;
+	unsigned long pfn_last, pfn_first;
+
+	size = kvmppc_get_secmem_size();
+	if (!size) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	res = request_free_mem_region(&iomem_resource, size, "kvmppc_devm");
+	if (IS_ERR(res)) {
+		ret = PTR_ERR(res);
+		goto out;
+	}
+
+	kvmppc_devm_pgmap.type = MEMORY_DEVICE_PRIVATE;
+	kvmppc_devm_pgmap.res = *res;
+	kvmppc_devm_pgmap.ops = &kvmppc_devm_ops;
+	addr = memremap_pages(&kvmppc_devm_pgmap, -1);
+	if (IS_ERR(addr)) {
+		ret = PTR_ERR(addr);
+		goto out_free_region;
+	}
+
+	pfn_first = res->start >> PAGE_SHIFT;
+	pfn_last = pfn_first + (resource_size(res) >> PAGE_SHIFT);
+	kvmppc_devm_pfn_bitmap = kcalloc(BITS_TO_LONGS(pfn_last - pfn_first),
+					 sizeof(unsigned long), GFP_KERNEL);
+	if (!kvmppc_devm_pfn_bitmap) {
+		ret = -ENOMEM;
+		goto out_unmap;
+	}
+
+	pr_info("KVMPPC-DEVM: Secure Memory size 0x%lx\n", size);
+	return ret;
Nit: Blank line here?
+out_unmap:
+	memunmap_pages(&kvmppc_devm_pgmap);
+out_free_region:
+	release_mem_region(res->start, size);
+out:
+	return ret;
+}
+
+void kvmppc_devm_free(void)
+{
+	memunmap_pages(&kvmppc_devm_pgmap);
+	release_mem_region(kvmppc_devm_pgmap.res.start,
+			   resource_size(&kvmppc_devm_pgmap.res));
+	kfree(kvmppc_devm_pfn_bitmap);
+}
-- 
2.21.0
Thread (25 messages) 25 messages, 5 authors, last reply 2019-09-06
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help