From: Christoph Hellwig <hch@lst.de>
To: Bharata B Rao <bharata@linux.ibm.com>
Cc: linuxppc-dev@lists.ozlabs.org, kvm-ppc@vger.kernel.org,
linux-mm@kvack.org, paulus@au1.ibm.com,
aneesh.kumar@linux.vnet.ibm.com, jglisse@redhat.com,
linuxram@us.ibm.com, sukadev@linux.vnet.ibm.com,
cclaudio@linux.ibm.com, hch@lst.de
Subject: Re: [PATCH v6 1/7] kvmppc: Driver to manage pages of secure guest
Date: Sat, 10 Aug 2019 12:58:19 +0200 [thread overview]
Message-ID: <20190810105819.GA26030@lst.de> (raw)
In-Reply-To: <20190809084108.30343-2-bharata@linux.ibm.com>
> +#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.
next prev parent reply other threads:[~2019-08-10 10:58 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-09 8:41 [PATCH v6 0/7] KVMPPC driver to manage secure guest pages Bharata B Rao
2019-08-09 8:41 ` [PATCH v6 1/7] kvmppc: Driver to manage pages of secure guest Bharata B Rao
2019-08-10 10:58 ` Christoph Hellwig [this message]
2019-08-10 14:21 ` Bharata B Rao
2019-08-20 3:04 ` Thiago Jung Bauermann
2019-08-22 3:29 ` Bharata B Rao
2019-08-20 6:22 ` Suraj Jitindar Singh
2019-08-20 6:44 ` Bharata B Rao
2019-08-09 8:41 ` [PATCH v6 2/7] kvmppc: Shared pages support for secure guests Bharata B Rao
2019-08-09 8:41 ` [PATCH v6 3/7] kvmppc: H_SVM_INIT_START and H_SVM_INIT_DONE hcalls Bharata B Rao
2019-08-09 8:41 ` [PATCH v6 4/7] kvmppc: Handle memory plug/unplug to secure VM Bharata B Rao
2019-08-09 8:41 ` [PATCH v6 5/7] kvmppc: Radix changes for secure guest Bharata B Rao
2019-08-09 8:41 ` [PATCH v6 6/7] kvmppc: Support reset of " Bharata B Rao
2019-08-09 8:41 ` [PATCH v6 7/7] KVM: PPC: Ultravisor: Add PPC_UV config option Bharata B Rao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190810105819.GA26030@lst.de \
--to=hch@lst.de \
--cc=aneesh.kumar@linux.vnet.ibm.com \
--cc=bharata@linux.ibm.com \
--cc=cclaudio@linux.ibm.com \
--cc=jglisse@redhat.com \
--cc=kvm-ppc@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=linuxram@us.ibm.com \
--cc=paulus@au1.ibm.com \
--cc=sukadev@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).