From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 63D3CCCA479 for ; Mon, 20 Jun 2022 14:20:22 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D99736B0073; Mon, 20 Jun 2022 10:20:21 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D20876B0074; Mon, 20 Jun 2022 10:20:21 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B73506B0075; Mon, 20 Jun 2022 10:20:21 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id A20966B0073 for ; Mon, 20 Jun 2022 10:20:21 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 72D622043E for ; Mon, 20 Jun 2022 14:20:21 +0000 (UTC) X-FDA: 79598824242.04.DD94F56 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by imf19.hostedemail.com (Postfix) with ESMTP id A0E7A1A00A8 for ; Mon, 20 Jun 2022 14:20:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1655734819; x=1687270819; h=date:from:to:cc:subject:message-id:reply-to:references: mime-version:in-reply-to; bh=j5dmRBX11KpQ0doZYX6GRPhSnkODpaL7Hahti3PPX3U=; b=KSvYmvbQ5nRNJfxZ6+DsAO5cuiq4rAWbVPPEo0wehoMj2XIqi47ne6Pf jiuJ6vGWmPkxBlUdW/Zl1tPFQPBGG0QjvPNYXBQPumPN2+PX7s7K6Ei1T 2pENveo+NKkVXadJxUPapgwDN17F1okwaV5UiW/mYd+Q93xiVBm8F0m27 Ae+Sp+q3YhcXlaMW1dYVOx5skTutkcOMcdrqOHOmmJJQk1K2qIFHDL7M0 +h6xf44v5t/ch6PyP3wKphwqQPgl4n9OaJk0jIzZ5AzRGnKwhaAYGbBg6 qmNIFRCiLmreuRIrk/iRkOnoP6Fcg7QfLTx+OmZi2FikC2l4+QKc2Oj2P Q==; X-IronPort-AV: E=McAfee;i="6400,9594,10380"; a="280963581" X-IronPort-AV: E=Sophos;i="5.92,306,1650956400"; d="scan'208";a="280963581" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Jun 2022 07:20:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.92,306,1650956400"; d="scan'208";a="584914017" Received: from chaop.bj.intel.com (HELO localhost) ([10.240.192.101]) by orsmga007.jf.intel.com with ESMTP; 20 Jun 2022 07:20:09 -0700 Date: Mon, 20 Jun 2022 22:16:47 +0800 From: Chao Peng To: Sean Christopherson , kirill.shutemov@linux.intel.com Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org, linux-doc@vger.kernel.org, qemu-devel@nongnu.org, Paolo Bonzini , Jonathan Corbet , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , Thomas Gleixner , Ingo Molnar , Borislav Petkov , x86@kernel.org, "H . Peter Anvin" , Hugh Dickins , Jeff Layton , "J . Bruce Fields" , Andrew Morton , Mike Rapoport , Steven Price , "Maciej S . Szmigiero" , Vlastimil Babka , Vishal Annapurve , Yu Zhang , "Kirill A . Shutemov" , luto@kernel.org, jun.nakajima@intel.com, dave.hansen@intel.com, ak@linux.intel.com, david@redhat.com, aarcange@redhat.com, ddutile@redhat.com, dhildenb@redhat.com, Quentin Perret , Michael Roth , mhocko@suse.com Subject: Re: [PATCH v6 6/8] KVM: Handle page fault for private memory Message-ID: <20220620141647.GC2016793@chaop.bj.intel.com> Reply-To: Chao Peng References: <20220519153713.819591-1-chao.p.peng@linux.intel.com> <20220519153713.819591-7-chao.p.peng@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1655734820; a=rsa-sha256; cv=none; b=oItV0kZPETsqjY1gUZ29KA3+YN2w1VoHCiUQPNYKEb2H0iAfBTsWxPrydbnujoA+eHZwcF 343sbijRueqV8tvUO3cr2+ELs67W0ira6P4Hbt1wG4HGAo0mz+QWRMfKGN/UKx8WaQkFj1 1hbBHF548jUwZGd7k/9oEOXWq64cvK4= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=KSvYmvbQ; dmarc=pass (policy=none) header.from=intel.com; spf=none (imf19.hostedemail.com: domain of chao.p.peng@linux.intel.com has no SPF policy when checking 134.134.136.65) smtp.mailfrom=chao.p.peng@linux.intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1655734820; h=from:from:sender:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=9r3AkApC3Zx2evucl6LjhOY09th1lJmox9MVAXKgrA4=; b=iTRwsGfQ0ZtF4vcAIgrs+WfTTqJZC520m/CNxyiBj0Ndtl8VmXiFbXpnCP6EG5yJ/6dfYW M5rR1wR7weJMPFrtRTBu00yV0cNH3yEaRdvKypuXaY8d7w77hLecmWUhwp5IME7qFT5rox 4qo5pdPDseBGaOrl5vV8BSzT4Sj1xEI= Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=KSvYmvbQ; dmarc=pass (policy=none) header.from=intel.com; spf=none (imf19.hostedemail.com: domain of chao.p.peng@linux.intel.com has no SPF policy when checking 134.134.136.65) smtp.mailfrom=chao.p.peng@linux.intel.com X-Stat-Signature: 3raqtusbqfx7n7d3egr5prw8uj93azrr X-Rspamd-Queue-Id: A0E7A1A00A8 X-Rspam-User: X-Rspamd-Server: rspam03 X-HE-Tag: 1655734819-480184 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Jun 17, 2022 at 09:30:53PM +0000, Sean Christopherson wrote: > On Thu, May 19, 2022, Chao Peng wrote: > > @@ -4028,8 +4081,11 @@ static bool is_page_fault_stale(struct kvm_vcpu *vcpu, > > if (!sp && kvm_test_request(KVM_REQ_MMU_FREE_OBSOLETE_ROOTS, vcpu)) > > return true; > > > > - return fault->slot && > > - mmu_notifier_retry_hva(vcpu->kvm, mmu_seq, fault->hva); > > + if (fault->is_private) > > + return mmu_notifier_retry(vcpu->kvm, mmu_seq); > > Hmm, this is somewhat undesirable, because faulting in private pfns will be blocked > by unrelated mmu_notifier updates. The issue is mitigated to some degree by bumping > the sequence count if and only if overlap with a memslot is detected, e.g. mapping > changes that affects only userspace won't block the guest. > > It probably won't be an issue, but at the same time it's easy to solve, and I don't > like piggybacking mmu_notifier_seq as private mappings shouldn't be subject to the > mmu_notifier. > > That would also fix a theoretical bug in this patch where mmu_notifier_retry() > wouldn't be defined if CONFIG_MEMFILE_NOTIFIER=y && CONFIG_MMU_NOTIFIER=n.a Agreed, Thanks. > > --- > arch/x86/kvm/mmu/mmu.c | 11 ++++++----- > include/linux/kvm_host.h | 16 +++++++++++----- > virt/kvm/kvm_main.c | 2 +- > 3 files changed, 18 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 0b455c16ec64..a4cbd29433e7 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -4100,10 +4100,10 @@ static bool is_page_fault_stale(struct kvm_vcpu *vcpu, > return true; > > if (fault->is_private) > - return mmu_notifier_retry(vcpu->kvm, mmu_seq); > - else > - return fault->slot && > - mmu_notifier_retry_hva(vcpu->kvm, mmu_seq, fault->hva); > + return memfile_notifier_retry(vcpu->kvm, mmu_seq); > + > + return fault->slot && > + mmu_notifier_retry_hva(vcpu->kvm, mmu_seq, fault->hva); > } > > static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) > @@ -4127,7 +4127,8 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault > if (r) > return r; > > - mmu_seq = vcpu->kvm->mmu_notifier_seq; > + mmu_seq = fault->is_private ? vcpu->kvm->memfile_notifier_seq : > + vcpu->kvm->mmu_notifier_seq; > smp_rmb(); > > r = kvm_faultin_pfn(vcpu, fault); > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 92afa5bddbc5..31f704c83099 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -773,16 +773,15 @@ struct kvm { > struct hlist_head irq_ack_notifier_list; > #endif > > -#if (defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER)) ||\ > - defined(CONFIG_MEMFILE_NOTIFIER) > +#if (defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER)) > unsigned long mmu_notifier_seq; > -#endif > - > -#if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER) > struct mmu_notifier mmu_notifier; > long mmu_notifier_count; > unsigned long mmu_notifier_range_start; > unsigned long mmu_notifier_range_end; > +#endif > +#ifdef CONFIG_MEMFILE_NOTIFIER > + unsigned long memfile_notifier_seq; > #endif > struct list_head devices; > u64 manual_dirty_log_protect; > @@ -1964,6 +1963,13 @@ static inline int mmu_notifier_retry_hva(struct kvm *kvm, > } > #endif > > +#ifdef CONFIG_MEMFILE_NOTIFIER > +static inline bool memfile_notifier_retry(struct kvm *kvm, unsigned long mmu_seq) > +{ > + return kvm->memfile_notifier_seq != mmu_seq; > +} > +#endif > + > #ifdef CONFIG_HAVE_KVM_IRQ_ROUTING > > #define KVM_MAX_IRQ_ROUTES 4096 /* might need extension/rework in the future */ > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 2b416d3bd60e..e6d34c964d51 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -898,7 +898,7 @@ static void kvm_private_mem_notifier_handler(struct memfile_notifier *notifier, > KVM_MMU_LOCK(kvm); > if (kvm_unmap_gfn_range(kvm, &gfn_range)) > kvm_flush_remote_tlbs(kvm); > - kvm->mmu_notifier_seq++; > + kvm->memfile_notifier_seq++; > KVM_MMU_UNLOCK(kvm); > srcu_read_unlock(&kvm->srcu, idx); > } > > base-commit: 333ef501c7f6c6d4ef2b7678905cad0f8ef3e271 > -- > > > + else > > + return fault->slot && > > + mmu_notifier_retry_hva(vcpu->kvm, mmu_seq, fault->hva); > > } > > > > static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) > > @@ -4088,7 +4144,12 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault > > read_unlock(&vcpu->kvm->mmu_lock); > > else > > write_unlock(&vcpu->kvm->mmu_lock); > > - kvm_release_pfn_clean(fault->pfn); > > + > > + if (fault->is_private) > > + kvm_private_mem_put_pfn(fault->slot, fault->pfn); > > Why does the shmem path lock the page, and then unlock it here? Initially this is to prevent race between SLPT population and truncate/punch on the fd. Without this, a gfn may become stale before the page is populated in SLPT. However, with memfile_notifier_retry mechanism, this sounds not needed. > > Same question for why this path marks it dirty? The guest has the page mapped > so the dirty flag is immediately stale. I believe so. > > In other words, why does KVM need to do something different for private pfns? These two are inherited from Kirill's previous code. See if he has any comment. > > > + else > > + kvm_release_pfn_clean(fault->pfn); > > + > > return r; > > } > > > > ... > > > diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h > > index 7f8f1c8dbed2..1d857919a947 100644 > > --- a/arch/x86/kvm/mmu/paging_tmpl.h > > +++ b/arch/x86/kvm/mmu/paging_tmpl.h > > @@ -878,7 +878,10 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault > > > > out_unlock: > > write_unlock(&vcpu->kvm->mmu_lock); > > - kvm_release_pfn_clean(fault->pfn); > > + if (fault->is_private) > > Indirect MMUs can't support private faults, i.e. this is unnecessary. Okay. > > > + kvm_private_mem_put_pfn(fault->slot, fault->pfn); > > + else > > + kvm_release_pfn_clean(fault->pfn); > > return r; > > } > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > index 3fd168972ecd..b0a7910505ed 100644 > > --- a/include/linux/kvm_host.h > > +++ b/include/linux/kvm_host.h > > @@ -2241,4 +2241,26 @@ static inline void kvm_handle_signal_exit(struct kvm_vcpu *vcpu) > > /* Max number of entries allowed for each kvm dirty ring */ > > #define KVM_DIRTY_RING_MAX_ENTRIES 65536 > > > > +#ifdef CONFIG_HAVE_KVM_PRIVATE_MEM > > +static inline int kvm_private_mem_get_pfn(struct kvm_memory_slot *slot, > > + gfn_t gfn, kvm_pfn_t *pfn, int *order) > > +{ > > + int ret; > > + pfn_t pfnt; > > + pgoff_t index = gfn - slot->base_gfn + > > + (slot->private_offset >> PAGE_SHIFT); > > + > > + ret = slot->notifier.bs->get_lock_pfn(slot->private_file, index, &pfnt, > > + order); > > + *pfn = pfn_t_to_pfn(pfnt); > > + return ret; > > +} > > + > > +static inline void kvm_private_mem_put_pfn(struct kvm_memory_slot *slot, > > + kvm_pfn_t pfn) > > +{ > > + slot->notifier.bs->put_unlock_pfn(pfn_to_pfn_t(pfn)); > > +} > > +#endif /* CONFIG_HAVE_KVM_PRIVATE_MEM */ > > + > > #endif > > -- > > 2.25.1 > >