Re: [Xen-devel] [PATCH v1 03/10] KVM: Prepare kvm_is_reserved_pfn() for PG_reserved changes

[David Hildenbrand <david@xxxxxxxxxx>]

On 05.11.19 10:17, David Hildenbrand wrote:
> On 05.11.19 05:38, Dan Williams wrote:
> > On Thu, Oct 24, 2019 at 5:11 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
> > > Right now, ZONE_DEVICE memory is always set PG_reserved. We want to
> > > change that.
> > >
> > > KVM has this weird use case that you can map anything from /dev/mem
> > > into the guest. pfn_valid() is not a reliable check whether the memmap
> > > was initialized and can be touched. pfn_to_online_page() makes sure
> > > that we have an initialized memmap (and don't have ZONE_DEVICE memory).
> > >
> > > Rewrite kvm_is_reserved_pfn() to make sure the function produces the
> > > same result once we stop setting ZONE_DEVICE pages PG_reserved.
> > >
> > > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > > Cc: "Radim Krčmář" <rkrcmar@xxxxxxxxxx>
> > > Cc: Michal Hocko <mhocko@xxxxxxxxxx>
> > > Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> > > Cc: KarimAllah Ahmed <karahmed@xxxxxxxxx>
> > > Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> > > ---
> > >    virt/kvm/kvm_main.c | 10 ++++++++--
> > >    1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > index e9eb666eb6e8..9d18cc67d124 100644
> > > --- a/virt/kvm/kvm_main.c
> > > +++ b/virt/kvm/kvm_main.c
> > > @@ -151,9 +151,15 @@ __weak int kvm_arch_mmu_notifier_invalidate_range(struct 
> > > kvm *kvm,
> > >
> > >    bool kvm_is_reserved_pfn(kvm_pfn_t pfn)
> > >    {
> > > -       if (pfn_valid(pfn))
> > > -               return PageReserved(pfn_to_page(pfn));
> > > +       struct page *page = pfn_to_online_page(pfn);
> > >
> > > +       /*
> > > +        * We treat any pages that are not online (not managed by the buddy)
> > > +        * as reserved - this includes ZONE_DEVICE pages and pages without
> > > +        * a memmap (e.g., mapped via /dev/mem).
> > > +        */
> > > +       if (page)
> > > +               return PageReserved(page);
> > >           return true;
> > >    }
> > So after this all the pfn_valid() usage in kvm_main.c is replaced with
> > pfn_to_online_page()? Looks correct to me.
> >
> > However, I'm worried that kvm is taking reference on ZONE_DEVICE pages
> > through some other path resulting in this:
> >
> >       https://lore.kernel.org/linux-nvdimm/20190919154708.GA24650@xxxxxxxxxx/
> >
> > I'll see if this patch set modulates or maintains that failure mode.
> I'd assume that the behavior is unchanged. Ithink we get a reference to
> these ZONE_DEVICE pages via __get_user_pages_fast() and friends in
> hva_to_pfn_fast() and friends in virt/kvm/kvm_main.c
I think I know what's going wrong:

Pages that are pinned via gfn_to_pfn() and friends take a references, 
however are often released via 
kvm_release_pfn_clean()/kvm_release_pfn_dirty()/kvm_release_page_clean()...

E.g., in arch/x86/kvm/x86.c:reexecute_instruction()

...
pfn = gfn_to_pfn(vcpu->kvm, gpa_to_gfn(gpa));
...
kvm_release_pfn_clean(pfn);



void kvm_release_pfn_clean(kvm_pfn_t pfn)
{
        if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn))
                put_page(pfn_to_page(pfn));
}

This function makes perfect sense as the counterpart for kvm_get_pfn():

void kvm_get_pfn(kvm_pfn_t pfn)
{
        if (!kvm_is_reserved_pfn(pfn))
                get_page(pfn_to_page(pfn));
}


As all ZONE_DEVICE pages are currently reserved, pages pinned via 
gfn_to_pfn() and friends will often not see a put_page() AFAIKS.
Now, my patch does not change that, the result of 
kvm_is_reserved_pfn(pfn) will be unchanged. A proper fix for that would 
probably be
a) To drop the reference to ZONE_DEVICE pages in gfn_to_pfn() and 
friends, after you successfully pinned the pages. (not sure if that's 
the right thing to do but you're the expert)
b) To not use kvm_release_pfn_clean() and friends on pages that were 
definitely pinned.
--

Thanks,

David / dhildenb


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel