Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <43c18a3d57305cf52a1c3643fa8f714ae3769551.camel@redhat.com>
Subject: Re: [RFC PATCH] KVM: x86: inhibit APICv upon detecting direct APIC
 access from L2
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Ake Koomsin <ake@igel.co.jp>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H . Peter Anvin" <hpa@zytor.com>
Date:   Mon, 07 Aug 2023 17:00:58 +0300
In-Reply-To: <20230807062611.12596-1-ake@igel.co.jp>
References: <20230807062611.12596-1-ake@igel.co.jp>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

У пн, 2023-08-07 у 15:26 +0900, Ake Koomsin пише:
> Current KVM does not expect L1 hypervisor to allow L2 guest to access
> APIC page directly when APICv is enabled. When this happens, KVM
> emulates the access itself resulting in interrupt lost.
> 
> As this kind of hypervisor is rare, it is simpler to inhibit APICv upon
> detecting direct APIC access from L2 to avoid unexpected interrupt lost.
> 
> Signed-off-by: Ake Koomsin <ake@igel.co.jp>
> ---
>  arch/x86/include/asm/kvm_host.h |  6 ++++++
>  arch/x86/kvm/mmu/mmu.c          | 33 ++++++++++++++++++++++++++-------
>  arch/x86/kvm/svm/svm.h          |  3 ++-
>  arch/x86/kvm/vmx/vmx.c          |  3 ++-
>  4 files changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 3bc146dfd38d..8764b11922a0 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1188,6 +1188,12 @@ enum kvm_apicv_inhibit {
>  	APICV_INHIBIT_REASON_APIC_ID_MODIFIED,
>  	APICV_INHIBIT_REASON_APIC_BASE_MODIFIED,
>  
> +	/*
> +	 * APICv is disabled because L1 hypervisor allows L2 guest to access
> +	 * APIC directly.
> +	 */
> +	APICV_INHIBIT_REASON_L2_PASSTHROUGH_ACCESS,
> +
>  	/******************************************************/
>  	/* INHIBITs that are relevant only to the AMD's AVIC. */
>  	/******************************************************/
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index ec169f5c7dce..c1150ef9fce1 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4293,6 +4293,30 @@ void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu, struct kvm_async_pf *work)
>  	kvm_mmu_do_page_fault(vcpu, work->cr2_or_gpa, 0, true, NULL);
>  }
>  
> +static int __kvm_faultin_pfn_guest_mode(struct kvm_vcpu *vcpu,
> +					struct kvm_page_fault *fault)
> +{
> +	struct kvm_memory_slot *slot = fault->slot;
> +
> +	/* Don't expose private memslots to L2. */
> +	fault->slot = NULL;
> +	fault->pfn = KVM_PFN_NOSLOT;
> +	fault->map_writable = false;
> +
> +	/*
> +	 * APICv does not work when L1 hypervisor allows L2 guest to access
> +	 * APIC directly. As this kind of L1 hypervisor is rare, it is simpler
> +	 * to inhibit APICv when we detect direct APIC access from L2, and
> +	 * fallback to emulation path to avoid interrupt lost.
> +	 */
> +	if (unlikely(slot && slot->id == APIC_ACCESS_PAGE_PRIVATE_MEMSLOT &&
> +		     kvm_apicv_activated(vcpu->kvm)))
> +		kvm_set_apicv_inhibit(vcpu->kvm,
> +				      APICV_INHIBIT_REASON_L2_PASSTHROUGH_ACCESS);

Is there a good reason why KVM doesn't expose APIC memslot to a nested guest?
While nested guest runs, the L1's APICv is "inhibited" effectively anyway, so writes to this memslot
should update APIC registers and be picked up by APICv hardware when L1 resumes execution.

Since APICv alows itself to be inhibited due to other reasons, it means that just like AVIC, it should be able
to pick up arbitrary changes to APIC registers which happened while it was inhibited,
just like AVIC does.

I'll take a look at the code to see if APICv does this (I know AVIC's code much better that APICv's)

Is there a reproducer for this bug?

Best regards,
	Maxim Levitsky

> +
> +	return RET_PF_CONTINUE;
> +}
> +
>  static int __kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
>  {
>  	struct kvm_memory_slot *slot = fault->slot;
> @@ -4307,13 +4331,8 @@ static int __kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
>  		return RET_PF_RETRY;
>  
>  	if (!kvm_is_visible_memslot(slot)) {
> -		/* Don't expose private memslots to L2. */
> -		if (is_guest_mode(vcpu)) {
> -			fault->slot = NULL;
> -			fault->pfn = KVM_PFN_NOSLOT;
> -			fault->map_writable = false;
> -			return RET_PF_CONTINUE;
> -		}
> +		if (is_guest_mode(vcpu))
> +			return __kvm_faultin_pfn_guest_mode(vcpu, fault);
>  		/*
>  		 * If the APIC access page exists but is disabled, go directly
>  		 * to emulation without caching the MMIO access or creating a
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 18af7e712a5a..8d77932ee0fb 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -683,7 +683,8 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
>  	BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) |	\
>  	BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED) |	\
>  	BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED) |	\
> -	BIT(APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED)	\
> +	BIT(APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED) |	\
> +	BIT(APICV_INHIBIT_REASON_L2_PASSTHROUGH_ACCESS)	\
>  )
>  
>  bool avic_hardware_setup(void);
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index df461f387e20..f652397c9765 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -8189,7 +8189,8 @@ static void vmx_hardware_unsetup(void)
>  	BIT(APICV_INHIBIT_REASON_BLOCKIRQ) |		\
>  	BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) |	\
>  	BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED) |	\
> -	BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED)	\
> +	BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED) |	\
> +	BIT(APICV_INHIBIT_REASON_L2_PASSTHROUGH_ACCESS)	\
>  )
>  
>  static void vmx_vm_destroy(struct kvm *kvm)