Re: [PATCH v10 3/8] KVM: PPC: Shared pages support for secure guests

Paul Mackerras
2019-11-06T04:52:38Z

On Mon, Nov 04, 2019 at 09:47:55AM +0530, Bharata B Rao wrote:
> A secure guest will share some of its pages with hypervisor (Eg. virtio
> bounce buffers etc). Support sharing of pages between hypervisor and
> ultravisor.
>
> Shared page is reachable via both HV and UV side page tables. Once a
> secure page is converted to shared page, the device page that represents
> the secure page is unmapped from the HV side page tables.

I'd like to understand a little better what's going on - see below...

> +/*
> + * Shares the page with HV, thus making it a normal page.
> + *
> + * - If the page is already secure, then provision a new page and share
> + * - If the page is a normal page, share the existing page
> + *
> + * In the former case, uses dev_pagemap_ops.migrate_to_ram handler
> + * to unmap the device page from QEMU's page tables.
> + */
> +static unsigned long
> +kvmppc_share_page(struct kvm *kvm, unsigned long gpa, unsigned long page_shift)
> +{
> +
> + int ret = H_PARAMETER;
> + struct page *uvmem_page;
> + struct kvmppc_uvmem_page_pvt *pvt;
> + unsigned long pfn;
> + unsigned long gfn = gpa >> page_shift;
> + int srcu_idx;
> + unsigned long uvmem_pfn;
> +
> + srcu_idx = srcu_read_lock(&kvm->srcu);
> + mutex_lock(&kvm->arch.uvmem_lock);
> + if (kvmppc_gfn_is_uvmem_pfn(gfn, kvm, &uvmem_pfn)) {
> + uvmem_page = pfn_to_page(uvmem_pfn);
> + pvt = uvmem_page->zone_device_data;
> + pvt->skip_page_out = true;
> + }
> +
> +retry:
> + mutex_unlock(&kvm->arch.uvmem_lock);
> + pfn = gfn_to_pfn(kvm, gfn);

At this point, pfn is the value obtained from the page table for
userspace (e.g. QEMU), right? I would think it should be equal to
uvmem_pfn in most cases, shouldn't it? If not, what is it going to
be?

> + if (is_error_noslot_pfn(pfn))
> + goto out;
> +
> + mutex_lock(&kvm->arch.uvmem_lock);
> + if (kvmppc_gfn_is_uvmem_pfn(gfn, kvm, &uvmem_pfn)) {
> + uvmem_page = pfn_to_page(uvmem_pfn);
> + pvt = uvmem_page->zone_device_data;
> + pvt->skip_page_out = true;
> + kvm_release_pfn_clean(pfn);

This is going to do a put_page(), unless pfn is a reserved pfn. If it
does a put_page(), where did we do the corresponding get_page()?
However, since kvmppc_gfn_is_uvmem_pfn() returned true, doesn't that
mean that pfn here should be a device pfn, and in fact should be the
same as uvmem_pfn (possibly with some extra bit(s) set)? What does
kvm_is_reserved_pfn() return for a device pfn?

Paul.