Re: [PATCH v6 1/7] kvmppc: Driver to manage pages of secure guest
On Tue, Aug 20, 2019 at 12:04:33AM -0300, Thiago Jung Bauermann wrote:
>
> Hello Bharata,
>
> I have just a couple of small comments.
>
> Bharata B Rao writes:
>
> > +/*
> > + * 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 corresonding guest
>
> Typo: corresponding
>
> > +static u64 kvmppc_get_secmem_size(void)
> > +{
> > + struct device_node *np;
> > + int i, len;
> > + const __be32 *prop;
> > + u64 size = 0;
> > +
> > + np = of_find_node_by_path("/ibm,ultravisor/ibm,uv-firmware");
> > + if (!np)
> > + goto out;
>
> I believe that in general we try to avoid hard-coding the path when a
> node is accessed and searched instead via its compatible property.
Thanks, will switch to of_find_compatible_node().
Regards,
Bharata.
Re: [PATCH v6 1/7] kvmppc: Driver to manage pages of secure guest
On Tue, Aug 20, 2019 at 04:22:15PM +1000, Suraj Jitindar Singh wrote:
> On Fri, 2019-08-09 at 14:11 +0530, Bharata B Rao wrote:
> > KVMPPC driver to manage page transitions of secure guest
> > via H_SVM_PAGE_IN and H_SVM_PAGE_OUT hcalls.
> >
> > H_SVM_PAGE_IN: Move the content of a normal page to secure page
> > H_SVM_PAGE_OUT: Move the content of a secure page to normal page
> >
> > Private ZONE_DEVICE memory equal to the amount of secure memory
> > available in the platform for running secure guests is created
> > via a char device. Whenever a page belonging to the guest becomes
> > secure, a page from this private device memory is used to
> > represent and track that secure page on the HV side. The movement
> > of pages between normal and secure memory is done via
> > migrate_vma_pages() using UV_PAGE_IN and UV_PAGE_OUT ucalls.
>
> Hi Bharata,
>
> please see my patch where I define the bits which define the type of
> the rmap entry:
> https://patchwork.ozlabs.org/patch/1149791/
>
> Please add an entry for the devm pfn type like:
> #define KVMPPC_RMAP_PFN_DEVM 0x0200 /* secure guest devm
> pfn */
>
> And the following in the appropriate header file
>
> static inline bool kvmppc_rmap_is_pfn_demv(unsigned long *rmapp)
> {
> return !!((*rmapp & KVMPPC_RMAP_TYPE_MASK) ==
> KVMPPC_RMAP_PFN_DEVM));
> }
>
Sure, I have the equivalents defined locally, will move to appropriate
headers.
> Also see comment below.
>
> > +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 nr_pfns = kvmppc_devm.pfn_last -
> > + kvmppc_devm.pfn_first;
> > + unsigned long flags;
> > + struct kvmppc_devm_page_pvt *pvt;
> > +
> > + if (kvmppc_is_devm_pfn(*rmap))
> > + return NULL;
> > +
> > + spin_lock_irqsave(_devm_lock, flags);
> > + bit = find_first_zero_bit(kvmppc_devm.pfn_bitmap, nr_pfns);
> > + if (bit >= nr_pfns)
> > + goto out;
> > +
> > + bitmap_set(kvmppc_devm.pfn_bitmap, bit, 1);
> > + devm_pfn = bit + kvmppc_devm.pfn_first;
> > + dpage = pfn_to_page(devm_pfn);
> > +
> > + if (!trylock_page(dpage))
> > + goto out_clear;
> > +
> > + *rmap = devm_pfn | KVMPPC_PFN_DEVM;
> > + pvt = kzalloc(sizeof(*pvt), GFP_ATOMIC);
> > + if (!pvt)
> > + goto out_unlock;
> > + pvt->rmap = rmap;
>
> Am I missing something, why does the rmap need to be stored in pvt?
> Given the gpa is already stored and this is enough to get back to the
> rmap entry, right?
I use rmap entry to note that this guest page is secure and is being
represented by device memory page on the HV side. When the page becomes
normal again, I need to undo that from dev_pagemap_ops.page_free()
where I don't have gpa.
Regards,
Bharata.
Re: [PATCH v6 1/7] kvmppc: Driver to manage pages of secure guest
On Fri, 2019-08-09 at 14:11 +0530, Bharata B Rao wrote:
> KVMPPC driver to manage page transitions of secure guest
> via H_SVM_PAGE_IN and H_SVM_PAGE_OUT hcalls.
>
> H_SVM_PAGE_IN: Move the content of a normal page to secure page
> H_SVM_PAGE_OUT: Move the content of a secure page to normal page
>
> Private ZONE_DEVICE memory equal to the amount of secure memory
> available in the platform for running secure guests is created
> via a char device. Whenever a page belonging to the guest becomes
> secure, a page from this private device memory is used to
> represent and track that secure page on the HV side. The movement
> of pages between normal and secure memory is done via
> migrate_vma_pages() using UV_PAGE_IN and UV_PAGE_OUT ucalls.
Hi Bharata,
please see my patch where I define the bits which define the type of
the rmap entry:
https://patchwork.ozlabs.org/patch/1149791/
Please add an entry for the devm pfn type like:
#define KVMPPC_RMAP_PFN_DEVM 0x0200 /* secure guest devm
pfn */
And the following in the appropriate header file
static inline bool kvmppc_rmap_is_pfn_demv(unsigned long *rmapp)
{
return !!((*rmapp & KVMPPC_RMAP_TYPE_MASK) ==
KVMPPC_RMAP_PFN_DEVM));
}
Also see comment below.
Thanks,
Suraj
>
> Signed-off-by: Bharata B Rao
> ---
> arch/powerpc/include/asm/hvcall.h | 4 +
> arch/powerpc/include/asm/kvm_book3s_devm.h | 29 ++
> arch/powerpc/include/asm/kvm_host.h| 12 +
> arch/powerpc/include/asm/ultravisor-api.h | 2 +
> arch/powerpc/include/asm/ultravisor.h | 14 +
> arch/powerpc/kvm/Makefile | 3 +
> arch/powerpc/kvm/book3s_hv.c | 19 +
> arch/powerpc/kvm/book3s_hv_devm.c | 492
> +
> 8 files changed, 575 insertions(+)
> create mode 100644 arch/powerpc/include/asm/kvm_book3s_devm.h
> create mode 100644 arch/powerpc/kvm/book3s_hv_devm.c
>
[snip]
> +
> +struct kvmppc_devm_page_pvt {
> + unsigned long *rmap;
> + unsigned int lpid;
> + unsigned long gpa;
> +};
> +
> +struct kvmppc_devm_copy_args {
> + unsigned long *rmap;
> + unsigned int lpid;
> + unsigned long gpa;
> + unsigned long page_shift;
> +};
> +
> +/*
> + * Bits 60:56 in the rmap entry will be used to identify the
> + * different uses/functions of rmap. This definition with move
> + * to a proper header when all other functions are defined.
> + */
> +#define KVMPPC_PFN_DEVM (0x2ULL << 56)
> +
> +static inline bool kvmppc_is_devm_pfn(unsigned long pfn)
> +{
> + return !!(pfn & KVMPPC_PFN_DEVM);
> +}
> +
> +/*
> + * 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 corresonding 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 nr_pfns = kvmppc_devm.pfn_last -
> + kvmppc_devm.pfn_first;
> + unsigned long flags;
> + struct kvmppc_devm_page_pvt *pvt;
> +
> + if (kvmppc_is_devm_pfn(*rmap))
> + return NULL;
> +
> + spin_lock_irqsave(_devm_lock, flags);
> + bit = find_first_zero_bit(kvmppc_devm.pfn_bitmap, nr_pfns);
> + if (bit >= nr_pfns)
> + goto out;
> +
> + bitmap_set(kvmppc_devm.pfn_bitmap, bit, 1);
> + devm_pfn = bit + kvmppc_devm.pfn_first;
> + dpage = pfn_to_page(devm_pfn);
> +
> + if (!trylock_page(dpage))
> + goto out_clear;
> +
> + *rmap = devm_pfn | KVMPPC_PFN_DEVM;
> + pvt = kzalloc(sizeof(*pvt), GFP_ATOMIC);
> + if (!pvt)
> + goto out_unlock;
> + pvt->rmap = rmap;
Am I missing something, why does the rmap need to be stored in pvt?
Given the gpa is already stored and this is enough to get back to the
rmap entry, right?
> + pvt->gpa = gpa;
> + pvt->lpid = lpid;
> + dpage->zone_device_data = pvt;
> + spin_unlock_irqrestore(_devm_lock, flags);
> +
> + get_page(dpage);
> + return dpage;
> +
> +out_unlock:
> + unlock_page(dpage);
> +out_clear:
> + bitmap_clear(kvmppc_devm.pfn_bitmap,
> + devm_pfn - kvmppc_devm.pfn_first, 1);
> +out:
> + spin_unlock_irqrestore(_devm_lock, flags);
> + return NULL;
> +}
> +
>
[snip]
Re: [PATCH v6 1/7] kvmppc: Driver to manage pages of secure guest
Hello Bharata,
I have just a couple of small comments.
Bharata B Rao writes:
> +/*
> + * 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 corresonding guest
Typo: corresponding
> +static u64 kvmppc_get_secmem_size(void)
> +{
> + struct device_node *np;
> + int i, len;
> + const __be32 *prop;
> + u64 size = 0;
> +
> + np = of_find_node_by_path("/ibm,ultravisor/ibm,uv-firmware");
> + if (!np)
> + goto out;
I believe that in general we try to avoid hard-coding the path when a
node is accessed and searched instead via its compatible property.
--
Thiago Jung Bauermann
IBM Linux Technology Center
Re: [PATCH v6 1/7] kvmppc: Driver to manage pages of secure guest
On Sat, Aug 10, 2019 at 12:58:19PM +0200, Christoph Hellwig wrote:
>
> > +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(_devm.devt, 0, 1,
> > + "kvmppc-devm");
> > + if (ret)
> > + goto out;
> > +
> > + dev_set_name(_devm.dev, "kvmppc_devm_device%d", 0);
> > + kvmppc_devm.dev.release = kvmppc_devm_release;
> > + device_initialize(_devm.dev);
> > + res = devm_request_free_mem_region(_devm.dev,
> > + _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 = _devm_ops;
> > + addr = devm_memremap_pages(_devm.dev, _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.
There is no reason for us to create a device really. If non-dev versions
of the above two routines are present, I can switch.
I will take care of the rest of your comments. Thanks for the review.
Regards,
Bharata.
Re: [PATCH v6 1/7] kvmppc: Driver to manage pages of secure guest
> +#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(_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(_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(_devm.devt, 0, 1,
> + "kvmppc-devm");
> + if (ret)
> + goto out;
> +
> + dev_set_name(_devm.dev, "kvmppc_devm_device%d", 0);
> + kvmppc_devm.dev.release = kvmppc_devm_release;
> + device_initialize(_devm.dev);
> + res = devm_request_free_mem_region(_devm.dev,
> + _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 = _devm_ops;
> + addr = devm_memremap_pages(_devm.dev, _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.
[PATCH v6 1/7] kvmppc: Driver to manage pages of secure guest
KVMPPC driver to manage page transitions of secure guest
via H_SVM_PAGE_IN and H_SVM_PAGE_OUT hcalls.
H_SVM_PAGE_IN: Move the content of a normal page to secure page
H_SVM_PAGE_OUT: Move the content of a secure page to normal page
Private ZONE_DEVICE memory equal to the amount of secure memory
available in the platform for running secure guests is created
via a char device. Whenever a page belonging to the guest becomes
secure, a page from this private device memory is used to
represent and track that secure page on the HV side. The movement
of pages between normal and secure memory is done via
migrate_vma_pages() using UV_PAGE_IN and UV_PAGE_OUT ucalls.
Signed-off-by: Bharata B Rao
---
arch/powerpc/include/asm/hvcall.h | 4 +
arch/powerpc/include/asm/kvm_book3s_devm.h | 29 ++
arch/powerpc/include/asm/kvm_host.h| 12 +
arch/powerpc/include/asm/ultravisor-api.h | 2 +
arch/powerpc/include/asm/ultravisor.h | 14 +
arch/powerpc/kvm/Makefile | 3 +
arch/powerpc/kvm/book3s_hv.c | 19 +
arch/powerpc/kvm/book3s_hv_devm.c | 492 +
8 files changed, 575 insertions(+)
create mode 100644 arch/powerpc/include/asm/kvm_book3s_devm.h
create mode 100644 arch/powerpc/kvm/book3s_hv_devm.c
diff --git a/arch/powerpc/include/asm/hvcall.h
b/arch/powerpc/include/asm/hvcall.h
index 463c63a9fcf1..2f6b952deb0f 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -337,6 +337,10 @@
#define H_TLB_INVALIDATE 0xF808
#define H_COPY_TOFROM_GUEST0xF80C
+/* Platform-specific hcalls used by the Ultravisor */
+#define H_SVM_PAGE_IN 0xEF00
+#define H_SVM_PAGE_OUT 0xEF04
+
/* Values for 2nd argument to H_SET_MODE */
#define H_SET_MODE_RESOURCE_SET_CIABR 1
#define H_SET_MODE_RESOURCE_SET_DAWR 2
diff --git a/arch/powerpc/include/asm/kvm_book3s_devm.h
b/arch/powerpc/include/asm/kvm_book3s_devm.h
new file mode 100644
index ..21f3de5f2acb
--- /dev/null
+++ b/arch/powerpc/include/asm/kvm_book3s_devm.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __POWERPC_KVM_PPC_HMM_H__
+#define __POWERPC_KVM_PPC_HMM_H__
+
+#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);
+#else
+static inline unsigned long
+kvmppc_h_svm_page_in(struct kvm *kvm, unsigned long gra,
+unsigned long flags, unsigned long page_shift)
+{
+ return H_UNSUPPORTED;
+}
+
+static inline unsigned long
+kvmppc_h_svm_page_out(struct kvm *kvm, unsigned long gra,
+ unsigned long flags, unsigned long page_shift)
+{
+ return H_UNSUPPORTED;
+}
+#endif /* CONFIG_PPC_UV */
+#endif /* __POWERPC_KVM_PPC_HMM_H__ */
diff --git a/arch/powerpc/include/asm/kvm_host.h
b/arch/powerpc/include/asm/kvm_host.h
index 4bb552d639b8..86bbe607ad7e 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -849,4 +849,16 @@ static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu
*vcpu) {}
static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {}
static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {}
+#ifdef CONFIG_PPC_UV
+extern int kvmppc_devm_init(void);
+extern void kvmppc_devm_free(void);
+#else
+static inline int kvmppc_devm_init(void)
+{
+ return 0;
+}
+
+static inline void kvmppc_devm_free(void) {}
+#endif /* CONFIG_PPC_UV */
+
#endif /* __POWERPC_KVM_HOST_H__ */
diff --git a/arch/powerpc/include/asm/ultravisor-api.h
b/arch/powerpc/include/asm/ultravisor-api.h
index 6a0f9c74f959..1cd1f595fd81 100644
--- a/arch/powerpc/include/asm/ultravisor-api.h
+++ b/arch/powerpc/include/asm/ultravisor-api.h
@@ -25,5 +25,7 @@
/* opcodes */
#define UV_WRITE_PATE 0xF104
#define UV_RETURN 0xF11C
+#define UV_PAGE_IN 0xF128
+#define UV_PAGE_OUT0xF12C
#endif /* _ASM_POWERPC_ULTRAVISOR_API_H */
diff --git a/arch/powerpc/include/asm/ultravisor.h
b/arch/powerpc/include/asm/ultravisor.h
index 6fe1f365dec8..d668a59e099b 100644
--- a/arch/powerpc/include/asm/ultravisor.h
+++ b/arch/powerpc/include/asm/ultravisor.h
@@ -19,4 +19,18 @@ static inline int uv_register_pate(u64 lpid, u64 dw0, u64
dw1)
return ucall_norets(UV_WRITE_PATE, lpid, dw0, dw1);
}
+static inline int uv_page_in(u64 lpid, u64 src_ra, u64 dst_gpa, u64 flags,
+u64 page_shift)
+{
+ return ucall_norets(UV_PAGE_IN, lpid, src_ra,
