Re: [PATCH] KVM: PPC: Book3S HV: Handle non-present PTEs in page fault functions

From: David Gibson
Date: Thu Apr 16 2020 - 21:04:15 EST


On Thu, Apr 16, 2020 at 10:07:49AM +0200, Cédric Le Goater wrote:
> On 4/16/20 7:03 AM, Paul Mackerras wrote:
> > Since cd758a9b57ee "KVM: PPC: Book3S HV: Use __gfn_to_pfn_memslot in HPT
> > page fault handler", it's been possible in fairly rare circumstances to
> > load a non-present PTE in kvmppc_book3s_hv_page_fault() when running a
> > guest on a POWER8 host.
> >
> > Because that case wasn't checked for, we could misinterpret the non-present
> > PTE as being a cache-inhibited PTE. That could mismatch with the
> > corresponding hash PTE, which would cause the function to fail with -EFAULT
> > a little further down. That would propagate up to the KVM_RUN ioctl()
> > generally causing the KVM userspace (usually qemu) to fall over.
> >
> > This addresses the problem by catching that case and returning to the guest
> > instead, letting it fault again, and retrying the whole page fault from
> > the beginning.
> >
> > For completeness, this fixes the radix page fault handler in the same
> > way. For radix this didn't cause any obvious misbehaviour, because we
> > ended up putting the non-present PTE into the guest's partition-scoped
> > page tables, leading immediately to another hypervisor data/instruction
> > storage interrupt, which would go through the page fault path again
> > and fix things up.
> >
> > Fixes: cd758a9b57ee "KVM: PPC: Book3S HV: Use __gfn_to_pfn_memslot in HPT page fault handler"
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1820402
> > Reported-by: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Paul Mackerras <paulus@xxxxxxxxxx>
>
> I didn't see the reported issue with the current 5.7-rc1. Anyhow I gave
> this patch a try on a P8 host and a P9 host with a radix guest and a hash
> guest (using rhel6). Passthrough is fine also.
>
> Tested-by: Cédric Le Goater <clg@xxxxxxxx>
>
> The code looks correct,
>
> Reviewed-by: Cédric Le Goater <clg@xxxxxxxx>

I ran my test case overnight with this patch for over 1000 iterations,
without any apparent problems so

Tested-by: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>

>
> Thanks,
>
> C.
>
>
> > ---
> > This is a reworked version of the patch David Gibson sent recently,
> > with the fix applied to the radix case as well. The commit message
> > is mostly stolen from David's patch.
> >
> > arch/powerpc/kvm/book3s_64_mmu_hv.c | 9 +++++----
> > arch/powerpc/kvm/book3s_64_mmu_radix.c | 9 +++++----
> > 2 files changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > index 3aecec8..20b7dce 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > @@ -604,18 +604,19 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
> > */
> > local_irq_disable();
> > ptep = __find_linux_pte(vcpu->arch.pgdir, hva, NULL, &shift);
> > + pte = __pte(0);
> > + if (ptep)
> > + pte = *ptep;
> > + local_irq_enable();
> > /*
> > * If the PTE disappeared temporarily due to a THP
> > * collapse, just return and let the guest try again.
> > */
> > - if (!ptep) {
> > - local_irq_enable();
> > + if (!pte_present(pte)) {
> > if (page)
> > put_page(page);
> > return RESUME_GUEST;
> > }
> > - pte = *ptep;
> > - local_irq_enable();
> > hpa = pte_pfn(pte) << PAGE_SHIFT;
> > pte_size = PAGE_SIZE;
> > if (shift)
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > index 134fbc1..7bf94ba 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> > @@ -815,18 +815,19 @@ int kvmppc_book3s_instantiate_page(struct kvm_vcpu *vcpu,
> > */
> > local_irq_disable();
> > ptep = __find_linux_pte(vcpu->arch.pgdir, hva, NULL, &shift);
> > + pte = __pte(0);
> > + if (ptep)
> > + pte = *ptep;
> > + local_irq_enable();
> > /*
> > * If the PTE disappeared temporarily due to a THP
> > * collapse, just return and let the guest try again.
> > */
> > - if (!ptep) {
> > - local_irq_enable();
> > + if (!pte_present(pte)) {
> > if (page)
> > put_page(page);
> > return RESUME_GUEST;
> > }
> > - pte = *ptep;
> > - local_irq_enable();
> >
> > /* If we're logging dirty pages, always map single pages */
> > large_enable = !(memslot->flags & KVM_MEM_LOG_DIRTY_PAGES);
> >
>

--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson

Attachment: signature.asc
Description: PGP signature