Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <68e1fd5f-d58b-695b-0fc7-bdc3e5491de7@redhat.com>
Date:   Tue, 5 Apr 2022 15:17:09 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [RFC PATCH v5 036/104] KVM: x86/mmu: Explicitly check for MMIO
 spte in fast page fault
Content-Language: en-US
To:     isaku.yamahata@intel.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     isaku.yamahata@gmail.com, Jim Mattson <jmattson@google.com>,
        erdemaktas@google.com, Connor Kuehl <ckuehl@redhat.com>,
        Sean Christopherson <seanjc@google.com>
References: <cover.1646422845.git.isaku.yamahata@intel.com>
 <b0e81b4a4abfbe8bd6d43e4b1c0349a79517dfb0.1646422845.git.isaku.yamahata@intel.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
In-Reply-To: <b0e81b4a4abfbe8bd6d43e4b1c0349a79517dfb0.1646422845.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/4/22 20:48, isaku.yamahata@intel.com wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Explicitly check for an MMIO spte in the fast page fault flow.  TDX will
> use a not-present entry for MMIO sptes, which can be mistaken for an
> access-tracked spte since both have SPTE_SPECIAL_MASK set.
> 
> The fast page fault handles the case of changing access bits without
> obtaining mmu_lock.  For example, clear write protect bit for dirty page
> tracking.  MMIO emulation is handled in a slow path.  So it doesn't affect

"MMIO sptes are handled in handle_mmio_page_fault for non-TDX VMs, so 
this patch does not affect them.  TDX will handle MMIO emulation through 
a hypercall instead".

For this comment, it is not necessary to talk about the slow path, since 
that is just where MMIO sptes are installed.  If the slow path is 
reached, fast_page_fault must not have seen is_mmio_spte(spte).

> @@ -3167,7 +3167,7 @@ static int fast_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
>   			break;
>   
>   		sp = sptep_to_sp(sptep);
> -		if (!is_last_spte(spte, sp->role.level))
> +		if (!is_last_spte(spte, sp->role.level) || is_mmio_spte(spte))
>   			break;
>   
>   		/*

I would include the check a couple lines before:

	if (!is_shadow_present_pte(spte) || is_mmio_spte(spte))

This matches what is in the commit message: the problem is that MMIO 
SPTEs are present in the TDX case, so you need to check them even if 
is_shadow_present_pte(spte) returns true.

Paolo