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 E3373C32793 for ; Wed, 18 Jan 2023 08:31:14 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4EE7C6B0071; Wed, 18 Jan 2023 03:31:14 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 49E736B0072; Wed, 18 Jan 2023 03:31:14 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 33EF36B0074; Wed, 18 Jan 2023 03:31:14 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 23E886B0071 for ; Wed, 18 Jan 2023 03:31:14 -0500 (EST) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id CDA11AB192 for ; Wed, 18 Jan 2023 08:31:13 +0000 (UTC) X-FDA: 80367250026.23.C14A499 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by imf07.hostedemail.com (Postfix) with ESMTP id 5401940009 for ; Wed, 18 Jan 2023 08:31:10 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=RYnOD3ir; spf=none (imf07.hostedemail.com: domain of chao.p.peng@linux.intel.com has no SPF policy when checking 192.55.52.136) smtp.mailfrom=chao.p.peng@linux.intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1674030670; a=rsa-sha256; cv=none; b=t1G/dZ4i+XRbZkZOr1gqH9A4tjb51zSiQYFn4C1Ymqkw6qt9YAuo6MDBHikJTvnMPM1Iky 7Az9ZAmhvylueB65IIV7wX+1YxYfzwROLh+3KD7DSWfO8wycqfEO2UPzsj5EoJPmH09kpK 25FUIK5O21JkXAkFjdWI1d5shsI8UBg= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=RYnOD3ir; spf=none (imf07.hostedemail.com: domain of chao.p.peng@linux.intel.com has no SPF policy when checking 192.55.52.136) smtp.mailfrom=chao.p.peng@linux.intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1674030670; 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=IrXb5q/yNvEQ/A7Y/LioSSMx8EOCJK3IEGbPa3Sb3hE=; b=4Obqa8j5AxwBhdlbbnK2OdTc9LxOE5PPicnEaLukiG3Fnpqtcctil+0CBG0A7f/QCHeNbB p4jPrMNINdz0gLmIYV0SvzKsN8xoV4i6cJRspT22q64k8a6dUmQepR2yKdfhs/fsPJI94G G2lsGwUPbom1yzKECQ6cT9ZMo5nhNYE= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1674030670; x=1705566670; h=date:from:to:cc:subject:message-id:reply-to:references: mime-version:in-reply-to; bh=IaruPOH1D3wCRj1gFgYkLc0hyR2BZ5VY51Nzyl07GbA=; b=RYnOD3ir0BJNpiCcm0uyKZpntl5v1vcypYwXpsWxWE3e2+qrh6JuQ/2k sMUAML6UgMQNo36n1TUSVK33Dih9Ybxond+VN6Ph7ndU27Vnat7TtodgK Rx5F5Y7nkUVrrnBnWp9SmesjxR18o8SQXcneAGVIMLvWS0B0qERdhC9PZ ORLGLg5ZM6HhpDylImR0/MinVh6RSJVdMdMrzB4ZjNk5Nd5j9GbyPvwZj xN3sN1OLoBJujJWlyfotuk1rRbt+JU5uQpiLlzv94JiJNcsHNCi4AC9Jg 75kOJH4VD4oOHZ+lCTSeHP5rDW5wFT54c09rS7rjwcvcHOHjQA6Dzzect w==; X-IronPort-AV: E=McAfee;i="6500,9779,10593"; a="304612203" X-IronPort-AV: E=Sophos;i="5.97,224,1669104000"; d="scan'208";a="304612203" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Jan 2023 00:31:07 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10593"; a="661627035" X-IronPort-AV: E=Sophos;i="5.97,224,1669104000"; d="scan'208";a="661627035" Received: from chaop.bj.intel.com (HELO localhost) ([10.240.192.105]) by fmsmga007.fm.intel.com with ESMTP; 18 Jan 2023 00:30:56 -0800 Date: Wed, 18 Jan 2023 16:23:09 +0800 From: Chao Peng To: Sean Christopherson Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-arch@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 , Arnd Bergmann , Naoya Horiguchi , Miaohe Lin , x86@kernel.org, "H . Peter Anvin" , Hugh Dickins , Jeff Layton , "J . Bruce Fields" , Andrew Morton , Shuah Khan , 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 , tabba@google.com, Michael Roth , mhocko@suse.com, wei.w.wang@intel.com Subject: Re: [PATCH v10 9/9] KVM: Enable and expose KVM_MEM_PRIVATE Message-ID: <20230118082309.GB303785@chaop.bj.intel.com> Reply-To: Chao Peng References: <20221202061347.1070246-1-chao.p.peng@linux.intel.com> <20221202061347.1070246-10-chao.p.peng@linux.intel.com> <20230117131251.GC273037@chaop.bj.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspam-User: X-Rspamd-Queue-Id: 5401940009 X-Rspamd-Server: rspam01 X-Stat-Signature: 8jbtyhhatzcgya75xgk97spobbu4udf8 X-HE-Tag: 1674030670-446447 X-HE-Meta: U2FsdGVkX1/cUB81osmcPEcjMfP9oBppzaU0WH1b3HMjRFxELmqEIw30Iu8Oj7yUi38dcEKdlPXw/Ve3Sd3vMXy7bVbnmrs83qeg0W3huBBp4gqF49IOa5xr3QYEcsWjWjvRqh5mIRtpDkl+83zdpgilvyf+GMKHMyu/Gf5cSRsNOeK+87C6kTDQM5JtY9CzBOOG3rOc+ynyorrEh6c8KSqqt99FEI99G3wnQYXARYR62kFt6VeBkLR51Yssiu578D7lC3et2+6UiRlNsGKQhpxzIVxDbQE4yjFD6ik2rX88x+uwnhUrjJzdlktFRgqd6ZBCzyz0e9ATX+JdDK+gEPm7J8BYAwgCEPOD6Q3MrkHtSRPOAlnqRLJGAsast0oC9UGjAqrIqQhJBiHYKAhsgOhmK5cI8YVXv6HuFyBaK9NNz0Rt39cl0XICNEyay0r5DmQxORevSfzFNGt8WHeA2SnusnWqVkzadQSnXM/KwlTcp7XMipnJVFKdQIwOG3Dup7prpEXuDiXDdId6yHFrad+u4VWDQBi2zhs/5obz7sbuTQmoooAuaUyb670OGjOwylpbDxqs+ryY0VBW+qTZxBjuCkMjLAKztEstI1X6Ak3LwjWNOuAFi4/c3rJtOFfR86z0AMvGa/GE7oDRSGAG4SfFrn2FqFH0DtfDAWT/fd8FdnjK4Tcu3W1mPsI8IhQkcT2IpDow3Ks3nzsXDgCwgKHc5kN9YbBXCi5UKWhny/r9AzrGZZDQy5QmFSp+ScnJF+KzeKDGyXbmlJfSEwuJBeWi6dCK2jd1PAqQmXN1I0PCKELMt7TNs+FWGc50dqbwAJOskWGgyvtqFevAlBr5QuqEOKHifYazeqVqccaYRmut9XQufAezH1Qpg0j8nSApZfWw0K3WLMpiPFEld/sV2J6O/u1+HXnX09TpZBDtPGktTiZNUir+N7F8EjD0PtoNEX9GeAjW5X8ckZESb3C 4gaGWcCS +DWFdkQLj+B/uddZrAf/MbFaxLJWqaHggnqpE 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 Tue, Jan 17, 2023 at 07:35:58PM +0000, Sean Christopherson wrote: > On Tue, Jan 17, 2023, Chao Peng wrote: > > On Sat, Jan 14, 2023 at 12:01:01AM +0000, Sean Christopherson wrote: > > > On Fri, Dec 02, 2022, Chao Peng wrote: > > > > @@ -10357,6 +10364,12 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > > > > > > > > if (kvm_check_request(KVM_REQ_UPDATE_CPU_DIRTY_LOGGING, vcpu)) > > > > static_call(kvm_x86_update_cpu_dirty_logging)(vcpu); > > > > + > > > > + if (kvm_check_request(KVM_REQ_MEMORY_MCE, vcpu)) { > > > > + vcpu->run->exit_reason = KVM_EXIT_SHUTDOWN; > > > > > > Synthesizing triple fault shutdown is not the right approach. Even with TDX's > > > MCE "architecture" (heavy sarcasm), it's possible that host userspace and the > > > guest have a paravirt interface for handling memory errors without killing the > > > host. > > > > Agree shutdown is not the correct choice. I see you made below change: > > > > send_sig_mceerr(BUS_MCEERR_AR, (void __user *)hva, PAGE_SHIFT, current) > > > > The MCE may happen in any thread than KVM thread, sending siginal to > > 'current' thread may not be the expected behavior. > > This is already true today, e.g. a #MC in memory that is mapped into the guest can > be triggered by a host access. Hrm, but in this case we actually have a KVM > instance, and we know that the #MC is relevant to the KVM instance, so I agree > that signaling 'current' is kludgy. > > > Also how userspace can tell is the MCE on the shared page or private page? > > Do we care? > > We care. I was originally thinking we could require userspace to keep track of > things, but that's quite prescriptive and flawed, e.g. could race with conversions. > > One option would be to KVM_EXIT_MEMORY_FAULT, and then wire up a generic (not x86 > specific) KVM request to exit to userspace, e.g. > > /* KVM_EXIT_MEMORY_FAULT */ > struct { > #define KVM_MEMORY_EXIT_FLAG_PRIVATE (1ULL << 3) > #define KVM_MEMORY_EXIT_FLAG_HW_ERROR (1ULL << 4) > __u64 flags; > __u64 gpa; > __u64 size; > } memory; > > But I'm not sure that's the correct approach. It kinda feels like we're reinventing > the wheel. It seems like restrictedmem_get_page() _must_ be able to reject attempts > to get a poisoned page, i.e. restrictedmem_get_page() should yield KVM_PFN_ERR_HWPOISON. Yes, I see there is -EHWPOISON handling for hva_to_pfn() for shared memory. It makes sense doing similar for private page. > Assuming that's the case, then I believe KVM simply needs to zap SPTEs in response > to an error notification in order to force vCPUs to fault on the poisoned page. Agree, this is waht we should do anyway. > > > > > + return -EINVAL; > > > > if (as_id >= KVM_ADDRESS_SPACE_NUM || id >= KVM_MEM_SLOTS_NUM) > > > > return -EINVAL; > > > > if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr) > > > > @@ -2020,6 +2154,9 @@ int __kvm_set_memory_region(struct kvm *kvm, > > > > if ((kvm->nr_memslot_pages + npages) < kvm->nr_memslot_pages) > > > > return -EINVAL; > > > > } else { /* Modify an existing slot. */ > > > > + /* Private memslots are immutable, they can only be deleted. */ > > > > > > I'm 99% certain I suggested this, but if we're going to make these memslots > > > immutable, then we should straight up disallow dirty logging, otherwise we'll > > > end up with a bizarre uAPI. > > > > But in my mind dirty logging will be needed in the very short time, when > > live migration gets supported? > > Ya, but if/when live migration support is added, private memslots will no longer > be immutable as userspace will want to enable dirty logging only when a VM is > being migrated, i.e. something will need to change. > > Given that it looks like we have clear line of sight to SEV+UPM guests, my > preference would be to allow toggling dirty logging from the get-go. It doesn't > necessarily have to be in the first patch, e.g. KVM could initially reject > KVM_MEM_LOG_DIRTY_PAGES + KVM_MEM_PRIVATE and then add support separately to make > the series easier to review, test, and bisect. > > static int check_memory_region_flags(struct kvm *kvm, > const struct kvm_userspace_memory_region2 *mem) > { > u32 valid_flags = KVM_MEM_LOG_DIRTY_PAGES; > > if (kvm_arch_has_private_mem(kvm) && > ~(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) > valid_flags |= KVM_MEM_PRIVATE; Adding this limitation is OK to me. It's not too hard to remove it when live migration gets added. > > > ... > } > > > > > + if (mem->flags & KVM_MEM_PRIVATE) > > > > + return -EINVAL; > > > > if ((mem->userspace_addr != old->userspace_addr) || > > > > (npages != old->npages) || > > > > ((mem->flags ^ old->flags) & KVM_MEM_READONLY)) > > > > @@ -2048,10 +2185,28 @@ int __kvm_set_memory_region(struct kvm *kvm, > > > > new->npages = npages; > > > > new->flags = mem->flags; > > > > new->userspace_addr = mem->userspace_addr; > > > > + if (mem->flags & KVM_MEM_PRIVATE) { > > > > + new->restricted_file = fget(mem->restricted_fd); > > > > + if (!new->restricted_file || > > > > + !file_is_restrictedmem(new->restricted_file)) { > > > > + r = -EINVAL; > > > > + goto out; > > > > + } > > > > + new->restricted_offset = mem->restricted_offset; > > > > I see you changed slot->restricted_offset type from loff_t to gfn_t and > > used pgoff_t when doing the restrictedmem_bind/unbind(). Using page > > index is reasonable KVM internally and sounds simpler than loff_t. But > > we also need initialize it to page index here as well as changes in > > another two cases. This is needed when restricted_offset != 0. > > Oof. I'm pretty sure I completely missed that loff_t is used for byte offsets, > whereas pgoff_t is a frame index. > > Given that the restrictmem APIs take pgoff_t, I definitely think it makes sense > to the index, but I'm very tempted to store pgoff_t instead of gfn_t, and name > the field "index" to help connect the dots to the rest of kernel, where "pgoff_t index" > is quite common. > > And looking at those bits again, we should wrap all of the restrictedmem fields > with CONFIG_KVM_PRIVATE_MEM. It'll require minor tweaks to __kvm_set_memory_region(), > but I think will yield cleaner code (and internal APIs) overall. > > And wrap the three fields in an anonymous struct? E.g. this is a little more > versbose (restrictedmem instead restricted), but at first glance it doesn't seem > to cause widespared line length issues. > > #ifdef CONFIG_KVM_PRIVATE_MEM > struct { > struct file *file; > pgoff_t index; > struct restrictedmem_notifier notifier; > } restrictedmem; > #endif Looks better. Thanks, Chao > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > index 547b92215002..49e375e78f30 100644 > > --- a/include/linux/kvm_host.h > > +++ b/include/linux/kvm_host.h > > @@ -2364,8 +2364,7 @@ static inline int kvm_restricted_mem_get_pfn(struct kvm_memory_slot *slot, > > gfn_t gfn, kvm_pfn_t *pfn, > > int *order) > > { > > - pgoff_t index = gfn - slot->base_gfn + > > - (slot->restricted_offset >> PAGE_SHIFT); > > + pgoff_t index = gfn - slot->base_gfn + slot->restricted_offset; > > struct page *page; > > int ret; > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 01db35ddd5b3..7439bdcb0d04 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -935,7 +935,7 @@ static bool restrictedmem_range_is_valid(struct kvm_memory_slot *slot, > > pgoff_t start, pgoff_t end, > > gfn_t *gfn_start, gfn_t *gfn_end) > > { > > - unsigned long base_pgoff = slot->restricted_offset >> PAGE_SHIFT; > > + unsigned long base_pgoff = slot->restricted_offset; > > > > if (start > base_pgoff) > > *gfn_start = slot->base_gfn + start - base_pgoff; > > @@ -2275,7 +2275,7 @@ int __kvm_set_memory_region(struct kvm *kvm, > > r = -EINVAL; > > goto out; > > } > > - new->restricted_offset = mem->restricted_offset; > > + new->restricted_offset = mem->restricted_offset >> PAGE_SHIFT; > > } > > > > r = kvm_set_memslot(kvm, old, new, change); > > > > Chao > > > > + } > > > > + > > > > + new->kvm = kvm; > > > > > > Set this above, just so that the code flows better.