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;
MIME-Version: 1.0
References: <20200619153925.79106-1-mgamal@redhat.com> <20200619153925.79106-7-mgamal@redhat.com>
In-Reply-To: <20200619153925.79106-7-mgamal@redhat.com>
From:   Jim Mattson <jmattson@google.com>
Date:   Fri, 19 Jun 2020 15:45:52 -0700
Message-ID: <CALMp9eQYRoB5vmxL0U7Mn0rWqotxLpUAJD15YX0DDYop1ZmuEg@mail.gmail.com>
Subject: Re: [PATCH v2 06/11] KVM: VMX: introduce vmx_need_pf_intercept
To:     Mohammed Gamal <mgamal@redhat.com>
Cc:     kvm list <kvm@vger.kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Joerg Roedel <joro@8bytes.org>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        "Moger, Babu" <babu.moger@amd.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jun 19, 2020 at 8:40 AM Mohammed Gamal <mgamal@redhat.com> wrote:
>
> From: Paolo Bonzini <pbonzini@redhat.com>
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 28 +++++++++++++++++-----------
>  arch/x86/kvm/vmx/vmx.c    |  2 +-
>  arch/x86/kvm/vmx/vmx.h    |  5 +++++
>  3 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index d1af20b050a8..328411919518 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2433,22 +2433,28 @@ static void prepare_vmcs02_rare(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
>
>         /*
>          * Whether page-faults are trapped is determined by a combination of
> -        * 3 settings: PFEC_MASK, PFEC_MATCH and EXCEPTION_BITMAP.PF.
> -        * If enable_ept, L0 doesn't care about page faults and we should
> -        * set all of these to L1's desires. However, if !enable_ept, L0 does
> -        * care about (at least some) page faults, and because it is not easy
> -        * (if at all possible?) to merge L0 and L1's desires, we simply ask
> -        * to exit on each and every L2 page fault. This is done by setting
> -        * MASK=MATCH=0 and (see below) EB.PF=1.
> +        * 3 settings: PFEC_MASK, PFEC_MATCH and EXCEPTION_BITMAP.PF.  If L0
> +        * doesn't care about page faults then we should set all of these to
> +        * L1's desires. However, if L0 does care about (some) page faults, it
> +        * is not easy (if at all possible?) to merge L0 and L1's desires, we
> +        * simply ask to exit on each and every L2 page fault. This is done by
> +        * setting MASK=MATCH=0 and (see below) EB.PF=1.
>          * Note that below we don't need special code to set EB.PF beyond the
>          * "or"ing of the EB of vmcs01 and vmcs12, because when enable_ept,
>          * vmcs01's EB.PF is 0 so the "or" will take vmcs12's value, and when
>          * !enable_ept, EB.PF is 1, so the "or" will always be 1.
>          */
> -       vmcs_write32(PAGE_FAULT_ERROR_CODE_MASK,
> -               enable_ept ? vmcs12->page_fault_error_code_mask : 0);
> -       vmcs_write32(PAGE_FAULT_ERROR_CODE_MATCH,
> -               enable_ept ? vmcs12->page_fault_error_code_match : 0);
> +       if (vmx_need_pf_intercept(&vmx->vcpu)) {
> +               /*
> +                * TODO: if both L0 and L1 need the same MASK and MATCH,
> +                * go ahead and use it?
> +                */

I'm not sure there's much "TODO", since L0's MASK and MATCH are both
0. So, if L1's MASK and MATCH are also both 0, we would go ahead and
use 0's, which it seems we already do here:

> +               vmcs_write32(PAGE_FAULT_ERROR_CODE_MASK, 0);
> +               vmcs_write32(PAGE_FAULT_ERROR_CODE_MATCH, 0);
> +       } else {
> +               vmcs_write32(PAGE_FAULT_ERROR_CODE_MASK, vmcs12->page_fault_error_code_mask);
> +               vmcs_write32(PAGE_FAULT_ERROR_CODE_MATCH, vmcs12->page_fault_error_code_match);
> +       }
>
>         if (cpu_has_vmx_apicv()) {
>                 vmcs_write64(EOI_EXIT_BITMAP0, vmcs12->eoi_exit_bitmap0);
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index f82c42ac87f9..46d522ee5cb1 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -783,7 +783,7 @@ void update_exception_bitmap(struct kvm_vcpu *vcpu)
>                 eb |= 1u << BP_VECTOR;
>         if (to_vmx(vcpu)->rmode.vm86_active)
>                 eb = ~0;
> -       if (enable_ept)
> +       if (!vmx_need_pf_intercept(vcpu))
>                 eb &= ~(1u << PF_VECTOR);
>
>         /* When we are running a nested L2 guest and L1 specified for it a
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 8a83b5edc820..5e2da15fe94f 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -552,6 +552,11 @@ static inline bool vmx_has_waitpkg(struct vcpu_vmx *vmx)
>                 SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE;
>  }
>
> +static inline bool vmx_need_pf_intercept(struct kvm_vcpu *vcpu)
> +{
> +       return !enable_ept;
> +}
> +
>  void dump_vmcs(void);
>
>  #endif /* __KVM_X86_VMX_H */
> --
> 2.26.2
>
Reviewed-by: Jim Mattson <jmattson@google.com>