Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <e249bd60cef49706e39cc619876e26c90d88ecb0.camel@redhat.com>
Subject: Re: [PATCH v6 20/38] KVM: nVMX: hyper-v: Cache VP assist page in
 'struct kvm_vcpu_hv'
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Vitaly Kuznetsov <vkuznets@redhat.com>, kvm@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>
Cc:     Sean Christopherson <seanjc@google.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Michael Kelley <mikelley@microsoft.com>,
        Siddharth Chandrasekaran <sidcha@amazon.de>,
        Yuan Yao <yuan.yao@linux.intel.com>,
        linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Tue, 07 Jun 2022 12:56:02 +0300
In-Reply-To: <20220606083655.2014609-21-vkuznets@redhat.com>
References: <20220606083655.2014609-1-vkuznets@redhat.com>
         <20220606083655.2014609-21-vkuznets@redhat.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.40.4 (3.40.4-2.fc34) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Mon, 2022-06-06 at 10:36 +0200, Vitaly Kuznetsov wrote:
> In preparation to enabling L2 TLB flush, cache VP assist page in
> 'struct kvm_vcpu_hv'. While on it, rename nested_enlightened_vmentry()
> to nested_get_evmptr() and make it return eVMCS GPA directly.
> 
> No functional change intended.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  2 ++
>  arch/x86/kvm/hyperv.c           | 10 ++++++----
>  arch/x86/kvm/hyperv.h           |  3 +--
>  arch/x86/kvm/vmx/evmcs.c        | 21 +++++++--------------
>  arch/x86/kvm/vmx/evmcs.h        |  2 +-
>  arch/x86/kvm/vmx/nested.c       |  6 +++---
>  6 files changed, 20 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index f9a34af0a5cc..e62db76c8d37 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -650,6 +650,8 @@ struct kvm_vcpu_hv {
>         /* Preallocated buffer for handling hypercalls passing sparse vCPU set */
>         u64 sparse_banks[HV_MAX_SPARSE_VCPU_BANKS];
>  
> +       struct hv_vp_assist_page vp_assist_page;
> +
>         struct {
>                 u64 pa_page_gpa;
>                 u64 vm_id;
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 4396d75588d8..91310774c0b9 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -903,13 +903,15 @@ bool kvm_hv_assist_page_enabled(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_hv_assist_page_enabled);
>  
> -bool kvm_hv_get_assist_page(struct kvm_vcpu *vcpu,
> -                           struct hv_vp_assist_page *assist_page)
> +bool kvm_hv_get_assist_page(struct kvm_vcpu *vcpu)
>  {
> -       if (!kvm_hv_assist_page_enabled(vcpu))
> +       struct kvm_vcpu_hv *hv_vcpu = to_hv_vcpu(vcpu);
> +
> +       if (!hv_vcpu || !kvm_hv_assist_page_enabled(vcpu))
>                 return false;
> +
>         return !kvm_read_guest_cached(vcpu->kvm, &vcpu->arch.pv_eoi.data,
> -                                     assist_page, sizeof(*assist_page));
> +                                     &hv_vcpu->vp_assist_page, sizeof(struct hv_vp_assist_page));
>  }
>  EXPORT_SYMBOL_GPL(kvm_hv_get_assist_page);
>  
> diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
> index 2aa6fb7fc599..139beb55b781 100644
> --- a/arch/x86/kvm/hyperv.h
> +++ b/arch/x86/kvm/hyperv.h
> @@ -105,8 +105,7 @@ int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages);
>  void kvm_hv_vcpu_uninit(struct kvm_vcpu *vcpu);
>  
>  bool kvm_hv_assist_page_enabled(struct kvm_vcpu *vcpu);
> -bool kvm_hv_get_assist_page(struct kvm_vcpu *vcpu,
> -                           struct hv_vp_assist_page *assist_page);
> +bool kvm_hv_get_assist_page(struct kvm_vcpu *vcpu);
>  
>  static inline struct kvm_vcpu_hv_stimer *to_hv_stimer(struct kvm_vcpu *vcpu,
>                                                       int timer_index)
> diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
> index 805afc170b5b..7cd7b16942c6 100644
> --- a/arch/x86/kvm/vmx/evmcs.c
> +++ b/arch/x86/kvm/vmx/evmcs.c
> @@ -307,24 +307,17 @@ __init void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf)
>  }
>  #endif
>  
> -bool nested_enlightened_vmentry(struct kvm_vcpu *vcpu, u64 *evmcs_gpa)
> +u64 nested_get_evmptr(struct kvm_vcpu *vcpu)
>  {
> -       struct hv_vp_assist_page assist_page;
> +       struct kvm_vcpu_hv *hv_vcpu = to_hv_vcpu(vcpu);
>  
> -       *evmcs_gpa = -1ull;
> +       if (unlikely(!kvm_hv_get_assist_page(vcpu)))
> +               return EVMPTR_INVALID;
>  
> -       if (unlikely(!kvm_hv_get_assist_page(vcpu, &assist_page)))
> -               return false;
> +       if (unlikely(!hv_vcpu->vp_assist_page.enlighten_vmentry))
> +               return EVMPTR_INVALID;
>  
> -       if (unlikely(!assist_page.enlighten_vmentry))
> -               return false;
> -
> -       if (unlikely(!evmptr_is_valid(assist_page.current_nested_vmcs)))
> -               return false;
> -
> -       *evmcs_gpa = assist_page.current_nested_vmcs;
> -
> -       return true;
> +       return hv_vcpu->vp_assist_page.current_nested_vmcs;
>  }
>  
>  uint16_t nested_get_evmcs_version(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
> index 584741b85eb6..22d238b36238 100644
> --- a/arch/x86/kvm/vmx/evmcs.h
> +++ b/arch/x86/kvm/vmx/evmcs.h
> @@ -239,7 +239,7 @@ enum nested_evmptrld_status {
>         EVMPTRLD_ERROR,
>  };
>  
> -bool nested_enlightened_vmentry(struct kvm_vcpu *vcpu, u64 *evmcs_gpa);
> +u64 nested_get_evmptr(struct kvm_vcpu *vcpu);
>  uint16_t nested_get_evmcs_version(struct kvm_vcpu *vcpu);
>  int nested_enable_evmcs(struct kvm_vcpu *vcpu,
>                         uint16_t *vmcs_version);
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 4a827b3d929a..87bff81f7f3e 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -1995,7 +1995,8 @@ static enum nested_evmptrld_status nested_vmx_handle_enlightened_vmptrld(
>         if (likely(!vmx->nested.enlightened_vmcs_enabled))
>                 return EVMPTRLD_DISABLED;
>  
> -       if (!nested_enlightened_vmentry(vcpu, &evmcs_gpa)) {
> +       evmcs_gpa = nested_get_evmptr(vcpu);
> +       if (!evmptr_is_valid(evmcs_gpa)) {
>                 nested_release_evmcs(vcpu);
>                 return EVMPTRLD_DISABLED;
>         }
> @@ -5084,7 +5085,6 @@ static int handle_vmclear(struct kvm_vcpu *vcpu)
>         struct vcpu_vmx *vmx = to_vmx(vcpu);
>         u32 zero = 0;
>         gpa_t vmptr;
> -       u64 evmcs_gpa;
>         int r;
>  
>         if (!nested_vmx_check_permission(vcpu))
> @@ -5110,7 +5110,7 @@ static int handle_vmclear(struct kvm_vcpu *vcpu)
>          * vmx->nested.hv_evmcs but this shouldn't be a problem.
>          */
>         if (likely(!vmx->nested.enlightened_vmcs_enabled ||
> -                  !nested_enlightened_vmentry(vcpu, &evmcs_gpa))) {
> +                  !evmptr_is_valid(nested_get_evmptr(vcpu)))) {
>                 if (vmptr == vmx->nested.current_vmptr)
>                         nested_release_vmcs12(vcpu);
>  


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky