Re: [PATCH v6 08/14] KVM/nVMX: Use kvm_vcpu_map when mapping the posted interrupt descriptor table

From: Jim Mattson <jmattson@google.com>
To: KarimAllah Ahmed <karahmed@amazon.de>
Cc: "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
Date: Thu, 6 May 2021 16:06:19 -0700	[thread overview]
Message-ID: <CALMp9eR-Kt5wcveYmmmOe7HfWBB4r5nF+SjMfybPRR-b9TXiTg@mail.gmail.com> (raw)
In-Reply-To: <1548966284-28642-9-git-send-email-karahmed@amazon.de>

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. 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?

> -               page = kvm_vcpu_gpa_to_page(vcpu, vmcs12->posted_intr_desc_addr);
> -               if (is_error_page(page))
> -                       return;
> -               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
>