Received-SPF: pass (google.com: domain of linux-kernel+bounces-70699-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 19 Feb 2024 11:44:14 +0800
From: Yan Zhao <yan.y.zhao@intel.com>
To: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, Friedrich Weber <f.weber@proxmox.com>, "Kai
 Huang" <kai.huang@intel.com>, Yuan Yao <yuan.yao@linux.intel.com>, Xu Yilun
	<yilun.xu@linux.intel.com>, Yu Zhang <yu.c.zhang@linux.intel.com>, Chao Peng
	<chao.p.peng@linux.intel.com>, Fuad Tabba <tabba@google.com>, Michael Roth
	<michael.roth@amd.com>, Isaku Yamahata <isaku.yamahata@intel.com>, "David
 Matlack" <dmatlack@google.com>
Subject: Re: [PATCH v4 3/4] KVM: x86/mmu: Move slot checks from
 __kvm_faultin_pfn() to kvm_faultin_pfn()
Message-ID: <ZdLOjuCP2pDjhsJl@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <20240209222858.396696-1-seanjc@google.com>
 <20240209222858.396696-4-seanjc@google.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240209222858.396696-4-seanjc@google.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?dZvtnR7bdwLPUBLuAQ/DJR59E4BHw/C4CbCmLY7Q3s1RunAwI7Kg1QqK0PUz?=
 =?us-ascii?Q?ZoyErrZkQhaKMhfV6IyqjTbCCH9MLC7dwF5w64Gj4MvNNS2AIxLDFL+5C454?=
 =?us-ascii?Q?OZN2+L3XVvxjMRufdyH87ENchfSeudjqo+WZjuLKQMnIs5x6Bc7BEvjCzA/6?=
 =?us-ascii?Q?0uB+2kpE5lu4CpKOLI4vZEQDf2vf68mqAyyeRklXc6vKIEAGJwwPO1iEJv48?=
 =?us-ascii?Q?/NMIrIhrYHlibeCx2KCfEQRXXA/KVDfFEaBp+yM9NyGIHW3Y3A7bLRRGf1Pw?=
 =?us-ascii?Q?xNTq6jUezQ2M5V44rkFEnjuZNvppjgR6H9zKyzH/CtVMAVfmdmQ7BiM2Ev1I?=
 =?us-ascii?Q?pjZ1NMCE0vCkjYCkIcBxXB82Y3voDACIxjXQ0umvPaB6d9N3WTTMhbVYOe3R?=
 =?us-ascii?Q?waKvYu7TN4tGwZlXKRX9n09bKL3CnkDSdHuha2oT/nY/e8II4ywldfMd6eaL?=
 =?us-ascii?Q?pu3v2FRXEJWfsQmUfoxNatmomAZ8IwQ6PFtKGA9D+csa2ITmXxmtGkLJbNQ5?=
 =?us-ascii?Q?LGiVsErVJn3rMdwuldq2xkMVUkLYZeI5uPSTQChBLCPppWvCHRJ/Pcy/IXBk?=
 =?us-ascii?Q?d2iEdWheJSl2vSd00jrEYqlJYtssosDxTy2n23muoyjLK2ys58hKLc3gJNt3?=
 =?us-ascii?Q?pvozWzneDKUS3clzqzhRrfdK3UAMpOKpsgQIZS1bbniFmFg1T5h0HBvSqGMC?=
 =?us-ascii?Q?KWqlsjA9qIChmfB24nQ51yWDeqDVmv9SBFwREk6TajdZeOsP8hNW/epe7O8K?=
 =?us-ascii?Q?/97Oa3HqsouFtZBdXg/li+DXDm0BYQ/4hc6QsOVJt1BWnTRUzcTI1USb6LTG?=
 =?us-ascii?Q?5Ogv4Llg18XwD9LeMb55WPUbji7w4JC/1aNd9g5C9LgN4o5veeED9gqdcPaQ?=
 =?us-ascii?Q?J+Zq+S5x665EMDUvrtbUkvm7Y6p2XU7CtdhPuUNOlDMWLotvRrmINWrxw0aH?=
 =?us-ascii?Q?XpmvCTN/AUrqx+G3/leS8/VwBa72Upo4Ao3VmyNMho8tZsbtV2jUB0PXeaFr?=
 =?us-ascii?Q?iVxMnBSvY7Hv8FbfWVcWZ8TBpjS9NnJ1PVqJpTjz3c85+eP+wi/O7oQa7GtV?=
 =?us-ascii?Q?j8P0pgyhH9qtlYozCDh+QCAaD4L9kd9Fl8FgKt+poZYHiZ1CgTQgo8QNaa5j?=
 =?us-ascii?Q?M8dFAGZlvichpGavCqgyu03q/sMc+Pq667Ig81YDbT3g/e53fx+zH0LILESH?=
 =?us-ascii?Q?J3IbqlUqBVewsNuqJuwRJiDq+fHx8YnbY3PWfHsaRKViGKgE5NsRBkeKFES8?=
 =?us-ascii?Q?sSZSrTTSbJZBikZJN7JmB/yqXNt3YRG2DflGQJ2fPICBXV36j7X0V6cs++Hb?=
 =?us-ascii?Q?mVKS420bd5e5TJUHbD7ryM/GIf4JDNHCf9B8YNyM/b0TGGz26u3YN9WKDJ1S?=
 =?us-ascii?Q?t9aHPVt4gfei40r8eVf5yrgPorkpWRIgzV+fTqFttRywF9hPMSrtkRDOjNEh?=
 =?us-ascii?Q?K6C87FHYXS18+cEQMWSIxQv4vMvOs8k7jjivWFY8K/py21XNVpM996DtB9s2?=
 =?us-ascii?Q?BqIwnE6zVbtcUT4zLRniQCS/PXhhisLcEoEf/WwvBHsEubEAnPcktm0GSCtP?=
 =?us-ascii?Q?YzHoxq/M4Z3Eqhut41998Cxn1HjEhhUn/pCgt51r?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9897641f-d060-4e7b-83cd-08dc310130e5
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Feb 2024 04:13:56.6056
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: wJt7pvVJyvYEDB+/LiM//YHkydzzVkSKLTWF5ixCo6c8x4iansUN8CMhx8KE8GsoMcfqNnZ9/RDILQcCQfnGoA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV2PR11MB5973
X-OriginatorOrg: intel.com

On Fri, Feb 09, 2024 at 02:28:57PM -0800, Sean Christopherson wrote:
> Move the checks related to the validity of an access to a memslot from the
> inner __kvm_faultin_pfn() to its sole caller, kvm_faultin_pfn().  This
> allows emulating accesses to the APIC access page, which don't need to
> resolve a pfn, even if there is a relevant in-progress mmu_notifier
> invalidation.  Ditto for accesses to KVM internal memslots from L2, which
> KVM also treats as emulated MMIO.
> 
> More importantly, this will allow for future cleanup by having the
> "no memslot" case bail from kvm_faultin_pfn() very early on.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/mmu/mmu.c | 62 ++++++++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 50bfaa53f3f2..505fc7eef533 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4333,33 +4333,6 @@ static int __kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
>  	struct kvm_memory_slot *slot = fault->slot;
>  	bool async;
>  
> -	/*
> -	 * Retry the page fault if the gfn hit a memslot that is being deleted
> -	 * or moved.  This ensures any existing SPTEs for the old memslot will
> -	 * be zapped before KVM inserts a new MMIO SPTE for the gfn.
> -	 */
> -	if (slot && (slot->flags & KVM_MEMSLOT_INVALID))
> -		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 the APIC access page exists but is disabled, go directly
> -		 * to emulation without caching the MMIO access or creating a
> -		 * MMIO SPTE.  That way the cache doesn't need to be purged
> -		 * when the AVIC is re-enabled.
> -		 */
> -		if (slot && slot->id == APIC_ACCESS_PAGE_PRIVATE_MEMSLOT &&
> -		    !kvm_apicv_activated(vcpu->kvm))
> -			return RET_PF_EMULATE;
> -	}
> -
>  	if (fault->is_private)
>  		return kvm_faultin_pfn_private(vcpu, fault);
>  
> @@ -4406,6 +4379,37 @@ static int kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
>  	fault->mmu_seq = vcpu->kvm->mmu_invalidate_seq;
>  	smp_rmb();
>  
> +	if (!slot)
> +		goto faultin_pfn;
> +
> +	/*
> +	 * Retry the page fault if the gfn hit a memslot that is being deleted
> +	 * or moved.  This ensures any existing SPTEs for the old memslot will
> +	 * be zapped before KVM inserts a new MMIO SPTE for the gfn.
> +	 */
> +	if (slot->flags & KVM_MEMSLOT_INVALID)
> +		return RET_PF_RETRY;
> +
> +	if (!kvm_is_visible_memslot(slot)) {
> +		/* Don't expose KVM's internal memslots to L2. */
> +		if (is_guest_mode(vcpu)) {
> +			fault->slot = NULL;
> +			fault->pfn = KVM_PFN_NOSLOT;
> +			fault->map_writable = false;
> +			return RET_PF_CONTINUE;
Call kvm_handle_noslot_fault() to replace returning RET_PF_CONTINUE?

> +		}
> +
> +		/*
> +		 * If the APIC access page exists but is disabled, go directly
> +		 * to emulation without caching the MMIO access or creating a
> +		 * MMIO SPTE.  That way the cache doesn't need to be purged
> +		 * when the AVIC is re-enabled.
> +		 */
> +		if (slot->id == APIC_ACCESS_PAGE_PRIVATE_MEMSLOT &&
> +		    !kvm_apicv_activated(vcpu->kvm))
> +			return RET_PF_EMULATE;
> +	}
> +
>  	/*
>  	 * Check for a relevant mmu_notifier invalidation event before getting
>  	 * the pfn from the primary MMU, and before acquiring mmu_lock.
> @@ -4427,10 +4431,10 @@ static int kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
>  	 * *guaranteed* to need to retry, i.e. waiting until mmu_lock is held
>  	 * to detect retry guarantees the worst case latency for the vCPU.
>  	 */
> -	if (!slot &&
> -	    mmu_invalidate_retry_gfn_unsafe(vcpu->kvm, fault->mmu_seq, fault->gfn))
> +	if (mmu_invalidate_retry_gfn_unsafe(vcpu->kvm, fault->mmu_seq, fault->gfn))
>  		return RET_PF_RETRY;
>  
> +faultin_pfn:
>  	ret = __kvm_faultin_pfn(vcpu, fault);
>  	if (ret != RET_PF_CONTINUE)
>  		return ret;
> -- 
> 2.43.0.687.g38aa6559b0-goog
>