Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 6 May 2021 23:27:27 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Jim Mattson <jmattson@google.com>
Cc:     KarimAllah Ahmed <karahmed@amazon.de>,
        the arch/x86 maintainers <x86@kernel.org>,
        kvm list <kvm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v6 08/14] KVM/nVMX: Use kvm_vcpu_map when mapping the
 posted interrupt descriptor table
Message-ID: <YJR7X7nN7sFwvesj@google.com>
References: <1548966284-28642-1-git-send-email-karahmed@amazon.de>
 <1548966284-28642-9-git-send-email-karahmed@amazon.de>
 <CALMp9eR-Kt5wcveYmmmOe7HfWBB4r5nF+SjMfybPRR-b9TXiTg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CALMp9eR-Kt5wcveYmmmOe7HfWBB4r5nF+SjMfybPRR-b9TXiTg@mail.gmail.com>
Precedence: bulk

On Thu, May 06, 2021, Jim Mattson wrote:
> On Thu, Jan 31, 2019 at 12:28 PM KarimAllah Ahmed <karahmed@amazon.de> wrote:
> >
> > Use kvm_vcpu_map when mapping the posted interrupt descriptor table since
> > using kvm_vcpu_gpa_to_page() and kmap() will only work for guest memory
> > that has a "struct page".
> >
> > One additional semantic change is that the virtual host mapping lifecycle
> > has changed a bit. It now has the same lifetime of the pinning of the
> > interrupt descriptor table page on the host side.
> >
> > Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de>
> > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> > v4 -> v5:
> > - unmap with dirty flag
> >
> > v1 -> v2:
> > - Do not change the lifecycle of the mapping (pbonzini)
> > ---
> >  arch/x86/kvm/vmx/nested.c | 43 ++++++++++++-------------------------------
> >  arch/x86/kvm/vmx/vmx.h    |  2 +-
> >  2 files changed, 13 insertions(+), 32 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 31b352c..53b1063 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -230,12 +230,8 @@ static void free_nested(struct kvm_vcpu *vcpu)
> >                 vmx->nested.apic_access_page = NULL;
> >         }
> >         kvm_vcpu_unmap(vcpu, &vmx->nested.virtual_apic_map, true);
> > -       if (vmx->nested.pi_desc_page) {
> > -               kunmap(vmx->nested.pi_desc_page);
> > -               kvm_release_page_dirty(vmx->nested.pi_desc_page);
> > -               vmx->nested.pi_desc_page = NULL;
> > -               vmx->nested.pi_desc = NULL;
> > -       }
> > +       kvm_vcpu_unmap(vcpu, &vmx->nested.pi_desc_map, true);
> > +       vmx->nested.pi_desc = NULL;
> >
> >         kvm_mmu_free_roots(vcpu, &vcpu->arch.guest_mmu, KVM_MMU_ROOTS_ALL);
> >
> > @@ -2868,26 +2864,15 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
> >         }
> >
> >         if (nested_cpu_has_posted_intr(vmcs12)) {
> > -               if (vmx->nested.pi_desc_page) { /* shouldn't happen */
> > -                       kunmap(vmx->nested.pi_desc_page);
> > -                       kvm_release_page_dirty(vmx->nested.pi_desc_page);
> > -                       vmx->nested.pi_desc_page = NULL;
> > -                       vmx->nested.pi_desc = NULL;
> > -                       vmcs_write64(POSTED_INTR_DESC_ADDR, -1ull);
> > +               map = &vmx->nested.pi_desc_map;
> > +
> > +               if (!kvm_vcpu_map(vcpu, gpa_to_gfn(vmcs12->posted_intr_desc_addr), map)) {
> > +                       vmx->nested.pi_desc =
> > +                               (struct pi_desc *)(((void *)map->hva) +
> > +                               offset_in_page(vmcs12->posted_intr_desc_addr));
> > +                       vmcs_write64(POSTED_INTR_DESC_ADDR,
> > +                                    pfn_to_hpa(map->pfn) + offset_in_page(vmcs12->posted_intr_desc_addr));
> >                 }
> 
> Previously, if there was no backing page for the
> vmcs12->posted_intr_desc_addr, we wrote an illegal value (-1ull) into
> the vmcs02 POSTED_INTR_DESC_ADDR field to force VM-entry failure.

The "vmcs_write64(POSTED_INTR_DESC_ADDR, -1ull)" above is for the "impossible"
case where the PI descriptor was already mapped.  The error handling for failure
to map is below.  The (forced) VM-Exit unmap paths don't stuff vmcs02 either.
In other words, I think the bug was pre-existing.

> Now, AFAICT, we leave that field unmodified. For a newly constructed vmcs02,
> doesn't that mean we're going to treat physical address 0 as the address of
> the vmcs02 posted interrupt descriptor?

PA=0 is the happy path.  Thanks to L1TF, that memory is always unused.  If
mapping for a previous VM-Enter succeeded, vmcs02.POSTED_INTR_DESC_ADDR will
hold whatever PA was used for the last VM-Enter.
 
> > -               page = kvm_vcpu_gpa_to_page(vcpu, vmcs12->posted_intr_desc_addr);
> > -               if (is_error_page(page))
> > -                       return;

Error path for failure to map.

> > -               vmx->nested.pi_desc_page = page;
> > -               vmx->nested.pi_desc = kmap(vmx->nested.pi_desc_page);
> > -               vmx->nested.pi_desc =
> > -                       (struct pi_desc *)((void *)vmx->nested.pi_desc +
> > -                       (unsigned long)(vmcs12->posted_intr_desc_addr &
> > -                       (PAGE_SIZE - 1)));
> > -               vmcs_write64(POSTED_INTR_DESC_ADDR,
> > -                       page_to_phys(vmx->nested.pi_desc_page) +
> > -                       (unsigned long)(vmcs12->posted_intr_desc_addr &
> > -                       (PAGE_SIZE - 1)));
> >         }
> >         if (nested_vmx_prepare_msr_bitmap(vcpu, vmcs12))
> >                 vmcs_set_bits(CPU_BASED_VM_EXEC_CONTROL,
> > @@ -3911,12 +3896,8 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
> >                 vmx->nested.apic_access_page = NULL;
> >         }
> >         kvm_vcpu_unmap(vcpu, &vmx->nested.virtual_apic_map, true);
> > -       if (vmx->nested.pi_desc_page) {
> > -               kunmap(vmx->nested.pi_desc_page);
> > -               kvm_release_page_dirty(vmx->nested.pi_desc_page);
> > -               vmx->nested.pi_desc_page = NULL;
> > -               vmx->nested.pi_desc = NULL;
> > -       }
> > +       kvm_vcpu_unmap(vcpu, &vmx->nested.pi_desc_map, true);
> > +       vmx->nested.pi_desc = NULL;
> >
> >         /*
> >          * We are now running in L2, mmu_notifier will force to reload the
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index f618f52..bd04725 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -143,7 +143,7 @@ struct nested_vmx {
> >          */
> >         struct page *apic_access_page;
> >         struct kvm_host_map virtual_apic_map;
> > -       struct page *pi_desc_page;
> > +       struct kvm_host_map pi_desc_map;
> >
> >         struct kvm_host_map msr_bitmap_map;
> >
> > --
> > 2.7.4
> >