Re: [PATCH v3 63/66] KVM: arm64: nv: Allocate VNCR page when required

From: Haibo Xu <haibo.xu@linaro.org>
To: Marc Zyngier <maz@kernel.org>
Cc: arm-mail-list <linux-arm-kernel@lists.infradead.org>,
	kvmarm <kvmarm@lists.cs.columbia.edu>,
	kvm@vger.kernel.org, kernel-team@android.com,
	Andre Przywara <andre.przywara@arm.com>
Subject: Re: [PATCH v3 63/66] KVM: arm64: nv: Allocate VNCR page when required
Date: Thu, 21 Jan 2021 10:47:45 +0800	[thread overview]
Message-ID: <CAJc+Z1G6QEEiUh=KLdS6Xut7q63zZ6zQxfpGCkCtY5tTSXPeZw@mail.gmail.com> (raw)
In-Reply-To: <20201210160002.1407373-64-maz@kernel.org>

On Fri, 11 Dec 2020 at 00:04, Marc Zyngier <maz@kernel.org> wrote:
>
> If running a NV guest on an ARMv8.4-NV capable system, let's
> allocate an additional page that will be used by the hypervisor
> to fulfill system register accesses.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_host.h | 3 ++-
>  arch/arm64/kvm/nested.c           | 8 ++++++++
>  arch/arm64/kvm/reset.c            | 1 +
>  3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 78630bd5124d..dada0678c28e 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -523,7 +523,8 @@ struct kvm_vcpu_arch {
>   */
>  static inline u64 *__ctxt_sys_reg(const struct kvm_cpu_context *ctxt, int r)
>  {
> -       if (unlikely(r >= __VNCR_START__ && ctxt->vncr_array))
> +       if (unlikely(cpus_have_final_cap(ARM64_HAS_ENHANCED_NESTED_VIRT) &&
> +                    r >= __VNCR_START__ && ctxt->vncr_array))
>                 return &ctxt->vncr_array[r - __VNCR_START__];
>
>         return (u64 *)&ctxt->sys_regs[r];
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index eef8f9873814..88147ec99755 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -47,6 +47,12 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
>         if (!cpus_have_final_cap(ARM64_HAS_NESTED_VIRT))
>                 return -EINVAL;
>
> +       if (cpus_have_final_cap(ARM64_HAS_ENHANCED_NESTED_VIRT)) {
> +               vcpu->arch.ctxt.vncr_array = (u64 *)__get_free_page(GFP_KERNEL | __GFP_ZERO);
> +               if (!vcpu->arch.ctxt.vncr_array)
> +                       return -ENOMEM;
> +       }
> +

If KVM_ARM_VCPU_INIT was called multiple times, the above codes would
try to allocate a new page
without free-ing the previous one. Besides that, the following
kvm_free_stage2_pgd() call would fail in the
second call with the error message "kvm_arch already initialized?".
I think a possible fix is to add a new flag to indicate whether the NV
related meta data have been initialized,
and only initialize them for the first call.

>         mutex_lock(&kvm->lock);
>
>         /*
> @@ -64,6 +70,8 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
>                     kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 2])) {
>                         kvm_free_stage2_pgd(&tmp[num_mmus - 1]);
>                         kvm_free_stage2_pgd(&tmp[num_mmus - 2]);
> +                       free_page((unsigned long)vcpu->arch.ctxt.vncr_array);
> +                       vcpu->arch.ctxt.vncr_array = NULL;
>                 } else {
>                         kvm->arch.nested_mmus_size = num_mmus;
>                         ret = 0;
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 2d2c780e6c69..d281eb39036f 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -150,6 +150,7 @@ bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu)
>  void kvm_arm_vcpu_destroy(struct kvm_vcpu *vcpu)
>  {
>         kfree(vcpu->arch.sve_state);
> +       free_page((unsigned long)vcpu->arch.ctxt.vncr_array);
>  }
>
>  static void kvm_vcpu_reset_sve(struct kvm_vcpu *vcpu)
> --
> 2.29.2
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm