On Tue, Oct 30, 2018 at 04:26:46PM +1100, Paul Mackerras wrote:
> On Mon, Oct 22, 2018 at 10:48:35AM +0530, Bharata B Rao wrote:
> > A secure guest will share some of its pages with hypervisor (Eg. virtio
> > bounce buffers etc). Support shared pages in HMM driver.
> >
> > Signed-off-by: Bharata B Rao <bharata@xxxxxxxxxxxxx>
>
> Comments below...
>
> > ---
> > arch/powerpc/kvm/book3s_hv_hmm.c | 69 ++++++++++++++++++++++++++++++--
> > 1 file changed, 65 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_hv_hmm.c b/arch/powerpc/kvm/book3s_hv_hmm.c
> > index a2ee3163a312..09b8e19b7605 100644
> > --- a/arch/powerpc/kvm/book3s_hv_hmm.c
> > +++ b/arch/powerpc/kvm/book3s_hv_hmm.c
> > @@ -50,6 +50,7 @@ struct kvmppc_hmm_page_pvt {
> > struct hlist_head *hmm_hash;
> > unsigned int lpid;
> > unsigned long gpa;
> > + bool skip_page_out;
> > };
> >
> > struct kvmppc_hmm_migrate_args {
> > @@ -278,6 +279,65 @@ static unsigned long kvmppc_gpa_to_hva(struct kvm *kvm, unsigned long gpa,
> > return hva;
> > }
> >
> > +/*
> > + * 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 the HMM fault handler to release the HMM page.
> > + */
> > +static unsigned long
> > +kvmppc_share_page(struct kvm *kvm, unsigned long gpa,
> > + unsigned long addr, unsigned long page_shift)
> > +{
> > +
> > + int ret;
> > + struct hlist_head *list, *hmm_hash;
> > + unsigned int lpid = kvm->arch.lpid;
> > + unsigned long flags;
> > + struct kvmppc_hmm_pfn_entry *p;
> > + struct page *hmm_page, *page;
> > + struct kvmppc_hmm_page_pvt *pvt;
> > + unsigned long pfn;
> > +
> > + /*
> > + * First check if the requested page has already been given to
> > + * UV as a secure page. If so, ensure that we don't issue a
> > + * UV_PAGE_OUT but instead directly send the page
> > + */
> > + spin_lock_irqsave(&kvmppc_hmm_lock, flags);
> > + hmm_hash = kvm->arch.hmm_hash;
> > + list = &hmm_hash[kvmppc_hmm_pfn_hash_fn(gpa)];
> > + hlist_for_each_entry(p, list, hlist) {
> > + if (p->addr == gpa) {
> > + hmm_page = pfn_to_page(p->hmm_pfn);
> > + get_page(hmm_page); /* TODO: Necessary ? */
> > + pvt = (struct kvmppc_hmm_page_pvt *)
> > + hmm_devmem_page_get_drvdata(hmm_page);
> > + pvt->skip_page_out = true;
> > + put_page(hmm_page);
> > + break;
> > + }
> > + }
> > + spin_unlock_irqrestore(&kvmppc_hmm_lock, flags);
> > +
> > + ret = get_user_pages_fast(addr, 1, 0, &page);
>
> Why are we calling this with write==0? Surely in general the secure
> guest will expect to be able to write to the shared page?
>
> Also, in general get_user_pages_fast isn't sufficient to translate a
> host virtual address (derived from a guest real address) into a pfn.
> See for example hva_to_pfn() in virt/kvm/kvm_main.c and the things it
> does to cope with the various cases that one can hit. I can imagine
> in future that the secure guest might want to establish a shared
> mapping to a PCI device, for instance.
I switched to using gfn_to_pfn() which should cover all the cases.
>
> > + if (ret != 1)
> > + return H_PARAMETER;
> > +
> > + pfn = page_to_pfn(page);
> > + if (is_zero_pfn(pfn)) {
> > + put_page(page);
> > + return H_SUCCESS;
> > + }
>
> The ultravisor still needs a page to map into the guest in this case,
> doesn't it? What's the point of returning without giving the
> ultravisor a page to use?
Yes, missed it.
Regards,
Bharata.
