On Wed, Nov 06, 2019 at 03:52:38PM +1100, Paul Mackerras wrote:
> 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?
Yes.
> I would think it should be equal to
> uvmem_pfn in most cases, shouldn't it?
Yes, in most cases (Common case is to share a page that is already secure)
> If not, what is it going to
> be?
It can be a regular pfn if non-secure page is being shared again.
>
> > + 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()?
gfn_to_pfn() will come with a reference held.
> 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)?
If secure page is being converted to share, pfn will be uvmem_pfn (device pfn).
If not, it will be regular pfn.
> What does
> kvm_is_reserved_pfn() return for a device pfn?
>From this code patch, we will never call kvm_release_pfn_clean() on a device
pfn. The prior call to gfn_to_pfn() would fault, result in page-out thus
converting the device pfn to regular pfn (page share request for secure page
case).
Regards,
Bharata.
