> +#ifdef CONFIG_PPC_UV
> +extern unsigned long kvmppc_h_svm_page_in(struct kvm *kvm,
> + unsigned long gra,
> + unsigned long flags,
> + unsigned long page_shift);
> +extern unsigned long kvmppc_h_svm_page_out(struct kvm *kvm,
> + unsigned long gra,
> + unsigned long flags,
> + unsigned long page_shift);
No need for externs on function declarations.
> +struct kvmppc_devm_device {
> + struct device dev;
> + dev_t devt;
> + struct dev_pagemap pagemap;
> + unsigned long pfn_first, pfn_last;
> + unsigned long *pfn_bitmap;
> +};
We shouldn't really need this conaining structucture given that there
is only a single global instance of it anyway.
> +struct kvmppc_devm_copy_args {
> + unsigned long *rmap;
> + unsigned int lpid;
> + unsigned long gpa;
> + unsigned long page_shift;
> +};
Do we really need this args structure? It is just used in a single
function call where passing the arguments might be cleaner.
> +static void kvmppc_devm_put_page(struct page *page)
> +{
> + unsigned long pfn = page_to_pfn(page);
> + unsigned long flags;
> + struct kvmppc_devm_page_pvt *pvt;
> +
> + spin_lock_irqsave(&kvmppc_devm_lock, flags);
> + pvt = (struct kvmppc_devm_page_pvt *)page->zone_device_data;
No need for the cast.
> + page->zone_device_data = 0;
This should be NULL.
> +
> + bitmap_clear(kvmppc_devm.pfn_bitmap,
> + pfn - kvmppc_devm.pfn_first, 1);
> + *(pvt->rmap) = 0;
No need for the braces.
> + dpage = alloc_page_vma(GFP_HIGHUSER, mig->vma, mig->start);
> + if (!dpage)
> + return -EINVAL;
> + lock_page(dpage);
> + pvt = (struct kvmppc_devm_page_pvt *)spage->zone_device_data;
No need for the cast here.
> +static void kvmppc_devm_page_free(struct page *page)
> +{
> + kvmppc_devm_put_page(page);
> +}
This seems to be the only caller of kvmppc_devm_put_page, any reason
not to just merge the two functions?
> +static int kvmppc_devm_pages_init(void)
> +{
> + unsigned long nr_pfns = kvmppc_devm.pfn_last -
> + kvmppc_devm.pfn_first;
> +
> + kvmppc_devm.pfn_bitmap = kcalloc(BITS_TO_LONGS(nr_pfns),
> + sizeof(unsigned long), GFP_KERNEL);
> + if (!kvmppc_devm.pfn_bitmap)
> + return -ENOMEM;
> +
> + spin_lock_init(&kvmppc_devm_lock);
Just initialize the spinlock using DEFINE_SPINLOCK() at compile time.
The rest of the function is so trivial that it can be inlined into the
caller.
Also is kvmppc_devm_lock such a good name? This mostly just protects
the allocation bitmap, so reflecting that in the name might be a good
idea.
> +int kvmppc_devm_init(void)
> +{
> + int ret = 0;
> + unsigned long size;
> + struct resource *res;
> + void *addr;
> +
> + size = kvmppc_get_secmem_size();
> + if (!size) {
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + ret = alloc_chrdev_region(&kvmppc_devm.devt, 0, 1,
> + "kvmppc-devm");
> + if (ret)
> + goto out;
> +
> + dev_set_name(&kvmppc_devm.dev, "kvmppc_devm_device%d", 0);
> + kvmppc_devm.dev.release = kvmppc_devm_release;
> + device_initialize(&kvmppc_devm.dev);
> + res = devm_request_free_mem_region(&kvmppc_devm.dev,
> + &iomem_resource, size);
> + if (IS_ERR(res)) {
> + ret = PTR_ERR(res);
> + goto out_unregister;
> + }
> +
> + kvmppc_devm.pagemap.type = MEMORY_DEVICE_PRIVATE;
> + kvmppc_devm.pagemap.res = *res;
> + kvmppc_devm.pagemap.ops = &kvmppc_devm_ops;
> + addr = devm_memremap_pages(&kvmppc_devm.dev, &kvmppc_devm.pagemap);
> + if (IS_ERR(addr)) {
> + ret = PTR_ERR(addr);
> + goto out_unregister;
> + }
It seems a little silly to allocate a struct device just so that we can
pass it to devm_request_free_mem_region and devm_memremap_pages. I think
we should just create non-dev_ versions of those as well.
> +
> + kvmppc_devm.pfn_first = res->start >> PAGE_SHIFT;
> + kvmppc_devm.pfn_last = kvmppc_devm.pfn_first +
> + (resource_size(res) >> PAGE_SHIFT);
pfn_last is only used to calculat a size. Also I think we could
just use kvmppc_devm.pagemap.res directly instead of copying these
values out. the ">> PAGE_SHIFT" is cheap enough.
